Message ID | eb41483d0f3c054eb8420d0134d6c1a3c4af8c54.1666877952.git.szabolcs.nagy@arm.com |
---|---|
State | New |
Headers | show |
Series | patches from the morello port | expand |
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> >
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> > >
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 --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>