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

[Christophe Lyon <christophe.lyon@linaro.org>]

Ping?

On Tue, 30 Jul 2019 at 15:35, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> Hi,
>
> Thanks for the useful feedback.
>
>
> On Tue, 16 Jul 2019 at 11:54, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > Thanks for doing this in a generic way.
> >
> > Christophe Lyon <christophe.lyon@linaro.org> writes:
> > > @@ -2224,6 +2234,50 @@ handle_weak_attribute (tree *node, tree name,
> > >    return NULL_TREE;
> > >  }
> > >
> > > +/* Handle a "noinit" attribute; arguments as in struct
> > > +   attribute_spec.handler.  Check whether the attribute is allowed
> > > +   here and add the attribute to the variable decl tree or otherwise
> > > +   issue a diagnostic.  This function checks NODE is of the expected
> > > +   type and issues diagnostics otherwise using NAME.  If it is not of
> > > +   the expected type *NO_ADD_ATTRS will be set to true.  */
> > > +
> > > +static tree
> > > +handle_noinit_attribute (tree * node,
> > > +               tree   name,
> > > +               tree   args,
> > > +               int    flags ATTRIBUTE_UNUSED,
> > > +               bool *no_add_attrs)
> > > +{
> > > +  const char *message = NULL;
> > > +
> > > +  gcc_assert (DECL_P (*node));
> > > +  gcc_assert (args == NULL);
> > > +
> > > +  if (TREE_CODE (*node) != VAR_DECL)
> > > +    message = G_("%qE attribute only applies to variables");
> > > +
> > > +  /* Check that it's possible for the variable to have a section.  */
> > > +  if ((TREE_STATIC (*node) || DECL_EXTERNAL (*node) || in_lto_p)
> > > +      && DECL_SECTION_NAME (*node))
> > > +    message = G_("%qE attribute cannot be applied to variables "
> > > +              "with specific sections");
> > > +
> > > +  /* If this var is thought to be common, then change this.  Common
> > > +     variables are assigned to sections before the backend has a
> > > +     chance to process them.  */
> > > +  if (DECL_COMMON (*node))
> > > +    DECL_COMMON (*node) = 0;
> > > +
> > > +  if (message)
> > > +    {
> > > +      warning (OPT_Wattributes, message, name);
> > > +      *no_add_attrs = true;
> > > +    }
> > > +
> > > +  return NULL_TREE;
> > > +}
> >
> > This might cause us to clear DECL_COMMON even when rejecting the
> > attribute.  Also, the first message should win over the others
> > (e.g. for a function in a particular section).
> >
> > So I think the "/* Check that it's possible ..." should be an else-if
> > and the DECL_COMMON stuff should be specific to !message.
>
> You are right, thanks.
>
> Jozef, this is true for msp430_data_attr() too. I'll let you update it.
>
> >
> > Since this is specific to ELF targets, I think we should also
> > warn if !targetm.have_switchable_bss_sections.
> >
> OK
>
> > > @@ -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))
> >
> > I guess adding a FIXME is OK.  It's very tempting to remove
> > the noinit stuff and use default_elf_select_section instead of
> > default_select_section, but I realise that'd be difficult to test.
>
> I added that as suggested by Jozef, it's best if he handles the
> change you propose, I guess he can do proper testing.
>
>
> > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > > index f2619e1..850153e 100644
> > > --- a/gcc/doc/extend.texi
> > > +++ b/gcc/doc/extend.texi
> > > @@ -7129,6 +7129,12 @@ The @code{visibility} attribute is described in
> > >  The @code{weak} attribute is described in
> > >  @ref{Common Function Attributes}.
> > >
> > > +@item noinit
> > > +@cindex @code{noinit} variable attribute
> > > +Any data with the @code{noinit} attribute will not be initialized by
> > > +the C runtime startup code, or the program loader.  Not initializing
> > > +data in this way can reduce program startup times.
> > > +
> > >  @end table
> > >
> > >  @node ARC Variable Attributes
> >
> > Would be good to mention that the attribute is specific to ELF targets.
> > (Yeah, we don't seem to do that consistently for other attributes.)
> > It might also be worth saying that it requires specific linker support.
> OK, done.
>
> >
> > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c
> > > new file mode 100644
> > > index 0000000..f33c0d0
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c
> > > @@ -0,0 +1,59 @@
> > > +/* { dg-do run } */
> > > +/* { dg-require-effective-target noinit */
> > > +/* { dg-options "-O2" } */
> > > +
> > > +/* This test checks that noinit data is handled correctly.  */
> > > +
> > > +extern void _start (void) __attribute__ ((noreturn));
> > > +extern void abort (void) __attribute__ ((noreturn));
> > > +extern void exit (int) __attribute__ ((noreturn));
> > > +
> > > +int var_common;
> > > +int var_zero = 0;
> > > +int var_one = 1;
> > > +int __attribute__((noinit)) var_noinit;
> > > +int var_init = 2;
> > > +
> > > +int __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */
> > > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */
> > > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */
> > > +
> > > +
> > > +int
> > > +main (void)
> > > +{
> > > +  /* Make sure that the C startup code has correctly initialised the ordinary variables.  */
> >
> > initialized (alas).  Same for the rest of the file.
>
> That was a copy-and-paste from msp430 testcase; Jozef, you have 3
> typos to fix :-)
>
> > > +  if (var_common != 0)
> > > +    abort ();
> > > +
> > > +  /* Initialised variables are not re-initialised during startup, so
> > > +     check their original values only during the first run of this
> > > +     test.  */
> > > +  if (var_init == 2)
> > > +    if (var_zero != 0 || var_one != 1)
> > > +      abort ();
> > > +
> > > +  switch (var_init)
> > > +    {
> > > +    case 2:
> > > +      /* First time through - change all the values.  */
> > > +      var_common = var_zero = var_one = var_noinit = var_init = 3;
> > > +      break;
> > > +
> > > +    case 3:
> > > +      /* Second time through - make sure that d has not been reset.  */
> > > +      if (var_noinit != 3)
> > > +     abort ();
> > > +      exit (0);
> > > +
> > > +    default:
> > > +      /* Any other value for var_init is an error.  */
> > > +      abort ();
> > > +    }
> > > +
> > > +  /* Simulate a processor reset by calling the C startup code.  */
> > > +  _start ();
> > > +
> > > +  /* Should never reach here.  */
> > > +  abort ();
> > > +}
> > > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> > > index 815e837..ae05c0a 100644
> > > --- a/gcc/testsuite/lib/target-supports.exp
> > > +++ b/gcc/testsuite/lib/target-supports.exp
> > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } {
> > >      return [check_weak_available]
> > >  }
> > >
> > > +# The noinit attribute is only supported by some targets.
> > > +# This proc returns 1 if it's supported, 0 if it's not.
> > > +
> > > +proc check_effective_target_noinit { } {
> > > +    if { [istarget arm*-*-eabi]
> > > +      || [istarget msp430-*-*] } {
> > > +     return 1
> > > +    }
> > > +
> > > +    return 0
> > > +}
> > > +
> >
> > Should be documented in sourcebuild.texi.  (Sometimes wonder how many
> > people actually use that instead of just reading this file.)
> >
> Sigh..... I keep forgetting this.
>
> > > diff --git a/gcc/varasm.c b/gcc/varasm.c
> > > index 626a4c9..7740e88 100644
> > > --- a/gcc/varasm.c
> > > +++ b/gcc/varasm.c
> > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc)
> > >        || strncmp (name, ".gnu.linkonce.tb.", 17) == 0)
> > >      flags |= SECTION_TLS | SECTION_BSS;
> > >
> > > +  if (strcmp (name, ".noinit") == 0)
> > > +    flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE;
> > > +
> > >    /* Various sections have special ELF types that the assembler will
> > >       assign by default based on the name.  They are neither SHT_PROGBITS
> > >       nor SHT_NOBITS, so when changing sections we don't want to print a
> > > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc)
> > >
> > >  /* Select a section based on the above categorization.  */
> > >
> > > +static section *noinit_section = NULL;
> > > +
> > >  section *
> > >  default_elf_select_section (tree decl, int reloc,
> > >                           unsigned HOST_WIDE_INT align)
> > >  {
> > >    const char *sname;
> > > +
> > >    switch (categorize_decl_for_section (decl, reloc))
> > >      {
> > >      case SECCAT_TEXT:
> > > @@ -6790,6 +6796,14 @@ default_elf_select_section (tree decl, int reloc,
> > >        sname = ".tdata";
> > >        break;
> > >      case SECCAT_BSS:
> > > +      if (DECL_P (decl)
> > > +       && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE)
> > > +     {
> > > +       if (noinit_section == NULL)
> > > +         noinit_section = get_named_section (decl, ".noinit", reloc);
> > > +       return noinit_section;
> > > +     }
> > > +
> >
> > I don't think the special global for noinit_section is worth it, since
> > gen_named_section does its own caching.  So IMO we should just have:
> >
> >   name = ".noinit";
> >   break;
> OK
>
> > Did you consider supporting .noinit.*, e.g. for -fdata-sections?
> Not so far. I don't think we have received such a request yet?
>
> Thanks,
>
> Christophe
>
> > Thanks,
> > Richard