RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

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

RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
Hi,

There is currently a bug when using unsafe accesses off-heap:

1) We write into a thread that we enable crash protection (using
GuardUnsafeAccess):
2) We perform the access
3) We write into a thread that we disable crash protection (using
~GuardUnsafeAccess)

The problem is that the crash protection stores are volatile, but the
actual access is non-volatile. Compilers have different interpretation
whether volatile - non-volatile accesses are allowed to reorder. MSVC is
known to interpret such interactions as-if the volatile accesses have
acquire/release semantics from the compiler point of view, and others
such as GCC are known to reorder away freely.

To prevent any issues, the accesses involved when using
GuardUnsafeAccess should be at least volatile.
This change makes the few remaining ones volatile. The JMM-volatile
(SEQ_CST) accesses with crash protection already have stronger ordering
than volatile and hence do not need changing.

By making the address passed in to the Access API volatile, the
MO_VOLATILE decorator is automatically set, which not surprisingly makes
the access volatile. Therefore, the solution is to simply make the
address passed in to Access volatile in this case.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8186787

Webrev:
http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

coleen.phillimore

I think this is a nice simple fix and should be pushed for JDK10.
Thanks,
Coleen

On 11/27/17 7:36 AM, Erik Österlund wrote:

> Hi,
>
> There is currently a bug when using unsafe accesses off-heap:
>
> 1) We write into a thread that we enable crash protection (using
> GuardUnsafeAccess):
> 2) We perform the access
> 3) We write into a thread that we disable crash protection (using
> ~GuardUnsafeAccess)
>
> The problem is that the crash protection stores are volatile, but the
> actual access is non-volatile. Compilers have different interpretation
> whether volatile - non-volatile accesses are allowed to reorder. MSVC
> is known to interpret such interactions as-if the volatile accesses
> have acquire/release semantics from the compiler point of view, and
> others such as GCC are known to reorder away freely.
>
> To prevent any issues, the accesses involved when using
> GuardUnsafeAccess should be at least volatile.
> This change makes the few remaining ones volatile. The JMM-volatile
> (SEQ_CST) accesses with crash protection already have stronger
> ordering than volatile and hence do not need changing.
>
> By making the address passed in to the Access API volatile, the
> MO_VOLATILE decorator is automatically set, which not surprisingly
> makes the access volatile. Therefore, the solution is to simply make
> the address passed in to Access volatile in this case.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8186787
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>
> Thanks,
> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

David Holmes
+1

David

On 28/11/2017 4:27 AM, [hidden email] wrote:

>
> I think this is a nice simple fix and should be pushed for JDK10.
> Thanks,
> Coleen
>
> On 11/27/17 7:36 AM, Erik Österlund wrote:
>> Hi,
>>
>> There is currently a bug when using unsafe accesses off-heap:
>>
>> 1) We write into a thread that we enable crash protection (using
>> GuardUnsafeAccess):
>> 2) We perform the access
>> 3) We write into a thread that we disable crash protection (using
>> ~GuardUnsafeAccess)
>>
>> The problem is that the crash protection stores are volatile, but the
>> actual access is non-volatile. Compilers have different interpretation
>> whether volatile - non-volatile accesses are allowed to reorder. MSVC
>> is known to interpret such interactions as-if the volatile accesses
>> have acquire/release semantics from the compiler point of view, and
>> others such as GCC are known to reorder away freely.
>>
>> To prevent any issues, the accesses involved when using
>> GuardUnsafeAccess should be at least volatile.
>> This change makes the few remaining ones volatile. The JMM-volatile
>> (SEQ_CST) accesses with crash protection already have stronger
>> ordering than volatile and hence do not need changing.
>>
>> By making the address passed in to the Access API volatile, the
>> MO_VOLATILE decorator is automatically set, which not surprisingly
>> makes the access volatile. Therefore, the solution is to simply make
>> the address passed in to Access volatile in this case.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>
>> Thanks,
>> /Erik
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
In reply to this post by coleen.phillimore
Hi Coleen,

On 2017-11-27 19:27, [hidden email] wrote:
>
> I think this is a nice simple fix and should be pushed for JDK10.

Thank you for the review.

/Erik

> Thanks,
> Coleen
>
> On 11/27/17 7:36 AM, Erik Österlund wrote:
>> Hi,
>>
>> There is currently a bug when using unsafe accesses off-heap:
>>
>> 1) We write into a thread that we enable crash protection (using
>> GuardUnsafeAccess):
>> 2) We perform the access
>> 3) We write into a thread that we disable crash protection (using
>> ~GuardUnsafeAccess)
>>
>> The problem is that the crash protection stores are volatile, but the
>> actual access is non-volatile. Compilers have different
>> interpretation whether volatile - non-volatile accesses are allowed
>> to reorder. MSVC is known to interpret such interactions as-if the
>> volatile accesses have acquire/release semantics from the compiler
>> point of view, and others such as GCC are known to reorder away freely.
>>
>> To prevent any issues, the accesses involved when using
>> GuardUnsafeAccess should be at least volatile.
>> This change makes the few remaining ones volatile. The JMM-volatile
>> (SEQ_CST) accesses with crash protection already have stronger
>> ordering than volatile and hence do not need changing.
>>
>> By making the address passed in to the Access API volatile, the
>> MO_VOLATILE decorator is automatically set, which not surprisingly
>> makes the access volatile. Therefore, the solution is to simply make
>> the address passed in to Access volatile in this case.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>
>> Thanks,
>> /Erik
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
In reply to this post by David Holmes
Hi David,

Thanks for the review.

/Erik

On 2017-11-28 07:41, David Holmes wrote:

> +1
>
> David
>
> On 28/11/2017 4:27 AM, [hidden email] wrote:
>>
>> I think this is a nice simple fix and should be pushed for JDK10.
>> Thanks,
>> Coleen
>>
>> On 11/27/17 7:36 AM, Erik Österlund wrote:
>>> Hi,
>>>
>>> There is currently a bug when using unsafe accesses off-heap:
>>>
>>> 1) We write into a thread that we enable crash protection (using
>>> GuardUnsafeAccess):
>>> 2) We perform the access
>>> 3) We write into a thread that we disable crash protection (using
>>> ~GuardUnsafeAccess)
>>>
>>> The problem is that the crash protection stores are volatile, but
>>> the actual access is non-volatile. Compilers have different
>>> interpretation whether volatile - non-volatile accesses are allowed
>>> to reorder. MSVC is known to interpret such interactions as-if the
>>> volatile accesses have acquire/release semantics from the compiler
>>> point of view, and others such as GCC are known to reorder away freely.
>>>
>>> To prevent any issues, the accesses involved when using
>>> GuardUnsafeAccess should be at least volatile.
>>> This change makes the few remaining ones volatile. The JMM-volatile
>>> (SEQ_CST) accesses with crash protection already have stronger
>>> ordering than volatile and hence do not need changing.
>>>
>>> By making the address passed in to the Access API volatile, the
>>> MO_VOLATILE decorator is automatically set, which not surprisingly
>>> makes the access volatile. Therefore, the solution is to simply make
>>> the address passed in to Access volatile in this case.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Kim Barrett
In reply to this post by Erik Österlund-2
> On Nov 27, 2017, at 7:36 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi,
>
> There is currently a bug when using unsafe accesses off-heap:
>
> 1) We write into a thread that we enable crash protection (using GuardUnsafeAccess):
> 2) We perform the access
> 3) We write into a thread that we disable crash protection (using ~GuardUnsafeAccess)
>
> The problem is that the crash protection stores are volatile, but the actual access is non-volatile. Compilers have different interpretation whether volatile - non-volatile accesses are allowed to reorder. MSVC is known to interpret such interactions as-if the volatile accesses have acquire/release semantics from the compiler point of view, and others such as GCC are known to reorder away freely.
>
> To prevent any issues, the accesses involved when using GuardUnsafeAccess should be at least volatile.
> This change makes the few remaining ones volatile. The JMM-volatile (SEQ_CST) accesses with crash protection already have stronger ordering than volatile and hence do not need changing.
>
> By making the address passed in to the Access API volatile, the MO_VOLATILE decorator is automatically set, which not surprisingly makes the access volatile. Therefore, the solution is to simply make the address passed in to Access volatile in this case.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8186787
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>
> Thanks,
> /Erik

Not my first choice for a fix (you know how I feel about casts), but
it works, and is currently much simpler than my preferred solution.

Should there be a comment justifying the cast to volatile?  Perhaps
something like "volatile to keep access within guarded scope."  I
don't need a new webrev if you add such a comment.

Looks good.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Andrew Haley
On 28/11/17 20:30, Kim Barrett wrote:
> Should there be a comment justifying the cast to volatile?  Perhaps
> something like "volatile to keep access within guarded scope."  I
> don't need a new webrev if you add such a comment.

Better:

"This raw memory access may fault, so make sure it happens while
within guarded scope."

You really do have to spell this out.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
In reply to this post by Kim Barrett
Hi Kim,

I know how you feel about casts, and I also saw how Andrew Haley wanted
a more explicit comment about why it needs to be volatile.
To make both of you happy, I thought I'd try to make the address
(addr()) in MemoryAccess volatile T*. That way I can write a more
explicit comment about why it is volatile in one single place, and make
you happier to not cast the address to something else than it was declared.

I hope this makes both of you happy. If not, I am okay with the old
variant too, and write a comment that I copy around instead.

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/

Thanks,
/Erik

On 2017-11-28 21:30, Kim Barrett wrote:

>> On Nov 27, 2017, at 7:36 AM, Erik Österlund <[hidden email]> wrote:
>>
>> Hi,
>>
>> There is currently a bug when using unsafe accesses off-heap:
>>
>> 1) We write into a thread that we enable crash protection (using GuardUnsafeAccess):
>> 2) We perform the access
>> 3) We write into a thread that we disable crash protection (using ~GuardUnsafeAccess)
>>
>> The problem is that the crash protection stores are volatile, but the actual access is non-volatile. Compilers have different interpretation whether volatile - non-volatile accesses are allowed to reorder. MSVC is known to interpret such interactions as-if the volatile accesses have acquire/release semantics from the compiler point of view, and others such as GCC are known to reorder away freely.
>>
>> To prevent any issues, the accesses involved when using GuardUnsafeAccess should be at least volatile.
>> This change makes the few remaining ones volatile. The JMM-volatile (SEQ_CST) accesses with crash protection already have stronger ordering than volatile and hence do not need changing.
>>
>> By making the address passed in to the Access API volatile, the MO_VOLATILE decorator is automatically set, which not surprisingly makes the access volatile. Therefore, the solution is to simply make the address passed in to Access volatile in this case.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>
>> Thanks,
>> /Erik
> Not my first choice for a fix (you know how I feel about casts), but
> it works, and is currently much simpler than my preferred solution.
>
> Should there be a comment justifying the cast to volatile?  Perhaps
> something like "volatile to keep access within guarded scope."  I
> don't need a new webrev if you add such a comment.
>
> Looks good.
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Andrew Haley
On 29/11/17 13:48, Erik Österlund wrote:
> I hope this makes both of you happy.

:-)  :-)  :-)

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Dmitry Samersoff-2
In reply to this post by Erik Österlund-2
Erik,

I like this variant too :)

-Dmitry

On 29.11.2017 16:48, Erik Österlund wrote:

> Hi Kim,
>
> I know how you feel about casts, and I also saw how Andrew Haley wanted
> a more explicit comment about why it needs to be volatile.
> To make both of you happy, I thought I'd try to make the address
> (addr()) in MemoryAccess volatile T*. That way I can write a more
> explicit comment about why it is volatile in one single place, and make
> you happier to not cast the address to something else than it was declared.
>
> I hope this makes both of you happy. If not, I am okay with the old
> variant too, and write a comment that I copy around instead.
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/
>
> Thanks,
> /Erik
>
> On 2017-11-28 21:30, Kim Barrett wrote:
>>> On Nov 27, 2017, at 7:36 AM, Erik Österlund
>>> <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> There is currently a bug when using unsafe accesses off-heap:
>>>
>>> 1) We write into a thread that we enable crash protection (using
>>> GuardUnsafeAccess):
>>> 2) We perform the access
>>> 3) We write into a thread that we disable crash protection (using
>>> ~GuardUnsafeAccess)
>>>
>>> The problem is that the crash protection stores are volatile, but the
>>> actual access is non-volatile. Compilers have different
>>> interpretation whether volatile - non-volatile accesses are allowed
>>> to reorder. MSVC is known to interpret such interactions as-if the
>>> volatile accesses have acquire/release semantics from the compiler
>>> point of view, and others such as GCC are known to reorder away freely.
>>>
>>> To prevent any issues, the accesses involved when using
>>> GuardUnsafeAccess should be at least volatile.
>>> This change makes the few remaining ones volatile. The JMM-volatile
>>> (SEQ_CST) accesses with crash protection already have stronger
>>> ordering than volatile and hence do not need changing.
>>>
>>> By making the address passed in to the Access API volatile, the
>>> MO_VOLATILE decorator is automatically set, which not surprisingly
>>> makes the access volatile. Therefore, the solution is to simply make
>>> the address passed in to Access volatile in this case.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>> Not my first choice for a fix (you know how I feel about casts), but
>> it works, and is currently much simpler than my preferred solution.
>>
>> Should there be a comment justifying the cast to volatile?  Perhaps
>> something like "volatile to keep access within guarded scope."  I
>> don't need a new webrev if you add such a comment.
>>
>> Looks good.
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

coleen.phillimore
In reply to this post by Erik Österlund-2

I like this version.
Coleen

On 11/29/17 8:48 AM, Erik Österlund wrote:

> Hi Kim,
>
> I know how you feel about casts, and I also saw how Andrew Haley
> wanted a more explicit comment about why it needs to be volatile.
> To make both of you happy, I thought I'd try to make the address
> (addr()) in MemoryAccess volatile T*. That way I can write a more
> explicit comment about why it is volatile in one single place, and
> make you happier to not cast the address to something else than it was
> declared.
>
> I hope this makes both of you happy. If not, I am okay with the old
> variant too, and write a comment that I copy around instead.
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/
>
> Thanks,
> /Erik
>
> On 2017-11-28 21:30, Kim Barrett wrote:
>>> On Nov 27, 2017, at 7:36 AM, Erik Österlund
>>> <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> There is currently a bug when using unsafe accesses off-heap:
>>>
>>> 1) We write into a thread that we enable crash protection (using
>>> GuardUnsafeAccess):
>>> 2) We perform the access
>>> 3) We write into a thread that we disable crash protection (using
>>> ~GuardUnsafeAccess)
>>>
>>> The problem is that the crash protection stores are volatile, but
>>> the actual access is non-volatile. Compilers have different
>>> interpretation whether volatile - non-volatile accesses are allowed
>>> to reorder. MSVC is known to interpret such interactions as-if the
>>> volatile accesses have acquire/release semantics from the compiler
>>> point of view, and others such as GCC are known to reorder away freely.
>>>
>>> To prevent any issues, the accesses involved when using
>>> GuardUnsafeAccess should be at least volatile.
>>> This change makes the few remaining ones volatile. The JMM-volatile
>>> (SEQ_CST) accesses with crash protection already have stronger
>>> ordering than volatile and hence do not need changing.
>>>
>>> By making the address passed in to the Access API volatile, the
>>> MO_VOLATILE decorator is automatically set, which not surprisingly
>>> makes the access volatile. Therefore, the solution is to simply make
>>> the address passed in to Access volatile in this case.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>> Not my first choice for a fix (you know how I feel about casts), but
>> it works, and is currently much simpler than my preferred solution.
>>
>> Should there be a comment justifying the cast to volatile? Perhaps
>> something like "volatile to keep access within guarded scope." I
>> don't need a new webrev if you add such a comment.
>>
>> Looks good.
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

David Holmes
In reply to this post by Erik Österlund-2
+1 :)

David

On 29/11/2017 11:48 PM, Erik Österlund wrote:

> Hi Kim,
>
> I know how you feel about casts, and I also saw how Andrew Haley wanted
> a more explicit comment about why it needs to be volatile.
> To make both of you happy, I thought I'd try to make the address
> (addr()) in MemoryAccess volatile T*. That way I can write a more
> explicit comment about why it is volatile in one single place, and make
> you happier to not cast the address to something else than it was declared.
>
> I hope this makes both of you happy. If not, I am okay with the old
> variant too, and write a comment that I copy around instead.
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/
>
> Thanks,
> /Erik
>
> On 2017-11-28 21:30, Kim Barrett wrote:
>>> On Nov 27, 2017, at 7:36 AM, Erik Österlund
>>> <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> There is currently a bug when using unsafe accesses off-heap:
>>>
>>> 1) We write into a thread that we enable crash protection (using
>>> GuardUnsafeAccess):
>>> 2) We perform the access
>>> 3) We write into a thread that we disable crash protection (using
>>> ~GuardUnsafeAccess)
>>>
>>> The problem is that the crash protection stores are volatile, but the
>>> actual access is non-volatile. Compilers have different
>>> interpretation whether volatile - non-volatile accesses are allowed
>>> to reorder. MSVC is known to interpret such interactions as-if the
>>> volatile accesses have acquire/release semantics from the compiler
>>> point of view, and others such as GCC are known to reorder away freely.
>>>
>>> To prevent any issues, the accesses involved when using
>>> GuardUnsafeAccess should be at least volatile.
>>> This change makes the few remaining ones volatile. The JMM-volatile
>>> (SEQ_CST) accesses with crash protection already have stronger
>>> ordering than volatile and hence do not need changing.
>>>
>>> By making the address passed in to the Access API volatile, the
>>> MO_VOLATILE decorator is automatically set, which not surprisingly
>>> makes the access volatile. Therefore, the solution is to simply make
>>> the address passed in to Access volatile in this case.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>> Not my first choice for a fix (you know how I feel about casts), but
>> it works, and is currently much simpler than my preferred solution.
>>
>> Should there be a comment justifying the cast to volatile?  Perhaps
>> something like "volatile to keep access within guarded scope."  I
>> don't need a new webrev if you add such a comment.
>>
>> Looks good.
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
Hi David,

Thanks for the review.

/Erik

On 2017-11-30 05:44, David Holmes wrote:

> +1 :)
>
> David
>
> On 29/11/2017 11:48 PM, Erik Österlund wrote:
>> Hi Kim,
>>
>> I know how you feel about casts, and I also saw how Andrew Haley
>> wanted a more explicit comment about why it needs to be volatile.
>> To make both of you happy, I thought I'd try to make the address
>> (addr()) in MemoryAccess volatile T*. That way I can write a more
>> explicit comment about why it is volatile in one single place, and
>> make you happier to not cast the address to something else than it
>> was declared.
>>
>> I hope this makes both of you happy. If not, I am okay with the old
>> variant too, and write a comment that I copy around instead.
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-28 21:30, Kim Barrett wrote:
>>>> On Nov 27, 2017, at 7:36 AM, Erik Österlund
>>>> <[hidden email]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> There is currently a bug when using unsafe accesses off-heap:
>>>>
>>>> 1) We write into a thread that we enable crash protection (using
>>>> GuardUnsafeAccess):
>>>> 2) We perform the access
>>>> 3) We write into a thread that we disable crash protection (using
>>>> ~GuardUnsafeAccess)
>>>>
>>>> The problem is that the crash protection stores are volatile, but
>>>> the actual access is non-volatile. Compilers have different
>>>> interpretation whether volatile - non-volatile accesses are allowed
>>>> to reorder. MSVC is known to interpret such interactions as-if the
>>>> volatile accesses have acquire/release semantics from the compiler
>>>> point of view, and others such as GCC are known to reorder away
>>>> freely.
>>>>
>>>> To prevent any issues, the accesses involved when using
>>>> GuardUnsafeAccess should be at least volatile.
>>>> This change makes the few remaining ones volatile. The JMM-volatile
>>>> (SEQ_CST) accesses with crash protection already have stronger
>>>> ordering than volatile and hence do not need changing.
>>>>
>>>> By making the address passed in to the Access API volatile, the
>>>> MO_VOLATILE decorator is automatically set, which not surprisingly
>>>> makes the access volatile. Therefore, the solution is to simply
>>>> make the address passed in to Access volatile in this case.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>>>
>>>> Thanks,
>>>> /Erik
>>> Not my first choice for a fix (you know how I feel about casts), but
>>> it works, and is currently much simpler than my preferred solution.
>>>
>>> Should there be a comment justifying the cast to volatile? Perhaps
>>> something like "volatile to keep access within guarded scope."  I
>>> don't need a new webrev if you add such a comment.
>>>
>>> Looks good.
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
In reply to this post by Andrew Haley
Hi Andrew,

Thanks for the review!

/Erik

On 2017-11-29 15:25, Andrew Haley wrote:
> On 29/11/17 13:48, Erik Österlund wrote:
>> I hope this makes both of you happy.
> :-)  :-)  :-)
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
In reply to this post by Dmitry Samersoff-2
Hi Dmitry,

Thank you for the review.

/Erik

On 2017-11-29 15:43, Dmitry Samersoff wrote:

> Erik,
>
> I like this variant too :)
>
> -Dmitry
>
> On 29.11.2017 16:48, Erik Österlund wrote:
>> Hi Kim,
>>
>> I know how you feel about casts, and I also saw how Andrew Haley wanted
>> a more explicit comment about why it needs to be volatile.
>> To make both of you happy, I thought I'd try to make the address
>> (addr()) in MemoryAccess volatile T*. That way I can write a more
>> explicit comment about why it is volatile in one single place, and make
>> you happier to not cast the address to something else than it was declared.
>>
>> I hope this makes both of you happy. If not, I am okay with the old
>> variant too, and write a comment that I copy around instead.
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-28 21:30, Kim Barrett wrote:
>>>> On Nov 27, 2017, at 7:36 AM, Erik Österlund
>>>> <[hidden email]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> There is currently a bug when using unsafe accesses off-heap:
>>>>
>>>> 1) We write into a thread that we enable crash protection (using
>>>> GuardUnsafeAccess):
>>>> 2) We perform the access
>>>> 3) We write into a thread that we disable crash protection (using
>>>> ~GuardUnsafeAccess)
>>>>
>>>> The problem is that the crash protection stores are volatile, but the
>>>> actual access is non-volatile. Compilers have different
>>>> interpretation whether volatile - non-volatile accesses are allowed
>>>> to reorder. MSVC is known to interpret such interactions as-if the
>>>> volatile accesses have acquire/release semantics from the compiler
>>>> point of view, and others such as GCC are known to reorder away freely.
>>>>
>>>> To prevent any issues, the accesses involved when using
>>>> GuardUnsafeAccess should be at least volatile.
>>>> This change makes the few remaining ones volatile. The JMM-volatile
>>>> (SEQ_CST) accesses with crash protection already have stronger
>>>> ordering than volatile and hence do not need changing.
>>>>
>>>> By making the address passed in to the Access API volatile, the
>>>> MO_VOLATILE decorator is automatically set, which not surprisingly
>>>> makes the access volatile. Therefore, the solution is to simply make
>>>> the address passed in to Access volatile in this case.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8186787
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00/
>>>>
>>>> Thanks,
>>>> /Erik
>>> Not my first choice for a fix (you know how I feel about casts), but
>>> it works, and is currently much simpler than my preferred solution.
>>>
>>> Should there be a comment justifying the cast to volatile?  Perhaps
>>> something like "volatile to keep access within guarded scope."  I
>>> don't need a new webrev if you add such a comment.
>>>
>>> Looks good.
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Kim Barrett
In reply to this post by Erik Österlund-2
> On Nov 29, 2017, at 8:48 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi Kim,
>
> I know how you feel about casts, and I also saw how Andrew Haley wanted a more explicit comment about why it needs to be volatile.
> To make both of you happy, I thought I'd try to make the address (addr()) in MemoryAccess volatile T*. That way I can write a more explicit comment about why it is volatile in one single place, and make you happier to not cast the address to something else than it was declared.
>
> I hope this makes both of you happy. If not, I am okay with the old variant too, and write a comment that I copy around instead.
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/

Sorry to be late; I hadn't noticed there was followup and a new
version yesterday until Coleen pointed it out to me.

This is okay, though I think better would be to make addr() a function
template (with a non-deducable type parameter), rather than making
MemoryAccess a class template. The point of that difference is the
principle of minimizing the dependencies between members and type
parameters of a generic class (in this case by not making the class
generic at all).

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8186787: clang-4.0 SIGSEGV in Unsafe_PutByte

Erik Österlund-2
Hi Kim,

Thanks for the review.

/Erik

On 30 Nov 2017, at 20:34, Kim Barrett <[hidden email]> wrote:

>> On Nov 29, 2017, at 8:48 AM, Erik Österlund <[hidden email]> wrote:
>>
>> Hi Kim,
>>
>> I know how you feel about casts, and I also saw how Andrew Haley wanted a more explicit comment about why it needs to be volatile.
>> To make both of you happy, I thought I'd try to make the address (addr()) in MemoryAccess volatile T*. That way I can write a more explicit comment about why it is volatile in one single place, and make you happier to not cast the address to something else than it was declared.
>>
>> I hope this makes both of you happy. If not, I am okay with the old variant too, and write a comment that I copy around instead.
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.01/
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186787/webrev.00_01/
>
> Sorry to be late; I hadn't noticed there was followup and a new
> version yesterday until Coleen pointed it out to me.
>
> This is okay, though I think better would be to make addr() a function
> template (with a non-deducable type parameter), rather than making
> MemoryAccess a class template. The point of that difference is the
> principle of minimizing the dependencies between members and type
> parameters of a generic class (in this case by not making the class
> generic at all).
>