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

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

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

Hi Kyrill,

On Mon, 1 Jul 2019 at 17:58, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>
> Hi Christophe,
>
> On 6/13/19 4:13 PM, Christophe Lyon wrote:
> > Hi,
> >
> > Similar to what already exists for TI msp430 or in TI compilers for
> > arm, this patch adds support for "noinit" attribute for arm. It's very
> > similar to the corresponding code in GCC for msp430.
> >
> > It is useful 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.
> >
> > OK?
> >
>
> This is mostly ok by me, with a few code comments inline.
>
> I wonder whether this is something we could implement for all targets in
> the midend, but this would require linker script support for the target
> to be effective...
>
> Thanks,
>
> Kyrill
>
> > Thanks,
> >
> > Christophe
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index e3e71ea..332c41b 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -151,6 +151,7 @@ static tree arm_handle_notshared_attribute (tree *, tree, tree, int, bool *);
>   #endif
>   static tree arm_handle_cmse_nonsecure_entry (tree *, tree, tree, int, bool *);
>   static tree arm_handle_cmse_nonsecure_call (tree *, tree, tree, int, bool *);
> +static tree arm_data_attr (tree *, tree, tree, int, bool *);
>   static void arm_output_function_epilogue (FILE *);
>   static void arm_output_function_prologue (FILE *);
>   static int arm_comp_type_attributes (const_tree, const_tree);
> @@ -375,7 +376,8 @@ static const struct attribute_spec arm_attribute_table[] =
>       arm_handle_cmse_nonsecure_entry, NULL },
>     { "cmse_nonsecure_call", 0, 0, true, false, false, true,
>       arm_handle_cmse_nonsecure_call, NULL },
> -  { NULL, 0, 0, false, false, false, false, NULL, NULL }
> +  { "noinit", 0, 0, true, false, false, false, arm_data_attr, NULL },
> +  { NULL, 0, 0, false, false, false, false, NULL, NULL },
>   };
>
>   /* Initialize the GCC target structure.  */
> @@ -808,6 +810,10 @@ static const struct attribute_spec arm_attribute_table[] =
>
>   #undef TARGET_CONSTANT_ALIGNMENT
>   #define TARGET_CONSTANT_ALIGNMENT arm_constant_alignment
> +
> +#undef  TARGET_ASM_SELECT_SECTION
> +#define TARGET_ASM_SELECT_SECTION arm_select_section
> +
>
>   /* Obstack for minipool constant handling.  */
>   static struct obstack minipool_obstack;
> @@ -7150,6 +7156,47 @@ arm_handle_cmse_nonsecure_call (tree *node, tree name,
>     return NULL_TREE;
>   }
>
> +/* Called when the noinit attribute is used. 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
> +arm_data_attr (tree * node,
> +                 tree   name,
> +                 tree   args ATTRIBUTE_UNUSED,
> +                 int    flags ATTRIBUTE_UNUSED,
> +                 bool * no_add_attrs ATTRIBUTE_UNUSED)
> +{
>
> no_add_attrs is set in this function, so should not be ATTRIBUTE_UNUSED?
Right! Guess what? There's the same mistake in msp430.c :-)

> Arguably args is also checked against NULL, so it's technically not unused either.
Right.

> +  const char * message = NULL;
> +
> +  gcc_assert (DECL_P (* node));
>
> The house style doesn't have a space after '*'. Same elsewhere in the patch.
OK

>
> +  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;
> +}
> +
>   /* Return 0 if the attributes for two types are incompatible, 1 if they
>      are compatible, and 2 if they are nearly compatible (which causes a
>      warning to be generated).  */
> @@ -27890,6 +27937,8 @@ arm_asm_emit_except_personality (rtx personality)
>
>   /* Implement TARGET_ASM_INITIALIZE_SECTIONS.  */
>
> +static section *noinit_section;
> +
>   static void
>   arm_asm_init_sections (void)
>   {
> @@ -27902,6 +27951,19 @@ arm_asm_init_sections (void)
>     if (target_pure_code)
>       text_section->unnamed.data = "\t.section .text,\"0x20000006\",%progbits";
>   #endif
> +
> +  noinit_section = get_unnamed_section (0, output_section_asm_op, ".section .noinit,\"aw\"");
> +}
> +
> +static section *
> +arm_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align)
> +{
>
> Please add a function comment.
OK

>
> +  gcc_assert (decl != NULL_TREE);
> +
> +  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE)
> +    return noinit_section;
> +  else
> +    return default_elf_select_section (decl, reloc, align);
>   }
>
>   /* Output unwind directives for the start/end of a function.  */
> @@ -31520,6 +31582,9 @@ arm_elf_section_type_flags (tree decl, const char *name, int reloc)
>     if (decl && TREE_CODE (decl) == FUNCTION_DECL && target_pure_code)
>       flags |= SECTION_ARM_PURECODE;
>
> +  if (strcmp (name, ".noinit") == 0)
> +    flags = SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE;
> +
>
>
> You're overwriting the flags here. Are you sure you don't want "flags |= ..." here?
Indeed!

Here is an updated patch, which also addresses Sandra's comments.

Thanks

>
>
>   return flags;
>   }
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 2520835..d544527 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -6684,6 +6684,7 @@ attributes.
>   @menu
>   * Common Variable Attributes::
>   * ARC Variable Attributes::
> +* ARM Variable Attributes::
>   * AVR Variable Attributes::
>   * Blackfin Variable Attributes::
>   * H8/300 Variable Attributes::
> @@ -7131,6 +7132,18 @@ given via attribute argument.
>
>   @end table
>
> +@node ARM Variable Attributes
> +@subsection ARM Variable Attributes
> +
> +@table @code
> +@item noinit
> +@cindex @code{noinit} variable attribute, ARM
> +Any data with the @code{noinit} attribute will not be initialised by
> +the C runtime startup code, or the program loader.  Not initialising
> +data in this way can reduce program startup times.
> +
> +@end table
> +
>   @node AVR Variable Attributes
>   @subsection AVR Variable Attributes
>
> diff --git a/gcc/testsuite/gcc.target/arm/data-attributes.c b/gcc/testsuite/gcc.target/arm/data-attributes.c
> new file mode 100644
> index 0000000..323c8e0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/data-attributes.c
> @@ -0,0 +1,51 @@
> +/* { dg-do run { target { ! *-*-linux* } } } */
> +/* { 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
> +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 ();
> +}
>

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

commit d1f3585a50b0b4ff3df6c833d99c4be0065f1363
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Tue Jun 11 21:09:08 2019 +0000

    Add support for noinit attribute.
    
    Change-Id: Ib7090c037f67e521ad9753e1a78ed5731996fefe

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index e3e71ea..caac216 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -151,6 +151,7 @@ static tree arm_handle_notshared_attribute (tree *, tree, tree, int, bool *);
 #endif
 static tree arm_handle_cmse_nonsecure_entry (tree *, tree, tree, int, bool *);
 static tree arm_handle_cmse_nonsecure_call (tree *, tree, tree, int, bool *);
+static tree arm_data_attr (tree *, tree, tree, int, bool *);
 static void arm_output_function_epilogue (FILE *);
 static void arm_output_function_prologue (FILE *);
 static int arm_comp_type_attributes (const_tree, const_tree);
@@ -375,7 +376,8 @@ static const struct attribute_spec arm_attribute_table[] =
     arm_handle_cmse_nonsecure_entry, NULL },
   { "cmse_nonsecure_call", 0, 0, true, false, false, true,
     arm_handle_cmse_nonsecure_call, NULL },
-  { NULL, 0, 0, false, false, false, false, NULL, NULL }
+  { "noinit", 0, 0, true, false, false, false, arm_data_attr, NULL },
+  { NULL, 0, 0, false, false, false, false, NULL, NULL },
 };
 \f
 /* Initialize the GCC target structure.  */
@@ -808,6 +810,10 @@ static const struct attribute_spec arm_attribute_table[] =
 
 #undef TARGET_CONSTANT_ALIGNMENT
 #define TARGET_CONSTANT_ALIGNMENT arm_constant_alignment
+
+#undef  TARGET_ASM_SELECT_SECTION
+#define TARGET_ASM_SELECT_SECTION arm_select_section
+
 \f
 /* Obstack for minipool constant handling.  */
 static struct obstack minipool_obstack;
@@ -7150,6 +7156,47 @@ arm_handle_cmse_nonsecure_call (tree *node, tree name,
   return NULL_TREE;
 }
 
+/* Called when the noinit attribute is used. 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
+arm_data_attr (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;
+}
+
 /* Return 0 if the attributes for two types are incompatible, 1 if they
    are compatible, and 2 if they are nearly compatible (which causes a
    warning to be generated).  */
@@ -27890,6 +27937,8 @@ arm_asm_emit_except_personality (rtx personality)
 
 /* Implement TARGET_ASM_INITIALIZE_SECTIONS.  */
 
+static section *noinit_section;
+
 static void
 arm_asm_init_sections (void)
 {
@@ -27902,6 +27951,21 @@ arm_asm_init_sections (void)
   if (target_pure_code)
     text_section->unnamed.data = "\t.section .text,\"0x20000006\",%progbits";
 #endif
+
+  noinit_section = get_unnamed_section (0, output_section_asm_op, ".section .noinit,\"aw\"");
+}
+
+/* Select noinit_section if DECL has the "noinit" attribute, use the
+   default ELF rules otherwise.  */
+static section *
+arm_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align)
+{
+  gcc_assert (decl != NULL_TREE);
+
+  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE)
+    return noinit_section;
+  else
+    return default_elf_select_section (decl, reloc, align);
 }
 
 /* Output unwind directives for the start/end of a function.  */
@@ -31520,6 +31584,9 @@ arm_elf_section_type_flags (tree decl, const char *name, int reloc)
   if (decl && TREE_CODE (decl) == FUNCTION_DECL && target_pure_code)
     flags |= SECTION_ARM_PURECODE;
 
+  if (strcmp (name, ".noinit") == 0)
+    flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE;
+
   return flags;
 }
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 2520835..d27ebf9 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -6684,6 +6684,7 @@ attributes.
 @menu
 * Common Variable Attributes::
 * ARC Variable Attributes::
+* ARM Variable Attributes::
 * AVR Variable Attributes::
 * Blackfin Variable Attributes::
 * H8/300 Variable Attributes::
@@ -7131,6 +7132,18 @@ given via attribute argument.
 
 @end table
 
+@node ARM Variable Attributes
+@subsection ARM Variable Attributes
+
+@table @code
+@item noinit
+@cindex @code{noinit} variable attribute, ARM
+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 AVR Variable Attributes
 @subsection AVR Variable Attributes
 
diff --git a/gcc/testsuite/gcc.target/arm/data-attributes.c b/gcc/testsuite/gcc.target/arm/data-attributes.c
new file mode 100644
index 0000000..323c8e0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/data-attributes.c
@@ -0,0 +1,51 @@
+/* { dg-do run { target { ! *-*-linux* } } } */
+/* { 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
+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 ();
+}