diff mbox series

[4/9] Sync scratch_buffer with gnulib

Message ID 1504643122-14874-5-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series posix: glob fixes and refactor | expand

Commit Message

Adhemerval Zanella Netto Sept. 5, 2017, 8:25 p.m. UTC
This patch syncs the scratch_buffer grom gnulib commit 3866ef6 with
GLIBC code.

Checked on x86_64-linux-gnu and on a build using build-many-glibcs.py
for all major architectures.

	* include/scratch_buffer.h (scratch_buffer): Use a C99 align method
	instead of GCC extension.
	* malloc/scratch_buffer_grow.c [!_LIBC]: Include libc-config.h.
	* malloc/scratch_buffer_grow_preserve.c [!_LIBC]: Likewise.
	* malloc/scratch_buffer_set_array_size.c [!_LIBC]: Likewise.
---
 ChangeLog                              | 6 ++++++
 include/scratch_buffer.h               | 3 +--
 malloc/scratch_buffer_grow.c           | 6 +++++-
 malloc/scratch_buffer_grow_preserve.c  | 6 +++++-
 malloc/scratch_buffer_set_array_size.c | 6 +++++-
 5 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.7.4

Comments

Florian Weimer Sept. 18, 2017, 6:09 a.m. UTC | #1
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
Adhemerval Zanella Netto Sept. 18, 2017, 11:43 a.m. UTC | #2
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
Florian Weimer Sept. 18, 2017, 11:57 a.m. UTC | #3
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
Adhemerval Zanella Netto Sept. 18, 2017, 12:25 p.m. UTC | #4
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 mbox series

Patch

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)