From: Christophe Lyon <christophe.lyon@linaro.org>
To: Richard Earnshaw <Richard.Earnshaw@arm.com>
Cc: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>,
gcc Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH][ARM] Add support for "noinit" attribute
Date: Wed, 03 Jul 2019 12:56:00 -0000 [thread overview]
Message-ID: <CAKdteOYebf9V0i=56Rx_vzn0dugHGu742yYJ6bWit4izd6BwmQ@mail.gmail.com> (raw)
In-Reply-To: <783c8b88-c5f9-d87b-7a60-947c1d9d72b0@arm.com>
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 ();
> >>>>> +}
> >>>>>
next prev parent reply other threads:[~2019-07-03 12:41 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-13 15:13 Christophe Lyon
2019-07-01 12:16 ` Christophe Lyon
2019-07-01 15:58 ` Kyrill Tkachov
2019-07-02 8:39 ` Richard Earnshaw
2019-07-02 8:44 ` Richard Earnshaw
2019-07-02 10:13 ` Richard Earnshaw
2019-07-02 10:30 ` Richard Earnshaw
2019-07-02 14:49 ` Christophe Lyon
2019-07-03 9:51 ` Richard Earnshaw
2019-07-03 12:56 ` Christophe Lyon [this message]
2019-07-02 15:01 ` Christophe Lyon
2019-07-02 15:44 ` Martin Sebor
2019-07-02 17:12 ` Segher Boessenkool
2019-07-01 21:35 ` Sandra Loosemore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKdteOYebf9V0i=56Rx_vzn0dugHGu742yYJ6bWit4izd6BwmQ@mail.gmail.com' \
--to=christophe.lyon@linaro.org \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@foss.arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).