Skip to content

Fix invalid pointer types in ldloca/ldarga descriptions #9574

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
merged 4 commits into from
Mar 21, 2024

Conversation

MichalPetryka
Copy link
Contributor

Summary

Those are documented to push managed pointers on the stack in the ECMA 335 and the runtime implements them as such.

Those are documented to push managed pointers on the stack in the ECMA 335 and the runtime implements them as such.
@MichalPetryka MichalPetryka requested a review from a team as a code owner January 25, 2024 22:52
@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Reflection.Emit labels Jan 25, 2024
Copy link

Learn Build status updates of commit 8117734:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Reflection.Emit/OpCodes.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@reflectronic reflectronic left a comment

Choose a reason for hiding this comment

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

conv.u does not convert managed pointers to transient pointers (type *), so this patch is incorrect. The transient pointers concept was deleted from the standard recently in dotnet/runtime#86622, so any reference to transient pointers should be deleted. There are many more references elsewhere in this file.

@gewarren
Copy link
Contributor

gewarren commented Mar 6, 2024

Tagging @dotnet/area-system-reflection-emit

@steveharter
Copy link
Contributor

PTAL @AaronRobinsonMSFT

Co-authored-by: John Tur <john-tur@outlook.com>
Copy link

Learn Build status updates of commit f1e1045:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Reflection.Emit/OpCodes.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@AaronRobinsonMSFT
Copy link
Member

/cc @gewarren

@gewarren
Copy link
Contributor

@AaronRobinsonMSFT Is this how it should render?

image

@AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT Is this how it should render?

image

Definitely not. Let me try again. :)

Copy link

Learn Build status updates of commit a9f13cd:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Reflection.Emit/OpCodes.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@AaronRobinsonMSFT
Copy link
Member

@gewarren Should be fixed now.

Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

LGTM

@gewarren gewarren merged commit a31d808 into dotnet:main Mar 21, 2024
jkotas added a commit that referenced this pull request Jul 5, 2025
Apply #9574 to ldloca.s.

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection.Emit community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants