[2/2] Use C11 _Alignas on scratch_buffer internal buffer

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
Related show

Commit Message

Adhemerval Zanella Sept. 18, 2017, 2:42 p.m.
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(+)

-- 
2.7.4

Comments

Florian Weimer Sept. 18, 2017, 3:21 p.m. | #1
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.
Paul Eggert Sept. 18, 2017, 4:10 p.m. | #2
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.
Florian Weimer Sept. 18, 2017, 4:41 p.m. | #3
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
Paul Eggert Sept. 18, 2017, 4:57 p.m. | #4
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.
Andreas Schwab Sept. 18, 2017, 5:06 p.m. | #5
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."
Florian Weimer Sept. 18, 2017, 5:09 p.m. | #6
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
Paul Eggert Sept. 18, 2017, 5:24 p.m. | #7
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.
Adhemerval Zanella Sept. 19, 2017, 1:08 p.m. | #8
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
Andreas Schwab Sept. 19, 2017, 1:29 p.m. | #9
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."
Adhemerval Zanella Sept. 19, 2017, 5:34 p.m. | #10
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
Florian Weimer Sept. 19, 2017, 6:56 p.m. | #11
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
Paul Eggert Sept. 19, 2017, 7:16 p.m. | #12
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.
Adhemerval Zanella Sept. 19, 2017, 7:57 p.m. | #13
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.
Paul Eggert Sept. 20, 2017, 1:20 a.m. | #14
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.
Adhemerval Zanella Sept. 21, 2017, 9:33 p.m. | #15
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
Paul Eggert Sept. 21, 2017, 10:53 p.m. | #16
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
Adhemerval Zanella Sept. 22, 2017, 12:10 p.m. | #17
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.

Patch

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