[PATCH] Add generic support for "noinit" attribute

[PATCH] Add generic support for "noinit" attribute

[Christophe Lyon]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

Hi,

Similar to what already exists for TI msp430 or in TI compilers for
arm, this patch adds support for the "noinit" attribute.

It is convenient for embedded targets where the user wants to keep the
value of some data when the program is restarted: such variables are
not zero-initialized. It is mostly a helper/shortcut to placing
variables in a dedicated section.

It's probably desirable to add the following chunk to the GNU linker:
diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
index 272a8bc..9555cec 100644
--- a/ld/emulparams/armelf.sh
+++ b/ld/emulparams/armelf.sh
@@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
*(.vfp11_veneer) *(.v4_bx)'
 OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
.${CREATE_SHLIB+)};"
 OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
.${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
.${CREATE_SHLIB+)};"
 OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ = .${CREATE_SHLIB+)};"
 -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }'
 +OTHER_SECTIONS='
 +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
 +  /* This section contains data that is not initialised during load
 +     *or* application reset.  */
 +   .noinit (NOLOAD) :
 +   {
 +     . = ALIGN(2);
 +     PROVIDE (__noinit_start = .);
 +     *(.noinit)
 +     . = ALIGN(2);
 +     PROVIDE (__noinit_end = .);
 +   }
 +'

so that the noinit section has the "NOLOAD" flag.

I'll submit that part separately to the binutils project if OK.

However, I'm not sure for which other targets (beyond arm), I should
update the linker scripts.

I left the new testcase in gcc.target/arm, guarded by
dg-do run { target { *-*-eabi } }
but since this attribute is now generic, I suspect I should move the
test to some other place. But then, which target selector should I
use?

Finally, I tested on arm-eabi, but not on msp430 for which I do not
have the environment, so advice from msp430 maintainers is
appreciated. Since msp430 does not use the same default helpers as
arm, I left the "noinit" handling code in place, to avoid changing
other generic functions which I don't know how to test
(default_select_section, default_section_type_flags).

Thanks,

Christophe

gcc/ChangeLog:

2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>

* doc/extend.texi: Add "noinit" attribute documentation.
* varasm.c
(default_section_type_flags): Add support for "noinit" section.
(default_elf_select_section): Add support for "noinit" attribute.

gcc/c-family/ChangeLog:

2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>

* c-attribs.c (c_common_attribute_table): Add "noinit" entry.
(handle_section_attribute): Add support for "noinit" attribute.
(handle_noinit_attribute): New function.

gcc/config/ChangeLog:

2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>

* msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.

gcc/testsuite/ChangeLog:

2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>

* gcc.target/arm/noinit-attribute.c: New test.

[-- Attachment #2: noinit-generic.patch.txt --]
[-- Type: text/plain, Size: 8040 bytes --]

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 48819e7..3aefe84 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -92,6 +92,7 @@ static tree handle_section_attribute (tree *, tree, tree, int, bool *);
 static tree handle_aligned_attribute (tree *, tree, tree, int, bool *);
 static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree,
 						  int, bool *);
+static tree handle_noinit_attribute (tree *, tree, tree, int, bool *);
 static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *);
@@ -458,6 +459,8 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_nocf_check_attribute, NULL },
   { "copy",                   1, 1, false, false, false, false,
 			      handle_copy_attribute, NULL },
+  { "noinit",                 0, 0, true,  false, false, false,
+			      handle_noinit_attribute, NULL },
   { NULL,                     0, 0, false, false, false, false, NULL, NULL }
 };
 
@@ -1912,6 +1915,13 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
       goto fail;
     }
 
+  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE)
+    {
+      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+		"section attribute cannot be applied to variables with noinit attribute");
+      goto fail;
+    }
+
   set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
   return NULL_TREE;
 
@@ -2224,6 +2234,48 @@ 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;
+}
+
+
 /* Handle a "noplt" attribute; arguments as in
    struct attribute_spec.handler.  */
 
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
@@ -2108,8 +2107,6 @@ const struct attribute_spec msp430_attribute_table[] =
   { ATTR_EITHER,      0, 0, true,  false, false, false, msp430_section_attr,
     NULL },
 
-  { ATTR_NOINIT,      0, 0, true,  false, false, false, msp430_data_attr,
-    NULL },
   { ATTR_PERSIST,     0, 0, true,  false, false, false, msp430_data_attr,
     NULL },
 
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
diff --git a/gcc/testsuite/gcc.target/arm/noinit-attribute.c b/gcc/testsuite/gcc.target/arm/noinit-attribute.c
new file mode 100644
index 0000000..242de2f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/noinit-attribute.c
@@ -0,0 +1,56 @@
+/* { dg-do run { target { *-*-eabi } } } */
+/* { 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 "attribute cannot be applied to variables with specific sections" } */
+int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "attribute cannot be applied to variables with noinit attribute" } */
+
+
+int
+main (void)
+{
+  /* Make sure that the C startup code has correctly initialised the ordinary variables.  */
+  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/varasm.c b/gcc/varasm.c
index 626a4c9..d013486 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,13 @@ 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;
+	}
+
       if (bss_section)
 	return bss_section;
       sname = ".bss";

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

[Jozef Lawrynowicz]

On Thu, 4 Jul 2019 17:27:28 +0200
Christophe Lyon <christophe.lyon@linaro.org> wrote:

> Finally, I tested on arm-eabi, but not on msp430 for which I do not
> have the environment, so advice from msp430 maintainers is
> appreciated. Since msp430 does not use the same default helpers as
> arm, I left the "noinit" handling code in place, to avoid changing
> other generic functions which I don't know how to test
> (default_select_section, default_section_type_flags).
> 

I'll take a look at the MSP430 case soon.

Thanks,
Jozef

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

[Jozef Lawrynowicz]

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.

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.

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

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?

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.

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

Thanks and regards,
Jozef

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

[Christophe Lyon]

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

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

[Jozef Lawrynowicz]

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

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

[Christophe Lyon]

On Fri, 5 Jul 2019 at 12:57, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote:
>
> 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.

Not formally. I submitted a patch for arm only
https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00771.html
and got requests to make it generic instead, starting with:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00065.html

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

Currently, I think it will be accepted for all targets, with various
effects depending on the OS etc...

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

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

[Martin Sebor]

On 7/4/19 9:27 AM, Christophe Lyon wrote:
> Hi,
> 
> Similar to what already exists for TI msp430 or in TI compilers for
> arm, this patch adds support for the "noinit" attribute.
> 
> It is convenient for embedded targets where the user wants to keep the
> value of some data when the program is restarted: such variables are
> not zero-initialized. It is mostly a helper/shortcut to placing
> variables in a dedicated section.
> 
> It's probably desirable to add the following chunk to the GNU linker:
> diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
> index 272a8bc..9555cec 100644
> --- a/ld/emulparams/armelf.sh
> +++ b/ld/emulparams/armelf.sh
> @@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
> *(.vfp11_veneer) *(.v4_bx)'
>   OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
> .${CREATE_SHLIB+)};"
>   OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
> .${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
> .${CREATE_SHLIB+)};"
>   OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ = .${CREATE_SHLIB+)};"
>   -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }'
>   +OTHER_SECTIONS='
>   +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
>   +  /* This section contains data that is not initialised during load
>   +     *or* application reset.  */
>   +   .noinit (NOLOAD) :
>   +   {
>   +     . = ALIGN(2);
>   +     PROVIDE (__noinit_start = .);
>   +     *(.noinit)
>   +     . = ALIGN(2);
>   +     PROVIDE (__noinit_end = .);
>   +   }
>   +'
> 
> so that the noinit section has the "NOLOAD" flag.
> 
> I'll submit that part separately to the binutils project if OK.
> 
> However, I'm not sure for which other targets (beyond arm), I should
> update the linker scripts.
> 
> I left the new testcase in gcc.target/arm, guarded by
> dg-do run { target { *-*-eabi } }
> but since this attribute is now generic, I suspect I should move the
> test to some other place. But then, which target selector should I
> use?
> 
> Finally, I tested on arm-eabi, but not on msp430 for which I do not
> have the environment, so advice from msp430 maintainers is
> appreciated. Since msp430 does not use the same default helpers as
> arm, I left the "noinit" handling code in place, to avoid changing
> other generic functions which I don't know how to test
> (default_select_section, default_section_type_flags).
> 

Since the section attribute is mutually exclusive with noinit,
defining an attribute_exclusions array with the mutually exclusive
attributes and setting the last member of the corresponding
c_common_attribute_table element to that array (and updating
the arrays for the other mutually exclusive attributes) would
be the expected way to express that constraint:

+  { "noinit",                 0, 0, true,  false, false, false,
+			      handle_noinit_attribute, NULL },
                                                        ^^^^
This should also make it possible to remove the explicit handling
from handle_section_attribute and handle_noinit_attribute.  If
that's not completely possible then in the following please be
sure to quote the names of the attributes:

@@ -1912,6 +1915,13 @@ handle_section_attribute (tree *node, tree 
ARG_UNUSED (name), tree args,
        goto fail;
      }

+  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES 
(decl)) != NULL_TREE)
+    {
+      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+		"section attribute cannot be applied to variables with noinit 
attribute");

Martin

> Thanks,
> 
> Christophe
> 
> gcc/ChangeLog:
> 
> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> 
> * doc/extend.texi: Add "noinit" attribute documentation.
> * varasm.c
> (default_section_type_flags): Add support for "noinit" section.
> (default_elf_select_section): Add support for "noinit" attribute.
> 
> gcc/c-family/ChangeLog:
> 
> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> 
> * c-attribs.c (c_common_attribute_table): Add "noinit" entry.
> (handle_section_attribute): Add support for "noinit" attribute.
> (handle_noinit_attribute): New function.
> 
> gcc/config/ChangeLog:
> 
> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> 
> * msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> 
> * gcc.target/arm/noinit-attribute.c: New test.
> 

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

[Christophe Lyon]

[-- Attachment #1: Type: text/plain, Size: 4979 bytes --]

On Sat, 6 Jul 2019 at 19:57, Martin Sebor <msebor@gmail.com> wrote:
>
> On 7/4/19 9:27 AM, Christophe Lyon wrote:
> > Hi,
> >
> > Similar to what already exists for TI msp430 or in TI compilers for
> > arm, this patch adds support for the "noinit" attribute.
> >
> > It is convenient for embedded targets where the user wants to keep the
> > value of some data when the program is restarted: such variables are
> > not zero-initialized. It is mostly a helper/shortcut to placing
> > variables in a dedicated section.
> >
> > It's probably desirable to add the following chunk to the GNU linker:
> > diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
> > index 272a8bc..9555cec 100644
> > --- a/ld/emulparams/armelf.sh
> > +++ b/ld/emulparams/armelf.sh
> > @@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
> > *(.vfp11_veneer) *(.v4_bx)'
> >   OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
> > .${CREATE_SHLIB+)};"
> >   OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
> > .${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
> > .${CREATE_SHLIB+)};"
> >   OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ = .${CREATE_SHLIB+)};"
> >   -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }'
> >   +OTHER_SECTIONS='
> >   +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
> >   +  /* This section contains data that is not initialised during load
> >   +     *or* application reset.  */
> >   +   .noinit (NOLOAD) :
> >   +   {
> >   +     . = ALIGN(2);
> >   +     PROVIDE (__noinit_start = .);
> >   +     *(.noinit)
> >   +     . = ALIGN(2);
> >   +     PROVIDE (__noinit_end = .);
> >   +   }
> >   +'
> >
> > so that the noinit section has the "NOLOAD" flag.
> >
> > I'll submit that part separately to the binutils project if OK.
> >
> > However, I'm not sure for which other targets (beyond arm), I should
> > update the linker scripts.
> >
> > I left the new testcase in gcc.target/arm, guarded by
> > dg-do run { target { *-*-eabi } }
> > but since this attribute is now generic, I suspect I should move the
> > test to some other place. But then, which target selector should I
> > use?
> >
> > Finally, I tested on arm-eabi, but not on msp430 for which I do not
> > have the environment, so advice from msp430 maintainers is
> > appreciated. Since msp430 does not use the same default helpers as
> > arm, I left the "noinit" handling code in place, to avoid changing
> > other generic functions which I don't know how to test
> > (default_select_section, default_section_type_flags).
> >
>
> Since the section attribute is mutually exclusive with noinit,
> defining an attribute_exclusions array with the mutually exclusive
> attributes and setting the last member of the corresponding
> c_common_attribute_table element to that array (and updating
> the arrays for the other mutually exclusive attributes) would
> be the expected way to express that constraint:
>
> +  { "noinit",                 0, 0, true,  false, false, false,
> +                             handle_noinit_attribute, NULL },
>                                                         ^^^^
> This should also make it possible to remove the explicit handling
> from handle_section_attribute and handle_noinit_attribute.  If
> that's not completely possible then in the following please be
> sure to quote the names of the attributes:
>
> @@ -1912,6 +1915,13 @@ handle_section_attribute (tree *node, tree
> ARG_UNUSED (name), tree args,
>         goto fail;
>       }
>
> +  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES
> (decl)) != NULL_TREE)
> +    {
> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
> +               "section attribute cannot be applied to variables with noinit
> attribute");
>
> Martin
>

Thanks, here is an updated version:
- adds exclusion array
- updates testcase with new error message (because of exclusion)
- moves testcase to gcc.c-torture/execute
- adds "noinit" effective-target

Christophe

> > Thanks,
> >
> > Christophe
> >
> > gcc/ChangeLog:
> >
> > 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> > * doc/extend.texi: Add "noinit" attribute documentation.
> > * varasm.c
> > (default_section_type_flags): Add support for "noinit" section.
> > (default_elf_select_section): Add support for "noinit" attribute.
> >
> > gcc/c-family/ChangeLog:
> >
> > 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> > * c-attribs.c (c_common_attribute_table): Add "noinit" entry.
> > (handle_section_attribute): Add support for "noinit" attribute.
> > (handle_noinit_attribute): New function.
> >
> > gcc/config/ChangeLog:
> >
> > 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> > * msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> > * gcc.target/arm/noinit-attribute.c: New test.
> >
>

[-- Attachment #2: noinit-generic.patch-v2.txt --]
[-- Type: text/plain, Size: 12716 bytes --]

commit 3a9dd81162eae36a87a067018a5e5bf1e7b978df
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Thu Jul 4 14:46:00 2019 +0000

    Add generic support for noinit attribute.
    
    Similar to what already exists for TI msp430 or in TI compilers for
    arm, this patch adds support for the "noinit" attribute.
    
    It is convenient for embedded targets where the user wants to keep the
    value of some data when the program is restarted: such variables are
    not zero-initialized. It is mostly a helper/shortcut to placing
    variables in a dedicated section.
    
    It's probably desirable to add the following chunk to the GNU linker:
    diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
    index 272a8bc..9555cec 100644
    --- a/ld/emulparams/armelf.sh
    +++ b/ld/emulparams/armelf.sh
    @@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
    *(.vfp11_veneer) *(.v4_bx)'
     OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
    .${CREATE_SHLIB+)};"
     OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
    .${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
    .${CREATE_SHLIB+)};"
     OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ = .${CREATE_SHLIB+)};"
     -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }'
     +OTHER_SECTIONS='
     +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
     +  /* This section contains data that is not initialised during load
     +     *or* application reset.  */
     +   .noinit (NOLOAD) :
     +   {
     +     . = ALIGN(2);
     +     PROVIDE (__noinit_start = .);
     +     *(.noinit)
     +     . = ALIGN(2);
     +     PROVIDE (__noinit_end = .);
     +   }
     +'
    
    so that the noinit section has the "NOLOAD" flag.
    
    I'll submit that part separately to the binutils project if OK.
    
    However, I'm not sure for which other targets (beyond arm), I should
    update the linker scripts.
    
    I added a testcase if gcc.c-torture/execute, gated by the new noinit
    effective-target.
    
    Finally, I tested on arm-eabi, but not on msp430 for which I do not
    have the environment, so advice from msp430 maintainers is
    appreciated.
    
    Thanks,
    
    Christophe
    
    gcc/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* doc/extend.texi: Add "noinit" attribute documentation.
    	* varasm.c
    	(default_section_type_flags): Add support for "noinit" section.
    	(default_elf_select_section): Add support for "noinit" attribute.
    
    gcc/c-family/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* c-attribs.c (c_common_attribute_table): Add "noinit" entry. Add
    	exclusion with "section" attribute.
    	(attr_noinit_exclusions): New table.
    	(handle_noinit_attribute): New function.
    
    gcc/config/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
    
    gcc/testsuite/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* lib/target-supports.exp (check_effective_target_noinit): New
    	proc.
            * gcc.c-torture/execute/noinit-attribute.c: New test.
    
    Change-Id: Id8e0baa728d187e05541c7520bd5726ccf803c4f

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 48819e7..399fef3 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -92,6 +92,7 @@ static tree handle_section_attribute (tree *, tree, tree, int, bool *);
 static tree handle_aligned_attribute (tree *, tree, tree, int, bool *);
 static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree,
 						  int, bool *);
+static tree handle_noinit_attribute (tree *, tree, tree, int, bool *);
 static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *);
@@ -235,6 +236,13 @@ static const struct attribute_spec::exclusions attr_const_pure_exclusions[] =
   ATTR_EXCL (NULL, false, false, false)
 };
 
+static const struct attribute_spec::exclusions attr_noinit_exclusions[] =
+{
+  ATTR_EXCL ("noinit", true, true, true),
+  ATTR_EXCL ("section", true, true, true),
+  ATTR_EXCL (NULL, false, false, false),
+};
+
 /* Table of machine-independent attributes common to all C-like languages.
 
    Current list of processed common attributes: nonnull.  */
@@ -307,7 +315,7 @@ const struct attribute_spec c_common_attribute_table[] =
   { "mode",                   1, 1, false,  true, false, false,
 			      handle_mode_attribute, NULL },
   { "section",                1, 1, true,  false, false, false,
-			      handle_section_attribute, NULL },
+			      handle_section_attribute, attr_noinit_exclusions },
   { "aligned",                0, 1, false, false, false, false,
 			      handle_aligned_attribute,
 	                      attr_aligned_exclusions },
@@ -458,6 +466,8 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_nocf_check_attribute, NULL },
   { "copy",                   1, 1, false, false, false, false,
 			      handle_copy_attribute, NULL },
+  { "noinit",		      0, 0, true,  false, false, false,
+			      handle_noinit_attribute, attr_noinit_exclusions },
   { NULL,                     0, 0, false, false, false, false, NULL, NULL }
 };
 
@@ -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;
+}
+
+
 /* Handle a "noplt" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 365e9eb..264e852 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -2108,8 +2108,6 @@ const struct attribute_spec msp430_attribute_table[] =
   { ATTR_EITHER,      0, 0, true,  false, false, false, msp430_section_attr,
     NULL },
 
-  { ATTR_NOINIT,      0, 0, true,  false, false, false, msp430_data_attr,
-    NULL },
   { ATTR_PERSIST,     0, 0, true,  false, false, false, msp430_data_attr,
     NULL },
 
@@ -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))
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
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.  */
+  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
+}
+
 ###############################
 # proc check_visibility_available { what_kind }
 ###############################
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;
+	}
+
       if (bss_section)
 	return bss_section;
       sname = ".bss";

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

[Martin Sebor]

On 7/8/19 5:10 AM, Christophe Lyon wrote:
> On Sat, 6 Jul 2019 at 19:57, Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 7/4/19 9:27 AM, Christophe Lyon wrote:
>>> Hi,
>>>
>>> Similar to what already exists for TI msp430 or in TI compilers for
>>> arm, this patch adds support for the "noinit" attribute.
>>>
>>> It is convenient for embedded targets where the user wants to keep the
>>> value of some data when the program is restarted: such variables are
>>> not zero-initialized. It is mostly a helper/shortcut to placing
>>> variables in a dedicated section.
>>>
>>> It's probably desirable to add the following chunk to the GNU linker:
>>> diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
>>> index 272a8bc..9555cec 100644
>>> --- a/ld/emulparams/armelf.sh
>>> +++ b/ld/emulparams/armelf.sh
>>> @@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
>>> *(.vfp11_veneer) *(.v4_bx)'
>>>    OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
>>> .${CREATE_SHLIB+)};"
>>>    OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
>>> .${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
>>> .${CREATE_SHLIB+)};"
>>>    OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ = .${CREATE_SHLIB+)};"
>>>    -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }'
>>>    +OTHER_SECTIONS='
>>>    +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
>>>    +  /* This section contains data that is not initialised during load
>>>    +     *or* application reset.  */
>>>    +   .noinit (NOLOAD) :
>>>    +   {
>>>    +     . = ALIGN(2);
>>>    +     PROVIDE (__noinit_start = .);
>>>    +     *(.noinit)
>>>    +     . = ALIGN(2);
>>>    +     PROVIDE (__noinit_end = .);
>>>    +   }
>>>    +'
>>>
>>> so that the noinit section has the "NOLOAD" flag.
>>>
>>> I'll submit that part separately to the binutils project if OK.
>>>
>>> However, I'm not sure for which other targets (beyond arm), I should
>>> update the linker scripts.
>>>
>>> I left the new testcase in gcc.target/arm, guarded by
>>> dg-do run { target { *-*-eabi } }
>>> but since this attribute is now generic, I suspect I should move the
>>> test to some other place. But then, which target selector should I
>>> use?
>>>
>>> Finally, I tested on arm-eabi, but not on msp430 for which I do not
>>> have the environment, so advice from msp430 maintainers is
>>> appreciated. Since msp430 does not use the same default helpers as
>>> arm, I left the "noinit" handling code in place, to avoid changing
>>> other generic functions which I don't know how to test
>>> (default_select_section, default_section_type_flags).
>>>
>>
>> Since the section attribute is mutually exclusive with noinit,
>> defining an attribute_exclusions array with the mutually exclusive
>> attributes and setting the last member of the corresponding
>> c_common_attribute_table element to that array (and updating
>> the arrays for the other mutually exclusive attributes) would
>> be the expected way to express that constraint:
>>
>> +  { "noinit",                 0, 0, true,  false, false, false,
>> +                             handle_noinit_attribute, NULL },
>>                                                          ^^^^
>> This should also make it possible to remove the explicit handling
>> from handle_section_attribute and handle_noinit_attribute.  If
>> that's not completely possible then in the following please be
>> sure to quote the names of the attributes:
>>
>> @@ -1912,6 +1915,13 @@ handle_section_attribute (tree *node, tree
>> ARG_UNUSED (name), tree args,
>>          goto fail;
>>        }
>>
>> +  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES
>> (decl)) != NULL_TREE)
>> +    {
>> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
>> +               "section attribute cannot be applied to variables with noinit
>> attribute");
>>
>> Martin
>>
> 
> Thanks, here is an updated version:
> - adds exclusion array
> - updates testcase with new error message (because of exclusion)

The exclusion bits look good, thank you!

Martin

> - moves testcase to gcc.c-torture/execute
> - adds "noinit" effective-target
> 
> Christophe
> 
>>> Thanks,
>>>
>>> Christophe
>>>
>>> gcc/ChangeLog:
>>>
>>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
>>>
>>> * doc/extend.texi: Add "noinit" attribute documentation.
>>> * varasm.c
>>> (default_section_type_flags): Add support for "noinit" section.
>>> (default_elf_select_section): Add support for "noinit" attribute.
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
>>>
>>> * c-attribs.c (c_common_attribute_table): Add "noinit" entry.
>>> (handle_section_attribute): Add support for "noinit" attribute.
>>> (handle_noinit_attribute): New function.
>>>
>>> gcc/config/ChangeLog:
>>>
>>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
>>>
>>> * msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
>>>
>>> * gcc.target/arm/noinit-attribute.c: New test.
>>>
>>

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

[Christophe Lyon]

Ping?

Le mar. 9 juil. 2019 à 00:04, Martin Sebor <msebor@gmail.com> a écrit :

> On 7/8/19 5:10 AM, Christophe Lyon wrote:
> > On Sat, 6 Jul 2019 at 19:57, Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 7/4/19 9:27 AM, Christophe Lyon wrote:
> >>> Hi,
> >>>
> >>> Similar to what already exists for TI msp430 or in TI compilers for
> >>> arm, this patch adds support for the "noinit" attribute.
> >>>
> >>> It is convenient for embedded targets where the user wants to keep the
> >>> value of some data when the program is restarted: such variables are
> >>> not zero-initialized. It is mostly a helper/shortcut to placing
> >>> variables in a dedicated section.
> >>>
> >>> It's probably desirable to add the following chunk to the GNU linker:
> >>> diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
> >>> index 272a8bc..9555cec 100644
> >>> --- a/ld/emulparams/armelf.sh
> >>> +++ b/ld/emulparams/armelf.sh
> >>> @@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
> >>> *(.vfp11_veneer) *(.v4_bx)'
> >>>    OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
> >>> .${CREATE_SHLIB+)};"
> >>>    OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
> >>> .${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
> >>> .${CREATE_SHLIB+)};"
> >>>    OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ =
> .${CREATE_SHLIB+)};"
> >>>    -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP
> (*(.note.gnu.arm.ident)) }'
> >>>    +OTHER_SECTIONS='
> >>>    +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
> >>>    +  /* This section contains data that is not initialised during load
> >>>    +     *or* application reset.  */
> >>>    +   .noinit (NOLOAD) :
> >>>    +   {
> >>>    +     . = ALIGN(2);
> >>>    +     PROVIDE (__noinit_start = .);
> >>>    +     *(.noinit)
> >>>    +     . = ALIGN(2);
> >>>    +     PROVIDE (__noinit_end = .);
> >>>    +   }
> >>>    +'
> >>>
> >>> so that the noinit section has the "NOLOAD" flag.
> >>>
> >>> I'll submit that part separately to the binutils project if OK.
> >>>
> >>> However, I'm not sure for which other targets (beyond arm), I should
> >>> update the linker scripts.
> >>>
> >>> I left the new testcase in gcc.target/arm, guarded by
> >>> dg-do run { target { *-*-eabi } }
> >>> but since this attribute is now generic, I suspect I should move the
> >>> test to some other place. But then, which target selector should I
> >>> use?
> >>>
> >>> Finally, I tested on arm-eabi, but not on msp430 for which I do not
> >>> have the environment, so advice from msp430 maintainers is
> >>> appreciated. Since msp430 does not use the same default helpers as
> >>> arm, I left the "noinit" handling code in place, to avoid changing
> >>> other generic functions which I don't know how to test
> >>> (default_select_section, default_section_type_flags).
> >>>
> >>
> >> Since the section attribute is mutually exclusive with noinit,
> >> defining an attribute_exclusions array with the mutually exclusive
> >> attributes and setting the last member of the corresponding
> >> c_common_attribute_table element to that array (and updating
> >> the arrays for the other mutually exclusive attributes) would
> >> be the expected way to express that constraint:
> >>
> >> +  { "noinit",                 0, 0, true,  false, false, false,
> >> +                             handle_noinit_attribute, NULL },
> >>                                                          ^^^^
> >> This should also make it possible to remove the explicit handling
> >> from handle_section_attribute and handle_noinit_attribute.  If
> >> that's not completely possible then in the following please be
> >> sure to quote the names of the attributes:
> >>
> >> @@ -1912,6 +1915,13 @@ handle_section_attribute (tree *node, tree
> >> ARG_UNUSED (name), tree args,
> >>          goto fail;
> >>        }
> >>
> >> +  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES
> >> (decl)) != NULL_TREE)
> >> +    {
> >> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
> >> +               "section attribute cannot be applied to variables with
> noinit
> >> attribute");
> >>
> >> Martin
> >>
> >
> > Thanks, here is an updated version:
> > - adds exclusion array
> > - updates testcase with new error message (because of exclusion)
>
> The exclusion bits look good, thank you!
>
> Martin
>
> > - moves testcase to gcc.c-torture/execute
> > - adds "noinit" effective-target
> >
> > Christophe
> >
> >>> Thanks,
> >>>
> >>> Christophe
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * doc/extend.texi: Add "noinit" attribute documentation.
> >>> * varasm.c
> >>> (default_section_type_flags): Add support for "noinit" section.
> >>> (default_elf_select_section): Add support for "noinit" attribute.
> >>>
> >>> gcc/c-family/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * c-attribs.c (c_common_attribute_table): Add "noinit" entry.
> >>> (handle_section_attribute): Add support for "noinit" attribute.
> >>> (handle_noinit_attribute): New function.
> >>>
> >>> gcc/config/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * gcc.target/arm/noinit-attribute.c: New test.
> >>>
> >>
>
>

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

[Richard Sandiford]

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.

Since this is specific to ELF targets, I think we should also
warn if !targetm.have_switchable_bss_sections.

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

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

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

> +  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.)

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

Did you consider supporting .noinit.*, e.g. for -fdata-sections?

Thanks,
Richard

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

[Christophe Lyon]

[-- Attachment #1: Type: text/plain, Size: 9277 bytes --]

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

[-- Attachment #2: noinit-generic.patch-v3.txt --]
[-- Type: text/plain, Size: 13435 bytes --]

commit 833e8ee8a5ca1ccc53121c7fd86b37e0eadc0086
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Thu Jul 4 14:46:00 2019 +0000

    Add generic support for noinit attribute.
    
    Similar to what already exists for TI msp430 or in TI compilers for
    arm, this patch adds support for the "noinit" attribute.
    
    It is convenient for embedded targets where the user wants to keep the
    value of some data when the program is restarted: such variables are
    not zero-initialized. It is mostly a helper/shortcut to placing
    variables in a dedicated section.
    
    It's probably desirable to add the following chunk to the GNU linker:
    diff --git a/ld/emulparams/armelf.sh b/ld/emulparams/armelf.sh
    index 272a8bc..9555cec 100644
    --- a/ld/emulparams/armelf.sh
    +++ b/ld/emulparams/armelf.sh
    @@ -10,7 +10,19 @@ OTHER_TEXT_SECTIONS='*(.glue_7t) *(.glue_7)
    *(.vfp11_veneer) *(.v4_bx)'
     OTHER_BSS_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__bss_start__ =
    .${CREATE_SHLIB+)};"
     OTHER_BSS_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}_bss_end__ =
    .${CREATE_SHLIB+)}; ${CREATE_SHLIB+PROVIDE (}__bss_end__ =
    .${CREATE_SHLIB+)};"
     OTHER_END_SYMBOLS="${CREATE_SHLIB+PROVIDE (}__end__ = .${CREATE_SHLIB+)};"
     -OTHER_SECTIONS='.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }'
     +OTHER_SECTIONS='
     +.note.gnu.arm.ident 0 : { KEEP (*(.note.gnu.arm.ident)) }
     +  /* This section contains data that is not initialised during load
     +     *or* application reset.  */
     +   .noinit (NOLOAD) :
     +   {
     +     . = ALIGN(2);
     +     PROVIDE (__noinit_start = .);
     +     *(.noinit)
     +     . = ALIGN(2);
     +     PROVIDE (__noinit_end = .);
     +   }
     +'
    
    so that the noinit section has the "NOLOAD" flag.
    
    I'll submit that part separately to the binutils project if OK.
    
    However, I'm not sure for which other targets (beyond arm), I should
    update the linker scripts.
    
    I added a testcase if gcc.c-torture/execute, gated by the new noinit
    effective-target.
    
    Finally, I tested on arm-eabi, but not on msp430 for which I do not
    have the environment, so advice from msp430 maintainers is
    appreciated.
    
    Thanks,
    
    Christophe
    
    gcc/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* doc/extend.texi: Add "noinit" attribute documentation.
    	* doc/sourcebuild.texi: Add noinit effective target documentation.
    	* varasm.c
    	(default_section_type_flags): Add support for "noinit" section.
    	(default_elf_select_section): Add support for "noinit" attribute.
    
    gcc/c-family/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* c-attribs.c (c_common_attribute_table): Add "noinit" entry. Add
    	exclusion with "section" attribute.
    	(attr_noinit_exclusions): New table.
    	(handle_noinit_attribute): New function.
    
    gcc/config/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
    
    gcc/testsuite/ChangeLog:
    
    2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* lib/target-supports.exp (check_effective_target_noinit): New
    	proc.
            * gcc.c-torture/execute/noinit-attribute.c: New test.
    
    Change-Id: Id8e0baa728d187e05541c7520bd5726ccf803c4f

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 48819e7..77ed56d 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -92,6 +92,7 @@ static tree handle_section_attribute (tree *, tree, tree, int, bool *);
 static tree handle_aligned_attribute (tree *, tree, tree, int, bool *);
 static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree,
 						  int, bool *);
+static tree handle_noinit_attribute (tree *, tree, tree, int, bool *);
 static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *);
@@ -235,6 +236,13 @@ static const struct attribute_spec::exclusions attr_const_pure_exclusions[] =
   ATTR_EXCL (NULL, false, false, false)
 };
 
+static const struct attribute_spec::exclusions attr_noinit_exclusions[] =
+{
+  ATTR_EXCL ("noinit", true, true, true),
+  ATTR_EXCL ("section", true, true, true),
+  ATTR_EXCL (NULL, false, false, false),
+};
+
 /* Table of machine-independent attributes common to all C-like languages.
 
    Current list of processed common attributes: nonnull.  */
@@ -307,7 +315,7 @@ const struct attribute_spec c_common_attribute_table[] =
   { "mode",                   1, 1, false,  true, false, false,
 			      handle_mode_attribute, NULL },
   { "section",                1, 1, true,  false, false, false,
-			      handle_section_attribute, NULL },
+			      handle_section_attribute, attr_noinit_exclusions },
   { "aligned",                0, 1, false, false, false, false,
 			      handle_aligned_attribute,
 	                      attr_aligned_exclusions },
@@ -458,6 +466,8 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_nocf_check_attribute, NULL },
   { "copy",                   1, 1, false, false, false, false,
 			      handle_copy_attribute, NULL },
+  { "noinit",		      0, 0, true,  false, false, false,
+			      handle_noinit_attribute, attr_noinit_exclusions },
   { NULL,                     0, 0, false, false, false, false, NULL, NULL }
 };
 
@@ -2224,6 +2234,54 @@ 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.  */
+  else 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 (!targetm.have_switchable_bss_sections)
+    message = G_("%qE attribute is specific to ELF targets");
+
+  if (message)
+    {
+      warning (OPT_Wattributes, message, name);
+      *no_add_attrs = true;
+    }
+  else
+  /* 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.  Do this only if the attribute is
+     valid.  */
+    if (DECL_COMMON (*node))
+      DECL_COMMON (*node) = 0;
+
+  return NULL_TREE;
+}
+
+
 /* Handle a "noplt" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 365e9eb..264e852 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -2108,8 +2108,6 @@ const struct attribute_spec msp430_attribute_table[] =
   { ATTR_EITHER,      0, 0, true,  false, false, false, msp430_section_attr,
     NULL },
 
-  { ATTR_NOINIT,      0, 0, true,  false, false, false, msp430_data_attr,
-    NULL },
   { ATTR_PERSIST,     0, 0, true,  false, false, false, msp430_data_attr,
     NULL },
 
@@ -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))
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index f2619e1..f1af1dc 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -7129,6 +7129,14 @@ 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.  Specific to ELF
+targets, this attribute relies on the linker to place such data in the
+right location.
+
 @end table
 
 @node ARC Variable Attributes
diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index 6a66515..d67608a 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2302,6 +2302,9 @@ Target uses natural alignment (aligned to type size) for types of
 Target uses natural alignment (aligned to type size) for types of
 64 bits or less.
 
+@item noinit
+Target supports the @code{noinit} variable attribute.
+
 @item nonpic
 Target does not generate PIC by default.
 
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..ffcf8c6
--- /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 initialized the ordinary variables.  */
+  if (var_common != 0)
+    abort ();
+
+  /* Initialized variables are not re-initialized 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
+}
+
 ###############################
 # proc check_visibility_available { what_kind }
 ###############################
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 626a4c9..6ddab0ce 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,13 @@ 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)
+	{
+	  sname = ".noinit";
+	  break;
+	}
+
       if (bss_section)
 	return bss_section;
       sname = ".bss";

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

[Jozef Lawrynowicz]

[-- Attachment #1: Type: text/plain, Size: 10359 bytes --]

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  


[-- Attachment #2: msp430_select_section.diff --]
[-- Type: text/x-patch, Size: 2981 bytes --]

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

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

[Christophe Lyon]

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

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

[Richard Sandiford]

Sorry for the slow response, I'd missed that there was an updated patch...

Christophe Lyon <christophe.lyon@linaro.org> writes:
>     2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
>     
>     	* lib/target-supports.exp (check_effective_target_noinit): New
>     	proc.
>             * gcc.c-torture/execute/noinit-attribute.c: New test.

Second line should be indented by tabs rather than spaces.

> @@ -2224,6 +2234,54 @@ 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.  */
> +  else 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 (!targetm.have_switchable_bss_sections)
> +    message = G_("%qE attribute is specific to ELF targets");

Maybe make this an else if too?  Or make the VAR_DECL an else if
if you think the ELF one should win.  Either way, it seems odd to
have the mixture between else if and not.

> +  if (message)
> +    {
> +      warning (OPT_Wattributes, message, name);
> +      *no_add_attrs = true;
> +    }
> +  else
> +  /* 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.  Do this only if the attribute is
> +     valid.  */

Comment should be indented two spaces more.

> +    if (DECL_COMMON (*node))
> +      DECL_COMMON (*node) = 0;
> +
> +  return NULL_TREE;
> +}
> +
> +
>  /* Handle a "noplt" attribute; arguments as in
>     struct attribute_spec.handler.  */
>  
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index f2619e1..f1af1dc 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -7129,6 +7129,14 @@ 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.  Specific to ELF
> +targets, this attribute relies on the linker to place such data in the
> +right location.

Maybe:

   This attribute is specific to ELF targets and relies on the linker to
   place such data in the right location.

> 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..ffcf8c6
> --- /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 initialized the ordinary variables.  */
> +  if (var_common != 0)
> +    abort ();
> +
> +  /* Initialized variables are not re-initialized 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
> +}
> +
>  ###############################
>  # proc check_visibility_available { what_kind }
>  ###############################
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 626a4c9..6ddab0ce 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;
> +

No longer needed.

OK with those changes, thanks.

Richard

>  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,13 @@ 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)
> +	{
> +	  sname = ".noinit";
> +	  break;
> +	}
> +
>        if (bss_section)
>  	return bss_section;
>        sname = ".bss";

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

[Christophe Lyon]

On Wed, 14 Aug 2019 at 14:14, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Sorry for the slow response, I'd missed that there was an updated patch...
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> >     2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> >       * lib/target-supports.exp (check_effective_target_noinit): New
> >       proc.
> >             * gcc.c-torture/execute/noinit-attribute.c: New test.
>
> Second line should be indented by tabs rather than spaces.
>
> > @@ -2224,6 +2234,54 @@ 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.  */
> > +  else 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 (!targetm.have_switchable_bss_sections)
> > +    message = G_("%qE attribute is specific to ELF targets");
>
> Maybe make this an else if too?  Or make the VAR_DECL an else if
> if you think the ELF one should win.  Either way, it seems odd to
> have the mixture between else if and not.
>
Right, I changed this into an else if.

> > +  if (message)
> > +    {
> > +      warning (OPT_Wattributes, message, name);
> > +      *no_add_attrs = true;
> > +    }
> > +  else
> > +  /* 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.  Do this only if the attribute is
> > +     valid.  */
>
> Comment should be indented two spaces more.
>
> > +    if (DECL_COMMON (*node))
> > +      DECL_COMMON (*node) = 0;
> > +
> > +  return NULL_TREE;
> > +}
> > +
> > +
> >  /* Handle a "noplt" attribute; arguments as in
> >     struct attribute_spec.handler.  */
> >
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > index f2619e1..f1af1dc 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -7129,6 +7129,14 @@ 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.  Specific to ELF
> > +targets, this attribute relies on the linker to place such data in the
> > +right location.
>
> Maybe:
>
>    This attribute is specific to ELF targets and relies on the linker to
>    place such data in the right location.
>
Thanks, I thought I had chosen a nice turn of phrase :-)


> > 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..ffcf8c6
> > --- /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 initialized the ordinary variables.  */
> > +  if (var_common != 0)
> > +    abort ();
> > +
> > +  /* Initialized variables are not re-initialized 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
> > +}
> > +
> >  ###############################
> >  # proc check_visibility_available { what_kind }
> >  ###############################
> > diff --git a/gcc/varasm.c b/gcc/varasm.c
> > index 626a4c9..6ddab0ce 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;
> > +
>
> No longer needed.
Indeed.

>
> OK with those changes, thanks.
Thanks, committed as r274482.

Christophe

> Richard
>
> >  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,13 @@ 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)
> > +     {
> > +       sname = ".noinit";
> > +       break;
> > +     }
> > +
> >        if (bss_section)
> >       return bss_section;
> >        sname = ".bss";

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

[Tamar Christina]

Hi Christoph,

The noinit testcase is currently failing on x86_64.

Is the test supposed to be running there?

Thanks,
Tamar

-----Original Message-----
From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On Behalf Of Christophe Lyon
Sent: Wednesday, August 14, 2019 2:18 PM
To: Christophe Lyon <christophe.lyon@linaro.org>; Martin Sebor <msebor@gmail.com>; gcc Patches <gcc-patches@gcc.gnu.org>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Jozef Lawrynowicz <jozef.l@mittosystems.com>; Richard Sandiford <Richard.Sandiford@arm.com>
Subject: Re: [PATCH] Add generic support for "noinit" attribute

On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote:
>
> Sorry for the slow response, I'd missed that there was an updated patch...
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> >     2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >
> >       * lib/target-supports.exp (check_effective_target_noinit): New
> >       proc.
> >             * gcc.c-torture/execute/noinit-attribute.c: New test.
>
> Second line should be indented by tabs rather than spaces.
>
> > @@ -2224,6 +2234,54 @@ 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.  
> > + */  else 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 (!targetm.have_switchable_bss_sections)
> > +    message = G_("%qE attribute is specific to ELF targets");
>
> Maybe make this an else if too?  Or make the VAR_DECL an else if if 
> you think the ELF one should win.  Either way, it seems odd to have 
> the mixture between else if and not.
>
Right, I changed this into an else if.

> > +  if (message)
> > +    {
> > +      warning (OPT_Wattributes, message, name);
> > +      *no_add_attrs = true;
> > +    }
> > +  else
> > +  /* 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.  Do this only if the attribute is
> > +     valid.  */
>
> Comment should be indented two spaces more.
>
> > +    if (DECL_COMMON (*node))
> > +      DECL_COMMON (*node) = 0;
> > +
> > +  return NULL_TREE;
> > +}
> > +
> > +
> >  /* Handle a "noplt" attribute; arguments as in
> >     struct attribute_spec.handler.  */
> >
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index 
> > f2619e1..f1af1dc 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -7129,6 +7129,14 @@ 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.  Specific to ELF targets, 
> > +this attribute relies on the linker to place such data in the right 
> > +location.
>
> Maybe:
>
>    This attribute is specific to ELF targets and relies on the linker to
>    place such data in the right location.
>
Thanks, I thought I had chosen a nice turn of phrase :-)


> > 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..ffcf8c6
> > --- /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 initialized 
> > +the ordinary variables.  */
> > +  if (var_common != 0)
> > +    abort ();
> > +
> > +  /* Initialized variables are not re-initialized 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
> > +}
> > +
> >  ###############################
> >  # proc check_visibility_available { what_kind }  
> > ############################### diff --git a/gcc/varasm.c 
> > b/gcc/varasm.c index 626a4c9..6ddab0ce 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;
> > +
>
> No longer needed.
Indeed.

>
> OK with those changes, thanks.
Thanks, committed as r274482.

Christophe

> Richard
>
> >  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,13 @@ 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)
> > +     {
> > +       sname = ".noinit";
> > +       break;
> > +     }
> > +
> >        if (bss_section)
> >       return bss_section;
> >        sname = ".bss";

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

[Christophe Lyon]

On Wed, 14 Aug 2019 at 17:59, Tamar Christina <Tamar.Christina@arm.com> wrote:
>
> Hi Christoph,
>
> The noinit testcase is currently failing on x86_64.
>
> Is the test supposed to be running there?
>
No, there's an effective-target to skip it.
But I notice a typo:
+/* { dg-require-effective-target noinit */
(missing closing brace)
Could it explain why it's failing on x86_64 ?

> Thanks,
> Tamar
>
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On Behalf Of Christophe Lyon
> Sent: Wednesday, August 14, 2019 2:18 PM
> To: Christophe Lyon <christophe.lyon@linaro.org>; Martin Sebor <msebor@gmail.com>; gcc Patches <gcc-patches@gcc.gnu.org>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Jozef Lawrynowicz <jozef.l@mittosystems.com>; Richard Sandiford <Richard.Sandiford@arm.com>
> Subject: Re: [PATCH] Add generic support for "noinit" attribute
>
> On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote:
> >
> > Sorry for the slow response, I'd missed that there was an updated patch...
> >
> > Christophe Lyon <christophe.lyon@linaro.org> writes:
> > >     2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> > >
> > >       * lib/target-supports.exp (check_effective_target_noinit): New
> > >       proc.
> > >             * gcc.c-torture/execute/noinit-attribute.c: New test.
> >
> > Second line should be indented by tabs rather than spaces.
> >
> > > @@ -2224,6 +2234,54 @@ 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.
> > > + */  else 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 (!targetm.have_switchable_bss_sections)
> > > +    message = G_("%qE attribute is specific to ELF targets");
> >
> > Maybe make this an else if too?  Or make the VAR_DECL an else if if
> > you think the ELF one should win.  Either way, it seems odd to have
> > the mixture between else if and not.
> >
> Right, I changed this into an else if.
>
> > > +  if (message)
> > > +    {
> > > +      warning (OPT_Wattributes, message, name);
> > > +      *no_add_attrs = true;
> > > +    }
> > > +  else
> > > +  /* 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.  Do this only if the attribute is
> > > +     valid.  */
> >
> > Comment should be indented two spaces more.
> >
> > > +    if (DECL_COMMON (*node))
> > > +      DECL_COMMON (*node) = 0;
> > > +
> > > +  return NULL_TREE;
> > > +}
> > > +
> > > +
> > >  /* Handle a "noplt" attribute; arguments as in
> > >     struct attribute_spec.handler.  */
> > >
> > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > > f2619e1..f1af1dc 100644
> > > --- a/gcc/doc/extend.texi
> > > +++ b/gcc/doc/extend.texi
> > > @@ -7129,6 +7129,14 @@ 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.  Specific to ELF targets,
> > > +this attribute relies on the linker to place such data in the right
> > > +location.
> >
> > Maybe:
> >
> >    This attribute is specific to ELF targets and relies on the linker to
> >    place such data in the right location.
> >
> Thanks, I thought I had chosen a nice turn of phrase :-)
>
>
> > > 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..ffcf8c6
> > > --- /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 initialized
> > > +the ordinary variables.  */
> > > +  if (var_common != 0)
> > > +    abort ();
> > > +
> > > +  /* Initialized variables are not re-initialized 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
> > > +}
> > > +
> > >  ###############################
> > >  # proc check_visibility_available { what_kind }
> > > ############################### diff --git a/gcc/varasm.c
> > > b/gcc/varasm.c index 626a4c9..6ddab0ce 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;
> > > +
> >
> > No longer needed.
> Indeed.
>
> >
> > OK with those changes, thanks.
> Thanks, committed as r274482.
>
> Christophe
>
> > Richard
> >
> > >  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,13 @@ 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)
> > > +     {
> > > +       sname = ".noinit";
> > > +       break;
> > > +     }
> > > +
> > >        if (bss_section)
> > >       return bss_section;
> > >        sname = ".bss";

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

[Christophe Lyon]

On Wed, 14 Aug 2019 at 19:07, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Wed, 14 Aug 2019 at 17:59, Tamar Christina <Tamar.Christina@arm.com> wrote:
> >
> > Hi Christoph,
> >
> > The noinit testcase is currently failing on x86_64.
> >
> > Is the test supposed to be running there?
> >
> No, there's an effective-target to skip it.
> But I notice a typo:
> +/* { dg-require-effective-target noinit */
> (missing closing brace)
> Could it explain why it's failing on x86_64 ?

I fixed the typo as obvious in r274489.

>
> > Thanks,
> > Tamar
> >
> > -----Original Message-----
> > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On Behalf Of Christophe Lyon
> > Sent: Wednesday, August 14, 2019 2:18 PM
> > To: Christophe Lyon <christophe.lyon@linaro.org>; Martin Sebor <msebor@gmail.com>; gcc Patches <gcc-patches@gcc.gnu.org>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Jozef Lawrynowicz <jozef.l@mittosystems.com>; Richard Sandiford <Richard.Sandiford@arm.com>
> > Subject: Re: [PATCH] Add generic support for "noinit" attribute
> >
> > On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote:
> > >
> > > Sorry for the slow response, I'd missed that there was an updated patch...
> > >
> > > Christophe Lyon <christophe.lyon@linaro.org> writes:
> > > >     2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> > > >
> > > >       * lib/target-supports.exp (check_effective_target_noinit): New
> > > >       proc.
> > > >             * gcc.c-torture/execute/noinit-attribute.c: New test.
> > >
> > > Second line should be indented by tabs rather than spaces.
> > >
> > > > @@ -2224,6 +2234,54 @@ 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.
> > > > + */  else 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 (!targetm.have_switchable_bss_sections)
> > > > +    message = G_("%qE attribute is specific to ELF targets");
> > >
> > > Maybe make this an else if too?  Or make the VAR_DECL an else if if
> > > you think the ELF one should win.  Either way, it seems odd to have
> > > the mixture between else if and not.
> > >
> > Right, I changed this into an else if.
> >
> > > > +  if (message)
> > > > +    {
> > > > +      warning (OPT_Wattributes, message, name);
> > > > +      *no_add_attrs = true;
> > > > +    }
> > > > +  else
> > > > +  /* 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.  Do this only if the attribute is
> > > > +     valid.  */
> > >
> > > Comment should be indented two spaces more.
> > >
> > > > +    if (DECL_COMMON (*node))
> > > > +      DECL_COMMON (*node) = 0;
> > > > +
> > > > +  return NULL_TREE;
> > > > +}
> > > > +
> > > > +
> > > >  /* Handle a "noplt" attribute; arguments as in
> > > >     struct attribute_spec.handler.  */
> > > >
> > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index
> > > > f2619e1..f1af1dc 100644
> > > > --- a/gcc/doc/extend.texi
> > > > +++ b/gcc/doc/extend.texi
> > > > @@ -7129,6 +7129,14 @@ 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.  Specific to ELF targets,
> > > > +this attribute relies on the linker to place such data in the right
> > > > +location.
> > >
> > > Maybe:
> > >
> > >    This attribute is specific to ELF targets and relies on the linker to
> > >    place such data in the right location.
> > >
> > Thanks, I thought I had chosen a nice turn of phrase :-)
> >
> >
> > > > 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..ffcf8c6
> > > > --- /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 initialized
> > > > +the ordinary variables.  */
> > > > +  if (var_common != 0)
> > > > +    abort ();
> > > > +
> > > > +  /* Initialized variables are not re-initialized 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
> > > > +}
> > > > +
> > > >  ###############################
> > > >  # proc check_visibility_available { what_kind }
> > > > ############################### diff --git a/gcc/varasm.c
> > > > b/gcc/varasm.c index 626a4c9..6ddab0ce 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;
> > > > +
> > >
> > > No longer needed.
> > Indeed.
> >
> > >
> > > OK with those changes, thanks.
> > Thanks, committed as r274482.
> >
> > Christophe
> >
> > > Richard
> > >
> > > >  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,13 @@ 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)
> > > > +     {
> > > > +       sname = ".noinit";
> > > > +       break;
> > > > +     }
> > > > +
> > > >        if (bss_section)
> > > >       return bss_section;
> > > >        sname = ".bss";