Manually backporting #74599 to 7.0 for RC2.

[StephenMolloy]

Backport of #74599 to release/7.0 for RC2

Customer Impact

DataContractSerializer has not been properly accounting for big-or-little endian-ness when writing a few types "to the wire." This has not previously been an issue, since to my knowledge there have not been any commonly used big-endian platforms with .Net. DCS writes little-endian on the wire, so not accounting for endian-ness has simply worked because little-endian platforms write little-endian on the wire as expected. IBM helped bring .Net to their big-endian s360 platform in 6.0 however, causing failures when serializing between different platforms.

Testing

Tests were added to check the correct little-endian output for affected types and arrays of types.

Regression

No... but kind of yes. Strictly speaking, the code has always missed this -endian handling for these types and was not regressed. But the introduction of big-endian implementations of dotnet in 6.0 has exposed this issue. So in a sense, DCS was fine in 5.0 and now is not correct on some platforms in 6.0 and later.

Risk

Low. The change is faithful to current execution for little-endian platforms and only does different things on a big-endian platform... which is currently broken without this fix.

[carlossanlop]

@StephenMolloy @mconnew @HongGit can one of you provide a code review sign off?
@HeathAr (or @danmoseley, since this is libraries) we need an M2 approval.

[danmoseley]

@stephentoub would you be available to have a quick extra review?

Assuming that's OK, approved, this seems like it's close to the bar we'd use for servicing, as it makes this feature broken on this arch.

[stephentoub]

I think it'd be better for the BitConverter.IsLittleEndian check to be first in all of these cases, e.g.

if (BitConverter.IsLittleEndian && IsStartArray(localName, namespaceUri, XmlBinaryNodeType.FloatTextWithEndElement))

If IsStartArray isn't inlineable, the JIT needs to assume it might have side effects that can't be elided, and so to preserve order of operations, in the big endian case the JIT would need to compile this as:

IsStartArray(localName, namespaceUri, XmlBinaryNodeType.FloatTextWithEndElement);
return base.ReadArray(localName, namespaceUri, array, offset, count);

But with the check first, when it's false the second clause would never be evaluated, so the JIT can compile it just as:

return base.ReadArray(localName, namespaceUri, array, offset, count);

[StephenMolloy]

I believe the author was just following the pattern we had established on other types like short/int/long where this check is done second. I don't feel strongly about this, but my inclination would be to leave the order as is for 7.0 and we can invert the check for all types in 8.0 where we have more runway. If you feel strongly about this though, I wouldn't object to making the change here.

[stephentoub]

We should fix it in main. I don't feel strongly about it for release/7.0, since to my knowledge we don't actually ship supported S390X bits from that branch. I am curious, though, where we have existing BitConverter.IsLittleEndian checks at the end of a clause; those should be fixed, too.

[carlossanlop]

@StephenMolloy Don't forget to also copy the changes from this PR into main.

[StephenMolloy]

@StephenMolloy Don't forget to also copy the changes from this PR into main.

Done in #75379

[StephenMolloy]

@stephentoub - Does this look good with the updates pushed on Friday?

[carlossanlop]

Test failures are unrelated: Timeouts in build during trimming process. in Build MacCatalyst x64 Release AllSubsets_Mono and Build tvOS arm64 Release AllSubsets_Mono.
Approved and signed off. Ready to merge.

[StephenMolloy]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/3055795949

[github-actions]

@StephenMolloy backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Manually backporting #74599 to 7.0 for RC2.
Using index info to reconstruct a base tree...
M	src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryReader.cs
M	src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryWriter.cs
M	src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBufferReader.cs
M	src/libraries/System.Runtime.Serialization.Xml/tests/XmlDictionaryReaderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Runtime.Serialization.Xml/tests/XmlDictionaryReaderTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Runtime.Serialization.Xml/tests/XmlDictionaryReaderTests.cs
Auto-merging src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBufferReader.cs
CONFLICT (content): Merge conflict in src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBufferReader.cs
Auto-merging src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryWriter.cs
CONFLICT (content): Merge conflict in src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryWriter.cs
Auto-merging src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryReader.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Manually backporting #74599 to 7.0 for RC2.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!