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.

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

PseudoCustomAttribute.GetStructLayoutCustomAttribute returns incorrect data #12480

Closed
Tracked by #44655
tannergooding opened this issue Apr 12, 2019 · 4 comments · Fixed by #64965
Closed
Tracked by #44655

PseudoCustomAttribute.GetStructLayoutCustomAttribute returns incorrect data #12480

tannergooding opened this issue Apr 12, 2019 · 4 comments · Fixed by #64965
Labels
area-System.Reflection bug needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@tannergooding
Copy link
Member

PseudoCustomAttribute.GetStructLayoutCustomAttribute currently returns incorrect values for various types. See the implementation here: https://github.com/dotnet/coreclr/blob/72d49127a0c25e4b931c81e621c2411bfb6633a5/src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs#L2058-L2096

Namely, for the packing it currently changes 0 to 8; but it doesn't take into account any changes that the VM may have made for a given types data.

I would expect that this API either return the exact values in metadata or it should return the correct values computed by the VM at runtime.

An example of where this is problematic is for types like float it reports 8, when the actual packing is 4. Likewise, for Vector128<>, the packing of 16 is preserved by the VM. Vector<T> is another case where it currently reports Pack = 8, but Size = 0.

@steveharter
Copy link
Member

Propose moving to future; not in scope for 3.0 unless someone picks this up.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@steveharter steveharter added bug and removed untriaged New issue has not been triaged by the area owner labels Mar 19, 2020
@steveharter steveharter modified the milestones: Future, 5.0 Mar 19, 2020
@steveharter
Copy link
Member

Moving to future based on schedule + priority.

@buyaa-n
Copy link
Member

buyaa-n commented Feb 8, 2022

I would expect that this API either return the exact values in metadata or it should return the correct values computed by the VM at runtime.

Isn't that will be a breaking change? Also I wonder why it is written this way frist place, tagging @jkotas as he might know more about this code

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 8, 2022
@jkotas
Copy link
Member

jkotas commented Feb 8, 2022

Also I wonder why it is written this way first place

The comment explains it: The developer writing this code thought that changing the packing from 0 to 8 is better for some reason. FWIW: This code was checked in on 2003/1/14.

Isn't that will be a breaking change?

Every bug fix is technically a breaking change.

I think it would be fine to delete the adjustment and just return the raw value in metadata. If you are going to fix this, make sure to fix it in all places:

src\coreclr\nativeaot\System.Private.Reflection.Core\src\System\Reflection\Runtime\TypeInfos\RuntimeNamedTypeInfo.cs(163):// Metadata parameter checking should not have allowed 0 for packing size.
src\coreclr\System.Private.CoreLib\src\System\Reflection\RuntimeCustomAttributeData.cs(1876):// Metadata parameter checking should not have allowed 0 for packing size.
src\libraries\System.Reflection.MetadataLoadContext\src\System\Reflection\TypeLoading\Types\RoDefinitionType.cs(173):// Metadata parameter checking should not have allowed 0 for packing size.
src\mono\System.Private.CoreLib\src\System\RuntimeType.Mono.cs(2348):// Metadata parameter checking should not have allowed 0 for packing size.

jkotas added a commit to jkotas/runtime that referenced this issue Feb 8, 2022
This adjustment was trying to minic adjustment performed by type loader, but the logic has been out-of-sync with the type loader evolution for years.
It does not make sense for custom attribute reading via reflection to perform this adjustment.

Fixes dotnet#12480
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2022
jkotas added a commit that referenced this issue Feb 8, 2022
This adjustment was trying to minic adjustment performed by type loader, but the logic has been out-of-sync with the type loader evolution for years.
It does not make sense for custom attribute reading via reflection to perform this adjustment.

Fixes #12480
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection bug needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants