| Message ID | 1504643122-14874-5-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | posix: glob fixes and refactor | expand |
On 09/05/2017 10:25 PM, Adhemerval Zanella wrote: > - char __space[1024] > - __attribute__ ((aligned (__alignof__ (max_align_t)))); > + max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; This change needs a declaration from the GCC folks (probably from Richard Biener) that it does not break aliasing analysis. The old code uses a GCC extension (writing to a char array changes its dynamic type); it is not valid C. I don't know if the GCC extension also applies if the storage site is declared with a non-char type. Thanks, Florian
On 18/09/2017 03:09, Florian Weimer wrote: > On 09/05/2017 10:25 PM, Adhemerval Zanella wrote: >> - char __space[1024] >> - __attribute__ ((aligned (__alignof__ (max_align_t)))); >> + max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; > > This change needs a declaration from the GCC folks (probably from Richard Biener) that it does not break aliasing analysis. The old code uses a GCC extension (writing to a char array changes its dynamic type); it is not valid C. I don't know if the GCC extension also applies if the storage site is declared with a non-char type. > > Thanks, > Florian What about this below instead: --- diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h index bb04662..f77e7da 100644 --- a/include/scratch_buffer.h +++ b/include/scratch_buffer.h @@ -60,13 +60,18 @@ #include <stdbool.h> #include <stddef.h> #include <stdlib.h> +#include <stdalign.h> /* Scratch buffer. Must be initialized with scratch_buffer_init before its use. */ struct scratch_buffer { void *data; /* Pointer to the beginning of the scratch area. */ size_t length; /* Allocated space at the data pointer, in bytes. */ +#if __alignas_is_defined + _Alignas (max_align_t) char __space[1024]; +#else max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; +#endif }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space --- _Alignas/stdalign.h is supported since GCC 4.7 [1] (so it is safe to use on glibc without configure support) and gnulib have its own version which defines __alignas_is_defined depending on underlying compiler support. [1] https://gcc.gnu.org/wiki/C11Status
On 09/18/2017 01:43 PM, Adhemerval Zanella wrote: > +#if __alignas_is_defined > + _Alignas (max_align_t) char __space[1024]; > +#else > max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; > +#endif This works for me on the glibc side. By the way, do you have a rebased version of this patch? Subject: [PATCH 05/18] posix: Rewrite to use struct scratch_buffer instead of extend_alloca Thanks, Florian
On 18/09/2017 08:57, Florian Weimer wrote: > On 09/18/2017 01:43 PM, Adhemerval Zanella wrote: >> +#if __alignas_is_defined >> + _Alignas (max_align_t) char __space[1024]; >> +#else >> max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; >> +#endif > > This works for me on the glibc side. Right, I will prepare a patch then. > > By the way, do you have a rebased version of this patch? > > Subject: [PATCH 05/18] posix: Rewrite to use struct scratch_buffer instead of extend_alloca > > Thanks, > Florian No, but I can send along with some more glob fixes I plan to.
diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h index dd17a4a..bb04662 100644 --- a/include/scratch_buffer.h +++ b/include/scratch_buffer.h @@ -66,8 +66,7 @@ struct scratch_buffer { void *data; /* Pointer to the beginning of the scratch area. */ size_t length; /* Allocated space at the data pointer, in bytes. */ - char __space[1024] - __attribute__ ((aligned (__alignof__ (max_align_t)))); + max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space diff --git a/malloc/scratch_buffer_grow.c b/malloc/scratch_buffer_grow.c index 22bae50..d2df028 100644 --- a/malloc/scratch_buffer_grow.c +++ b/malloc/scratch_buffer_grow.c @@ -16,6 +16,10 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#ifndef _LIBC +# include <libc-config.h> +#endif + #include <scratch_buffer.h> #include <errno.h> @@ -49,4 +53,4 @@ __libc_scratch_buffer_grow (struct scratch_buffer *buffer) buffer->length = new_length; return true; } -libc_hidden_def (__libc_scratch_buffer_grow); +libc_hidden_def (__libc_scratch_buffer_grow) diff --git a/malloc/scratch_buffer_grow_preserve.c b/malloc/scratch_buffer_grow_preserve.c index 18543ef..9268615 100644 --- a/malloc/scratch_buffer_grow_preserve.c +++ b/malloc/scratch_buffer_grow_preserve.c @@ -16,6 +16,10 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#ifndef _LIBC +# include <libc-config.h> +#endif + #include <scratch_buffer.h> #include <errno.h> #include <string.h> @@ -60,4 +64,4 @@ __libc_scratch_buffer_grow_preserve (struct scratch_buffer *buffer) buffer->length = new_length; return true; } -libc_hidden_def (__libc_scratch_buffer_grow_preserve); +libc_hidden_def (__libc_scratch_buffer_grow_preserve) diff --git a/malloc/scratch_buffer_set_array_size.c b/malloc/scratch_buffer_set_array_size.c index 8ab6d9d..6fcc115 100644 --- a/malloc/scratch_buffer_set_array_size.c +++ b/malloc/scratch_buffer_set_array_size.c @@ -16,6 +16,10 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#ifndef _LIBC +# include <libc-config.h> +#endif + #include <scratch_buffer.h> #include <errno.h> #include <limits.h> @@ -57,4 +61,4 @@ __libc_scratch_buffer_set_array_size (struct scratch_buffer *buffer, buffer->length = new_length; return true; } -libc_hidden_def (__libc_scratch_buffer_set_array_size); +libc_hidden_def (__libc_scratch_buffer_set_array_size)