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

  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(+)
  

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.
  

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.