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

[Erik Österlund]

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 <erik.osterlund at oracle.com> 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.
>