Re: [PATCH 10/20] malloc: Fix alignment logic in obstack

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

On 01/11/22 06:43, Szabolcs Nagy wrote:
> The 10/31/2022 13:14, Adhemerval Zanella Netto wrote:
>> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
>>> If sizeof(ptrdiff_t) < sizeof(void*) the alignment logic was wrong:
>>> incorrectly assumed that base was already sufficiently aligned.
>>>
>>> Use more robust alignment logic: this one should work on any target.
>>> Note: this is an installed header so it must be namespace clean and
>>> portable hence it uses unsigned long for the alignment offset.
>>> ---
>>>  malloc/obstack.h | 19 +++----------------
>>>  1 file changed, 3 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/malloc/obstack.h b/malloc/obstack.h
>>> index 4b01cdfe4d..1cf18e5464 100644
>>> --- a/malloc/obstack.h
>>> +++ b/malloc/obstack.h
>>> @@ -116,22 +116,9 @@
>>>  # define PTR_INT_TYPE ptrdiff_t
>>>  #endif
>>>  
>>> -/* If B is the base of an object addressed by P, return the result of
>>> -   aligning P to the next multiple of A + 1.  B and P must be of type
>>> -   char *.  A + 1 must be a power of 2.  */
>>> -
>>> -#define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))
>>> -
>>> -/* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
>>> -   where pointers can be converted to integers, aligned as integers,
>>> -   and converted back again.  If PTR_INT_TYPE is narrower than a
>>> -   pointer (e.g., the AS/400), play it safe and compute the alignment
>>> -   relative to B.  Otherwise, use the faster strategy of computing the
>>> -   alignment relative to 0.  */
>>> -
>>> -#define __PTR_ALIGN(B, P, A)						      \
>>> -  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
>>> -		P, A)
>>> +/* Align P to the next multiple of A + 1, where A + 1 is a power of 2,
>>> +   A fits into unsigned long and P has type char *.  */
>>> +#define __PTR_ALIGN(B, P, A) ((P) + (-(unsigned long)(P) & (A)))
>>
>> Shouldn't you use uintptr_t here to be consistent with your other changes
>> that exactly change using long to cast from pointers?
> 
> here the offset part is unsigned long, but the pointer is kept
> char *. in other patches the problem was that the pointer
> was turned into long.
> 
> here unsigned int would be enough, since obstack->alignment_mask
> is int, larger alignments are not supported.
> 
> the new formula may not be the fastest to compute, but if the
> goal is portability then i think it's better than the current
> code.

Alright, although I still why not use uintptr_t here for consistency (as we
do for all other pointer conversions).  And the code already include stddef.h.

> 
>>
>> It would be good to check with gnulib as well, since this header is also
>> shared with it.
> 
> i see. i haven't looked at gnulib