[NinoFloris]

Fixes #136936, a longstanding bug that was kept around as a quirk for zero-diffs at the time. Disasm tests here are a bit fragile, though arm has stabilized its instructions around this now, but I'd rather have them than not.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jakobbotsch]

Disasm tests here are a bit fragile, though arm has stabilized its instructions around this now, but I'd rather have them than not.

I don't agree. These tests are much too fragile, relying on exact promotion heuristics that are going to be changing in this release. I don't think the test is needed.

I would be ok with outerloop test that validates that a volatile load was not dropped by testing for a race.

[jakobbotsch]

a longstanding bug that was kept around as a quirk for zero-diffs at the time

FWIW, this used to have quite significant diffs around structs that were later proved to be not exposed. Looks like something changed since the diffs are very minimal now, however.

[NinoFloris]

Disasm tests here are a bit fragile, though arm has stabilized its instructions around this now, but I'd rather have them than not.

I don't agree. These tests are much too fragile, relying on exact promotion heuristics that are going to be changing in this release. I don't think the test is needed.

I would be ok with outerloop test that validates that a volatile load was not dropped by testing for a race.

The checks count loads and pass whether or not the struct is promoted. The only fragility I see here is the acquire instruction regex. Which heuristic do you think will break it?

I have also added an outerloop race test next to the volatile class field tests.

[jakobbotsch]

The checks count loads and pass whether or not the struct is promoted. The only fragility I see here is the acquire instruction regex. Which heuristic do you think will break it?

We do not have good experiences with disassembly tests for anything but validating the absence of instructions or exact expected instruction foldings. A few things that can go wrong in your patterns:

1. Registers that don't match your pattern (like r8d or any APX register)
2. The JIT emitting any load not covered by your pattern (e.g. movsxd or movzx if we widened the arithmetic)
3. Future IPA can prove facts about Publish whether or not it is marked NoInlining. I don't think it is likely we will get there, but actually native compilers do this.

I would rather see the behavior validated, whenever possible.

I have also added an outerloop race test next to the volatile class field tests.

Is it possible to write this test in a way where we can have a signal that we would have expected to see differing values?
The shape of this test does not look very appealing to me either.

I would personally suggest just not adding a test.

[NinoFloris]

Dropped the tests, under mild protest ;)

[jakobbotsch]

Would a test doing

int first = shared.Value;
int changed = shared.Changed;
int second = shared.Value;
if (changed > first && second < changed)
  Assert.Fail(...);

from one thread, and

for (int i = 0; i < int.MaxValue; i++)
{
  shared->Value = i;
  shared->Changed = i;
}

from another be able to show the failure? That is the kind of test I would be ok with adding.

[NinoFloris]

That shape does not catch the issue. Without the volatile flag the JIT hoists or reorders the reads around Changed, so all three variants I tried pass on main. The disagreement test I had earlier fails on main by timeout and passes with the fix in milliseconds.

[jakobbotsch]

Thanks

[jakobbotsch]

/ba-g Failure is #136758