Re: [lttng-dev] [PATCH 6/7] Fix: uatomic_or() need retyping to uintptr_t in rculfhash.c

[Mathieu Desnoyers via lttng-dev <lttng-dev@lists.lttng.org>]

On 2023-03-21 10:44, Mathieu Desnoyers wrote:
> On 2023-03-21 06:15, Ondřej Surý wrote:
>>
>>> On 20. 3. 2023, at 19:31, Mathieu Desnoyers 
>>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> On 2023-03-17 17:37, Ondřej Surý via lttng-dev wrote:
>>>> When adding REMOVED_FLAG to the pointers in the rculfhash
>>>> implementation, retype the generic pointer to uintptr_t to fix the
>>>> compiler error.
>>>
>>> What is the compiler error ? I'm wondering whether the expected choice
>>> to match the rest of this file's content would be to use "uintptr_t 
>>> *" or "unsigned long *" ?
>>
>> This is the error:
>>
>> rculfhash.c:1201:2: error: address argument to atomic operation must 
>> be a pointer to integer ('struct cds_lfht_node **' invalid)
>>          uatomic_or(&node->next, REMOVED_FLAG);
>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../include/urcu/uatomic.h:60:8: note: expanded from macro 'uatomic_or'
>>          (void)__atomic_or_fetch((addr), (mask), __ATOMIC_RELAXED)
>>                ^                 ~~~~~~
>> rculfhash.c:1444:3: error: address argument to atomic operation must 
>> be a pointer to integer ('struct cds_lfht_node **' invalid)
>>                  uatomic_or(&fini_bucket->next, REMOVED_FLAG);
>>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../include/urcu/uatomic.h:60:8: note: expanded from macro 'uatomic_or'
>>          (void)__atomic_or_fetch((addr), (mask), __ATOMIC_RELAXED)
>>                ^                 ~~~~~~
>>
>> uintptr_t is defined as "unsigned integer type capable of holding a 
>> pointer to void" while unsigned long is at least 32-bit;
>>
>> I guess that works in a practise, but using unsigned long to retype 
>> the pointers might blow up (thinking of x32 which I know
>> little about, but it's kind of hybrid architecture, isn't it?)
> 
> x32 uses 4 bytes for unsigned long, uintptr_t, and void * size. So even 
> that architecture is OK with casting pointer to unsigned long.
> 
> I agree with you that uintptr_t is the semantically correct type, but it 
> should come as a separate change across the urcu code base: currently 
> there are many places where void * is cast to unsigned long to do 
> bitwise operations.
> 
> I therefore recommend to use unsigned long here to stay similar to the 
> rest of the code base, and keep the transition from unsigned long to 
> uintptr_t for the future, as it is not an immediate issue we have to 
> address.

I forgot to mention: you should add the compiler error to the commit 
message.

You should also explain why this was not an issue until now. It's 
probably related to the introduced use of __atomic builtins.

Thanks,

Mathieu

> 
> Thanks,
> 
> Mathieu
> 
> 
>>
>> Ondrej
>> -- 
>> Ondřej Surý (He/Him)
>> ondrej@sury.org
>>
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev