Runtime: Test failed: System.Runtime.Serialization.Formatters.Tests.EqualityExtensions.IsEqual

[VincentBu]

Job:
https://mc.dot.net/#/user/coreclr-corefx-jitstressregs/ci~2Fdotnet~2Fcoreclr~2Frefs~2Fheads~2Fmaster/test~2Ffunctional~2Fcorefx~2F/20190627.1/workItem/System.Runtime.Serialization.Formatters.Tests/analysis/xunit/System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests~2FRoundtripManyObjectsInOneStream

Failed tests:
System.Runtime.Serialization.Formatters.Tests.EqualityExtensions.IsEqual

Log:

Assert.Equal() Failure
Expected: 103469184
Actual:   103439960

[RussKeldorph]

JitStressRegs=0x1000

[CarolEidt]

This passes with commit https://github.com/dotnet/coreclr/commit/eacbd6238ce38c2920b27978e56813d3469232d1, but fails with the immediately preceding commit. This commit updated corefx.

So, between corefx commit https://github.com/dotnet/corefx/commit/219b67118e3a2d539afc1ff3bb2c10e1bc7b03d2 and https://github.com/dotnet/corefx/commit/6d723b8e5ae3129c0a94252292322fc19673478f, something was changed in corefx that either

• fixed a latent bug that was exposed by this run, OR
• changed the code such that a latent JIT bug was masked

More analysis is needed to determine which is the case.

[CarolEidt]

