diff mbox series

[RFC,02/11] libasm: Fix xdefault_pattern initialization

Message ID 20230206222513.1773039-3-iii@linux.ibm.com
State Superseded
Headers
Series Add Memory Sanitizer support |

Commit Message

Ilya Leoshkevich Feb. 6, 2023, 10:25 p.m. UTC 
  clang complains:

    asm_newscn.c:48:22: error: field 'pattern' with variable sized type 'struct FillPattern' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
      struct FillPattern pattern;
                         ^

Fix by using a union instead. Define the second union member to be a
char array 1 byte larger than struct FillPattern. This should be legal
according to 6.7.9:

    If an object that has static or thread storage duration is not
    initialized explicitly, then ... if it is a union, the first named
    member is initialized (recursively) according to these rules, and
    any padding is initialized to zero bits.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 libasm/asm_newscn.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Mark Wielaard Feb. 7, 2023, 7:41 p.m. UTC | #1 
Hi Ilya,

On Mon, Feb 06, 2023 at 11:25:04PM +0100, Ilya Leoshkevich via Elfutils-devel wrote:
> clang complains:
> 
>     asm_newscn.c:48:22: error: field 'pattern' with variable sized type 'struct FillPattern' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>       struct FillPattern pattern;
>                          ^
> 
> Fix by using a union instead. Define the second union member to be a
> char array 1 byte larger than struct FillPattern. This should be legal
> according to 6.7.9:
> 
>     If an object that has static or thread storage duration is not
>     initialized explicitly, then ... if it is a union, the first named
>     member is initialized (recursively) according to these rules, and
>     any padding is initialized to zero bits.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  libasm/asm_newscn.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
> index d258d969..32a3b598 100644
> --- a/libasm/asm_newscn.c
> +++ b/libasm/asm_newscn.c
> @@ -43,17 +43,16 @@
>  /* Memory for the default pattern.  The type uses a flexible array
>     which does work well with a static initializer.  So we play some
>     dirty tricks here.  */
> -static const struct
> +static const union
>  {
>    struct FillPattern pattern;
> -  char zero;
> +  char zeroes[sizeof(struct FillPattern) + 1];
>  } xdefault_pattern =
>    {
>      .pattern =
>      {
>        .len = 1
>      },
> -    .zero = '\0'
>    };

Yes, I think this works. Could you update the comment just before this
with some of the commit message explanation? Your explanation is much
better than "play some dirty trick" :)

>  const struct FillPattern *__libasm_default_pattern = &xdefault_pattern.pattern;

I am surprised this doesn't need a cast. Do you know why?

Thanks,

Mark
Ilya Leoshkevich Feb. 7, 2023, 7:49 p.m. UTC | #2 
On Tue, 2023-02-07 at 20:41 +0100, Mark Wielaard wrote:
> Hi Ilya,
> 
> On Mon, Feb 06, 2023 at 11:25:04PM +0100, Ilya Leoshkevich via
> Elfutils-devel wrote:
> > clang complains:
> > 
> >     asm_newscn.c:48:22: error: field 'pattern' with variable sized
> > type 'struct FillPattern' not at the end of a struct or class is a
> > GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> >       struct FillPattern pattern;
> >                          ^
> > 
> > Fix by using a union instead. Define the second union member to be
> > a
> > char array 1 byte larger than struct FillPattern. This should be
> > legal
> > according to 6.7.9:
> > 
> >     If an object that has static or thread storage duration is not
> >     initialized explicitly, then ... if it is a union, the first
> > named
> >     member is initialized (recursively) according to these rules,
> > and
> >     any padding is initialized to zero bits.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  libasm/asm_newscn.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
> > index d258d969..32a3b598 100644
> > --- a/libasm/asm_newscn.c
> > +++ b/libasm/asm_newscn.c
> > @@ -43,17 +43,16 @@
> >  /* Memory for the default pattern.  The type uses a flexible array
> >     which does work well with a static initializer.  So we play
> > some
> >     dirty tricks here.  */
> > -static const struct
> > +static const union
> >  {
> >    struct FillPattern pattern;
> > -  char zero;
> > +  char zeroes[sizeof(struct FillPattern) + 1];
> >  } xdefault_pattern =
> >    {
> >      .pattern =
> >      {
> >        .len = 1
> >      },
> > -    .zero = '\0'
> >    };
> 
> Yes, I think this works. Could you update the comment just before
> this
> with some of the commit message explanation? Your explanation is much
> better than "play some dirty trick" :)

Thanks, will do.

> >  const struct FillPattern *__libasm_default_pattern =
> > &xdefault_pattern.pattern;
> 
> I am surprised this doesn't need a cast. Do you know why?

We are referencing the union's .pattern member, not the entire union,
so the types still match.

> 
> Thanks,
> 
> Mark
diff mbox series

Patch

diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
index d258d969..32a3b598 100644
--- a/libasm/asm_newscn.c
+++ b/libasm/asm_newscn.c
@@ -43,17 +43,16 @@ 
 /* Memory for the default pattern.  The type uses a flexible array
    which does work well with a static initializer.  So we play some
    dirty tricks here.  */
-static const struct
+static const union
 {
   struct FillPattern pattern;
-  char zero;
+  char zeroes[sizeof(struct FillPattern) + 1];
 } xdefault_pattern =
   {
     .pattern =
     {
       .len = 1
     },
-    .zero = '\0'
   };
 const struct FillPattern *__libasm_default_pattern = &xdefault_pattern.pattern;