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

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

[Christophe Lyon]

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

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?

Thanks,

Christophe

[-- Attachment #2: noinit-v3.chlog.txt --]
[-- Type: text/plain, Size: 506 bytes --]

gcc/ChangeLog:

2019-06-13  Christophe Lyon  <christophe.lyon@linaro.org>

	* config/arm/arm.c (arm_attribute_table): Add "noinit" entry.
	(arm_data_attr): New helper function.
	(TARGET_ASM_SELECT_SECTION): New.
	(arm_select_section): New function.
	(arm_elf_section_type_flags): Add support for "noinit" section.
	* doc/extend.texi: Add "noinit" attribute documentation.

gcc/testsuite/ChangeLog:

2019-06-13  Christophe Lyon  <christophe.lyon@linaro.org>

	* gcc.target/arm/data-attributes.c: New test.


[-- Attachment #3: noinit-v3.patch.txt --]
[-- Type: text/plain, Size: 7002 bytes --]

commit e04bcd361a87925ac8240bc106f64a37917bb804
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..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 },
 };
 \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 ATTRIBUTE_UNUSED,
+		  int    flags ATTRIBUTE_UNUSED,
+		  bool * no_add_attrs ATTRIBUTE_UNUSED)
+{
+  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,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)
+{
+  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;
+
   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 ();
+}

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

[Christophe Lyon]

ping?

On Thu, 13 Jun 2019 at 17:13, Christophe Lyon
<christophe.lyon@linaro.org> 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?
>
> Thanks,
>
> Christophe

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

[Kyrill Tkachov]

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?
Arguably args is also checked against NULL, so it's technically not unused either.

+  const char * message = NULL;
+
+  gcc_assert (DECL_P (* node));

The house style doesn't have a space after '*'. Same elsewhere in the patch.

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

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


  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 ();
+}

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

[Sandra Loosemore]

On 6/13/19 9:13 AM, Christophe Lyon wrote:
> @@ -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
>  

Minor nit:  please use American spelling:  initialized, initializing.

-Sandra

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

[Richard Earnshaw]

On 01/07/2019 16:58, Kyrill Tkachov 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...

Can't this be done using named sections?  If the sections were of the 
form .bss.<foo> then it would be easy to make linker scripts do 
something sane by default and users could filter them out to special 
noinit sections if desired.

R.

> 
> 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?
> Arguably args is also checked against NULL, so it's technically not 
> unused either.
> 
> +  const char * message = NULL;
> +
> +  gcc_assert (DECL_P (* node));
> 
> The house style doesn't have a space after '*'. Same elsewhere in the 
> patch.
> 
> +  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.
> 
> +  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?
> 
> 
>   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 ();
> +}
> 

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

[Richard Earnshaw]

On 02/07/2019 09:39, Richard Earnshaw wrote:
> 
> 
> On 01/07/2019 16:58, Kyrill Tkachov 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...
> 
> Can't this be done using named sections?  If the sections were of the 
> form .bss.<foo> then it would be easy to make linker scripts do 
> something sane by default and users could filter them out to special 
> noinit sections if desired.
> 

Oh, and the tests need to handle the other OSs we support that won't 
support this extension.  The list is longer than just 'linux'

R.

> R.
> 
>>
>> 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?
>> Arguably args is also checked against NULL, so it's technically not 
>> unused either.
>>
>> +  const char * message = NULL;
>> +
>> +  gcc_assert (DECL_P (* node));
>>
>> The house style doesn't have a space after '*'. Same elsewhere in the 
>> patch.
>>
>> +  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.
>>
>> +  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?
>>
>>
>>   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 ();
>> +}
>>

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

[Richard Earnshaw]

On 02/07/2019 09:39, Richard Earnshaw wrote:
> 
> 
> On 01/07/2019 16:58, Kyrill Tkachov 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...
> 
> Can't this be done using named sections?  If the sections were of the 
> form .bss.<foo> then it would be easy to make linker scripts do 
> something sane by default and users could filter them out to special 
> noinit sections if desired.
> 

