This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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




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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]