I was mistaken - I was apparently seeing failures due to mismatched coreclr & corefx versions. I am unable to repro this failure, though the data indicates that this test continues to fail (including in today's run).
I've tried building with the run-corefx-tests.py script as well as dowloading from https://dotnetfeed.blob.core.windows.net/dotnet-core/corefx-tests/4.6.0-preview8.19352.11/Linux.arm/netcoreapp/tests/AnyOS.AnyCPU.Release/System.Runtime.Serialization.Formatters.Tests.zip.

[BruceForstall]

Today's build shows 14 similar failures.

The "Infrastructure Logs" shows more detail, IMO, than the summary logs:

https://mc.dot.net/#/user/coreclr-corefx-jitstressregs/ci~2Fdotnet~2Fcoreclr~2Frefs~2Fheads~2Fmaster/test~2Ffunctional~2Fcorefx~2F/20190708.1/workItem/System.Runtime.Serialization.Formatters.Tests/wilogs

Select "Ubuntu.1804.Arm32.Open-arm:Checked-jitstressregs0x1000"

[BruceForstall]

The JitStress=2 + JitStressRegs=0x1000 variations don't show any failures

[CarolEidt]

This is very likely related to dotnet/coreclr#24703. In fact, backing that out fixes this bug. I'm moving the milestone to 3.next and will continue to investigate. My working assumption at this point is that I've missed some assumption somewhere that expects one or more of the incoming registers to either remain in a register in the prolog, or to have a dedicated home either on-stack or in a register (as in dotnet/runtime#12554).

[BruceForstall]

dotnet/coreclr#24703 Only has a single line of code change that is not under JIT32_GCENCODER:

                // Under stress mode, don't attempt to allocate a reg to
                // reg optional ref position, unless it's a ParamDef.
-                if (allocateReg && regOptionalNoAlloc() && (currentRefPosition->refType != RefTypeParamDef))
+                if (allocateReg && regOptionalNoAlloc())
                {
                    allocateReg = false;
                }

and JIT32_GCENCODER is not defined for arm32. So are you saying it's this line of code that is at fault?

(btw, shouldn't the ", unless it's a ParamDef." part of the comment be removed?)

[CarolEidt]

So are you saying it's this line of code that is at fault?

Sort of - dotnet/coreclr#24703 was caused by making ParamDefs RegOptional in dotnet/coreclr#24043, which in theory they are. However, on x86 this exposed an issue with allowing the this pointer to have multiple homes, which was fixed by the rest of dotnet/coreclr#24703. This failure is fixed by eliminating the ParamDef case for forcing RegOptional in the regOptionalNoAlloc (jitstressregs 0x1000) case (i.e. reverting that one line). So I'm presuming that there's some implicit assumption somewhere that is similarly being violated.

shouldn't the ", unless it's a ParamDef." part of the comment be removed?

Yes, I'd also noticed that. I'll remove that when I find the fix for this.

[CarolEidt]

The bug is an uninitialized variable sent by reference from gdip_load_png_properties in libgdiplus to png_get_pHYs in libpng. It seems that the version of libgdiplus that is being used on arm predates this commit: https://github.com/mono/libgdiplus/commit/81e45a1d5a3ac3cf035bcc3fabb2859818b6cc04 which fixed these uninitialized variables.

Changing the parameters that get registers, and thereby changing the size of the stacks, causes the uninitialized variables to get different values, thus the fact that this only fails with jitStressRegs0x1000.

@akoeplinger - I repro'd this on my Ubuntu 18.04 pi by installing libgdiplus with a simple sudo apt-get install libgdiplus, and I would assume that's how the CI machines are setup as well, but this installs a libgdiplus.so dated 19 Feb 13 2017. What would be the best way to make a more current version (with the fixes you introduced in the above commit) available?

[danmosemsft]

Thanks for the investigation @caroleidt. Cc @safern

[safern]

Thanks for the investigation @CarolEidt

I repro'd this on my Ubuntu 18.04 pi by installing libgdiplus with a simple sudo apt-get install libgdiplus, and I would assume that's how the CI machines are setup as well, but this installs a libgdiplus.so dated 19 Feb 13 2017. What would be the best way to make a more current version (with the fixes you introduced in the above commit) available?

cc: @directhex who can also answer this question.

[directhex]

What is the package version of libgdiplus under discussion?

[CarolEidt]

I'm not sure how to identify the package version. I installed this on a raspberry pi running Ubuntu 18.04, with a simple sudo apt-get install libgdiplus, and it's dated Feb of 2017. Walking through the assembly clearly shows that the initializations here: https://github.com/mono/libgdiplus/blob/master/src/pngcodec.c#L118-L120 aren't happening, and indeed they were added by @akoeplinger in this commit: https://github.com/mono/libgdiplus/commit/81e45a1d5a3ac3cf035bcc3fabb2859818b6cc04#diff-c96a8261ecb168c12b44248208da21c0

[directhex]

apt-cache policy libgdiplus will tell you the installed version and all available versions in configured repositories

[CarolEidt]

Here's what it says:

libgdiplus:
  Installed: 4.2-2
  Candidate: 4.2-2
  Version table:
 *** 4.2-2 500
        500 http://ports.ubuntu.com/ubuntu-ports bionic/universe armhf Packages
        100 /var/lib/dpkg/status

[directhex]

If you want newer than is available via the Ubuntu repositories, add the repositories on https://www.mono-project.com/download/stable/

[CarolEidt]

@directhex - I have updated libgdiplus, but it still has the uninitialized variable, i.e. it doesn't seem to have the commit I referenced above. I confirmed that unit_type already has the value 1 when its address is passed to png_get_pHYs on this line: https://github.com/mono/libgdiplus/blob/master/src/pngcodec.c#L121.

apt-cache policy libgdiplus now gives:

libgdiplus:
  Installed: 5.6.1-0xamarin6+ubuntu1804b1
  Candidate: 5.6.1-0xamarin6+ubuntu1804b1
  Version table:
 *** 5.6.1-0xamarin6+ubuntu1804b1 500
        500 https://download.mono-project.com/repo/ubuntu stable-bionic/main armhf Packages
        100 /var/lib/dpkg/status
     4.2-2 500
        500 http://ports.ubuntu.com/ubuntu-ports bionic/universe armhf Packages

I don't know how to translate that back to which commit it corresponds to, but it still has the issue.

[CarolEidt]

Note that it takes a very specific scenario for this bug to be exposed. The uninitialized stack location for unit_type must be exactly 1.
Before this bug just disappears due to unrelated changes in code generation that would cause the stack to grow or shrink (and thus change the uninitialized value), I'd like to see us get a new version that we can validate does not have the bug, so then we can get it into our CI system so we don't run into this painful-to-debug situation again.

[CarolEidt]

@danmosemsft - @RussKeldorph suggested that you might be able to help shepherd this along. Long story short, We need a version of libgdiplus that has at least this commit: mono/libgdiplus@81e45a1#diff-c96a8261ecb168c12b44248208da21c0 which fixed some uninitialized variables.

[CarolEidt]

Also, I have a branch https://github.com/CarolEidt/coreclr/tree/Fix25468 that includes a smaller test case that demonstrates the failure when COMPlus_TieredCompilation=0 and COMPlus_JitStressRegs=0x1000 are set in a Checked build of the jit.

[danmosemsft]

@directhex @akoeplinger can you give more pointers? I think @CarolEidt is asking

1. Is there a stable version of mono that contains the commit above (sounds like it's not https://www.mono-project.com/download/stable/) that she can try with.
2. How to get libgdiplus updated in the official feed(s) to also have this fix.

@CarolEidt I would assume you could also build libgdiplus, if you want to give that a try.

cc @safern

[CarolEidt]

I'm not sure that building libgdiplus would be a productive use of my time. I wasn't able to quickly discern how to build for armhf, and even if I did, I'm not sure that's the appropriate way to come up with a version to use for the CI system - not to mention that we should be testing with libraries that are available to our users, given that it is used by System.Drawing.Common.

[safern]

with a version to use for the CI system - not to mention that we should be testing with libraries that are available to our users, given that it is used by System.Drawing.Common.

I totally agree on that, it is always really painful to have all our CI machines to have the "latest" version of libgdiplus because the different flavors, and usually the way the Helix sets the test machines up is with a startup script that just installs packages using the default package manager with the default repositories for that OS.

I think libgdiplus should be part of the official/default feeds for the supported OSs, as that is the most common way our customers get this dependency into their system (if not already installed by the OS).

[danmosemsft]

I think libgdiplus should be part of the official/default feeds for the supported OSs, as that is the most common way our customers get this dependency into their system (if not already installed by the OS).

@directhex do you know who drives libgdiplus updates into the distro feeds?

@safern assuming that would take a long time to happen, is this just another CI image that we need to get updated?

[directhex]

We can cut a release of libgdiplus basically whenever we feel like it (typically @akoeplinger drives that), and I can push into Debian/Ubuntu, albeit that only ships in the next stable version

[safern]

@safern assuming that would take a long time to happen, is this just another CI image that we need to get updated?

Yes I guess I will need to work with the infra team in order to get libgdiplus installed from the mono sources, however, it seems that doesn't fix this issue, per: https://github.com/dotnet/coreclr/issues/25468#issuecomment-515154306

cc: @MattGal is that possible?

We can cut a release of libgdiplus basically whenever we feel like it (typically @akoeplinger drives that), and I can push into Debian/Ubuntu, albeit that only ships in the next stable version

That'd be great.

[MattGal]

@safern / @danmosemsft I'm not 100% sure I understand the question I've been CC'ed to answer, but if we're talking test execution, there's basically three things we need to do to get changes everywhere:

• If the OS that needs LibGDIPlus is docker-based (or we want to make it so) - can just update the dockerfile and make a PR to pick up the changed image.
• If it's a VM in our Azure VM Scale Sets - We need to update the way we install libgdiplus via PR to these machines and roll it out when possible (lately we've been blocked on rollouts with lockdowns followed by only 2-3 days until the next lockdown)- this may take a week or two.
• If it's an on-premises machine: The process is similar to the scale set version with the option to manually update the machines as a stopgap while waiting for rolling out.

Ping me on teams to chat further.

[danmosemsft]

@directhex is it possible for you to reach out to @akoeplinger with that request? Not sure whether he looks at this repo.

[directhex]

We have a backlog of libgdiplus PRs we want to look at first, but it's on the radar

[danmosemsft]

OK, thanks @directhex. I don't suppose there's an issue to track for that?

[directhex]

Current plan is to ship a release tomorrow, after running the current state of master through static analysis and fuzzing. It's a big release.

directhex@breakfast:~/Projects/libgdiplus$ git diff 5.6.1..master | diffstat | tail -1
 180 files changed, 74237 insertions(+), 16271 deletions(-)

[directhex]

Okay 6.0.1 is out, and published on the mono-project.com preview repos. It's _not_ in the stable repos, and probably won't be for a while. Give it a try, see if it fixes the bug you're experiencing.

[CarolEidt]

I've verified that 6.0.1-0xamarin1+ubuntu1804b1 fixes the issue, both for my extracted test case as well as for the corefx test with COMPlus_TieredCompilation=0 and COMPlus_JitStressRegs=0x1000.
The original failure was in a corefx test run that uses a docker container, but we should ensure that libgdiplus is updated wherever we're using it. In the near term this only impacts arm32, but a change in register allocation or code generation could cause this to fail on any platform, since it is an uninitialized variable bug.
@MattGal can you help me update at least our arm32 containers and/or other arm32 VMs or physical systems used in CI?

[CarolEidt]

The new libgdiplus has been included in the docker images, and I've also added a coreclr test to repro this.