From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85728 invoked by alias); 3 Jul 2019 09:51:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 85720 invoked by uid 89); 3 Jul 2019 09:51:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_PASS,UNSUBSCRIBE_BODY autolearn=ham version=3.3.1 spammy= X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.110.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 Jul 2019 09:51:10 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DA8E9344; Wed, 3 Jul 2019 02:51:08 -0700 (PDT) Received: from e120077-lin.cambridge.arm.com (e120077-lin.cambridge.arm.com [10.2.206.226]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 026013F246; Wed, 3 Jul 2019 02:51:07 -0700 (PDT) Subject: Re: [PATCH][ARM] Add support for "noinit" attribute To: Christophe Lyon Cc: Kyrill Tkachov , gcc Patches References: <02c6a306-9f00-c3f0-ce34-0a3341111857@arm.com> From: Richard Earnshaw Message-ID: <783c8b88-c5f9-d87b-7a60-947c1d9d72b0@arm.com> Date: Wed, 03 Jul 2019 09:51:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2019-07/txt/msg00246.txt.bz2 On 02/07/2019 15:49, Christophe Lyon wrote: > On Tue, 2 Jul 2019 at 12:30, Richard Earnshaw 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. 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... > 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. 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 (); >>>>> +} >>>>>