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

[Erik Österlund]

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