diff mbox

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

Message ID 53984fd3-83cf-1688-1aa4-5ccb7b72e416@cs.ucla.edu
State New, archived
Headers show

Commit Message

Paul Eggert Sept. 21, 2017, 10:53 p.m. UTC 
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

Adhemerval Zanella Netto Sept. 22, 2017, 12:10 p.m. UTC | #1 
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 mbox

Patch

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