[2/2] Use C11 _Alignas on scratch_buffer internal buffer
Commit Message
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.
Comments
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.
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(-)
@@ -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
@@ -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);
}
@@ -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