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

[Jozef Lawrynowicz <jozef.l@mittosystems.com>]

On Fri, 5 Jul 2019 11:26:20 +0200
Christophe Lyon <christophe.lyon@linaro.org> wrote:

> On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote:
> >
> > 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)

Were there previous discussions on whether the noinit attribute should be
explicitly enabled for certain targets? e.g. so it will error if you try to use
it on x86_64.
If it will just be enabled by default for all targets then a hardcoded
list for check-effective target sounds ok. Otherwise if it does cause an error
when used on an unsupported target you could do a check that the
attribute compiles successfully instead.

> 
> > 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?

Hmm yes there seems to be an issue with trunks handling of noinit for msp430.
Even before your patch the static noinit variables are marked as
common symbols with ".comm" and are not placed in .noinit.

These static noinit variables work with my downstream msp430-gcc (which doesn't
yet have parity with upstream), so this is just something I'll fix separately,
and doesn't represent any issues with your patch.

Jozef