Make StructLayout work on Mono 64-bit #22794
Conversation
KirillOsenkov
changed the base branch from
features/vs-for-mac-packages
to
features/vs-for-mac-refactorings
|
Looks like this needs to be rebased? |
Need to work around a Mono 64 behavior where on 64-bit the size of the struct would be 24 and not 20 due to alignment (it ignores Size = 20, but Pack = 4 does it). Without this fix the ctor would throw because it would use sizeof(Sha1Hash) == 24.
KirillOsenkov
force-pushed
the
FixStructLayout
branch
from
88806f4
to
ace949b
Compare
|
Rebased. |
|
And this is a Mono runtime bug where we discuss whether the Mono runtime needs to be updated to match the CLR behavior here: https://bugzilla.xamarin.com/show_bug.cgi?id=60298 |
| /// struct would be 24 and not 20 due to alignment (it ignores Size = 20, but Pack = 4 does it). | ||
| /// Without this fix the ctor would throw because it would use sizeof(Sha1Hash) == 24. | ||
| /// </remarks> | ||
| [StructLayout(LayoutKind.Explicit, Size = 20, Pack = 4)] |
There was a problem hiding this comment.
Why does this even use StructLayout at all? Are we using this for blitting with something else? Can we update the doc to point to where that is being done?
There was a problem hiding this comment.
This is a question for @sharwell I presume. I would prefer to avoid digging too deep into this code since I don't understand it.
There was a problem hiding this comment.
@jasonmalinowski Considering this is a private nested type, the current comment and usage ~50 lines above seems to address the situation. It was certainly enough for Kirill to make the correct change in response to the Mono bug (coming in from never seeing it before).
There was a problem hiding this comment.
My motto is if somebody had to ask, your comments are never sufficient. 😄 Is the blitting above the only reason? If I just rewrote the ToString to not allocate the array entirely...could I remove this?
| /// struct would be 24 and not 20 due to alignment (it ignores Size = 20, but Pack = 4 does it). | ||
| /// Without this fix the ctor would throw because it would use sizeof(Sha1Hash) == 24. | ||
| /// </remarks> | ||
| [StructLayout(LayoutKind.Explicit, Size = 20, Pack = 4)] |
There was a problem hiding this comment.
@jasonmalinowski Considering this is a private nested type, the current comment and usage ~50 lines above seems to address the situation. It was certainly enough for Kirill to make the correct change in response to the Mono bug (coming in from never seeing it before).
|
The windows_debug_unit64_prtest failure is another instance of #21563. |
|
@dotnet-bot retest windows_debug_unit64_prtest please. |
|
@jasonmalinowski, @sharwell: we're seeing this same issue with Roslyn 2.6.0 on OmniSharp (which runs on Mono). Currently, OmniSharp is blocked taking Roslyn 2.6.0 and C# 7.2 because of this problem. Could we get this merged to master? |
|
💭 According to ECMA-335 §II.22.8 (ClassLayout), it seems that |
|
Looking at the discussion of the Mono bug that Kirill filed, it appears that there's some dispute as to whether it is a bug or not: https://bugzilla.xamarin.com/show_bug.cgi?id=60298. |
|
At the moment, this completely blocks C# 7.2 in VS Code. |
|
👍 Can you file a separate bug for this and assign it to me? I'm working on a solution. |
|
Issue submitted: #23722. |
Need to work around a Mono 64 behavior where on 64-bit the size of the struct would be 24 and not 20 due to alignment (it ignores Size = 20, but Pack = 4 does it). Without this fix the ctor would throw because it would use sizeof(Sha1Hash) == 24.
Customer scenario
When Roslyn is hosted in VS for Mac (64-bit Mono on Mac) reading metadata references crashes with this stack:
Workarounds, if any
None known
Risk
Low, fix verified
Performance impact
None
Is this a regression from a previous update?
Regression since June 2017
Root cause analysis:
Test Roslyn on Mono 64-bit
How was the bug found?
Adoption of Roslyn by VS for Mac team