On Tue, 2 Jul 2019 at 12:30, Richard Earnshaw <richard.earns...@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 ();
> >>> +}
> >>>
