Avoid depending on sizeof(T) for ExplicitLayout types due to Mono bug #23727
Conversation
|
We already have #22794 merged which addressed this issue for VS for Mac. Why do we need this one as well? Or should we undo the other fix? |
|
@jasonmalinowski Technically #22794 fails to meet the following condition from ECMA-335 §II.22.8 (ClassLayout):
The approach in this pull request adheres to the condition while still not relying on the CLI implementation to adhere to the specification with respect to explicitlayout types. |
|
Should we still leave the other fix in? That fix at least means the memory usage is smaller. I can imagine we're OK saying they complement each other. |
|
@jasonmalinowski My intent with this is to replace the other fix. The memory usage under 64-bit would be silently corrected in the future when Mono fixes the underlying bug. 📝 It's possible that even with the Mono correction memory usage doesn't change. Instead of padding |
|
Seems like a PR is up: https://github.com/mono/mono/pull/6223/files |
|
It will take some time for that PR to show up in xamarin releases. |
|
I'm super-happy to see the Mono PR (thanks @vargaz!), but the workaround will still be needed in the near future. Currently, OmniSharp requires Mono 5.2.0 or higher and moving that requirement to some future Mono version would be very disruptive for VS Code customers. |
sharwell
force-pushed
the
checksum-compat
branch
from
729e841
to
35a4b1d
Compare
|
Approved to merge via https://devdiv.visualstudio.com/DevDiv/_workitems/edit/536826 |
| @@ -123,6 +133,15 @@ public void WriteTo(ObjectWriter writer) | |||
| writer.WriteInt32(Data3); | |||
| } | |||
|
|
|||
| public static unsafe Sha1Hash FromPointer(Sha1Hash* hash) | |||
There was a problem hiding this comment.
Is there some way to use ref returns here? (not blocking on this PR, just curious)
There was a problem hiding this comment.
I'd be wary of this. The pointer is only guaranteed to be pointing at 20 bytes, which may be less than the size the VM assigns to Sha1Hash.
|
FYI: @jaredpar and I are looking at the ubuntu_16 failure. For some reason, it's picking up master rather than dev15.5.x. |
|
looks like it fixed itself. |
|
The infrastructure is afraid of me. Once I was mentioned it got scared and decided to behave nicely. |
Fixes #23722
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.