To answer my own question, it would appear to be yes.  You can write today:

int xxx __attribute__ ((section (".bss.noinit")));

int main ()
{
   return xxx;
}

And the compiler will generate
	.section	.bss.noinit,"aw",@nobits
	.align 4
	.type	xxx, @object
	.size	xxx, 4
xxx:
	.zero	4

So at this point, all you need is a linker script to filter .bss.noinit 
into your special part of the final image.

This will most likely work today on any GCC target that supports named 
sections, which is pretty much all of them these days.

R.

> R.
> 
>>
>> 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?
>> Arguably args is also checked against NULL, so it's technically not 
>> unused either.
>>
>> +  const char * message = NULL;
>> +
>> +  gcc_assert (DECL_P (* node));
>>
>> The house style doesn't have a space after '*'. Same elsewhere in the 
>> patch.
>>
>> +  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.
>>
>> +  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?
>>
>>
>>   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 ();
>> +}
>>

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

[Richard Earnshaw]

On 02/07/2019 11:13, Richard Earnshaw wrote:
> 
> 
> On 02/07/2019 09:39, Richard Earnshaw wrote:
>>
>>
>> On 01/07/2019 16:58, Kyrill Tkachov 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...
>>
>> Can't this be done using named sections?  If the sections were of the 
>> form .bss.<foo> then it would be easy to make linker scripts do 
>> something sane by default and users could filter them out to special 
>> noinit sections if desired.
>>
> 
> To answer my own question, it would appear to be yes.  You can write today:
> 
> int xxx __attribute__ ((section (".bss.noinit")));
> 
> int main ()
> {
>    return xxx;
> }
> 
> And the compiler will generate
>      .section    .bss.noinit,"aw",@nobits
>      .align 4
>      .type    xxx, @object
>      .size    xxx, 4
> xxx:
>      .zero    4
> 
> So at this point, all you need is a linker script to filter .bss.noinit 
> into your special part of the final image.
> 
> This will most likely work today on any GCC target that supports named 
> sections, which is pretty much all of them these days.
> 

Alternatively, we already have:

https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01302.html

R.

> R.
> 
>> R.
>>
>>>
>>> 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?
>>> Arguably args is also checked against NULL, so it's technically not 
>>> unused either.
>>>
>>> +  const char * message = NULL;
>>> +
>>> +  gcc_assert (DECL_P (* node));
>>>
>>> The house style doesn't have a space after '*'. Same elsewhere in the 
>>> patch.
>>>
>>> +  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.
>>>
>>> +  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?
>>>
>>>
>>>   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 ();
>>> +}
>>>

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

[Christophe Lyon]

On Tue, 2 Jul 2019 at 12:30, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
>
>
>
> On 02/07/2019 11:13, Richard Earnshaw wrote:
> >
> >
> > On 02/07/2019 09:39, Richard Earnshaw wrote:
> >>
> >>
> >> On 01/07/2019 16:58, Kyrill Tkachov 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...
> >>
> >> Can't this be done using named sections?  If the sections were of the
> >> form .bss.<foo> then it would be easy to make linker scripts do
> >> something sane by default and users could filter them out to special
> >> noinit sections if desired.
> >>
> >
> > To answer my own question, it would appear to be yes.  You can write today:
> >
> > int xxx __attribute__ ((section (".bss.noinit")));
> >
> > int main ()
> > {
> >    return xxx;
> > }
> >
> > And the compiler will generate
> >      .section    .bss.noinit,"aw",@nobits
> >      .align 4
> >      .type    xxx, @object
> >      .size    xxx, 4
> > xxx:
> >      .zero    4
> >
> > So at this point, all you need is a linker script to filter .bss.noinit
> > into your special part of the final image.
> >
> > This will most likely work today on any GCC target that supports named
> > sections, which is pretty much all of them these days.
> >
>
> Alternatively, we already have:
>
> https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01302.html
>
> R.
>

Hi Richard,

Indeed this can already be achieved with the "section" attribute as you propose.

The motivation for this patch came from user requests: this feature is
already available in some proprietary ARM toolchains (TI, IAR, ...)
and it's more convenient for the end-user than having to update linker
scripts in addition to adding an attribute to the variable.

I guess it's a balance between user-friendliness/laziness and GCC
developers ability to educate users :-)

Christophe


> > R.
> >
> >> R.
> >>
> >>>
> >>> 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?
> >>> Arguably args is also checked against NULL, so it's technically not
> >>> unused either.
> >>>
> >>> +  const char * message = NULL;
> >>> +
> >>> +  gcc_assert (DECL_P (* node));
> >>>
> >>> The house style doesn't have a space after '*'. Same elsewhere in the
> >>> patch.
> >>>
> >>> +  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.
> >>>
> >>> +  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?
> >>>
> >>>
> >>>   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 ();
> >>> +}
> >>>

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

[Christophe Lyon]

[-- 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 ();
+}

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

[Martin Sebor]

On 7/2/19 9:01 AM, Christophe Lyon wrote:
> 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...

Since it's not inherently ARM-specific I would suggest making
this a generic attribute, even if it's only supported by a small
subset of targets at the momemnt.  Adding to common attributes
will "reserve" its name and semantics for the targets where it
isn't supported yet.  It should also make it possible to issue
a more user-friendly diagnostic than the boilerplate "warning:
'noinit' attribute directive ignored" when the attribute is used
on a target that doesn't support it.

Martin

>> 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 ();
>> +}
>>

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

[Segher Boessenkool]

On Tue, Jul 02, 2019 at 05:01:33PM +0200, Christophe Lyon wrote:
> On Mon, 1 Jul 2019 at 17:58, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> > +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 :-)

This attribute means that parameter is *possibly* unused.  It doesn't have
to be.

Now that we have C++ you could write this as

static tree
arm_data_attr (tree *node, tree name, tree /*args*/, int /*flags*/,
               bool */*no_add_attrs*/)
{

(or leave the names completely away where that makes sense), and then
the parameter *has* to be unused (you *cannot* refer to it ;-) )


Segher

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

[Richard Earnshaw]

On 02/07/2019 15:49, Christophe Lyon wrote:
> On Tue, 2 Jul 2019 at 12:30, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
>>
>>
>>
>> On 02/07/2019 11:13, Richard Earnshaw wrote:
>>>
>>>
>>> On 02/07/2019 09:39, Richard Earnshaw wrote:
>>>>
>>>>
>>>> On 01/07/2019 16:58, Kyrill Tkachov 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...
>>>>
>>>> Can't this be done using named sections?  If the sections were of the
>>>> form .bss.<foo> then it would be easy to make linker scripts do
>>>> something sane by default and users could filter them out to special
>>>> noinit sections if desired.
>>>>
>>>
>>> To answer my own question, it would appear to be yes.  You can write today:
>>>
>>> int xxx __attribute__ ((section (".bss.noinit")));
>>>
>>> int main ()
>>> {
>>>     return xxx;
>>> }
>>>
>>> And the compiler will generate
>>>       .section    .bss.noinit,"aw",@nobits
>>>       .align 4
>>>       .type    xxx, @object
>>>       .size    xxx, 4
>>> xxx:
>>>       .zero    4
>>>
>>> So at this point, all you need is a linker script to filter .bss.noinit
>>> into your special part of the final image.
>>>
>>> This will most likely work today on any GCC target that supports named
>>> sections, which is pretty much all of them these days.
>>>
>>
>> Alternatively, we already have:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01302.html
>>
>> R.
>>
> 
> Hi Richard,
> 
> Indeed this can already be achieved with the "section" attribute as you propose.
> 
> The motivation for this patch came from user requests: this feature is
> already available in some proprietary ARM toolchains (TI, IAR, ...)
> and it's more convenient for the end-user than having to update linker
> scripts in addition to adding an attribute to the variable.
> 

? Your patch has an update to the linker scripts...



> I guess it's a balance between user-friendliness/laziness and GCC
> developers ability to educate users :-)

Well in that case, this should be done generically, not in just the arm 
backend, or any other backend for that matter.

R.

> 
> Christophe
> 
> 
>>> R.
>>>
>>>> R.
>>>>
>>>>>
>>>>> 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?
>>>>> Arguably args is also checked against NULL, so it's technically not
>>>>> unused either.
>>>>>
>>>>> +  const char * message = NULL;
>>>>> +
>>>>> +  gcc_assert (DECL_P (* node));
>>>>>
>>>>> The house style doesn't have a space after '*'. Same elsewhere in the
>>>>> patch.
>>>>>
>>>>> +  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.
>>>>>
>>>>> +  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?
>>>>>
>>>>>
>>>>>    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 ();
>>>>> +}
>>>>>

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

[Christophe Lyon]

On Wed, 3 Jul 2019 at 11:51, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
>
>
>
> On 02/07/2019 15:49, Christophe Lyon wrote:
> > On Tue, 2 Jul 2019 at 12:30, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
> >>
> >>
> >>
> >> On 02/07/2019 11:13, Richard Earnshaw wrote:
> >>>
> >>>
> >>> On 02/07/2019 09:39, Richard Earnshaw wrote:
> >>>>
> >>>>
> >>>> On 01/07/2019 16:58, Kyrill Tkachov 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...
> >>>>
> >>>> Can't this be done using named sections?  If the sections were of the
> >>>> form .bss.<foo> then it would be easy to make linker scripts do
> >>>> something sane by default and users could filter them out to special
> >>>> noinit sections if desired.
> >>>>
> >>>
> >>> To answer my own question, it would appear to be yes.  You can write today:
> >>>
> >>> int xxx __attribute__ ((section (".bss.noinit")));
> >>>
> >>> int main ()
> >>> {
> >>>     return xxx;
> >>> }
> >>>
> >>> And the compiler will generate
> >>>       .section    .bss.noinit,"aw",@nobits
> >>>       .align 4
> >>>       .type    xxx, @object
> >>>       .size    xxx, 4
> >>> xxx:
> >>>       .zero    4
> >>>
> >>> So at this point, all you need is a linker script to filter .bss.noinit
> >>> into your special part of the final image.
> >>>
> >>> This will most likely work today on any GCC target that supports named
> >>> sections, which is pretty much all of them these days.
> >>>
> >>
> >> Alternatively, we already have:
> >>
> >> https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01302.html
> >>
> >> R.
> >>
> >
> > Hi Richard,
> >
> > Indeed this can already be achieved with the "section" attribute as you propose.
> >
> > The motivation for this patch came from user requests: this feature is
> > already available in some proprietary ARM toolchains (TI, IAR, ...)
> > and it's more convenient for the end-user than having to update linker
> > scripts in addition to adding an attribute to the variable.
> >
>
> ? Your patch has an update to the linker scripts...

Right, but that becomes a feature of the toolchain, rather than having
to edit/create linker scripts for every application.

> > I guess it's a balance between user-friendliness/laziness and GCC
> > developers ability to educate users :-)
>
> Well in that case, this should be done generically, not in just the arm
> backend, or any other backend for that matter.
>

I thought it would be less controversial to mimic msp430, it seems I
was wrong  :)

I'm going to have a look at making this generic, then.

Christophe

> R.
>
> >
> > Christophe
> >
> >
> >>> R.
> >>>
> >>>> R.
> >>>>
> >>>>>
> >>>>> 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?
> >>>>> Arguably args is also checked against NULL, so it's technically not
> >>>>> unused either.
> >>>>>
> >>>>> +  const char * message = NULL;
> >>>>> +
> >>>>> +  gcc_assert (DECL_P (* node));
> >>>>>
> >>>>> The house style doesn't have a space after '*'. Same elsewhere in the
> >>>>> patch.
> >>>>>
> >>>>> +  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.
> >>>>>
> >>>>> +  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?
> >>>>>
> >>>>>
> >>>>>    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 ();
> >>>>> +}
> >>>>>