-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Classify type with empty base as non-blittable #46769
Classify type with empty base as non-blittable #46769
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to step through all of this in a debugger because I'm not sure what the logic should be. Layout-related things always require extra care. I don't have time/setup for that right now unfortunately (it's also why I just filed the issue instead of just fixing this :)).
Cc @dotnet/crossgen-contrib if someone has time to review this earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@am11, thank you for looking into this. Please look at my comments and respond. Once you've done that I'll have a larger test pass run on this.
src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/MarshalUtilsTests.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/MarshalUtilsTests.cs
Show resolved
Hide resolved
@am11 I see the new IsZeroSized bool property, but the implementation is quite confusing and feels like it is possibly fragile. Could you add a test that IsZeroSized returns false/true in a few interesting cases? In particular, if you could test the behavior of that api with both structs and classes that are empty/have a single field? |
@davidwrighton, admittedly, I am not an expert on crossgen2; but to my understanding, size calculation without padding: I have added a comment and renamed the property to |
@am11 Thank you for indulging me on adding these various tests and the refactorings. IMO the way you are checking for empty in the property isn't a behavior that I believe we are certain to keep for all of time; however, I agree the behavior is correct at this time, and the tests are in place to ensure we don't break this behavior in the future (and if we do, all we'll have to do is fix the one property implementation). |
@am11 Thank you for your contribution. We really appreciate it. |
Attempt to fix #46738.