[PATCH] flexmember: update comment

[PATCH] flexmember: update comment

[Paul Eggert]

* m4/flexmember.m4 (AC_C_FLEXIBLE_ARRAY_MEMBER): Improve comment.
---
 ChangeLog        |  5 +++++
 m4/flexmember.m4 | 10 +++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0c4c515a0..efa81e2ae 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2019-05-24  Paul Eggert  <eggert@cs.ucla.edu>
+
+	flexmember: update comment
+	* m4/flexmember.m4 (AC_C_FLEXIBLE_ARRAY_MEMBER): Improve comment.
+
 2019-05-20  Bruno Haible  <bruno@clisp.org>
 
 	setlocale: Improve fallback on macOS.
diff --git a/m4/flexmember.m4 b/m4/flexmember.m4
index 1347068fe..ef6373df2 100644
--- a/m4/flexmember.m4
+++ b/m4/flexmember.m4
@@ -34,10 +34,14 @@ AC_DEFUN([AC_C_FLEXIBLE_ARRAY_MEMBER],
     AC_DEFINE([FLEXIBLE_ARRAY_MEMBER], [],
       [Define to nothing if C supports flexible array members, and to
        1 if it does not.  That way, with a declaration like 'struct s
-       { int n; double d@<:@FLEXIBLE_ARRAY_MEMBER@:>@; };', the struct hack
+       { int n; char d@<:@FLEXIBLE_ARRAY_MEMBER@:>@; };', the struct hack
        can be used with pre-C99 compilers.
-       When computing the size of such an object, don't use 'sizeof (struct s)'
-       as it overestimates the size.  Use 'offsetof (struct s, d)' instead.
+       Use 'FLEXSIZEOF (struct s, d, N)' to calculate the size in bytes
+       of such a struct containing an N-element array, as both
+       'sizeof (struct s) + N * sizeof (char)' and
+       'offsetof (struct s, d) + N * sizeof (char)'
+       might compute a size that can cause malloc to align storage
+       improperly, even in C11.
        Don't use 'offsetof (struct s, d@<:@0@:>@)', as this doesn't work with
        MSVC and with C++ compilers.])
   else
-- 
2.21.0

Re: [PATCH] flexmember: update comment

[Bruno Haible]

Hi Paul,

> +       Use 'FLEXSIZEOF (struct s, d, N)' to calculate the size in bytes
> +       of such a struct containing an N-element array, as both
> +       'sizeof (struct s) + N * sizeof (char)' and
> +       'offsetof (struct s, d) + N * sizeof (char)'
> +       might compute a size that can cause malloc to align storage
> +       improperly, even in C11.

I'm confused.

1) What is the alignment problem if the array element type is 'char'?
I would understand an alignment problem if it is 'double'. But with 'char'?
'char' has the size 1, and - except on m68k - also the alignment 1.

2) If
     (struct s *) malloc (offsetof (struct s, d) + N * sizeof (char))
should be avoided in favour of
     (struct s *) malloc (FLEXSIZEOF (struct s, d, N))
don't we need to apply a 'ceil'-like alignment to the malloc result?

Bruno

Re: [PATCH] flexmember: update comment

[Paul Eggert]

On 5/24/19 3:17 PM, Bruno Haible wrote:
> 1) What is the alignment problem if the array element type is 'char'?

Suppose we have a platform where the alignment of each basic type is 
equal to its size, where sizeof (int) == 4 and sizeof (char) == 1, and 
where we have 'struct s { int n; char d[]; };' and suppose we want to 
allocate a 'struct s' with a 3-element flexible array member 'd'. Then 
'offsetof (struct s, d) + 3 * sizeof (char)' is 7, and when we call 
'malloc (7)' malloc is entitled to assume that memory is being allocated 
for an array of 7 one-byte objects, so it can return an address that is 
not a multiple of 4, which means that the pointer that malloc returns is 
invalid for 'struct s *'. To fix the problem, we can use 'malloc 
(FLEXSIZEOF (struct s, d, 3))' instead, as FLEXSIZEOF yields 8 on this 
platform, and malloc (8) must return an address that is a multiple of 4.

> don't we need to apply a 'ceil'-like alignment to the malloc result?
No, because if malloc is given an argument like 8 that is a multiple of 
sizeof (int), it must return a pointer suitable for 'int *'.

Perhaps I should update the comment to make this all clearer, though it 
is hard to be clear and accurate and terse in this area.

Re: [PATCH] flexmember: update comment

[Bruno Haible]

Hi Paul,

> > 1) What is the alignment problem if the array element type is 'char'?
> 
> Suppose we have a platform where the alignment of each basic type is 
> equal to its size, where sizeof (int) == 4 and sizeof (char) == 1, and 
> where we have 'struct s { int n; char d[]; };' and suppose we want to 
> allocate a 'struct s' with a 3-element flexible array member 'd'. Then 
> 'offsetof (struct s, d) + 3 * sizeof (char)' is 7, and when we call 
> 'malloc (7)' malloc is entitled to assume that memory is being allocated 
> for an array of 7 one-byte objects, so it can return an address that is 
> not a multiple of 4, which means that the pointer that malloc returns is 
> invalid for 'struct s *'. To fix the problem, we can use 'malloc 
> (FLEXSIZEOF (struct s, d, 3))' instead, as FLEXSIZEOF yields 8 on this 
> platform, and malloc (8) must return an address that is a multiple of 4.
> 
> > don't we need to apply a 'ceil'-like alignment to the malloc result?
> No, because if malloc is given an argument like 8 that is a multiple of 
> sizeof (int), it must return a pointer suitable for 'int *'.

