Skip to content

[release/8.0-staging] Do not generate broken debug info for non-methods #94757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 15, 2023

Backport of #94693 to release/8.0-staging

Fix an issue where the compiler was generating broken debug information for data symbols.

/cc @MichalStrehovsky

Customer Impact

Customer noticed setting wildcard breakpoints breaks the app under the debugger. This is because the debugger erroneously overwrote data with debugbreak instructions because it thought data is code thanks to wrong debug information about the data.

Testing

Tested with a debugger locally.

Risk

Low - we're stopping the emission of a wrong debug record. This only affects debug info generation.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

The related methods (`EmitDebugInfo`, `EmitEHClauseInfo`) already do this - if the node is not a method node, don't emit anything. It is not clear what purpose the "if this is not a method, emit broken debug information" serves. I traced it all the way back to https://github.com/dotnet/corert/blob/d78cf62480331f63b26eb08b86b838ffa355ff0d/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ObjectWriter.cs#L427-L447 - it was surrounded by TODOs so maybe it didn't fully stand out but it doesn't look right there already.
@ghost
Copy link

ghost commented Nov 15, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #94693 to release/8.0-staging

/cc @MichalStrehovsky

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved. we will take for consideration in 8.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Nov 15, 2023
@jeffschwMSFT jeffschwMSFT added this to the 8.0.x milestone Nov 15, 2023
@carlossanlop
Copy link
Contributor

@MichalStrehovsky @jeffschwMSFT did this get Tactics approval? Today's code complete.

@jeffschwMSFT
Copy link
Member

Today's code complete.

@carlossanlop for January?

@carlossanlop
Copy link
Contributor

@MichalStrehovsky
Copy link
Member

We're targeting January with this.

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 16, 2023
@jeffschwMSFT jeffschwMSFT modified the milestones: 8.0.x, 8.0.2 Nov 16, 2023
@leecow leecow modified the milestones: 8.0.2, 8.0.1 Nov 21, 2023
@jeffschwMSFT
Copy link
Member

@MichalStrehovsky can you take a look at the CI failures? ready to merge?

@carlossanlop
Copy link
Contributor

The runtime-dev-innerloop (Build osx-x64 release Runtime_Debug) leg has been stuck all day. @MichalStrehovsky do we care about it or can I merge?

@jkotas
Copy link
Member

jkotas commented Nov 22, 2023

The build machine went offline. It is fine to merge. runtime-dev-innerloop does not exercise the changed code path in any specific way.

@jkotas jkotas merged commit 72a375f into release/8.0-staging Nov 22, 2023
@jkotas jkotas deleted the backport/pr-94693-to-release/8.0-staging branch November 22, 2023 02:41
@MichalStrehovsky
Copy link
Member

The build machine went offline. It is fine to merge. runtime-dev-innerloop does not exercise the changed code path in any specific way.

Thanks for merging!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants