Re: [PATCH v5 1/6] stdlib: Optimization qsort{_r} swap implementation

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 17/07/23 13:57, Alexander Monakov wrote:
> 
> On Thu, 13 Jul 2023, Adhemerval Zanella via Libc-alpha wrote:
> 
>> +/* Returns true if elements can be copied using word loads and stores.
>> +   The SIZE and BASE must be a multiple of the ALIGN.  */
>> +__attribute_const__ __always_inline static bool
> 
> Kernel folks sure love their double underscores, but plain
> 'static inline bool' should work just as well here.
> 
> I'd recommend to properly credit Linux lib/sort.c since obviously this
> commit originated as a direct copy of the code therein, not just being
> inspired by what's there.

In fact our msort_with_tmp uses the exact same idea, but it is somewhat
convoluted because it adds an specific optimization to 'unsigned long'
and for large values it uses a inplace mempcpy.

> 
>> +is_aligned (const void *base, size_t size, unsigned char align)
>> +{
>> +  return (((uintptr_t) base | size) & (align - 1)) == 0;
>> +}
>> +
>> +static void
>> +swap_words_64 (void * restrict a, void * restrict b, size_t n)
>> +{
>> +  do
>> +   {
>> +     n -= 8;
>> +     uint64_t t = *(uint64_t *)(a + n);
>> +     *(uint64_t *)(a + n) = *(uint64_t *)(b + n);
>> +     *(uint64_t *)(b + n) = t;
>> +   } while (n);
>> +}
> 
> The undefined behavior resulting from attempt to access via an 'uint64_t *'
> something of a different type is known to cause miscompilation in practice:
> https://www.spec.org/cpu2017/Docs/benchmarks/605.mcf_s.html#portability
> 
> I see that the new string routines do it properly via 
> 
>   typedef unsigned long int __attribute__ ((__may_alias__)) op_t;
> 
> Would be nice to avoid new instances of such UB here.

Ack.