Re: [PATCH] Avoid atomic for guard acquire when that is expensive

[Jason Merrill <jason@redhat.com>]

On 12/5/20 7:37 AM, Bernd Edlinger wrote:
> On 12/2/20 7:57 PM, Jason Merrill wrote:
>> On 12/1/20 1:28 PM, Bernd Edlinger wrote:
>>> On 11/24/20 11:10 PM, Jason Merrill wrote:
>>>> On 11/22/20 3:05 AM, Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> this avoids the need to use -fno-threadsafe-statics on
>>>>> arm-none-eabi or working around that problem by supplying
>>>>> a dummy __sync_synchronize function which might
>>>>> just lead to silent code failure of the worst kind
>>>>> (non-reproducable, racy) at runtime, as was pointed out
>>>>> on previous discussions here.
>>>>>
>>>>> When the atomic access involves a call to __sync_synchronize
>>>>> it is better to call __cxa_guard_acquire unconditionally,
>>>>> since it handles the atomics too, or is a non-threaded
>>>>> implementation when there is no gthread support for this target.
>>>>>
>>>>> This fixes also a bug for the ARM EABI big-endian target,
>>>>> that is, previously the wrong bit was checked.
>>>>
>>>> Instead of a new target macro, can't you follow fold_builtin_atomic_always_lock_free/can_atomic_load_p?
>>>
>>> Yes, thanks, that should work too.
>>> Would you like this better?
>>
>>> +is_atomic_expensive_p (machine_mode mode)
>>> +{
>>> +  if (!flag_inline_atomics)
>>> +    return false;
>>
>> Why not true?
> 
> Ooops...
> Yes, I ought to return true here.
> I must have made a mistake when I tested the last version of this patch,
> sorry for the confusion.
> 
>>> +  if (!can_compare_and_swap_p (mode, false) || !can_atomic_load_p (mode))
>>> +    return false;
>>
>> This also seems backwards; I'd think we want to return false if either of those tests are true.  Or maybe just can_atomic_load_p, and not bother about compare-and-swap.
> 
> Yes, you are right.
> Unfortuately can_atomic_load_p is too weak, since it does not cover
> the memory barrier.
> 
> And can_compare_and_swap_p (..., false) is actually a bit too strong,
> but if it returns true, we should be able to use any atomic without
> need for a library call.

>>> +      tree type = targetm.cxx.guard_mask_bit ()
>>> +          ? TREE_TYPE (guard) : char_type_node;
>>> +
>>> +      if (is_atomic_expensive_p (TYPE_MODE (type)))
>>> +    guard = integer_zero_node;
>>> +      else
>>> +    guard = build_atomic_load_type (guard, MEMMODEL_ACQUIRE, type);
>>
>> It should still work to load a single byte, it just needs to be the least-significant byte.

I still don't think you need to load the whole word to check the guard bit.

> And this isn't an EABI issue; it looks like the non-EABI code is also broken for big-endian targets, both the atomic load and the normal load in get_guard_bits.
>>
> 
> I think the non-EABI code is always using bit 0 in the first byte,
> by using the endian-neutral #define _GLIBCXX_GUARD_BIT __guard_test_bit (0, 1).

Except that set_guard sets the least-significant bit on all targets.

> Only ARM EABI uses bit 0 in byte 3 if big-endian and bit 0 in byte 0 otherwise.
> 
> For all other targets when _GLIBCXX_USE_FUTEX is defined,
> __cxa_guard_XXX accesses the value as int* while the memory
> is a 64-bit long, so I could imagine that is an aliasing violation.
> 
> 
> But nothing that needs to be fixed immediately.

Agreed.

> Attached is the corrected patch.
> 
> Tested again on arm-none-eabi with arm-sim.
> Is it OK for trunk?
> 
> Thanks
> Bernd.
> 
>> Jason
>>