Interpreter: volatile ldobj appears to have incorrect semantics? #34

Open
alexrp opened this Issue Feb 3, 2015 · 9 comments

Comments

Projects
None yet
8 participants
@alexrp

alexrp commented Feb 3, 2015

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:

@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Feb 4, 2015

Contributor

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.

Contributor

mikedn commented Feb 4, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Feb 4, 2015

Member

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.

Member

jkotas commented Feb 4, 2015

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

This comment has been minimized.

Show comment
Hide comment
@cmckinsey

cmckinsey Feb 5, 2015

Contributor

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.

Contributor

cmckinsey commented Feb 5, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kangaroo

kangaroo Mar 14, 2015

Contributor

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

Contributor

kangaroo commented Mar 14, 2015

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

@alexrp

This comment has been minimized.

Show comment
Hide comment
@alexrp

alexrp Mar 14, 2015

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.

alexrp commented Mar 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kangaroo

kangaroo Mar 14, 2015

Contributor

@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?

Contributor

kangaroo commented Mar 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@alexrp

alexrp Mar 14, 2015

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.

alexrp commented Mar 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kangaroo

kangaroo Mar 14, 2015

Contributor

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

Contributor

kangaroo commented Mar 14, 2015

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

@cmckinsey

This comment has been minimized.

Show comment
Hide comment
@cmckinsey

cmckinsey Mar 20, 2015

Contributor

@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.

Contributor

cmckinsey commented Mar 20, 2015

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment