From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20092 invoked by alias); 1 Jul 2019 15:58:59 -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 20079 invoked by uid 89); 1 Jul 2019 15:58:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-15.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=1516, menu, personality 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; Mon, 01 Jul 2019 15:58:57 +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 492662B; Mon, 1 Jul 2019 08:58:56 -0700 (PDT) Received: from [10.2.206.47] (e120808-lin.cambridge.arm.com [10.2.206.47]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E6C303F246; Mon, 1 Jul 2019 08:58:55 -0700 (PDT) Subject: Re: [PATCH][ARM] Add support for "noinit" attribute To: Christophe Lyon , gcc Patches References: From: Kyrill Tkachov Message-ID: Date: Mon, 01 Jul 2019 15:58: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: 8bit X-SW-Source: 2019-07/txt/msg00065.txt.bz2 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... 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 (); +}