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

[Erik Österlund]

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