diff mbox series

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

Message ID eb41483d0f3c054eb8420d0134d6c1a3c4af8c54.1666877952.git.szabolcs.nagy@arm.com
State Dropped
Headers
Series patches from the morello port |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Szabolcs Nagy Oct. 27, 2022, 3:33 p.m. UTC 
  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(-)

Comments

Adhemerval Zanella Oct. 31, 2022, 4:14 p.m. UTC | #1 
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?

It would be good to check with gnulib as well, since this header is also
shared with it.

>  
>  #include <string.h>
>
Szabolcs Nagy Nov. 1, 2022, 9:43 a.m. UTC | #2 
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.

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

> 
> >  
> >  #include <string.h>
> >
Adhemerval Zanella Nov. 1, 2022, 1:07 p.m. UTC | #3 
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
diff mbox series

Patch

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)))
 
 #include <string.h>