Hi Kyrill,
On Mon, 1 Jul 2019 at 17:58, Kyrill Tkachov <kyrylo.tkac...@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 ();
> +}
>
commit d1f3585a50b0b4ff3df6c833d99c4be0065f1363
Author: Christophe Lyon <christophe.l...@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 },
};
/* 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,
+ 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 ();
+}
