[PATCH][ARM] Add support for "noinit" attribute

Richard Earnshaw Richard.Earnshaw@arm.com
Tue Jul 2 08:44:00 GMT 2019 


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

Oh, and the tests need to handle the other OSs we support that won't 
support this extension.  The list is longer than just 'linux'

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 ();
>> +}
>>

More information about the Gcc-patches mailing list