Ping?
On Tue, 30 Jul 2019 at 15:35, Christophe Lyon <christophe.l...@linaro.org> wrote: > > Hi, > > Thanks for the useful feedback. > > > On Tue, 16 Jul 2019 at 11:54, Richard Sandiford > <richard.sandif...@arm.com> wrote: > > > > Thanks for doing this in a generic way. > > > > Christophe Lyon <christophe.l...@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