Message ID | CAKdteObaTtriRDdWTkzr66Yym0zapsYM=0meT10ihC4K9dmFJg@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | Add generic support for "noinit" attribute | expand |
On Thu, 4 Jul 2019 17:27:28 +0200 Christophe Lyon <christophe.lyon@linaro.org> wrote: > 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). > I'll take a look at the MSP430 case soon. Thanks, Jozef
Hi, > diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c > index 365e9eb..8266fa0 100644 > --- a/gcc/config/msp430/msp430.c > +++ b/gcc/config/msp430/msp430.c > @@ -1807,7 +1807,6 @@ const char * const ATTR_CRIT = "critical"; > const char * const ATTR_LOWER = "lower"; > const char * const ATTR_UPPER = "upper"; > const char * const ATTR_EITHER = "either"; > -const char * const ATTR_NOINIT = "noinit"; > const char * const ATTR_PERSIST = "persistent"; > > static inline bool With this change removed so that ATTR_NOINIT is still defined, your patch is ok for msp430 - the attribute still behaves as expected. I'm holding off using default_elf_select_section instead of default_select_section in msp430.c since there could be some possible conflicts with the MSPABI introduced by using elf_select_section that need to be properly considered before making the change. I think default_select_section is supposed to be very terse, so I'm not sure if it would be even be suitable to extend it to handle noinit. With that said, could you please add the following FIXME to your patch: diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c index 365e9eba747..b403b1f5a42 100644 --- a/gcc/config/msp430/msp430.c +++ b/gcc/config/msp430/msp430.c @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) { if (TREE_CODE (decl) == FUNCTION_DECL) return text_section; + /* FIXME: ATTR_NOINIT is handled generically in + default_elf_select_section. */ else if (has_attr (ATTR_NOINIT, decl)) return noinit_section; else if (has_attr (ATTR_PERSIST, decl)) Also, the gcc.target/arm/noinit-attribute.c test works with msp430. Why not create a effective-target keyword which checks for noinit support, so the test can be gated on that condition and put in a generic part of the testsuite? After doing some further testing, I noticed the attribute does not work on static variables. The attribute handling code allows the attribute to be set on a static variable, but then that variable does not get placed in the .noinit section. What are your thoughts on this? Does it even makes sense for a static local variable to be declared as "noinit"? One should want a static local variable to be initialized, as having it not initialized and hold a random value could cause problems. Perhaps a user could think of some use for this, but I can't. Finally, I would point out that "contrib/check_GNU_style.sh" reports some style issues with your patch. Thanks and regards, Jozef
On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote: > > Hi, > > > diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c > > index 365e9eb..8266fa0 100644 > > --- a/gcc/config/msp430/msp430.c > > +++ b/gcc/config/msp430/msp430.c > > @@ -1807,7 +1807,6 @@ const char * const ATTR_CRIT = "critical"; > > const char * const ATTR_LOWER = "lower"; > > const char * const ATTR_UPPER = "upper"; > > const char * const ATTR_EITHER = "either"; > > -const char * const ATTR_NOINIT = "noinit"; > > const char * const ATTR_PERSIST = "persistent"; > > > > static inline bool > > With this change removed so that ATTR_NOINIT is still defined, your patch is > ok for msp430 - the attribute still behaves as expected. Oops sorry for this. > I'm holding off using default_elf_select_section instead of > default_select_section in msp430.c since there could be some possible conflicts > with the MSPABI introduced by using elf_select_section that need to be properly > considered before making the change. > > I think default_select_section is supposed to be very terse, so I'm not sure > if it would be even be suitable to extend it to handle noinit. Yes, that was my worry too. > With that said, could you please add the following FIXME to your patch: OK > diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c > index 365e9eba747..b403b1f5a42 100644 > --- a/gcc/config/msp430/msp430.c > +++ b/gcc/config/msp430/msp430.c > @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned > HOST_WIDE_INT align) { > if (TREE_CODE (decl) == FUNCTION_DECL) > return text_section; > + /* FIXME: ATTR_NOINIT is handled generically in > + default_elf_select_section. */ > else if (has_attr (ATTR_NOINIT, decl)) > return noinit_section; > else if (has_attr (ATTR_PERSIST, decl)) > > Also, the gcc.target/arm/noinit-attribute.c test works with msp430. > Why not create a effective-target keyword which checks for noinit support, so > the test can be gated on that condition and put in a generic part of the > testsuite? That was my intention, and I was waiting for feedback on this. In this case, I suppose the effective-target check would test a hardcoded list of targets, like many other effective-targets? (eg check_weak_available) > After doing some further testing, I noticed the attribute does not work on > static variables. The attribute handling code allows the attribute to be set on > a static variable, but then that variable does not get placed in the .noinit > section. > > What are your thoughts on this? > > Does it even makes sense for a static local variable to be declared as "noinit"? > One should want a static local variable to be initialized, as having it not > initialized and hold a random value could cause problems. > Perhaps a user could think of some use for this, but I can't. > I added: static int __attribute__((noinit)) var_noinit2; in global scope, and static int __attribute__((noinit)) var_noinit3; in main(), and the generate code contains: .section .noinit,"aw" .align 2 .set .LANCHOR2,. + 0 .type var_noinit2, %object .size var_noinit2, 4 var_noinit2: .space 4 .type var_noinit3.4160, %object .size var_noinit3.4160, 4 var_noinit3.4160: .space 4 .type var_noinit, %object .size var_noinit, 4 var_noinit: .space 4 So, all three of them are in the .noinit section, but only var_noinit has .global var_noinit So it seems to work? > Finally, I would point out that "contrib/check_GNU_style.sh" reports some style > issues with your patch. Thanks for noticing. > > Thanks and regards, > Jozef
On Fri, 5 Jul 2019 11:26:20 +0200 Christophe Lyon <christophe.lyon@linaro.org> wrote: > On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote: > > > > Also, the gcc.target/arm/noinit-attribute.c test works with msp430. > > Why not create a effective-target keyword which checks for noinit support, so > > the test can be gated on that condition and put in a generic part of the > > testsuite? > > That was my intention, and I was waiting for feedback on this. In this > case, I suppose the effective-target check would test a hardcoded list > of targets, like many other effective-targets? > (eg check_weak_available) Were there previous discussions on whether the noinit attribute should be explicitly enabled for certain targets? e.g. so it will error if you try to use it on x86_64. If it will just be enabled by default for all targets then a hardcoded list for check-effective target sounds ok. Otherwise if it does cause an error when used on an unsupported target you could do a check that the attribute compiles successfully instead. > > > After doing some further testing, I noticed the attribute does not work on > > static variables. The attribute handling code allows the attribute to be set on > > a static variable, but then that variable does not get placed in the .noinit > > section. > > > > What are your thoughts on this? > > > > Does it even makes sense for a static local variable to be declared as "noinit"? > > One should want a static local variable to be initialized, as having it not > > initialized and hold a random value could cause problems. > > Perhaps a user could think of some use for this, but I can't. > > > I added: > static int __attribute__((noinit)) var_noinit2; > in global scope, and > static int __attribute__((noinit)) var_noinit3; > in main(), and the generate code contains: > .section .noinit,"aw" > .align 2 > .set .LANCHOR2,. + 0 > .type var_noinit2, %object > .size var_noinit2, 4 > var_noinit2: > .space 4 > .type var_noinit3.4160, %object > .size var_noinit3.4160, 4 > var_noinit3.4160: > .space 4 > .type var_noinit, %object > .size var_noinit, 4 > var_noinit: > .space 4 > > So, all three of them are in the .noinit section, but only var_noinit has > .global var_noinit > > So it seems to work? Hmm yes there seems to be an issue with trunks handling of noinit for msp430. Even before your patch the static noinit variables are marked as common symbols with ".comm" and are not placed in .noinit. These static noinit variables work with my downstream msp430-gcc (which doesn't yet have parity with upstream), so this is just something I'll fix separately, and doesn't represent any issues with your patch. Jozef
On Fri, 5 Jul 2019 at 12:57, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote: > > On Fri, 5 Jul 2019 11:26:20 +0200 > Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote: > > > > > > Also, the gcc.target/arm/noinit-attribute.c test works with msp430. > > > Why not create a effective-target keyword which checks for noinit support, so > > > the test can be gated on that condition and put in a generic part of the > > > testsuite? > > > > That was my intention, and I was waiting for feedback on this. In this > > case, I suppose the effective-target check would test a hardcoded list > > of targets, like many other effective-targets? > > (eg check_weak_available) > > Were there previous discussions on whether the noinit attribute should be > explicitly enabled for certain targets? e.g. so it will error if you try to use > it on x86_64. Not formally. I submitted a patch for arm only https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00771.html and got requests to make it generic instead, starting with: https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00065.html > If it will just be enabled by default for all targets then a hardcoded > list for check-effective target sounds ok. Otherwise if it does cause an error > when used on an unsupported target you could do a check that the > attribute compiles successfully instead. Currently, I think it will be accepted for all targets, with various effects depending on the OS etc... > > > > > After doing some further testing, I noticed the attribute does not work on > > > static variables. The attribute handling code allows the attribute to be set on > > > a static variable, but then that variable does not get placed in the .noinit > > > section. > > > > > > What are your thoughts on this? > > > > > > Does it even makes sense for a static local variable to be declared as "noinit"? > > > One should want a static local variable to be initialized, as having it not > > > initialized and hold a random value could cause problems. > > > Perhaps a user could think of some use for this, but I can't. > > > > > I added: > > static int __attribute__((noinit)) var_noinit2; > > in global scope, and > > static int __attribute__((noinit)) var_noinit3; > > in main(), and the generate code contains: > > .section .noinit,"aw" > > .align 2 > > .set .LANCHOR2,. + 0 > > .type var_noinit2, %object > > .size var_noinit2, 4 > > var_noinit2: > > .space 4 > > .type var_noinit3.4160, %object > > .size var_noinit3.4160, 4 > > var_noinit3.4160: > > .space 4 > > .type var_noinit, %object > > .size var_noinit, 4 > > var_noinit: > > .space 4 > > > > So, all three of them are in the .noinit section, but only var_noinit has > > .global var_noinit > > > > So it seems to work? > > Hmm yes there seems to be an issue with trunks handling of noinit for msp430. > Even before your patch the static noinit variables are marked as > common symbols with ".comm" and are not placed in .noinit. > > These static noinit variables work with my downstream msp430-gcc (which doesn't > yet have parity with upstream), so this is just something I'll fix separately, > and doesn't represent any issues with your patch. > > Jozef
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, > > 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. >
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) - 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. > > > commit 3a9dd81162eae36a87a067018a5e5bf1e7b978df Author: Christophe Lyon <christophe.lyon@linaro.org> Date: Thu Jul 4 14:46:00 2019 +0000 Add generic support for noinit attribute. 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 added a testcase if gcc.c-torture/execute, gated by the new noinit effective-target. 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. 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. Add exclusion with "section" attribute. (attr_noinit_exclusions): New table. (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> * lib/target-supports.exp (check_effective_target_noinit): New proc. * gcc.c-torture/execute/noinit-attribute.c: New test. Change-Id: Id8e0baa728d187e05541c7520bd5726ccf803c4f diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 48819e7..399fef3 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -92,6 +92,7 @@ static tree handle_section_attribute (tree *, tree, tree, int, bool *); static tree handle_aligned_attribute (tree *, tree, tree, int, bool *); static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree, int, bool *); +static tree handle_noinit_attribute (tree *, tree, tree, int, bool *); static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ; static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ; static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *); @@ -235,6 +236,13 @@ static const struct attribute_spec::exclusions attr_const_pure_exclusions[] = ATTR_EXCL (NULL, false, false, false) }; +static const struct attribute_spec::exclusions attr_noinit_exclusions[] = +{ + ATTR_EXCL ("noinit", true, true, true), + ATTR_EXCL ("section", true, true, true), + ATTR_EXCL (NULL, false, false, false), +}; + /* Table of machine-independent attributes common to all C-like languages. Current list of processed common attributes: nonnull. */ @@ -307,7 +315,7 @@ const struct attribute_spec c_common_attribute_table[] = { "mode", 1, 1, false, true, false, false, handle_mode_attribute, NULL }, { "section", 1, 1, true, false, false, false, - handle_section_attribute, NULL }, + handle_section_attribute, attr_noinit_exclusions }, { "aligned", 0, 1, false, false, false, false, handle_aligned_attribute, attr_aligned_exclusions }, @@ -458,6 +466,8 @@ const struct attribute_spec c_common_attribute_table[] = handle_nocf_check_attribute, NULL }, { "copy", 1, 1, false, false, false, false, handle_copy_attribute, NULL }, + { "noinit", 0, 0, true, false, false, false, + handle_noinit_attribute, attr_noinit_exclusions }, { NULL, 0, 0, false, false, false, false, NULL, NULL } }; @@ -2224,6 +2234,50 @@ handle_weak_attribute (tree *node, tree name, return NULL_TREE; } +/* Handle a "noinit" attribute; arguments as in struct + attribute_spec.handler. 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 +handle_noinit_attribute (tree * node, + tree name, + tree args, + int flags ATTRIBUTE_UNUSED, + bool *no_add_attrs) +{ + const char *message = NULL; + + gcc_assert (DECL_P (*node)); + 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; +} + + /* Handle a "noplt" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c index 365e9eb..264e852 100644 --- a/gcc/config/msp430/msp430.c +++ b/gcc/config/msp430/msp430.c @@ -2108,8 +2108,6 @@ const struct attribute_spec msp430_attribute_table[] = { ATTR_EITHER, 0, 0, true, false, false, false, msp430_section_attr, NULL }, - { ATTR_NOINIT, 0, 0, true, false, false, false, msp430_data_attr, - NULL }, { ATTR_PERSIST, 0, 0, true, false, false, false, msp430_data_attr, NULL }, @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) { if (TREE_CODE (decl) == FUNCTION_DECL) return text_section; + /* FIXME: ATTR_NOINIT is handled generically in + default_elf_select_section. */ else if (has_attr (ATTR_NOINIT, decl)) return noinit_section; else if (has_attr (ATTR_PERSIST, decl)) diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index f2619e1..850153e 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -7129,6 +7129,12 @@ The @code{visibility} attribute is described in The @code{weak} attribute is described in @ref{Common Function Attributes}. +@item noinit +@cindex @code{noinit} variable attribute +Any data with the @code{noinit} attribute will not be initialized by +the C runtime startup code, or the program loader. Not initializing +data in this way can reduce program startup times. + @end table @node ARC Variable Attributes diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c new file mode 100644 index 0000000..f33c0d0 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c @@ -0,0 +1,59 @@ +/* { dg-do run } */ +/* { dg-require-effective-target noinit */ +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ + + +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 (); +} diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 815e837..ae05c0a 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -364,6 +364,18 @@ proc check_weak_override_available { } { return [check_weak_available] } +# The noinit attribute is only supported by some targets. +# This proc returns 1 if it's supported, 0 if it's not. + +proc check_effective_target_noinit { } { + if { [istarget arm*-*-eabi] + || [istarget msp430-*-*] } { + return 1 + } + + return 0 +} + ############################### # proc check_visibility_available { what_kind } ############################### diff --git a/gcc/varasm.c b/gcc/varasm.c index 626a4c9..7740e88 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) flags |= SECTION_TLS | SECTION_BSS; + if (strcmp (name, ".noinit") == 0) + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; + /* Various sections have special ELF types that the assembler will assign by default based on the name. They are neither SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't want to print a @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) /* Select a section based on the above categorization. */ +static section *noinit_section = NULL; + section * default_elf_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) { const char *sname; + switch (categorize_decl_for_section (decl, reloc)) { case SECCAT_TEXT: @@ -6790,6 +6796,14 @@ default_elf_select_section (tree decl, int reloc, sname = ".tdata"; break; case SECCAT_BSS: + if (DECL_P (decl) + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) + { + if (noinit_section == NULL) + noinit_section = get_named_section (decl, ".noinit", reloc); + return noinit_section; + } + if (bss_section) return bss_section; sname = ".bss";
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. >>> >>
Ping? Le mar. 9 juil. 2019 à 00:04, Martin Sebor <msebor@gmail.com> a écrit : > 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. > >>> > >> > >
Thanks for doing this in a generic way. Christophe Lyon <christophe.lyon@linaro.org> writes: > @@ -2224,6 +2234,50 @@ handle_weak_attribute (tree *node, tree name, > return NULL_TREE; > } > > +/* Handle a "noinit" attribute; arguments as in struct > + attribute_spec.handler. 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 > +handle_noinit_attribute (tree * node, > + tree name, > + tree args, > + int flags ATTRIBUTE_UNUSED, > + bool *no_add_attrs) > +{ > + const char *message = NULL; > + > + gcc_assert (DECL_P (*node)); > + 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; > +} This might cause us to clear DECL_COMMON even when rejecting the attribute. Also, the first message should win over the others (e.g. for a function in a particular section). So I think the "/* Check that it's possible ..." should be an else-if and the DECL_COMMON stuff should be specific to !message. Since this is specific to ELF targets, I think we should also warn if !targetm.have_switchable_bss_sections. > @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) > { > if (TREE_CODE (decl) == FUNCTION_DECL) > return text_section; > + /* FIXME: ATTR_NOINIT is handled generically in > + default_elf_select_section. */ > else if (has_attr (ATTR_NOINIT, decl)) > return noinit_section; > else if (has_attr (ATTR_PERSIST, decl)) I guess adding a FIXME is OK. It's very tempting to remove the noinit stuff and use default_elf_select_section instead of default_select_section, but I realise that'd be difficult to test. > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index f2619e1..850153e 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -7129,6 +7129,12 @@ The @code{visibility} attribute is described in > The @code{weak} attribute is described in > @ref{Common Function Attributes}. > > +@item noinit > +@cindex @code{noinit} variable attribute > +Any data with the @code{noinit} attribute will not be initialized by > +the C runtime startup code, or the program loader. Not initializing > +data in this way can reduce program startup times. > + > @end table > > @node ARC Variable Attributes Would be good to mention that the attribute is specific to ELF targets. (Yeah, we don't seem to do that consistently for other attributes.) It might also be worth saying that it requires specific linker support. > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > new file mode 100644 > index 0000000..f33c0d0 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > @@ -0,0 +1,59 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target noinit */ > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ > + > + > +int > +main (void) > +{ > + /* Make sure that the C startup code has correctly initialised the ordinary variables. */ initialized (alas). Same for the rest of the file. > + 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 (); > +} > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > index 815e837..ae05c0a 100644 > --- a/gcc/testsuite/lib/target-supports.exp > +++ b/gcc/testsuite/lib/target-supports.exp > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > return [check_weak_available] > } > > +# The noinit attribute is only supported by some targets. > +# This proc returns 1 if it's supported, 0 if it's not. > + > +proc check_effective_target_noinit { } { > + if { [istarget arm*-*-eabi] > + || [istarget msp430-*-*] } { > + return 1 > + } > + > + return 0 > +} > + Should be documented in sourcebuild.texi. (Sometimes wonder how many people actually use that instead of just reading this file.) > diff --git a/gcc/varasm.c b/gcc/varasm.c > index 626a4c9..7740e88 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > flags |= SECTION_TLS | SECTION_BSS; > > + if (strcmp (name, ".noinit") == 0) > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > + > /* Various sections have special ELF types that the assembler will > assign by default based on the name. They are neither SHT_PROGBITS > nor SHT_NOBITS, so when changing sections we don't want to print a > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) > > /* Select a section based on the above categorization. */ > > +static section *noinit_section = NULL; > + > section * > default_elf_select_section (tree decl, int reloc, > unsigned HOST_WIDE_INT align) > { > const char *sname; > + > switch (categorize_decl_for_section (decl, reloc)) > { > case SECCAT_TEXT: > @@ -6790,6 +6796,14 @@ default_elf_select_section (tree decl, int reloc, > sname = ".tdata"; > break; > case SECCAT_BSS: > + if (DECL_P (decl) > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > + { > + if (noinit_section == NULL) > + noinit_section = get_named_section (decl, ".noinit", reloc); > + return noinit_section; > + } > + I don't think the special global for noinit_section is worth it, since gen_named_section does its own caching. So IMO we should just have: name = ".noinit"; break; Did you consider supporting .noinit.*, e.g. for -fdata-sections? Thanks, Richard
Hi, Thanks for the useful feedback. On Tue, 16 Jul 2019 at 11:54, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Thanks for doing this in a generic way. > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > @@ -2224,6 +2234,50 @@ handle_weak_attribute (tree *node, tree name, > > return NULL_TREE; > > } > > > > +/* Handle a "noinit" attribute; arguments as in struct > > + attribute_spec.handler. 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 > > +handle_noinit_attribute (tree * node, > > + tree name, > > + tree args, > > + int flags ATTRIBUTE_UNUSED, > > + bool *no_add_attrs) > > +{ > > + const char *message = NULL; > > + > > + gcc_assert (DECL_P (*node)); > > + 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; > > +} > > This might cause us to clear DECL_COMMON even when rejecting the > attribute. Also, the first message should win over the others > (e.g. for a function in a particular section). > > So I think the "/* Check that it's possible ..." should be an else-if > and the DECL_COMMON stuff should be specific to !message. You are right, thanks. Jozef, this is true for msp430_data_attr() too. I'll let you update it. > > Since this is specific to ELF targets, I think we should also > warn if !targetm.have_switchable_bss_sections. > OK > > @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) > > { > > if (TREE_CODE (decl) == FUNCTION_DECL) > > return text_section; > > + /* FIXME: ATTR_NOINIT is handled generically in > > + default_elf_select_section. */ > > else if (has_attr (ATTR_NOINIT, decl)) > > return noinit_section; > > else if (has_attr (ATTR_PERSIST, decl)) > > I guess adding a FIXME is OK. It's very tempting to remove > the noinit stuff and use default_elf_select_section instead of > default_select_section, but I realise that'd be difficult to test. I added that as suggested by Jozef, it's best if he handles the change you propose, I guess he can do proper testing. > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > > index f2619e1..850153e 100644 > > --- a/gcc/doc/extend.texi > > +++ b/gcc/doc/extend.texi > > @@ -7129,6 +7129,12 @@ The @code{visibility} attribute is described in > > The @code{weak} attribute is described in > > @ref{Common Function Attributes}. > > > > +@item noinit > > +@cindex @code{noinit} variable attribute > > +Any data with the @code{noinit} attribute will not be initialized by > > +the C runtime startup code, or the program loader. Not initializing > > +data in this way can reduce program startup times. > > + > > @end table > > > > @node ARC Variable Attributes > > Would be good to mention that the attribute is specific to ELF targets. > (Yeah, we don't seem to do that consistently for other attributes.) > It might also be worth saying that it requires specific linker support. OK, done. > > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > new file mode 100644 > > index 0000000..f33c0d0 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > @@ -0,0 +1,59 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target noinit */ > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ > > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ > > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ > > + > > + > > +int > > +main (void) > > +{ > > + /* Make sure that the C startup code has correctly initialised the ordinary variables. */ > > initialized (alas). Same for the rest of the file. That was a copy-and-paste from msp430 testcase; Jozef, you have 3 typos to fix :-) > > + 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 (); > > +} > > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > > index 815e837..ae05c0a 100644 > > --- a/gcc/testsuite/lib/target-supports.exp > > +++ b/gcc/testsuite/lib/target-supports.exp > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > return [check_weak_available] > > } > > > > +# The noinit attribute is only supported by some targets. > > +# This proc returns 1 if it's supported, 0 if it's not. > > + > > +proc check_effective_target_noinit { } { > > + if { [istarget arm*-*-eabi] > > + || [istarget msp430-*-*] } { > > + return 1 > > + } > > + > > + return 0 > > +} > > + > > Should be documented in sourcebuild.texi. (Sometimes wonder how many > people actually use that instead of just reading this file.) > Sigh..... I keep forgetting this. > > diff --git a/gcc/varasm.c b/gcc/varasm.c > > index 626a4c9..7740e88 100644 > > --- a/gcc/varasm.c > > +++ b/gcc/varasm.c > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > flags |= SECTION_TLS | SECTION_BSS; > > > > + if (strcmp (name, ".noinit") == 0) > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > + > > /* Various sections have special ELF types that the assembler will > > assign by default based on the name. They are neither SHT_PROGBITS > > nor SHT_NOBITS, so when changing sections we don't want to print a > > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) > > > > /* Select a section based on the above categorization. */ > > > > +static section *noinit_section = NULL; > > + > > section * > > default_elf_select_section (tree decl, int reloc, > > unsigned HOST_WIDE_INT align) > > { > > const char *sname; > > + > > switch (categorize_decl_for_section (decl, reloc)) > > { > > case SECCAT_TEXT: > > @@ -6790,6 +6796,14 @@ default_elf_select_section (tree decl, int reloc, > > sname = ".tdata"; > > break; > > case SECCAT_BSS: > > + if (DECL_P (decl) > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > + { > > + if (noinit_section == NULL) > > + noinit_section = get_named_section (decl, ".noinit", reloc); > > + return noinit_section; > > + } > > + > > I don't think the special global for noinit_section is worth it, since > gen_named_section does its own caching. So IMO we should just have: > > name = ".noinit"; > break; OK > Did you consider supporting .noinit.*, e.g. for -fdata-sections? Not so far. I don't think we have received such a request yet? Thanks, Christophe > Thanks, > Richard commit 833e8ee8a5ca1ccc53121c7fd86b37e0eadc0086 Author: Christophe Lyon <christophe.lyon@linaro.org> Date: Thu Jul 4 14:46:00 2019 +0000 Add generic support for noinit attribute. 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 added a testcase if gcc.c-torture/execute, gated by the new noinit effective-target. 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. Thanks, Christophe gcc/ChangeLog: 2019-07-04 Christophe Lyon <christophe.lyon@linaro.org> * doc/extend.texi: Add "noinit" attribute documentation. * doc/sourcebuild.texi: Add noinit effective target 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. Add exclusion with "section" attribute. (attr_noinit_exclusions): New table. (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> * lib/target-supports.exp (check_effective_target_noinit): New proc. * gcc.c-torture/execute/noinit-attribute.c: New test. Change-Id: Id8e0baa728d187e05541c7520bd5726ccf803c4f diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 48819e7..77ed56d 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -92,6 +92,7 @@ static tree handle_section_attribute (tree *, tree, tree, int, bool *); static tree handle_aligned_attribute (tree *, tree, tree, int, bool *); static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree, int, bool *); +static tree handle_noinit_attribute (tree *, tree, tree, int, bool *); static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ; static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ; static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *); @@ -235,6 +236,13 @@ static const struct attribute_spec::exclusions attr_const_pure_exclusions[] = ATTR_EXCL (NULL, false, false, false) }; +static const struct attribute_spec::exclusions attr_noinit_exclusions[] = +{ + ATTR_EXCL ("noinit", true, true, true), + ATTR_EXCL ("section", true, true, true), + ATTR_EXCL (NULL, false, false, false), +}; + /* Table of machine-independent attributes common to all C-like languages. Current list of processed common attributes: nonnull. */ @@ -307,7 +315,7 @@ const struct attribute_spec c_common_attribute_table[] = { "mode", 1, 1, false, true, false, false, handle_mode_attribute, NULL }, { "section", 1, 1, true, false, false, false, - handle_section_attribute, NULL }, + handle_section_attribute, attr_noinit_exclusions }, { "aligned", 0, 1, false, false, false, false, handle_aligned_attribute, attr_aligned_exclusions }, @@ -458,6 +466,8 @@ const struct attribute_spec c_common_attribute_table[] = handle_nocf_check_attribute, NULL }, { "copy", 1, 1, false, false, false, false, handle_copy_attribute, NULL }, + { "noinit", 0, 0, true, false, false, false, + handle_noinit_attribute, attr_noinit_exclusions }, { NULL, 0, 0, false, false, false, false, NULL, NULL } }; @@ -2224,6 +2234,54 @@ handle_weak_attribute (tree *node, tree name, return NULL_TREE; } +/* Handle a "noinit" attribute; arguments as in struct + attribute_spec.handler. 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 +handle_noinit_attribute (tree * node, + tree name, + tree args, + int flags ATTRIBUTE_UNUSED, + bool *no_add_attrs) +{ + const char *message = NULL; + + gcc_assert (DECL_P (*node)); + 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. */ + else 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 (!targetm.have_switchable_bss_sections) + message = G_("%qE attribute is specific to ELF targets"); + + if (message) + { + warning (OPT_Wattributes, message, name); + *no_add_attrs = true; + } + else + /* 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. Do this only if the attribute is + valid. */ + if (DECL_COMMON (*node)) + DECL_COMMON (*node) = 0; + + return NULL_TREE; +} + + /* Handle a "noplt" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c index 365e9eb..264e852 100644 --- a/gcc/config/msp430/msp430.c +++ b/gcc/config/msp430/msp430.c @@ -2108,8 +2108,6 @@ const struct attribute_spec msp430_attribute_table[] = { ATTR_EITHER, 0, 0, true, false, false, false, msp430_section_attr, NULL }, - { ATTR_NOINIT, 0, 0, true, false, false, false, msp430_data_attr, - NULL }, { ATTR_PERSIST, 0, 0, true, false, false, false, msp430_data_attr, NULL }, @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) { if (TREE_CODE (decl) == FUNCTION_DECL) return text_section; + /* FIXME: ATTR_NOINIT is handled generically in + default_elf_select_section. */ else if (has_attr (ATTR_NOINIT, decl)) return noinit_section; else if (has_attr (ATTR_PERSIST, decl)) diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index f2619e1..f1af1dc 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -7129,6 +7129,14 @@ The @code{visibility} attribute is described in The @code{weak} attribute is described in @ref{Common Function Attributes}. +@item noinit +@cindex @code{noinit} variable attribute +Any data with the @code{noinit} attribute will not be initialized by +the C runtime startup code, or the program loader. Not initializing +data in this way can reduce program startup times. Specific to ELF +targets, this attribute relies on the linker to place such data in the +right location. + @end table @node ARC Variable Attributes diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi index 6a66515..d67608a 100644 --- a/gcc/doc/sourcebuild.texi +++ b/gcc/doc/sourcebuild.texi @@ -2302,6 +2302,9 @@ Target uses natural alignment (aligned to type size) for types of Target uses natural alignment (aligned to type size) for types of 64 bits or less. +@item noinit +Target supports the @code{noinit} variable attribute. + @item nonpic Target does not generate PIC by default. diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c new file mode 100644 index 0000000..ffcf8c6 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c @@ -0,0 +1,59 @@ +/* { dg-do run } */ +/* { dg-require-effective-target noinit */ +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ + + +int +main (void) +{ + /* Make sure that the C startup code has correctly initialized the ordinary variables. */ + if (var_common != 0) + abort (); + + /* Initialized variables are not re-initialized 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 (); +} diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 815e837..ae05c0a 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -364,6 +364,18 @@ proc check_weak_override_available { } { return [check_weak_available] } +# The noinit attribute is only supported by some targets. +# This proc returns 1 if it's supported, 0 if it's not. + +proc check_effective_target_noinit { } { + if { [istarget arm*-*-eabi] + || [istarget msp430-*-*] } { + return 1 + } + + return 0 +} + ############################### # proc check_visibility_available { what_kind } ############################### diff --git a/gcc/varasm.c b/gcc/varasm.c index 626a4c9..6ddab0ce 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) flags |= SECTION_TLS | SECTION_BSS; + if (strcmp (name, ".noinit") == 0) + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; + /* Various sections have special ELF types that the assembler will assign by default based on the name. They are neither SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't want to print a @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) /* Select a section based on the above categorization. */ +static section *noinit_section = NULL; + section * default_elf_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) { const char *sname; + switch (categorize_decl_for_section (decl, reloc)) { case SECCAT_TEXT: @@ -6790,6 +6796,13 @@ default_elf_select_section (tree decl, int reloc, sname = ".tdata"; break; case SECCAT_BSS: + if (DECL_P (decl) + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) + { + sname = ".noinit"; + break; + } + if (bss_section) return bss_section; sname = ".bss";
Hi, On Tue, 30 Jul 2019 15:35:23 +0200 Christophe Lyon <christophe.lyon@linaro.org> wrote: > Hi, > > Thanks for the useful feedback. > > > On Tue, 16 Jul 2019 at 11:54, Richard Sandiford > <richard.sandiford@arm.com> wrote: > > > > Thanks for doing this in a generic way. > > > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > > @@ -2224,6 +2234,50 @@ handle_weak_attribute (tree *node, tree name, > > > return NULL_TREE; > > > } > > > > > > +/* Handle a "noinit" attribute; arguments as in struct > > > + attribute_spec.handler. 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 > > > +handle_noinit_attribute (tree * node, > > > + tree name, > > > + tree args, > > > + int flags ATTRIBUTE_UNUSED, > > > + bool *no_add_attrs) > > > +{ > > > + const char *message = NULL; > > > + > > > + gcc_assert (DECL_P (*node)); > > > + 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; > > > +} > > > > This might cause us to clear DECL_COMMON even when rejecting the > > attribute. Also, the first message should win over the others > > (e.g. for a function in a particular section). > > > > So I think the "/* Check that it's possible ..." should be an else-if > > and the DECL_COMMON stuff should be specific to !message. > > You are right, thanks. > > Jozef, this is true for msp430_data_attr() too. I'll let you update it. > > > > > Since this is specific to ELF targets, I think we should also > > warn if !targetm.have_switchable_bss_sections. > > > OK > > > > @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) > > > { > > > if (TREE_CODE (decl) == FUNCTION_DECL) > > > return text_section; > > > + /* FIXME: ATTR_NOINIT is handled generically in > > > + default_elf_select_section. */ > > > else if (has_attr (ATTR_NOINIT, decl)) > > > return noinit_section; > > > else if (has_attr (ATTR_PERSIST, decl)) > > > > I guess adding a FIXME is OK. It's very tempting to remove > > the noinit stuff and use default_elf_select_section instead of > > default_select_section, but I realise that'd be difficult to test. > > I added that as suggested by Jozef, it's best if he handles the > change you propose, I guess he can do proper testing. BTW, regarding this, I have rearranged msp430_select_section in the attached WIP patch, so that it will use default_elf_select_section to handle "noinit". Once your patch is applied, I'll submit some variant of this patch. Still, better leave the FIXME in, and I'll remove it in my patch. Likewise for all the other fixes required in msp430.c, once this patch is applied, I'll fix those up. Thanks, Jozef > > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > > > index f2619e1..850153e 100644 > > > --- a/gcc/doc/extend.texi > > > +++ b/gcc/doc/extend.texi > > > @@ -7129,6 +7129,12 @@ The @code{visibility} attribute is described in > > > The @code{weak} attribute is described in > > > @ref{Common Function Attributes}. > > > > > > +@item noinit > > > +@cindex @code{noinit} variable attribute > > > +Any data with the @code{noinit} attribute will not be initialized by > > > +the C runtime startup code, or the program loader. Not initializing > > > +data in this way can reduce program startup times. > > > + > > > @end table > > > > > > @node ARC Variable Attributes > > > > Would be good to mention that the attribute is specific to ELF targets. > > (Yeah, we don't seem to do that consistently for other attributes.) > > It might also be worth saying that it requires specific linker support. > OK, done. > > > > > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > new file mode 100644 > > > index 0000000..f33c0d0 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > @@ -0,0 +1,59 @@ > > > +/* { dg-do run } */ > > > +/* { dg-require-effective-target noinit */ > > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ > > > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ > > > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ > > > + > > > + > > > +int > > > +main (void) > > > +{ > > > + /* Make sure that the C startup code has correctly initialised the ordinary variables. */ > > > > initialized (alas). Same for the rest of the file. > > That was a copy-and-paste from msp430 testcase; Jozef, you have 3 > typos to fix :-) > > > > + 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 (); > > > +} > > > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > > > index 815e837..ae05c0a 100644 > > > --- a/gcc/testsuite/lib/target-supports.exp > > > +++ b/gcc/testsuite/lib/target-supports.exp > > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > > return [check_weak_available] > > > } > > > > > > +# The noinit attribute is only supported by some targets. > > > +# This proc returns 1 if it's supported, 0 if it's not. > > > + > > > +proc check_effective_target_noinit { } { > > > + if { [istarget arm*-*-eabi] > > > + || [istarget msp430-*-*] } { > > > + return 1 > > > + } > > > + > > > + return 0 > > > +} > > > + > > > > Should be documented in sourcebuild.texi. (Sometimes wonder how many > > people actually use that instead of just reading this file.) > > > Sigh..... I keep forgetting this. > > > > diff --git a/gcc/varasm.c b/gcc/varasm.c > > > index 626a4c9..7740e88 100644 > > > --- a/gcc/varasm.c > > > +++ b/gcc/varasm.c > > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > > flags |= SECTION_TLS | SECTION_BSS; > > > > > > + if (strcmp (name, ".noinit") == 0) > > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > > + > > > /* Various sections have special ELF types that the assembler will > > > assign by default based on the name. They are neither SHT_PROGBITS > > > nor SHT_NOBITS, so when changing sections we don't want to print a > > > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) > > > > > > /* Select a section based on the above categorization. */ > > > > > > +static section *noinit_section = NULL; > > > + > > > section * > > > default_elf_select_section (tree decl, int reloc, > > > unsigned HOST_WIDE_INT align) > > > { > > > const char *sname; > > > + > > > switch (categorize_decl_for_section (decl, reloc)) > > > { > > > case SECCAT_TEXT: > > > @@ -6790,6 +6796,14 @@ default_elf_select_section (tree decl, int reloc, > > > sname = ".tdata"; > > > break; > > > case SECCAT_BSS: > > > + if (DECL_P (decl) > > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > > + { > > > + if (noinit_section == NULL) > > > + noinit_section = get_named_section (decl, ".noinit", reloc); > > > + return noinit_section; > > > + } > > > + > > > > I don't think the special global for noinit_section is worth it, since > > gen_named_section does its own caching. So IMO we should just have: > > > > name = ".noinit"; > > break; > OK > > > Did you consider supporting .noinit.*, e.g. for -fdata-sections? > Not so far. I don't think we have received such a request yet? > > Thanks, > > Christophe > > > Thanks, > > Richard diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c index 5ccb02997ad..8b8e9aa992e 100644 --- a/gcc/config/msp430/msp430.c +++ b/gcc/config/msp430/msp430.c @@ -1730,6 +1730,10 @@ msp430_init_sections (void) static section * msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) { + const char * prefix; + const char * sec_name; + const char * base_sec_name; + gcc_assert (decl != NULL_TREE); if (TREE_CODE (decl) == STRING_CST @@ -1744,29 +1748,41 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) if (TARGET_LARGE && TREE_CODE (decl) == FUNCTION_DECL && is_interrupt_func (decl)) return get_section (".lowtext", SECTION_CODE | SECTION_WRITE , decl); - const char * prefix = gen_prefix (decl); - if (prefix == NULL) - { - if (TREE_CODE (decl) == FUNCTION_DECL) - return text_section; - /* FIXME: ATTR_NOINIT is handled generically in - default_elf_select_section. */ - else if (has_attr (ATTR_NOINIT, decl)) - return noinit_section; - else if (has_attr (ATTR_PERSIST, decl)) - return persist_section; - else - return default_select_section (decl, reloc, align); - } + if (has_attr (ATTR_PERSIST, decl)) + return persist_section; + + /* ATTR_NOINIT is handled generically. */ + if (has_attr (ATTR_NOINIT, decl)) + return default_elf_select_section (decl, reloc, align); + + prefix = gen_prefix (decl); - const char * sec; switch (categorize_decl_for_section (decl, reloc)) { - case SECCAT_TEXT: sec = ".text"; break; - case SECCAT_DATA: sec = ".data"; break; - case SECCAT_BSS: sec = ".bss"; break; - case SECCAT_RODATA: sec = ".rodata"; break; + case SECCAT_TEXT: + if (!prefix) + return text_section; + base_sec_name = ".text"; + break; + case SECCAT_DATA: + if (!prefix) + return data_section; + base_sec_name = ".data"; + break; + case SECCAT_BSS: + if (!prefix) + return bss_section; + base_sec_name = ".bss"; + break; + case SECCAT_RODATA: + if (!prefix) + return readonly_data_section; + base_sec_name = ".rodata"; + break; + /* These sections are not supported (?). They should not be generated, + but in case they are, we use default_select_section so they get placed + in sections the msp430 assembler and linker understand. */ case SECCAT_RODATA_MERGE_STR: case SECCAT_RODATA_MERGE_STR_INIT: case SECCAT_RODATA_MERGE_CONST: @@ -1785,10 +1801,9 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) gcc_unreachable (); } - const char * dec_name = DECL_SECTION_NAME (decl); - char * name = ACONCAT ((prefix, sec, dec_name, NULL)); + sec_name = ACONCAT ((prefix, base_sec_name, DECL_SECTION_NAME (decl), NULL)); - return get_named_section (decl, name, 0); + return get_named_section (decl, sec_name, 0); } #undef TARGET_ASM_FUNCTION_SECTION
Ping? On Tue, 30 Jul 2019 at 15:35, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > Hi, > > Thanks for the useful feedback. > > > On Tue, 16 Jul 2019 at 11:54, Richard Sandiford > <richard.sandiford@arm.com> wrote: > > > > Thanks for doing this in a generic way. > > > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > > @@ -2224,6 +2234,50 @@ handle_weak_attribute (tree *node, tree name, > > > return NULL_TREE; > > > } > > > > > > +/* Handle a "noinit" attribute; arguments as in struct > > > + attribute_spec.handler. 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 > > > +handle_noinit_attribute (tree * node, > > > + tree name, > > > + tree args, > > > + int flags ATTRIBUTE_UNUSED, > > > + bool *no_add_attrs) > > > +{ > > > + const char *message = NULL; > > > + > > > + gcc_assert (DECL_P (*node)); > > > + 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; > > > +} > > > > This might cause us to clear DECL_COMMON even when rejecting the > > attribute. Also, the first message should win over the others > > (e.g. for a function in a particular section). > > > > So I think the "/* Check that it's possible ..." should be an else-if > > and the DECL_COMMON stuff should be specific to !message. > > You are right, thanks. > > Jozef, this is true for msp430_data_attr() too. I'll let you update it. > > > > > Since this is specific to ELF targets, I think we should also > > warn if !targetm.have_switchable_bss_sections. > > > OK > > > > @@ -2338,6 +2336,8 @@ msp430_select_section (tree decl, int reloc, unsigned HOST_WIDE_INT align) > > > { > > > if (TREE_CODE (decl) == FUNCTION_DECL) > > > return text_section; > > > + /* FIXME: ATTR_NOINIT is handled generically in > > > + default_elf_select_section. */ > > > else if (has_attr (ATTR_NOINIT, decl)) > > > return noinit_section; > > > else if (has_attr (ATTR_PERSIST, decl)) > > > > I guess adding a FIXME is OK. It's very tempting to remove > > the noinit stuff and use default_elf_select_section instead of > > default_select_section, but I realise that'd be difficult to test. > > I added that as suggested by Jozef, it's best if he handles the > change you propose, I guess he can do proper testing. > > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > > > index f2619e1..850153e 100644 > > > --- a/gcc/doc/extend.texi > > > +++ b/gcc/doc/extend.texi > > > @@ -7129,6 +7129,12 @@ The @code{visibility} attribute is described in > > > The @code{weak} attribute is described in > > > @ref{Common Function Attributes}. > > > > > > +@item noinit > > > +@cindex @code{noinit} variable attribute > > > +Any data with the @code{noinit} attribute will not be initialized by > > > +the C runtime startup code, or the program loader. Not initializing > > > +data in this way can reduce program startup times. > > > + > > > @end table > > > > > > @node ARC Variable Attributes > > > > Would be good to mention that the attribute is specific to ELF targets. > > (Yeah, we don't seem to do that consistently for other attributes.) > > It might also be worth saying that it requires specific linker support. > OK, done. > > > > > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > new file mode 100644 > > > index 0000000..f33c0d0 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > @@ -0,0 +1,59 @@ > > > +/* { dg-do run } */ > > > +/* { dg-require-effective-target noinit */ > > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ > > > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ > > > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ > > > + > > > + > > > +int > > > +main (void) > > > +{ > > > + /* Make sure that the C startup code has correctly initialised the ordinary variables. */ > > > > initialized (alas). Same for the rest of the file. > > That was a copy-and-paste from msp430 testcase; Jozef, you have 3 > typos to fix :-) > > > > + 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 (); > > > +} > > > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > > > index 815e837..ae05c0a 100644 > > > --- a/gcc/testsuite/lib/target-supports.exp > > > +++ b/gcc/testsuite/lib/target-supports.exp > > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > > return [check_weak_available] > > > } > > > > > > +# The noinit attribute is only supported by some targets. > > > +# This proc returns 1 if it's supported, 0 if it's not. > > > + > > > +proc check_effective_target_noinit { } { > > > + if { [istarget arm*-*-eabi] > > > + || [istarget msp430-*-*] } { > > > + return 1 > > > + } > > > + > > > + return 0 > > > +} > > > + > > > > Should be documented in sourcebuild.texi. (Sometimes wonder how many > > people actually use that instead of just reading this file.) > > > Sigh..... I keep forgetting this. > > > > diff --git a/gcc/varasm.c b/gcc/varasm.c > > > index 626a4c9..7740e88 100644 > > > --- a/gcc/varasm.c > > > +++ b/gcc/varasm.c > > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > > flags |= SECTION_TLS | SECTION_BSS; > > > > > > + if (strcmp (name, ".noinit") == 0) > > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > > + > > > /* Various sections have special ELF types that the assembler will > > > assign by default based on the name. They are neither SHT_PROGBITS > > > nor SHT_NOBITS, so when changing sections we don't want to print a > > > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) > > > > > > /* Select a section based on the above categorization. */ > > > > > > +static section *noinit_section = NULL; > > > + > > > section * > > > default_elf_select_section (tree decl, int reloc, > > > unsigned HOST_WIDE_INT align) > > > { > > > const char *sname; > > > + > > > switch (categorize_decl_for_section (decl, reloc)) > > > { > > > case SECCAT_TEXT: > > > @@ -6790,6 +6796,14 @@ default_elf_select_section (tree decl, int reloc, > > > sname = ".tdata"; > > > break; > > > case SECCAT_BSS: > > > + if (DECL_P (decl) > > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > > + { > > > + if (noinit_section == NULL) > > > + noinit_section = get_named_section (decl, ".noinit", reloc); > > > + return noinit_section; > > > + } > > > + > > > > I don't think the special global for noinit_section is worth it, since > > gen_named_section does its own caching. So IMO we should just have: > > > > name = ".noinit"; > > break; > OK > > > Did you consider supporting .noinit.*, e.g. for -fdata-sections? > Not so far. I don't think we have received such a request yet? > > Thanks, > > Christophe > > > Thanks, > > Richard
Sorry for the slow response, I'd missed that there was an updated patch... Christophe Lyon <christophe.lyon@linaro.org> writes: > 2019-07-04 Christophe Lyon <christophe.lyon@linaro.org> > > * lib/target-supports.exp (check_effective_target_noinit): New > proc. > * gcc.c-torture/execute/noinit-attribute.c: New test. Second line should be indented by tabs rather than spaces. > @@ -2224,6 +2234,54 @@ handle_weak_attribute (tree *node, tree name, > return NULL_TREE; > } > > +/* Handle a "noinit" attribute; arguments as in struct > + attribute_spec.handler. 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 > +handle_noinit_attribute (tree * node, > + tree name, > + tree args, > + int flags ATTRIBUTE_UNUSED, > + bool *no_add_attrs) > +{ > + const char *message = NULL; > + > + gcc_assert (DECL_P (*node)); > + 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. */ > + else 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 (!targetm.have_switchable_bss_sections) > + message = G_("%qE attribute is specific to ELF targets"); Maybe make this an else if too? Or make the VAR_DECL an else if if you think the ELF one should win. Either way, it seems odd to have the mixture between else if and not. > + if (message) > + { > + warning (OPT_Wattributes, message, name); > + *no_add_attrs = true; > + } > + else > + /* 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. Do this only if the attribute is > + valid. */ Comment should be indented two spaces more. > + if (DECL_COMMON (*node)) > + DECL_COMMON (*node) = 0; > + > + return NULL_TREE; > +} > + > + > /* Handle a "noplt" attribute; arguments as in > struct attribute_spec.handler. */ > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index f2619e1..f1af1dc 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -7129,6 +7129,14 @@ The @code{visibility} attribute is described in > The @code{weak} attribute is described in > @ref{Common Function Attributes}. > > +@item noinit > +@cindex @code{noinit} variable attribute > +Any data with the @code{noinit} attribute will not be initialized by > +the C runtime startup code, or the program loader. Not initializing > +data in this way can reduce program startup times. Specific to ELF > +targets, this attribute relies on the linker to place such data in the > +right location. Maybe: This attribute is specific to ELF targets and relies on the linker to place such data in the right location. > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > new file mode 100644 > index 0000000..ffcf8c6 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > @@ -0,0 +1,59 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target noinit */ > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ > + > + > +int > +main (void) > +{ > + /* Make sure that the C startup code has correctly initialized the ordinary variables. */ > + if (var_common != 0) > + abort (); > + > + /* Initialized variables are not re-initialized 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 (); > +} > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > index 815e837..ae05c0a 100644 > --- a/gcc/testsuite/lib/target-supports.exp > +++ b/gcc/testsuite/lib/target-supports.exp > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > return [check_weak_available] > } > > +# The noinit attribute is only supported by some targets. > +# This proc returns 1 if it's supported, 0 if it's not. > + > +proc check_effective_target_noinit { } { > + if { [istarget arm*-*-eabi] > + || [istarget msp430-*-*] } { > + return 1 > + } > + > + return 0 > +} > + > ############################### > # proc check_visibility_available { what_kind } > ############################### > diff --git a/gcc/varasm.c b/gcc/varasm.c > index 626a4c9..6ddab0ce 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > flags |= SECTION_TLS | SECTION_BSS; > > + if (strcmp (name, ".noinit") == 0) > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > + > /* Various sections have special ELF types that the assembler will > assign by default based on the name. They are neither SHT_PROGBITS > nor SHT_NOBITS, so when changing sections we don't want to print a > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) > > /* Select a section based on the above categorization. */ > > +static section *noinit_section = NULL; > + No longer needed. OK with those changes, thanks. Richard > section * > default_elf_select_section (tree decl, int reloc, > unsigned HOST_WIDE_INT align) > { > const char *sname; > + > switch (categorize_decl_for_section (decl, reloc)) > { > case SECCAT_TEXT: > @@ -6790,6 +6796,13 @@ default_elf_select_section (tree decl, int reloc, > sname = ".tdata"; > break; > case SECCAT_BSS: > + if (DECL_P (decl) > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > + { > + sname = ".noinit"; > + break; > + } > + > if (bss_section) > return bss_section; > sname = ".bss";
On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Sorry for the slow response, I'd missed that there was an updated patch... > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > 2019-07-04 Christophe Lyon <christophe.lyon@linaro.org> > > > > * lib/target-supports.exp (check_effective_target_noinit): New > > proc. > > * gcc.c-torture/execute/noinit-attribute.c: New test. > > Second line should be indented by tabs rather than spaces. > > > @@ -2224,6 +2234,54 @@ handle_weak_attribute (tree *node, tree name, > > return NULL_TREE; > > } > > > > +/* Handle a "noinit" attribute; arguments as in struct > > + attribute_spec.handler. 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 > > +handle_noinit_attribute (tree * node, > > + tree name, > > + tree args, > > + int flags ATTRIBUTE_UNUSED, > > + bool *no_add_attrs) > > +{ > > + const char *message = NULL; > > + > > + gcc_assert (DECL_P (*node)); > > + 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. */ > > + else 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 (!targetm.have_switchable_bss_sections) > > + message = G_("%qE attribute is specific to ELF targets"); > > Maybe make this an else if too? Or make the VAR_DECL an else if > if you think the ELF one should win. Either way, it seems odd to > have the mixture between else if and not. > Right, I changed this into an else if. > > + if (message) > > + { > > + warning (OPT_Wattributes, message, name); > > + *no_add_attrs = true; > > + } > > + else > > + /* 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. Do this only if the attribute is > > + valid. */ > > Comment should be indented two spaces more. > > > + if (DECL_COMMON (*node)) > > + DECL_COMMON (*node) = 0; > > + > > + return NULL_TREE; > > +} > > + > > + > > /* Handle a "noplt" attribute; arguments as in > > struct attribute_spec.handler. */ > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > > index f2619e1..f1af1dc 100644 > > --- a/gcc/doc/extend.texi > > +++ b/gcc/doc/extend.texi > > @@ -7129,6 +7129,14 @@ The @code{visibility} attribute is described in > > The @code{weak} attribute is described in > > @ref{Common Function Attributes}. > > > > +@item noinit > > +@cindex @code{noinit} variable attribute > > +Any data with the @code{noinit} attribute will not be initialized by > > +the C runtime startup code, or the program loader. Not initializing > > +data in this way can reduce program startup times. Specific to ELF > > +targets, this attribute relies on the linker to place such data in the > > +right location. > > Maybe: > > This attribute is specific to ELF targets and relies on the linker to > place such data in the right location. > Thanks, I thought I had chosen a nice turn of phrase :-) > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > new file mode 100644 > > index 0000000..ffcf8c6 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > @@ -0,0 +1,59 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target noinit */ > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only applies to variables" } */ > > +int __attribute__((section ("mysection"), noinit)) var_section1; /* { dg-warning "because it conflicts with attribute" } */ > > +int __attribute__((noinit, section ("mysection"))) var_section2; /* { dg-warning "because it conflicts with attribute" } */ > > + > > + > > +int > > +main (void) > > +{ > > + /* Make sure that the C startup code has correctly initialized the ordinary variables. */ > > + if (var_common != 0) > > + abort (); > > + > > + /* Initialized variables are not re-initialized 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 (); > > +} > > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > > index 815e837..ae05c0a 100644 > > --- a/gcc/testsuite/lib/target-supports.exp > > +++ b/gcc/testsuite/lib/target-supports.exp > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > return [check_weak_available] > > } > > > > +# The noinit attribute is only supported by some targets. > > +# This proc returns 1 if it's supported, 0 if it's not. > > + > > +proc check_effective_target_noinit { } { > > + if { [istarget arm*-*-eabi] > > + || [istarget msp430-*-*] } { > > + return 1 > > + } > > + > > + return 0 > > +} > > + > > ############################### > > # proc check_visibility_available { what_kind } > > ############################### > > diff --git a/gcc/varasm.c b/gcc/varasm.c > > index 626a4c9..6ddab0ce 100644 > > --- a/gcc/varasm.c > > +++ b/gcc/varasm.c > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > flags |= SECTION_TLS | SECTION_BSS; > > > > + if (strcmp (name, ".noinit") == 0) > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > + > > /* Various sections have special ELF types that the assembler will > > assign by default based on the name. They are neither SHT_PROGBITS > > nor SHT_NOBITS, so when changing sections we don't want to print a > > @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree decl, int reloc) > > > > /* Select a section based on the above categorization. */ > > > > +static section *noinit_section = NULL; > > + > > No longer needed. Indeed. > > OK with those changes, thanks. Thanks, committed as r274482. Christophe > Richard > > > section * > > default_elf_select_section (tree decl, int reloc, > > unsigned HOST_WIDE_INT align) > > { > > const char *sname; > > + > > switch (categorize_decl_for_section (decl, reloc)) > > { > > case SECCAT_TEXT: > > @@ -6790,6 +6796,13 @@ default_elf_select_section (tree decl, int reloc, > > sname = ".tdata"; > > break; > > case SECCAT_BSS: > > + if (DECL_P (decl) > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > + { > > + sname = ".noinit"; > > + break; > > + } > > + > > if (bss_section) > > return bss_section; > > sname = ".bss";
Hi Christoph, The noinit testcase is currently failing on x86_64. Is the test supposed to be running there? Thanks, Tamar -----Original Message----- From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On Behalf Of Christophe Lyon Sent: Wednesday, August 14, 2019 2:18 PM To: Christophe Lyon <christophe.lyon@linaro.org>; Martin Sebor <msebor@gmail.com>; gcc Patches <gcc-patches@gcc.gnu.org>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Jozef Lawrynowicz <jozef.l@mittosystems.com>; Richard Sandiford <Richard.Sandiford@arm.com> Subject: Re: [PATCH] Add generic support for "noinit" attribute On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Sorry for the slow response, I'd missed that there was an updated patch... > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > 2019-07-04 Christophe Lyon <christophe.lyon@linaro.org> > > > > * lib/target-supports.exp (check_effective_target_noinit): New > > proc. > > * gcc.c-torture/execute/noinit-attribute.c: New test. > > Second line should be indented by tabs rather than spaces. > > > @@ -2224,6 +2234,54 @@ handle_weak_attribute (tree *node, tree name, > > return NULL_TREE; > > } > > > > +/* Handle a "noinit" attribute; arguments as in struct > > + attribute_spec.handler. 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 > > +handle_noinit_attribute (tree * node, > > + tree name, > > + tree args, > > + int flags ATTRIBUTE_UNUSED, > > + bool *no_add_attrs) > > +{ > > + const char *message = NULL; > > + > > + gcc_assert (DECL_P (*node)); > > + 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. > > + */ else 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 (!targetm.have_switchable_bss_sections) > > + message = G_("%qE attribute is specific to ELF targets"); > > Maybe make this an else if too? Or make the VAR_DECL an else if if > you think the ELF one should win. Either way, it seems odd to have > the mixture between else if and not. > Right, I changed this into an else if. > > + if (message) > > + { > > + warning (OPT_Wattributes, message, name); > > + *no_add_attrs = true; > > + } > > + else > > + /* 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. Do this only if the attribute is > > + valid. */ > > Comment should be indented two spaces more. > > > + if (DECL_COMMON (*node)) > > + DECL_COMMON (*node) = 0; > > + > > + return NULL_TREE; > > +} > > + > > + > > /* Handle a "noplt" attribute; arguments as in > > struct attribute_spec.handler. */ > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index > > f2619e1..f1af1dc 100644 > > --- a/gcc/doc/extend.texi > > +++ b/gcc/doc/extend.texi > > @@ -7129,6 +7129,14 @@ The @code{visibility} attribute is described > > in The @code{weak} attribute is described in @ref{Common Function > > Attributes}. > > > > +@item noinit > > +@cindex @code{noinit} variable attribute Any data with the > > +@code{noinit} attribute will not be initialized by the C runtime > > +startup code, or the program loader. Not initializing data in this > > +way can reduce program startup times. Specific to ELF targets, > > +this attribute relies on the linker to place such data in the right > > +location. > > Maybe: > > This attribute is specific to ELF targets and relies on the linker to > place such data in the right location. > Thanks, I thought I had chosen a nice turn of phrase :-) > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > new file mode 100644 > > index 0000000..ffcf8c6 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > @@ -0,0 +1,59 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target noinit */ > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only > > +applies to variables" } */ int __attribute__((section > > +("mysection"), noinit)) var_section1; /* { dg-warning "because it > > +conflicts with attribute" } */ int __attribute__((noinit, section > > +("mysection"))) var_section2; /* { dg-warning "because it conflicts > > +with attribute" } */ > > + > > + > > +int > > +main (void) > > +{ > > + /* Make sure that the C startup code has correctly initialized > > +the ordinary variables. */ > > + if (var_common != 0) > > + abort (); > > + > > + /* Initialized variables are not re-initialized 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 (); > > +} > > diff --git a/gcc/testsuite/lib/target-supports.exp > > b/gcc/testsuite/lib/target-supports.exp > > index 815e837..ae05c0a 100644 > > --- a/gcc/testsuite/lib/target-supports.exp > > +++ b/gcc/testsuite/lib/target-supports.exp > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > return [check_weak_available] > > } > > > > +# The noinit attribute is only supported by some targets. > > +# This proc returns 1 if it's supported, 0 if it's not. > > + > > +proc check_effective_target_noinit { } { > > + if { [istarget arm*-*-eabi] > > + || [istarget msp430-*-*] } { > > + return 1 > > + } > > + > > + return 0 > > +} > > + > > ############################### > > # proc check_visibility_available { what_kind } > > ############################### diff --git a/gcc/varasm.c > > b/gcc/varasm.c index 626a4c9..6ddab0ce 100644 > > --- a/gcc/varasm.c > > +++ b/gcc/varasm.c > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > flags |= SECTION_TLS | SECTION_BSS; > > > > + if (strcmp (name, ".noinit") == 0) > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > + > > /* Various sections have special ELF types that the assembler will > > assign by default based on the name. They are neither SHT_PROGBITS > > nor SHT_NOBITS, so when changing sections we don't want to > > print a @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree > > decl, int reloc) > > > > /* Select a section based on the above categorization. */ > > > > +static section *noinit_section = NULL; > > + > > No longer needed. Indeed. > > OK with those changes, thanks. Thanks, committed as r274482. Christophe > Richard > > > section * > > default_elf_select_section (tree decl, int reloc, > > unsigned HOST_WIDE_INT align) { > > const char *sname; > > + > > switch (categorize_decl_for_section (decl, reloc)) > > { > > case SECCAT_TEXT: > > @@ -6790,6 +6796,13 @@ default_elf_select_section (tree decl, int reloc, > > sname = ".tdata"; > > break; > > case SECCAT_BSS: > > + if (DECL_P (decl) > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > + { > > + sname = ".noinit"; > > + break; > > + } > > + > > if (bss_section) > > return bss_section; > > sname = ".bss";
On Wed, 14 Aug 2019 at 17:59, Tamar Christina <Tamar.Christina@arm.com> wrote: > > Hi Christoph, > > The noinit testcase is currently failing on x86_64. > > Is the test supposed to be running there? > No, there's an effective-target to skip it. But I notice a typo: +/* { dg-require-effective-target noinit */ (missing closing brace) Could it explain why it's failing on x86_64 ? > Thanks, > Tamar > > -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On Behalf Of Christophe Lyon > Sent: Wednesday, August 14, 2019 2:18 PM > To: Christophe Lyon <christophe.lyon@linaro.org>; Martin Sebor <msebor@gmail.com>; gcc Patches <gcc-patches@gcc.gnu.org>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Jozef Lawrynowicz <jozef.l@mittosystems.com>; Richard Sandiford <Richard.Sandiford@arm.com> > Subject: Re: [PATCH] Add generic support for "noinit" attribute > > On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote: > > > > Sorry for the slow response, I'd missed that there was an updated patch... > > > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > > 2019-07-04 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > * lib/target-supports.exp (check_effective_target_noinit): New > > > proc. > > > * gcc.c-torture/execute/noinit-attribute.c: New test. > > > > Second line should be indented by tabs rather than spaces. > > > > > @@ -2224,6 +2234,54 @@ handle_weak_attribute (tree *node, tree name, > > > return NULL_TREE; > > > } > > > > > > +/* Handle a "noinit" attribute; arguments as in struct > > > + attribute_spec.handler. 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 > > > +handle_noinit_attribute (tree * node, > > > + tree name, > > > + tree args, > > > + int flags ATTRIBUTE_UNUSED, > > > + bool *no_add_attrs) > > > +{ > > > + const char *message = NULL; > > > + > > > + gcc_assert (DECL_P (*node)); > > > + 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. > > > + */ else 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 (!targetm.have_switchable_bss_sections) > > > + message = G_("%qE attribute is specific to ELF targets"); > > > > Maybe make this an else if too? Or make the VAR_DECL an else if if > > you think the ELF one should win. Either way, it seems odd to have > > the mixture between else if and not. > > > Right, I changed this into an else if. > > > > + if (message) > > > + { > > > + warning (OPT_Wattributes, message, name); > > > + *no_add_attrs = true; > > > + } > > > + else > > > + /* 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. Do this only if the attribute is > > > + valid. */ > > > > Comment should be indented two spaces more. > > > > > + if (DECL_COMMON (*node)) > > > + DECL_COMMON (*node) = 0; > > > + > > > + return NULL_TREE; > > > +} > > > + > > > + > > > /* Handle a "noplt" attribute; arguments as in > > > struct attribute_spec.handler. */ > > > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index > > > f2619e1..f1af1dc 100644 > > > --- a/gcc/doc/extend.texi > > > +++ b/gcc/doc/extend.texi > > > @@ -7129,6 +7129,14 @@ The @code{visibility} attribute is described > > > in The @code{weak} attribute is described in @ref{Common Function > > > Attributes}. > > > > > > +@item noinit > > > +@cindex @code{noinit} variable attribute Any data with the > > > +@code{noinit} attribute will not be initialized by the C runtime > > > +startup code, or the program loader. Not initializing data in this > > > +way can reduce program startup times. Specific to ELF targets, > > > +this attribute relies on the linker to place such data in the right > > > +location. > > > > Maybe: > > > > This attribute is specific to ELF targets and relies on the linker to > > place such data in the right location. > > > Thanks, I thought I had chosen a nice turn of phrase :-) > > > > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > new file mode 100644 > > > index 0000000..ffcf8c6 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > @@ -0,0 +1,59 @@ > > > +/* { dg-do run } */ > > > +/* { dg-require-effective-target noinit */ > > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only > > > +applies to variables" } */ int __attribute__((section > > > +("mysection"), noinit)) var_section1; /* { dg-warning "because it > > > +conflicts with attribute" } */ int __attribute__((noinit, section > > > +("mysection"))) var_section2; /* { dg-warning "because it conflicts > > > +with attribute" } */ > > > + > > > + > > > +int > > > +main (void) > > > +{ > > > + /* Make sure that the C startup code has correctly initialized > > > +the ordinary variables. */ > > > + if (var_common != 0) > > > + abort (); > > > + > > > + /* Initialized variables are not re-initialized 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 (); > > > +} > > > diff --git a/gcc/testsuite/lib/target-supports.exp > > > b/gcc/testsuite/lib/target-supports.exp > > > index 815e837..ae05c0a 100644 > > > --- a/gcc/testsuite/lib/target-supports.exp > > > +++ b/gcc/testsuite/lib/target-supports.exp > > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > > return [check_weak_available] > > > } > > > > > > +# The noinit attribute is only supported by some targets. > > > +# This proc returns 1 if it's supported, 0 if it's not. > > > + > > > +proc check_effective_target_noinit { } { > > > + if { [istarget arm*-*-eabi] > > > + || [istarget msp430-*-*] } { > > > + return 1 > > > + } > > > + > > > + return 0 > > > +} > > > + > > > ############################### > > > # proc check_visibility_available { what_kind } > > > ############################### diff --git a/gcc/varasm.c > > > b/gcc/varasm.c index 626a4c9..6ddab0ce 100644 > > > --- a/gcc/varasm.c > > > +++ b/gcc/varasm.c > > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > > flags |= SECTION_TLS | SECTION_BSS; > > > > > > + if (strcmp (name, ".noinit") == 0) > > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > > + > > > /* Various sections have special ELF types that the assembler will > > > assign by default based on the name. They are neither SHT_PROGBITS > > > nor SHT_NOBITS, so when changing sections we don't want to > > > print a @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree > > > decl, int reloc) > > > > > > /* Select a section based on the above categorization. */ > > > > > > +static section *noinit_section = NULL; > > > + > > > > No longer needed. > Indeed. > > > > > OK with those changes, thanks. > Thanks, committed as r274482. > > Christophe > > > Richard > > > > > section * > > > default_elf_select_section (tree decl, int reloc, > > > unsigned HOST_WIDE_INT align) { > > > const char *sname; > > > + > > > switch (categorize_decl_for_section (decl, reloc)) > > > { > > > case SECCAT_TEXT: > > > @@ -6790,6 +6796,13 @@ default_elf_select_section (tree decl, int reloc, > > > sname = ".tdata"; > > > break; > > > case SECCAT_BSS: > > > + if (DECL_P (decl) > > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > > + { > > > + sname = ".noinit"; > > > + break; > > > + } > > > + > > > if (bss_section) > > > return bss_section; > > > sname = ".bss";
On Wed, 14 Aug 2019 at 19:07, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > On Wed, 14 Aug 2019 at 17:59, Tamar Christina <Tamar.Christina@arm.com> wrote: > > > > Hi Christoph, > > > > The noinit testcase is currently failing on x86_64. > > > > Is the test supposed to be running there? > > > No, there's an effective-target to skip it. > But I notice a typo: > +/* { dg-require-effective-target noinit */ > (missing closing brace) > Could it explain why it's failing on x86_64 ? I fixed the typo as obvious in r274489. > > > Thanks, > > Tamar > > > > -----Original Message----- > > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On Behalf Of Christophe Lyon > > Sent: Wednesday, August 14, 2019 2:18 PM > > To: Christophe Lyon <christophe.lyon@linaro.org>; Martin Sebor <msebor@gmail.com>; gcc Patches <gcc-patches@gcc.gnu.org>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Jozef Lawrynowicz <jozef.l@mittosystems.com>; Richard Sandiford <Richard.Sandiford@arm.com> > > Subject: Re: [PATCH] Add generic support for "noinit" attribute > > > > On Wed, 14 Aug 2019 at 14:14, Richard Sandiford <richard.sandiford@arm.com> wrote: > > > > > > Sorry for the slow response, I'd missed that there was an updated patch... > > > > > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > > > 2019-07-04 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > > > * lib/target-supports.exp (check_effective_target_noinit): New > > > > proc. > > > > * gcc.c-torture/execute/noinit-attribute.c: New test. > > > > > > Second line should be indented by tabs rather than spaces. > > > > > > > @@ -2224,6 +2234,54 @@ handle_weak_attribute (tree *node, tree name, > > > > return NULL_TREE; > > > > } > > > > > > > > +/* Handle a "noinit" attribute; arguments as in struct > > > > + attribute_spec.handler. 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 > > > > +handle_noinit_attribute (tree * node, > > > > + tree name, > > > > + tree args, > > > > + int flags ATTRIBUTE_UNUSED, > > > > + bool *no_add_attrs) > > > > +{ > > > > + const char *message = NULL; > > > > + > > > > + gcc_assert (DECL_P (*node)); > > > > + 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. > > > > + */ else 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 (!targetm.have_switchable_bss_sections) > > > > + message = G_("%qE attribute is specific to ELF targets"); > > > > > > Maybe make this an else if too? Or make the VAR_DECL an else if if > > > you think the ELF one should win. Either way, it seems odd to have > > > the mixture between else if and not. > > > > > Right, I changed this into an else if. > > > > > > + if (message) > > > > + { > > > > + warning (OPT_Wattributes, message, name); > > > > + *no_add_attrs = true; > > > > + } > > > > + else > > > > + /* 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. Do this only if the attribute is > > > > + valid. */ > > > > > > Comment should be indented two spaces more. > > > > > > > + if (DECL_COMMON (*node)) > > > > + DECL_COMMON (*node) = 0; > > > > + > > > > + return NULL_TREE; > > > > +} > > > > + > > > > + > > > > /* Handle a "noplt" attribute; arguments as in > > > > struct attribute_spec.handler. */ > > > > > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index > > > > f2619e1..f1af1dc 100644 > > > > --- a/gcc/doc/extend.texi > > > > +++ b/gcc/doc/extend.texi > > > > @@ -7129,6 +7129,14 @@ The @code{visibility} attribute is described > > > > in The @code{weak} attribute is described in @ref{Common Function > > > > Attributes}. > > > > > > > > +@item noinit > > > > +@cindex @code{noinit} variable attribute Any data with the > > > > +@code{noinit} attribute will not be initialized by the C runtime > > > > +startup code, or the program loader. Not initializing data in this > > > > +way can reduce program startup times. Specific to ELF targets, > > > > +this attribute relies on the linker to place such data in the right > > > > +location. > > > > > > Maybe: > > > > > > This attribute is specific to ELF targets and relies on the linker to > > > place such data in the right location. > > > > > Thanks, I thought I had chosen a nice turn of phrase :-) > > > > > > > > diff --git a/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > > b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > > new file mode 100644 > > > > index 0000000..ffcf8c6 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c > > > > @@ -0,0 +1,59 @@ > > > > +/* { dg-do run } */ > > > > +/* { dg-require-effective-target noinit */ > > > > +/* { 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 __attribute__((noinit)) func(); /* { dg-warning "attribute only > > > > +applies to variables" } */ int __attribute__((section > > > > +("mysection"), noinit)) var_section1; /* { dg-warning "because it > > > > +conflicts with attribute" } */ int __attribute__((noinit, section > > > > +("mysection"))) var_section2; /* { dg-warning "because it conflicts > > > > +with attribute" } */ > > > > + > > > > + > > > > +int > > > > +main (void) > > > > +{ > > > > + /* Make sure that the C startup code has correctly initialized > > > > +the ordinary variables. */ > > > > + if (var_common != 0) > > > > + abort (); > > > > + > > > > + /* Initialized variables are not re-initialized 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 (); > > > > +} > > > > diff --git a/gcc/testsuite/lib/target-supports.exp > > > > b/gcc/testsuite/lib/target-supports.exp > > > > index 815e837..ae05c0a 100644 > > > > --- a/gcc/testsuite/lib/target-supports.exp > > > > +++ b/gcc/testsuite/lib/target-supports.exp > > > > @@ -364,6 +364,18 @@ proc check_weak_override_available { } { > > > > return [check_weak_available] > > > > } > > > > > > > > +# The noinit attribute is only supported by some targets. > > > > +# This proc returns 1 if it's supported, 0 if it's not. > > > > + > > > > +proc check_effective_target_noinit { } { > > > > + if { [istarget arm*-*-eabi] > > > > + || [istarget msp430-*-*] } { > > > > + return 1 > > > > + } > > > > + > > > > + return 0 > > > > +} > > > > + > > > > ############################### > > > > # proc check_visibility_available { what_kind } > > > > ############################### diff --git a/gcc/varasm.c > > > > b/gcc/varasm.c index 626a4c9..6ddab0ce 100644 > > > > --- a/gcc/varasm.c > > > > +++ b/gcc/varasm.c > > > > @@ -6428,6 +6428,9 @@ default_section_type_flags (tree decl, const char *name, int reloc) > > > > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > > > > flags |= SECTION_TLS | SECTION_BSS; > > > > > > > > + if (strcmp (name, ".noinit") == 0) > > > > + flags |= SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE; > > > > + > > > > /* Various sections have special ELF types that the assembler will > > > > assign by default based on the name. They are neither SHT_PROGBITS > > > > nor SHT_NOBITS, so when changing sections we don't want to > > > > print a @@ -6748,11 +6751,14 @@ decl_readonly_section (const_tree > > > > decl, int reloc) > > > > > > > > /* Select a section based on the above categorization. */ > > > > > > > > +static section *noinit_section = NULL; > > > > + > > > > > > No longer needed. > > Indeed. > > > > > > > > OK with those changes, thanks. > > Thanks, committed as r274482. > > > > Christophe > > > > > Richard > > > > > > > section * > > > > default_elf_select_section (tree decl, int reloc, > > > > unsigned HOST_WIDE_INT align) { > > > > const char *sname; > > > > + > > > > switch (categorize_decl_for_section (decl, reloc)) > > > > { > > > > case SECCAT_TEXT: > > > > @@ -6790,6 +6796,13 @@ default_elf_select_section (tree decl, int reloc, > > > > sname = ".tdata"; > > > > break; > > > > case SECCAT_BSS: > > > > + if (DECL_P (decl) > > > > + && lookup_attribute ("noinit", DECL_ATTRIBUTES (decl)) != NULL_TREE) > > > > + { > > > > + sname = ".noinit"; > > > > + break; > > > > + } > > > > + > > > > if (bss_section) > > > > return bss_section; > > > > sname = ".bss";
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 = .);