Interpreter: volatile ldobj appears to have incorrect semantics?

[alexrp]

Going by Ecma 335 III.4.13, it is my understanding that a sequence like

volatile.
ldobj MyType

should produce a volatile load. While one could argue that it's a store (the spec says "copy the value stored at address src to the stack"), interpreting it as a load seems more appropriate if one considers that the spec also says "if typeTok is not a generic parameter and either a reference type or a built-in value class, then the ldind instruction provides a shorthand for the ldobj instruction".

The relevant code:

• https://github.com/dotnet/coreclr/blob/cbf46fb0b6a0b209ed1caf4a680910b383e68cba/src/vm/interpreter.cpp#L5537
• https://github.com/dotnet/coreclr/blob/cbf46fb0b6a0b209ed1caf4a680910b383e68cba/src/vm/interpreter.cpp#L4292

category:correctness
theme:interpreter
skill-level:intermediate
cost:small

[mikedn]

But what is the issue here? That code you link to is not enabled in CoreCLR and AFAIK it's not enabled in the desktop version either.

[jkotas]

The interpreter is not a production quality code - it is not enabled in shipping bits. It is only meant to be used for initial bring up of new platforms, until the JIT starts working. If you would like to play with it or fix bugs in it, it can be enabled in src\inc\switches.h.

[cmckinsey]

Alexrp, we are still interested in understanding if there is a potential bug there? Is the bug concerned with the atomicity of the load operation or the memory order aspects? Thanks.

[kangaroo]

@alexrp Is this still an issue? Its been over a month. Can we close this out?

[alexrp]

There is certainly a bug in any case. The issue is that for a load-acquire, the barrier must come after the load. Conversely, for a store-release, the barrier must come before the store. The interpreter gets this the wrong way around for stobj/ldobj and some other instructions I forgot (but it's easy enough to spot if you look for BarrierIfVolatile).

There is no bug with regards to atomicity. The issue is just the placement of the memory barriers.

Should the bug be fixed? Well, if the code is not really used, it might not be worth the effort. On the other hand, it can be quite misleading to people who come across this code and think that it's a good reference to base their work on. So, I don't know. It's up to the maintainers.

[kangaroo]

@alexrp I agree, but it seems the interp is not really maintained to the point of following the spec.

@jkotas Are we going to take further action here, or just close this out with the fact that the interp is more meant for bring-up rather than full functional correctness?

[alexrp]

If you do close this, please at the very least put a reasonably visible comment in the interpreter making it clear that it isn't spec-compliant and is only meant for bring-up. It's not really obvious that that's the case.

[kangaroo]

@alexrp Agreed -- a comment makes sense if thats the final stance.

[cmckinsey]

@alexrp thanks. Yes you're correct. This code is not used in production however and I don't believe there would be any issue on an Intel based architecture due to the strongly ordered memory architecture. ARM could have a problem however.

We should still fix the bug so let's leave this active.