Re: [PATCH] Add generic support for "noinit" attribute

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote:
>
> Hi,
>
> > diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
> > index 365e9eb..8266fa0 100644
> > --- a/gcc/config/msp430/msp430.c
> > +++ b/gcc/config/msp430/msp430.c
> > @@ -1807,7 +1807,6 @@ const char * const  ATTR_CRIT   = "critical";
> >  const char * const  ATTR_LOWER  = "lower";
> >  const char * const  ATTR_UPPER  = "upper";
> >  const char * const  ATTR_EITHER = "either";
> > -const char * const  ATTR_NOINIT = "noinit";
> >  const char * const  ATTR_PERSIST = "persistent";
> >
> >  static inline bool
>
> With this change removed so that ATTR_NOINIT is still defined, your patch is
> ok for msp430 - the attribute still behaves as expected.
Oops sorry for this.

> I'm holding off using default_elf_select_section instead of
> default_select_section in msp430.c since there could be some possible conflicts
> with the MSPABI introduced by using elf_select_section that need to be properly
> considered before making the change.
>
> I think default_select_section is supposed to be very terse, so I'm not sure
> if it would be even be suitable to extend it to handle noinit.
Yes, that was my worry too.

> With that said, could you please add the following FIXME to your patch:
OK

> diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
> index 365e9eba747..b403b1f5a42 100644
> --- a/gcc/config/msp430/msp430.c
> +++ b/gcc/config/msp430/msp430.c
> @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned
> HOST_WIDE_INT align) {
>        if (TREE_CODE (decl) == FUNCTION_DECL)
>         return text_section;
> +      /* FIXME: ATTR_NOINIT is handled generically in
> +         default_elf_select_section.  */
>        else if (has_attr (ATTR_NOINIT, decl))
>         return noinit_section;
>        else if (has_attr (ATTR_PERSIST, decl))
>
> Also, the gcc.target/arm/noinit-attribute.c test works with msp430.
> Why not create a effective-target keyword which checks for noinit support, so
> the test can be gated on that condition and put in a generic part of the
> testsuite?

That was my intention, and I was waiting for feedback on this. In this
case, I suppose the effective-target check would test a hardcoded list
of targets, like many other effective-targets?
(eg check_weak_available)

> After doing some further testing, I noticed the attribute does not work on
> static variables. The attribute handling code allows the attribute to be set on
> a static variable, but then that variable does not get placed in the .noinit
> section.
>
> What are your thoughts on this?
>
> Does it even makes sense for a static local variable to be declared as "noinit"?
> One should want a static local variable to be initialized, as having it not
> initialized and hold a random value could cause problems.
> Perhaps a user could think of some use for this, but I can't.
>
I added:
static int __attribute__((noinit)) var_noinit2;
in global scope, and
static int __attribute__((noinit)) var_noinit3;
in main(), and the generate code contains:
        .section        .noinit,"aw"
        .align  2
        .set    .LANCHOR2,. + 0
        .type   var_noinit2, %object
        .size   var_noinit2, 4
var_noinit2:
        .space  4
        .type   var_noinit3.4160, %object
        .size   var_noinit3.4160, 4
var_noinit3.4160:
        .space  4
        .type   var_noinit, %object
        .size   var_noinit, 4
var_noinit:
        .space  4

So, all three of them are in the .noinit section, but only var_noinit has
        .global var_noinit

So it seems to work?

> Finally, I would point out that "contrib/check_GNU_style.sh" reports some style
> issues with your patch.

Thanks for noticing.

>
> Thanks and regards,
> Jozef