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

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Ping?

Le mar. 9 juil. 2019 à 00:04, Martin Sebor <msebor@gmail.com> a écrit :

> On 7/8/19 5:10 AM, Christophe Lyon wrote:
> > On Sat, 6 Jul 2019 at 19:57, Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 7/4/19 9:27 AM, Christophe Lyon wrote:
> >>> Hi,
> >>>
> >>> Similar to what already exists for TI msp430 or in TI compilers for
> >>> arm, this patch adds support for the "noinit" attribute.
> >>>
> >>> It is convenient 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.
> >>>
> >>> However, I'm not sure for which other targets (beyond arm), I should
> >>> update the linker scripts.
> >>>
> >>> I left the new testcase in gcc.target/arm, guarded by
> >>> dg-do run { target { *-*-eabi } }
> >>> but since this attribute is now generic, I suspect I should move the
> >>> test to some other place. But then, which target selector should I
> >>> use?
> >>>
> >>> Finally, I tested on arm-eabi, but not on msp430 for which I do not
> >>> have the environment, so advice from msp430 maintainers is
> >>> appreciated. Since msp430 does not use the same default helpers as
> >>> arm, I left the "noinit" handling code in place, to avoid changing
> >>> other generic functions which I don't know how to test
> >>> (default_select_section, default_section_type_flags).
> >>>
> >>
> >> Since the section attribute is mutually exclusive with noinit,
> >> defining an attribute_exclusions array with the mutually exclusive
> >> attributes and setting the last member of the corresponding
> >> c_common_attribute_table element to that array (and updating
> >> the arrays for the other mutually exclusive attributes) would
> >> be the expected way to express that constraint:
> >>
> >> +  { "noinit",                 0, 0, true,  false, false, false,
> >> +                             handle_noinit_attribute, NULL },
> >>                                                          ^^^^
> >> This should also make it possible to remove the explicit handling
> >> from handle_section_attribute and handle_noinit_attribute.  If
> >> that's not completely possible then in the following please be
> >> sure to quote the names of the attributes:
> >>
> >> @@ -1912,6 +1915,13 @@ handle_section_attribute (tree *node, tree
> >> ARG_UNUSED (name), tree args,
> >>          goto fail;
> >>        }
> >>
> >> +  if (DECL_P (decl) && lookup_attribute ("noinit", DECL_ATTRIBUTES
> >> (decl)) != NULL_TREE)
> >> +    {
> >> +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
> >> +               "section attribute cannot be applied to variables with
> noinit
> >> attribute");
> >>
> >> Martin
> >>
> >
> > Thanks, here is an updated version:
> > - adds exclusion array
> > - updates testcase with new error message (because of exclusion)
>
> The exclusion bits look good, thank you!
>
> Martin
>
> > - moves testcase to gcc.c-torture/execute
> > - adds "noinit" effective-target
> >
> > Christophe
> >
> >>> Thanks,
> >>>
> >>> Christophe
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * doc/extend.texi: Add "noinit" attribute documentation.
> >>> * varasm.c
> >>> (default_section_type_flags): Add support for "noinit" section.
> >>> (default_elf_select_section): Add support for "noinit" attribute.
> >>>
> >>> gcc/c-family/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * c-attribs.c (c_common_attribute_table): Add "noinit" entry.
> >>> (handle_section_attribute): Add support for "noinit" attribute.
> >>> (handle_noinit_attribute): New function.
> >>>
> >>> gcc/config/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> 2019-07-04  Christophe Lyon  <christophe.lyon@linaro.org>
> >>>
> >>> * gcc.target/arm/noinit-attribute.c: New test.
> >>>
> >>
>
>