| Message ID | 1505745774-29303-2-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] posix: Add compat glob symbol to not follow dangling symbols | expand |
On 09/18/2017 04:42 PM, Adhemerval Zanella wrote: > Checked on x86_64-linux-gnu. > > * include/scratch_buffer.h (scratch_buffer): Use C11 _Alignas on > __space field definition if compiler supports it. > --- > ChangeLog | 3 +++ > include/scratch_buffer.h | 5 +++++ > 2 files changed, 8 insertions(+) > > diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h > index bb04662..2e0c8b5 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 Okay, I think. (Richard has spoken: The code with max_align_t is okay as well as far as GCC is concerned. But I think it's useful to keep the initial buffer size in sync on all architectures to reduce variance, so please push this change.) Thanks, Florian.
This patch could cause problems on non-GCC platforms. According to the Gnulib documentation <https://www.gnu.org/software/gnulib/manual/html_node/stdalign_002eh.html>, Sun C 5.11 supports _Alignas but not on auto variables. In practice, the unpatched scratch_buffer.h uses 1024-byte buffers everywhere, so the initial buffer size will be in sync on all platforms even without the patch. Since the patch adds complexity, seems to have no technical advantage, and has potential downsides, I suggest leaving that part of scratch_buffer.h alone.
On 09/18/2017 06:10 PM, Paul Eggert wrote: > This patch could cause problems on non-GCC platforms. According to the > Gnulib documentation > <https://www.gnu.org/software/gnulib/manual/html_node/stdalign_002eh.html>, > Sun C 5.11 supports _Alignas but not on auto variables. In practice, the > unpatched scratch_buffer.h uses 1024-byte buffers everywhere, Are you sure? sizeof (max_align_t) is 48 on i386, after all. Thanks, Florian
Florian Weimer wrote:
> Are you sure? sizeof (max_align_t) is 48 on i386, after all.
Sorry, didn't know that. We could use 1056-byte buffers instead; would that be
the same on all glibc platforms? I'm not sure how important it is that the byte
count be the same everywhere.
On Sep 18 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h > index bb04662..2e0c8b5 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 You could use a union instead. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
On 09/18/2017 06:57 PM, Paul Eggert wrote: > Florian Weimer wrote: >> Are you sure? sizeof (max_align_t) is 48 on i386, after all. > > Sorry, didn't know that. We could use 1056-byte buffers instead; would > that be the same on all glibc platforms? I'm not sure how important it > is that the byte count be the same everywhere. I've chased ghosts due to such buffer size differences. I'd like to reduce it to the 32/64 difference if possible. Thanks, Florian
Florian Weimer wrote: >> Sorry, didn't know that. We could use 1056-byte buffers instead; would that be >> the same on all glibc platforms? I'm not sure how important it is that the >> byte count be the same everywhere. > > I've chased ghosts due to such buffer size differences. I'd like to reduce it > to the 32/64 difference if possible. Then Andreas's suggestion is a good way to go. A member like this in struct scratch_buffer: union { max_align_t __a; char __c[1024]; } __space; and then use __space.__c everywhere the code currently uses __space. This will make the buffer size 1024 on all platforms where sizeof (max_align_t) <= 1024, which should be a safe assumption for all glibc platforms now.
On 18/09/2017 14:24, Paul Eggert wrote: > Florian Weimer wrote: >>> Sorry, didn't know that. We could use 1056-byte buffers instead; would that be the same on all glibc platforms? I'm not sure how important it is that the byte count be the same everywhere. >> >> I've chased ghosts due to such buffer size differences. I'd like to reduce it to the 32/64 difference if possible. > > Then Andreas's suggestion is a good way to go. A member like this in struct scratch_buffer: > > union { max_align_t __a; char __c[1024]; } __space; > > and then use __space.__c everywhere the code currently uses __space. This will make the buffer size 1024 on all platforms where sizeof (max_align_t) <= 1024, which should be a safe assumption for all glibc platforms now. Right, the union does seems a better option. What about: --- * include/scratch_buffer.h (scratch_bufferi, scratch_buffer_init, scratch_buffer_free): Use an union instead of a max_align_t array for __space definition. * malloc/scratch_buffer_grow_preserve.c (__libc_scratch_buffer_grow_preserve): Likewise. * malloc/tst-scratch_buffer.c (array_size_must_fail): Likewise. --- diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h index bb04662..0f49dd9 100644 --- a/include/scratch_buffer.h +++ b/include/scratch_buffer.h @@ -66,7 +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. */ - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; + union { max_align_t __a; char __c[1024]; } __space; }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space @@ -74,7 +74,7 @@ struct scratch_buffer { static inline void scratch_buffer_init (struct scratch_buffer *buffer) { - buffer->data = buffer->__space; + buffer->data = buffer->__space.__c; buffer->length = sizeof (buffer->__space); } @@ -82,7 +82,7 @@ scratch_buffer_init (struct scratch_buffer *buffer) static inline void scratch_buffer_free (struct scratch_buffer *buffer) { - if (buffer->data != buffer->__space) + if (buffer->data != buffer->__space.__c) free (buffer->data); } diff --git a/malloc/scratch_buffer_grow_preserve.c b/malloc/scratch_buffer_grow_preserve.c index 9268615..59a922b 100644 --- a/malloc/scratch_buffer_grow_preserve.c +++ b/malloc/scratch_buffer_grow_preserve.c @@ -30,14 +30,14 @@ __libc_scratch_buffer_grow_preserve (struct scratch_buffer *buffer) size_t new_length = 2 * buffer->length; void *new_ptr; - if (buffer->data == buffer->__space) + if (buffer->data == buffer->__space.__c) { /* Move buffer to the heap. No overflow is possible because buffer->length describes a small buffer on the stack. */ new_ptr = malloc (new_length); if (new_ptr == NULL) return false; - memcpy (new_ptr, buffer->__space, buffer->length); + memcpy (new_ptr, buffer->__space.__c, buffer->length); } else { diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c index 5c9f344..86447b6 100644 --- a/malloc/tst-scratch_buffer.c +++ b/malloc/tst-scratch_buffer.c @@ -59,7 +59,7 @@ array_size_must_fail (size_t a, size_t b) pass, a, b); return false; } - if (buf.data != buf.__space) + if (buf.data != buf.__space.__c) { printf ("scratch_buffer_set_array_size did not free: %d %zu %zu\n", pass, a, b); -- 2.7.4
On Sep 19 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h > index bb04662..0f49dd9 100644 > --- a/include/scratch_buffer.h > +++ b/include/scratch_buffer.h > @@ -66,7 +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. */ > - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; > + union { max_align_t __a; char __c[1024]; } __space; If you make it an anonymous union you don't even need to change the rest of the code. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 19/09/2017 10:29, Andreas Schwab wrote: > On Sep 19 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h >> index bb04662..0f49dd9 100644 >> --- a/include/scratch_buffer.h >> +++ b/include/scratch_buffer.h >> @@ -66,7 +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. */ >> - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; >> + union { max_align_t __a; char __c[1024]; } __space; > > If you make it an anonymous union you don't even need to change the rest > of the code. > > Andreas. > Something like below maybe? --- * include/scratch_buffer.h (scratch_buffer): Use an union instead of a max_align_t array for __space definition. --- diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h index bb04662..cc394a7 100644 --- a/include/scratch_buffer.h +++ b/include/scratch_buffer.h @@ -66,7 +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. */ - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; + union { max_align_t __align; char __space[1024]; }; }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space
On 09/19/2017 07:34 PM, Adhemerval Zanella wrote: > - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; > + union { max_align_t __align; char __space[1024]; }; Does gnulib require compiler support for anonymous unions? (BTW, I view this kind of work largely as a distraction, which is why I'm against keeping things in sync with gnulib.) Thanks, Florian
On 09/19/2017 11:56 AM, Florian Weimer wrote: > Does gnulib require compiler support for anonymous unions? Unfortunately, no. Gnulib is intended to run on strict C99 and other platforms that do not support anonymous unions. There should be no trouble sticking with ordinary unions here, as all this stuff is private to scratch_buffer and the code is simple either way. > > (BTW, I view this kind of work largely as a distraction, which is why > I'm against keeping things in sync with gnulib.) Many of the recently-installed fixes came from Gnulib, and I hope that more such fixes will be on the way, so there is a practical advantage to staying in sync when that is reasonably easy, as it is here.
On 19/09/2017 16:16, Paul Eggert wrote: > On 09/19/2017 11:56 AM, Florian Weimer wrote: >> Does gnulib require compiler support for anonymous unions? > > Unfortunately, no. Gnulib is intended to run on strict C99 and other platforms that do not support anonymous unions. There should be no trouble sticking with ordinary unions here, as all this stuff is private to scratch_buffer and the code is simple either way. > >> >> (BTW, I view this kind of work largely as a distraction, which is why I'm against keeping things in sync with gnulib.) > > Many of the recently-installed fixes came from Gnulib, and I hope that more such fixes will be on the way, so there is a practical advantage to staying in sync when that is reasonably easy, as it is here. > Although I do see value in keep in sync with gnulib, with the cost of live with gnulib code constraints; the changes you mentioned came mostly because you pushed them without much discussion on libc-alpha and I decided to sync the changes back. Also, this very change we are discussing in another one you take from glibc and adjusted to gnulib without actually discuss it back on libc-alpha. I would expect, to make it easier for both glibc and gnulib, that once a patch is submitted on a maillist we finish consensus and pushed on the referred project and *after* we sync with either glibc/gnulib. It is hard and waster a lot of time trying to sync with gnulib once one push patches without much discussion on libc-alpha. However I would to iterate that I do appreciate the work you did on glob patches reviews, I would just like to ask if we can get a little more consensus on glibc part before actually commit the patches.
Adhemerval Zanella wrote: > the changes you mentioned came mostly because you pushed > them without much discussion From glibc's point of view I suggest thinking of Gnulib as being the "farm team". Gnulib is largely maintained by people who are drawn to it partly because it is simpler and doesn't have all the bureaucratic hassles that glibc does, so that one can fix bugs and add features more quickly and easily than one can in glibc. There are advantages to both projects' development styles of course. > I would expect, to make it easier for both glibc and gnulib, that once a patch > is submitted on a maillist we finish consensus and pushed on the referred > project and *after* we sync with either glibc/gnulib. I doubt whether we Gnulib folks would want to have a policy of waiting for the usual days (weeks?) for a glibc review before installing a glob fix. Often we have a software release to put out, and Gnulib is supposed to help, not to slow us down. With that in mind, perhaps it's better to continue keeping the collaboration close but not lock-step, so that the goal is for glob.c etc. to be identical in both projects, but we can tolerate short periods when this is not true. That being said, there's no rush in installing into Gnulib the current set of patches that you're proposing, and it's fine to wait for you to revise it along the lines suggested before installing it into both Gnulib and glibc.
On 19/09/2017 22:20, Paul Eggert wrote: > Adhemerval Zanella wrote: >> the changes you mentioned came mostly because you pushed >> them without much discussion > > From glibc's point of view I suggest thinking of Gnulib as being the "farm team". Gnulib is largely maintained by people who are drawn to it partly because it is simpler and doesn't have all the bureaucratic hassles that glibc does, so that one can fix bugs and add features more quickly and easily than one can in glibc. > > There are advantages to both projects' development styles of course. > >> I would expect, to make it easier for both glibc and gnulib, that once a patch is submitted on a maillist we finish consensus and pushed on the referred >> project and *after* we sync with either glibc/gnulib. > > I doubt whether we Gnulib folks would want to have a policy of waiting for the usual days (weeks?) for a glibc review before installing a glob fix. Often we have a software release to put out, and Gnulib is supposed to help, not to slow us down. With that in mind, perhaps it's better to continue keeping the collaboration close but not lock-step, so that the goal is for glob.c etc. to be identical in both projects, but we can tolerate short periods when this is not true. > > That being said, there's no rush in installing into Gnulib the current set of patches that you're proposing, and it's fine to wait for you to revise it along the lines suggested before installing it into both Gnulib and glibc. What I am proposing based on recent glob fixes is to just align a bit more with glibc and avoid push one side changes for the shared code. For instance, on this very change instead of push a slight modified version, it would be more productive if you just send a message stating your intention is to push this modified version and give us some days to see if anyone have anything to say. If thread get stalled, I think it safe to proceed on gnulib side and let us deal with any glibc requirement once we plan to sync it back. Now back to the patch topic, I think the first union version [1] should the most suitable one and it is what I am intended to push. [1] https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html
Adhemerval Zanella wrote: > Now back to the patch topic, I think the first union version [1] should > the most suitable one and it is what I am intended to push. > > [1]https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html The code for that looks fine for Gnulib. However, the commit message could be improved. It needs a summary line that does not mention _Alignas. "an union" should be "a union". There is no need to cite every use affected by the change; just say "All uses changed". It would be nice to credit Florian for pointing out the problem and Andreas for suggesting a solution. Attached is a proposed patch to Gnulib, which uses the same code as the abovementioned glibc patch, but which has an improved commit message; please use it as a guideline for the glibc commit. One more thing. Gnulib and other GNU projects are switching to using https: URLs to gnu.org and to fsf.org, and this causes the shared glob sources to differ in one line. Glibc should be fixed to use https: instead of http: for citations to GNU and FSF web sites. That should be a separate patch, of course. I'll look into it. From de22924261ba7cb489afdbc8fe9dd6013da6f1cb Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Thu, 21 Sep 2017 15:40:07 -0700 Subject: [PATCH] scratch_buffer: use union for internal buffer * lib/malloc/scratch_buffer.h (struct scratch_buffer): Use a union instead of a max_align_t array for __space, so that __space is the same size on all platforms. Problem reported by Florian Weimer in: https://sourceware.org/ml/libc-alpha/2017-09/msg00693.html Solution suggested by Andreas Schwab as discussed in: https://sourceware.org/ml/libc-alpha/2017-09/msg00695.html All uses changed. --- ChangeLog | 12 ++++++++++++ lib/malloc/scratch_buffer.h | 6 +++--- lib/malloc/scratch_buffer_grow_preserve.c | 4 ++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6a0b496..ecbab28 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2017-09-21 Adhemerval Zanella <adhemerval.zanella@linaro.org> + + scratch_buffer: use union for internal buffer + * lib/malloc/scratch_buffer.h (struct scratch_buffer): + Use an union instead of a max_align_t array for __space, + so that __space is the same size on all platforms. + Problem reported by Florian Weimer in: + https://sourceware.org/ml/libc-alpha/2017-09/msg00693.html + Solution suggested by Andreas Schwab as discussed in: + https://sourceware.org/ml/libc-alpha/2017-09/msg00695.html + All uses changed. + 2017-09-16 Paul Eggert <eggert@cs.ucla.edu> manywarnings: port to GCC on 64-bit MS-Windows diff --git a/lib/malloc/scratch_buffer.h b/lib/malloc/scratch_buffer.h index 206b320..7d9020c 100644 --- a/lib/malloc/scratch_buffer.h +++ b/lib/malloc/scratch_buffer.h @@ -66,7 +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. */ - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; + union { max_align_t __a; char __c[1024]; } __space; }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space @@ -74,7 +74,7 @@ struct scratch_buffer { static inline void scratch_buffer_init (struct scratch_buffer *buffer) { - buffer->data = buffer->__space; + buffer->data = buffer->__space.__c; buffer->length = sizeof (buffer->__space); } @@ -82,7 +82,7 @@ scratch_buffer_init (struct scratch_buffer *buffer) static inline void scratch_buffer_free (struct scratch_buffer *buffer) { - if (buffer->data != buffer->__space) + if (buffer->data != buffer->__space.__c) free (buffer->data); } diff --git a/lib/malloc/scratch_buffer_grow_preserve.c b/lib/malloc/scratch_buffer_grow_preserve.c index cf55cf2..c752bdd 100644 --- a/lib/malloc/scratch_buffer_grow_preserve.c +++ b/lib/malloc/scratch_buffer_grow_preserve.c @@ -30,14 +30,14 @@ __libc_scratch_buffer_grow_preserve (struct scratch_buffer *buffer) size_t new_length = 2 * buffer->length; void *new_ptr; - if (buffer->data == buffer->__space) + if (buffer->data == buffer->__space.__c) { /* Move buffer to the heap. No overflow is possible because buffer->length describes a small buffer on the stack. */ new_ptr = malloc (new_length); if (new_ptr == NULL) return false; - memcpy (new_ptr, buffer->__space, buffer->length); + memcpy (new_ptr, buffer->__space.__c, buffer->length); } else { -- 2.7.4
On 21/09/2017 19:53, Paul Eggert wrote: > Adhemerval Zanella wrote: >> Now back to the patch topic, I think the first union version [1] should >> the most suitable one and it is what I am intended to push. >> >> [1]https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html > > The code for that looks fine for Gnulib. However, the commit message could be improved. It needs a summary line that does not mention _Alignas. "an union" should be "a union". There is no need to cite every use affected by the change; just say "All uses changed". It would be nice to credit Florian for pointing out the problem and Andreas for suggesting a solution. > > Attached is a proposed patch to Gnulib, which uses the same code as the abovementioned glibc patch, but which has an improved commit message; please use it as a guideline for the glibc commit. Fair enough, I indeed had changes commit message but I hadn't the trouble to copied it on the message. I will use your suggestion. > > One more thing. Gnulib and other GNU projects are switching to using https: URLs to gnu.org and to fsf.org, and this causes the shared glob sources to differ in one line. Glibc should be fixed to use https: instead of http: for citations to GNU and FSF web sites. That should be a separate patch, of course. I'll look into it.
diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h index bb04662..2e0c8b5 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