Oh, I see. Thanks. I was assuming that malloc (N) always returns a
multiple of max_align_t.

> Perhaps I should update the comment to make this all clearer, though it 
> is hard to be clear and accurate and terse in this area.

You found the right words now :) - I would expect to see such
explanations in the .h file, not in the .m4 file.

Bruno

Re: [PATCH] flexmember: update comment

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 203 bytes --]

On 5/24/19 4:33 PM, Bruno Haible wrote:
> You found the right words now :) - I would expect to see such
> explanations in the .h file, not in the .m4 file.
I gave that a shot by installing the attached.

[-- Attachment #2: 0001-flexmember-update-comments-again.patch --]
[-- Type: text/x-patch, Size: 3772 bytes --]

From eb7977e908bd3b2a1367ab2871647e76055b4ba0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 24 May 2019 17:31:34 -0700
Subject: [PATCH] flexmember: update comments again

* lib/flexmember.h, m4/flexmember.m4: Improve comments further.
---
 ChangeLog        |  3 +++
 lib/flexmember.h | 25 ++++++++++++++++++++-----
 m4/flexmember.m4 | 12 +++---------
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index efa81e2ae..2cd6145c7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2019-05-24  Paul Eggert  <eggert@cs.ucla.edu>
 
+	flexmember: update comments again
+	* lib/flexmember.h, m4/flexmember.m4: Improve comments further.
+
 	flexmember: update comment
 	* m4/flexmember.m4 (AC_C_FLEXIBLE_ARRAY_MEMBER): Improve comment.
 
diff --git a/lib/flexmember.h b/lib/flexmember.h
index 8e79bce03..431bec11e 100644
--- a/lib/flexmember.h
+++ b/lib/flexmember.h
@@ -33,11 +33,26 @@
 # define FLEXALIGNOF(type) _Alignof (type)
 #endif
 
-/* Upper bound on the size of a struct of type TYPE with a flexible
-   array member named MEMBER that is followed by N bytes of other data.
-   This is not simply sizeof (TYPE) + N, since it may require
-   alignment on unusually picky C11 platforms, and
-   FLEXIBLE_ARRAY_MEMBER may be 1 on pre-C11 platforms.
+/* Yield a properly aligned upper bound on the size of a struct of
+   type TYPE with a flexible array member named MEMBER that is
+   followed by N bytes of other data.  The result is suitable as an
+   argument to malloc.  For example:
+
+     struct s { int n; char d[FLEXIBLE_ARRAY_MEMBER]; };
+     struct s *p = malloc (FLEXSIZEOF (struct s, d, n * sizeof (char)));
+
+   FLEXSIZEOF (TYPE, MEMBER, N) is not simply (sizeof (TYPE) + N),
+   since FLEXIBLE_ARRAY_MEMBER may be 1 on pre-C11 platforms.  Nor is
+   it simply (offsetof (TYPE, MEMBER) + N), as that might yield a size
+   that causes malloc to yield a pointer that is not properly aligned
+   for TYPE; for example, if sizeof (int) == alignof (int) == 4,
+   malloc (offsetof (struct s, d) + 3 * sizeof (char)) is equivalent
+   to malloc (7) and might yield a pointer that is not a multiple of 4
+   (which means the pointer is not properly aligned for struct s),
+   whereas malloc (FLEXSIZEOF (struct s, d, 3 * sizeof (char))) is
+   equivalent to malloc (8) and must yield a pointer that is a
+   multiple of 4.
+
    Yield a value less than N if and only if arithmetic overflow occurs.  */
 
 #define FLEXSIZEOF(type, member, n) \
diff --git a/m4/flexmember.m4 b/m4/flexmember.m4
index ef6373df2..c245ab025 100644
--- a/m4/flexmember.m4
+++ b/m4/flexmember.m4
@@ -34,16 +34,10 @@ AC_DEFUN([AC_C_FLEXIBLE_ARRAY_MEMBER],
     AC_DEFINE([FLEXIBLE_ARRAY_MEMBER], [],
       [Define to nothing if C supports flexible array members, and to
        1 if it does not.  That way, with a declaration like 'struct s
-       { int n; char d@<:@FLEXIBLE_ARRAY_MEMBER@:>@; };', the struct hack
+       { int n; short d@<:@FLEXIBLE_ARRAY_MEMBER@:>@; };', the struct hack
        can be used with pre-C99 compilers.
-       Use 'FLEXSIZEOF (struct s, d, N)' to calculate the size in bytes
-       of such a struct containing an N-element array, as both
-       'sizeof (struct s) + N * sizeof (char)' and
-       'offsetof (struct s, d) + N * sizeof (char)'
-       might compute a size that can cause malloc to align storage
-       improperly, even in C11.
-       Don't use 'offsetof (struct s, d@<:@0@:>@)', as this doesn't work with
-       MSVC and with C++ compilers.])
+       Use 'FLEXSIZEOF (struct s, d, N * sizeof (short))' to calculate
+       the size in bytes of such a struct containing an N-element array.])
   else
     AC_DEFINE([FLEXIBLE_ARRAY_MEMBER], [1])
   fi
-- 
2.21.0

Re: [PATCH] flexmember: update comment

[Bruno Haible]

Hi Paul,

> I gave that a shot by installing the attached.

Thanks.

I found the relevant references now, in [1]. The fact that some people see it not
as a property of the malloc() function but as a GCC bug [2] indicates that it should
also apply when alloca() is used to allocate the memory. (In this case, a program
would not crash when it makes out-of-bounds accesses on the stack, but valgrind
may still report accesses to uninitialized memory.)

Bruno

[1] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=4c32543a4f05706ca3a8c8d583a5fb35d0e58828
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66661