Skip to content

Do not generate broken debug info for non-methods#94693

Merged
MichalStrehovsky merged 1 commit intodotnet:mainfrom
MichalStrehovsky:fix94650
Nov 15, 2023
Merged

Do not generate broken debug info for non-methods#94693
MichalStrehovsky merged 1 commit intodotnet:mainfrom
MichalStrehovsky:fix94650

Conversation

@MichalStrehovsky
Copy link
Member

Fixes #94650.

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 it didn't fully stand out but it doesn't look right there already.

Cc @dotnet/ilc-contrib

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 14, 2023

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

Issue Details

Fixes #94650.

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 it didn't fully stand out but it doesn't look right there already.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member Author

Cc @markples @TIHan this is an objwriter fix.

@MichalStrehovsky
Copy link
Member Author

cc @filipnavara - in case this needs porting to the managed objwriter

@markples
Copy link
Contributor

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas
Copy link
Member

jkotas commented Nov 14, 2023

.NET 8 backport candidate?

@JulieLeeMSFT
Copy link
Member

@filipnavara
Copy link
Member

cc @filipnavara - in case this needs porting to the managed objwriter

Thanks for the heads up. I need to update it here. In my case it's actually mostly eliminated by the check in the caller method but this makes it easier to reason about.

@MichalStrehovsky MichalStrehovsky merged commit 18cac43 into dotnet:main Nov 15, 2023
@MichalStrehovsky MichalStrehovsky deleted the fix94650 branch November 15, 2023 08:04
@MichalStrehovsky
Copy link
Member Author

/backport to release/8.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6874512132

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windbg breakpoints on AOT binary induce crash

5 participants