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

[Richard Sandiford <richard.sandiford@arm.com>]

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