This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add generic support for "noinit" attribute
- From: Christophe Lyon <christophe dot lyon at linaro dot org>
- To: Tamar Christina <Tamar dot Christina at arm dot com>
- Cc: Martin Sebor <msebor at gmail dot com>, gcc Patches <gcc-patches at gcc dot gnu dot org>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, "nickc at redhat dot com" <nickc at redhat dot com>, Jozef Lawrynowicz <jozef dot l at mittosystems dot com>, Richard Sandiford <Richard dot Sandiford at arm dot com>, nd <nd at arm dot com>
- Date: Wed, 14 Aug 2019 19:57:58 +0200
- Subject: Re: [PATCH] Add generic support for "noinit" attribute
- References: <CAKdteObaTtriRDdWTkzr66Yym0zapsYM=0meT10ihC4K9dmFJg@mail.gmail.com> <d6bc458d-298e-a1f1-31cb-ab48a6bd2280@gmail.com> <CAKdteObPa22MWCK-LpwR2nUFPX2upzawfZ8EUM5ce=rOk4Rftw@mail.gmail.com> <mpty30y9ueo.fsf@arm.com> <CAKdteOYaj2RPmLzXH2GOBUejM2NJ5oC0RM6Kkdfc4+OaHRNHfw@mail.gmail.com> <mptlfvwkkq3.fsf@arm.com> <CAKdteOYZC+Ftx+PYTmrfPays8sZGBjkh-NMK9HG8V1icQ5y40Q@mail.gmail.com> <DB6PR0802MB2309910A9F4E713C15E10770FFAD0@DB6PR0802MB2309.eurprd08.prod.outlook.com> <CAKdteOYRDb8x_v7fOeCxc2YXf8ew=vdLjBmEydu4DHjOmzYrqw@mail.gmail.com>
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";