On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <joze...@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
