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

[David Holmes]

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