Fixes for big-endian systems #6204
Conversation
* In MSBuildNameIgnoreCaseComparer::GetHashCode, when reading pairs of characters aliased to an int, handle the single last remaing char correctly on big-endian systems. * In NodeProviderOutOfProcBase, byte-swap the packet length on big-endian systems.
|
Thanks for the review! One additional comment on the NodeProviderOutOfProcBase change (which I just noticed as I was trying to backport the changes to older releases): The packet length problem is actually quite recent, it was introduced only 4 weeks ago by this commit by @KirillOsenkov Before that patch the packet length used to be in native byte order. The patch changed the sender to write the packet length always in little-endian byte order (using the BinaryWriter which always uses little-endian, and/or the new WriteInt32 routine. However, the receiver side was not changed, which caused the mismatch. Now, once this PR is applied, that mismatch will be fixed again. However, I'm now wondering if that switch in encoding was even intentional in the first place. Could there ever be the situation where an "old" version of msbuild (using native-endian encoding) would have to communicate with a "new" version (using little-endian endian encoding)? That might still be a problem then. |
* Reduce byte array allocations when reading/writing packets Byte arrays are a major source of LOH allocations when streaming logging events across nodes. Allocating a large MemoryStream once and then growing it as needed almost completely removes allocations for byte arrays. This should significantly improve memory traffic during large builds. * Write the last part of the packet synchronously on Mono. If we use WriteAsync and don't await it, then subsequent WriteAsync may be called before the first continuation returns. If both calls share the same buffer and they overlap, we will overwrite the data in the buffer and cause junk to arrive at receiver. * Make SendData write packets asynchronously. This avoids blocking the main loop. Roughly equivalent to writing the packet asynchronously using fire-and-forget (BeginWrite).
|
MSBuild never communicates cross-version and the only part of the protocol we have to be careful about is the handshake where we check that the version (plus a few other things) matches. So no concerns here, we don't have to be backward or forward compatible when it comes to the actual packet exchange. |
|
OK, then this patch should be all that is required to fix it again. Thanks! |
|
Thanks, I didn't pay attention to endianness when I made the change. It was driven purely by the need to reduce allocations since BitConverter required a 4-byte array. In general I'll need to learn more about endianness and pay more attention to it going forward, so this is educational. |
Forgind
added
the
merge-when-branch-open
Context
We are currently working on bringing up .NET on the IBM Z architecture (s390x-ibm-linux), which uses big-endian byte order. During that process we have run into a couple of endian-specific bugs in msbuild code.
Changes Made
In MSBuildNameIgnoreCaseComparer::GetHashCode, when reading pairs of characters aliased to an int, handle the single last remaining char correctly on big-endian systems.
In NodeProviderOutOfProcBase, byte-swap the packet length on big-endian systems.
Testing
On IBM Z, a msbuild package built with this patch successfully builds and executes a "hello world" program. We were also able to successfully bootstrap msbuild (build it with itself) on IBM Z. (This of course also requires the work-in-progress dotnet runtime port, and for the bootstrap an endian bugfix to nuget is also required.)
Also verified that "./build.sh --test" still succeeds on a little-endian (Intel) Linux system.