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

[Dmitry Samersoff]

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