C++ and inserting loads after null-checks

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
Hi,

This is the question about C++ [pre-11] memory model. Hans?

Suppose you have this code under the concurrent update to "*p":

 int m(int** p) {
   int* t = *p;
   if (t != NULL) {
     return *t; // <--- can it SEGV on NULL here?
   }
   return 0;
 }

It certainly cannot NPE in the similar code in Java. But the arguments in the recent bug reports [1]
and bug fixes [2][3] seem to imply it would be legal for C++ compiler to reload *p after the
null-check in the example above, and thus be exposed to the concurrent write of NULL.

Is this a known thing in C++ MM? (Please say "no").
Or is the "legality" interpretation in [1][2][3] wrong? (Please say "yes").

-Aleksey

[1] https://bugs.openjdk.java.net/browse/JDK-8129440
[2] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-December/020994.html
[3] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013924.html


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
On 14/03/18 18:24, Aleksey Shipilev via Concurrency-interest wrote:
> This is the question about C++ [pre-11] memory model. Hans?

Is there one?

> Suppose you have this code under the concurrent update to "*p":
>
>  int m(int** p) {
>    int* t = *p;
>    if (t != NULL) {
>      return *t; // <--- can it SEGV on NULL here?
>    }
>    return 0;
>  }
>
> It certainly cannot NPE in the similar code in Java. But the arguments in the recent bug reports [1]
> and bug fixes [2][3] seem to imply it would be legal for C++ compiler to reload *p after the
> null-check in the example above, and thus be exposed to the concurrent write of NULL.
>
> Is this a known thing in C++ MM? (Please say "no").
> Or is the "legality" interpretation in [1][2][3] wrong? (Please say "yes").

You've got a data race on *p.  Undefined behaviour: do not pass Go, do
not collect $200.  More generally, a C++ compiler is entitled to
assume that no other threads exist unless told otherwise by
atomic fences.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
Pre-11, C++ was a single-threaded language.

Post-11, there is a data race on the access to *p, so this generates undefined behavior. So yes, this is allowed.

As a practical matter, I think gcc always allowed this, though it rarely generated code prone to this.

The load from *p should be from an atomic. The correct way to do this is probably to use atomic_view<>, which doesn't really exist yet. A hack, that's not absolutely guaranteed to work, is to cast to an atomic before loading.

On Wed, Mar 14, 2018 at 11:24 AM, Aleksey Shipilev <[hidden email]> wrote:
Hi,

This is the question about C++ [pre-11] memory model. Hans?

Suppose you have this code under the concurrent update to "*p":

 int m(int** p) {
   int* t = *p;
   if (t != NULL) {
     return *t; // <--- can it SEGV on NULL here?
   }
   return 0;
 }

It certainly cannot NPE in the similar code in Java. But the arguments in the recent bug reports [1]
and bug fixes [2][3] seem to imply it would be legal for C++ compiler to reload *p after the
null-check in the example above, and thus be exposed to the concurrent write of NULL.

Is this a known thing in C++ MM? (Please say "no").
Or is the "legality" interpretation in [1][2][3] wrong? (Please say "yes").

-Aleksey

[1] https://bugs.openjdk.java.net/browse/JDK-8129440
[2] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-December/020994.html
[3] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013924.html



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
On 03/14/2018 07:43 PM, Hans Boehm wrote:
> Pre-11, C++ was a single-threaded language.
>
> Post-11, there is a data race on the access to *p, so this generates undefined behavior. So yes,
> this is allowed.

Okay, thanks. I was afraid of that. So with UB on our hands, it does not even matter we have a local
variable that we have checked for NULL, right?

> As a practical matter, I think gcc always allowed this, though it rarely generated code prone to this.
>
> The load from *p should be from an atomic. The correct way to do this is probably to use
> atomic_view<>, which doesn't really exist yet. A hack, that's not absolutely guaranteed to work, is
> to cast to an atomic before loading.

For the practical matter, in pre-11 world, what would you do? The fixes in [2][3] do "volatile",
which seems to be wishful thinking as well.

Thanks,
-Aleksey

>     [1] https://bugs.openjdk.java.net/browse/JDK-8129440
>     [2] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-December/020994.html
>     [3] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013924.html



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
On 03/14/2018 07:38 PM, Andrew Haley wrote:
> On 14/03/18 18:24, Aleksey Shipilev via Concurrency-interest wrote:
>> Is this a known thing in C++ MM? (Please say "no").
>> Or is the "legality" interpretation in [1][2][3] wrong? (Please say "yes").
>
> You've got a data race on *p.  Undefined behaviour: do not pass Go, do
> not collect $200.  More generally, a C++ compiler is entitled to
> assume that no other threads exist unless told otherwise by
> atomic fences.

Yes-yes, I have successfully suppressed the memory that in C++ the data race is UB.
Hard to unlearn this bit from Java, where data races can be benign.

-Aleksey



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
I would expect that making the load of *p volatile would indeed solve the problem in practice. The semantics of C++ volatiles are not crystal clear, but it is reasonably clear that the compiler should not reload one when the source doesn't ask it to.

This of course isn't guaranteed to work, especially since C and C++ volatile doesn't remove the data race, and it's conceivable that the load may not be atomic on some really old processors, but I think on modern processors it will generally do the right thing. Assuming you need no memory ordering, at least.

On Wed, Mar 14, 2018 at 11:48 AM, Aleksey Shipilev <[hidden email]> wrote:
On 03/14/2018 07:43 PM, Hans Boehm wrote:
> Pre-11, C++ was a single-threaded language.
>
> Post-11, there is a data race on the access to *p, so this generates undefined behavior. So yes,
> this is allowed.

Okay, thanks. I was afraid of that. So with UB on our hands, it does not even matter we have a local
variable that we have checked for NULL, right?

> As a practical matter, I think gcc always allowed this, though it rarely generated code prone to this.
>
> The load from *p should be from an atomic. The correct way to do this is probably to use
> atomic_view<>, which doesn't really exist yet. A hack, that's not absolutely guaranteed to work, is
> to cast to an atomic before loading.

For the practical matter, in pre-11 world, what would you do? The fixes in [2][3] do "volatile",
which seems to be wishful thinking as well.

Thanks,


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: C++ and inserting loads after null-checks

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
On 14/03/18 19:27, Aleksey Shipilev wrote:

> On 03/14/2018 07:38 PM, Andrew Haley wrote:
>> On 14/03/18 18:24, Aleksey Shipilev via Concurrency-interest wrote:
>>> Is this a known thing in C++ MM? (Please say "no").
>>> Or is the "legality" interpretation in [1][2][3] wrong? (Please say "yes").
>>
>> You've got a data race on *p.  Undefined behaviour: do not pass Go, do
>> not collect $200.  More generally, a C++ compiler is entitled to
>> assume that no other threads exist unless told otherwise by
>> atomic fences.
>
> Yes-yes, I have successfully suppressed the memory that in C++ the data race is UB.
> Hard to unlearn this bit from Java, where data races can be benign.

The easiest way to think about this is that "ordinary" Java memory
accesses are sort-of equivalent to C++'s memory_order_relaxed atomic
accesses.  It's not quite a perfect analogy, but it's close enough for
most purposes.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest