Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stack traces should include parameter names #73051

Closed
MichalStrehovsky opened this issue Jul 29, 2022 · 2 comments · Fixed by #73578
Closed

Stack traces should include parameter names #73051

MichalStrehovsky opened this issue Jul 29, 2022 · 2 comments · Fixed by #73578
Assignees
@jasper-d
Labels
area-NativeAOT-coreclr help wanted [up-for-grabs] Good issue for external contributors
Milestone
8.0.0

Comments

@MichalStrehovsky
Copy link
Member

EnvironmentStackTrace.StackTraceTest test expects parameter names to show up. We don't print them even if we have them.

The original motivation was consistency - we don't necessarily have them.

But since then, IL Linker started removing parameter names for methods that are not demonstratable targets of reflection. This matches what we do. We can just start printing them if we have MethodDefinition metadata (and keep not printing them if we don't have it).

The fix will be in src\coreclr\nativeaot\System.Private.StackTraceMetadata\src\Internal\StackTraceMetadata\MethodNameFormatter.cs.
@MichalStrehovsky MichalStrehovsky added help wanted [up-for-grabs] Good issue for external contributors area-NativeAOT-coreclr labels Jul 29, 2022
@MichalStrehovsky MichalStrehovsky added this to the 8.0.0 milestone Jul 29, 2022
@jasper-d
Copy link
Contributor

jasper-d commented Aug 4, 2022

I would like to take a stab at it. Feel free to assign me.

@jkotas
Copy link
Member

jkotas commented Aug 4, 2022

It is yours!

jasper-d added a commit to jasper-d/runtime that referenced this issue Aug 5, 2022
@jasper-d
Emit parameter names
6ba83e1 
Fixes dotnet#73051
@jasper-d jasper-d mentioned this issue Aug 8, 2022
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 8, 2022
MichalStrehovsky pushed a commit that referenced this issue Aug 18, 2022
@jasper-d
[NativeAOT] Add method parameter names to stack trace if available (#…
519d1e3 
…73578)

Fixes #73051
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 18, 2022
github-actions bot pushed a commit that referenced this issue Aug 18, 2022
@jasper-d
Emit parameter names
ed70b0a 
Fixes #73051
carlossanlop pushed a commit that referenced this issue Aug 18, 2022
@github-actions @jasper-d @MichalStrehovsky
[release/7.0-rc1] [NativeAOT] Add method parameter names to stack tra…
0ec29b6 
…ce if available (#74145)

* Emit parameter names

Fixes #73051

* Address PR feedback

* Rename things

* Apply suggestions from code review

Co-authored-by: jasperd <jasper-d@users.noreply.github.com>
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jasper-d jasper-d

Labels
area-NativeAOT-coreclr help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Milestone
8.0.0
Development

Successfully merging a pull request may close this issue.

[NativeAOT] Add method parameter names to stack trace if available jasper-d/runtime
3 participants
@jasper-d @jkotas @MichalStrehovsky