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

[Jozef Lawrynowicz <jozef dot l at mittosystems dot com>]

Hi,

On Tue, 30 Jul 2019 15:35:23 +0200
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.

BTW, regarding this, I have rearranged msp430_select_section in the attached WIP
patch, so that it will use default_elf_select_section to handle "noinit". Once
your patch is applied, I'll submit some variant of this patch.

Still, better leave the FIXME in, and I'll remove it in my patch.

Likewise for all the other fixes required in msp430.c, once this patch is
applied, I'll fix those up.

Thanks,
Jozef

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

diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 5ccb02997ad..8b8e9aa992e 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -1730,6 +1730,10 @@ msp430_init_sections (void)
 static section *
 msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align)
 {
+  const char * prefix;
+  const char * sec_name;
+  const char * base_sec_name;
+
   gcc_assert (decl != NULL_TREE);
 
   if (TREE_CODE (decl) == STRING_CST
@@ -1744,29 +1748,41 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align)
   if (TARGET_LARGE && TREE_CODE (decl) == FUNCTION_DECL && is_interrupt_func (decl))
     return get_section (".lowtext", SECTION_CODE | SECTION_WRITE , decl);
 
-  const char * prefix = gen_prefix (decl);
-  if (prefix == NULL)
-    {
-      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))
-	return persist_section;
-      else
-	return default_select_section (decl, reloc, align);
-    }
+  if (has_attr (ATTR_PERSIST, decl))
+    return persist_section;
+
+  /* ATTR_NOINIT is handled generically.  */
+  if (has_attr (ATTR_NOINIT, decl))
+    return default_elf_select_section (decl, reloc, align);
+
+  prefix = gen_prefix (decl);
   
-  const char * sec;
   switch (categorize_decl_for_section (decl, reloc))
     {
-    case SECCAT_TEXT:   sec = ".text";   break;
-    case SECCAT_DATA:   sec = ".data";   break;
-    case SECCAT_BSS:    sec = ".bss";    break;
-    case SECCAT_RODATA: sec = ".rodata"; break;
+    case SECCAT_TEXT:
+      if (!prefix)
+	return text_section;
+      base_sec_name = ".text";
+      break;
+    case SECCAT_DATA:
+      if (!prefix)
+	return data_section;
+      base_sec_name = ".data";
+      break;
+    case SECCAT_BSS:
+      if (!prefix)
+	return bss_section;
+      base_sec_name = ".bss";
+      break;
+    case SECCAT_RODATA:
+      if (!prefix)
+	return readonly_data_section;
+      base_sec_name = ".rodata";
+      break;
 
+    /* These sections are not supported (?).  They should not be generated,
+       but in case they are, we use default_select_section so they get placed
+       in sections the msp430 assembler and linker understand.  */
     case SECCAT_RODATA_MERGE_STR:
     case SECCAT_RODATA_MERGE_STR_INIT:
     case SECCAT_RODATA_MERGE_CONST:
@@ -1785,10 +1801,9 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align)
       gcc_unreachable ();
     }
   
-  const char * dec_name = DECL_SECTION_NAME (decl);
-  char * name = ACONCAT ((prefix, sec, dec_name, NULL));
+  sec_name = ACONCAT ((prefix, base_sec_name, DECL_SECTION_NAME (decl), NULL));
 
-  return get_named_section (decl, name, 0);
+  return get_named_section (decl, sec_name, 0);
 }
 
 #undef  TARGET_ASM_FUNCTION_SECTION