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

[Martin Sebor <msebor@gmail.com>]

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.
>>>
>>