From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67706 invoked by alias); 3 Jul 2019 12:41:38 -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 67698 invoked by uid 89); 3 Jul 2019 12:41:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS,UNSUBSCRIBE_BODY autolearn=ham version=3.3.1 spammy= X-HELO: mail-lj1-f196.google.com Received: from mail-lj1-f196.google.com (HELO mail-lj1-f196.google.com) (209.85.208.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 Jul 2019 12:41:35 +0000 Received: by mail-lj1-f196.google.com with SMTP id 205so2247775ljj.8 for ; Wed, 03 Jul 2019 05:41:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pfti6C3LvaotneUS7+FGiDp8eRoSZV33Vh/xB1oVbQI=; b=nrN/IeffGOToZ970ntvvttga0or/T7P9/LvsUh6BV6HBpJcjrjoj/CqXDZ2V6oUwJz Scf3BihHqFBy18uEGdLT5Te9FXcROyGGoSyKrZzEuHmYlwcGzA+HG2las25KKGva7Cpu MZE6fz7oZZHuaFrikuvxmnT6I1eNTapMFOHCYvL2A22WZ2wz+ljiyaYNklT9SdPCdjR/ 6TreEMIMXuQIBSH2EEpdIT6Kt2gAreqxFOwMMrrpAeWm1M4uxvQVq5nrWpNyO6jbm4X5 wg1myWgGfEc4Fb2zAN2TQWxzfNxeRQJB1449KAFWkyfYVLc687GBTPssMpZwck0zT3OG MyBw== MIME-Version: 1.0 References: <02c6a306-9f00-c3f0-ce34-0a3341111857@arm.com> <783c8b88-c5f9-d87b-7a60-947c1d9d72b0@arm.com> In-Reply-To: <783c8b88-c5f9-d87b-7a60-947c1d9d72b0@arm.com> From: Christophe Lyon Date: Wed, 03 Jul 2019 12:56:00 -0000 Message-ID: Subject: Re: [PATCH][ARM] Add support for "noinit" attribute To: Richard Earnshaw Cc: Kyrill Tkachov , gcc Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-07/txt/msg00270.txt.bz2 On Wed, 3 Jul 2019 at 11:51, Richard Earnshaw wrote: > > > > 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... 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 (); > >>>>> +} > >>>>>