Re: [PATCH] Add generic support for "noinit" attribute
On Wed, 14 Aug 2019 at 19:07, Christophe Lyon
wrote:
>
> On Wed, 14 Aug 2019 at 17:59, Tamar Christina 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-ow...@gcc.gnu.org On
> > Behalf Of Christophe Lyon
> > Sent: Wednesday, August 14, 2019 2:18 PM
> > To: Christophe Lyon ; Martin Sebor
> > ; gcc Patches ; Richard Earnshaw
> > ; ni...@redhat.com; Jozef Lawrynowicz
> > ; Richard Sandiford
> > Subject: Re: [PATCH] Add generic support for "noinit" attribute
> >
> > On Wed, 14 Aug 2019 at 14:14, Richard Sandiford
> > wrote:
> > >
> > > Sorry for the slow response, I'd missed that there was an updated patch...
> > >
> > > Christophe Lyon writes:
> > > > 2019-07-04 Christophe Lyon
> > > >
> > > > * 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,
> > > > + intflags 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_spe
Re: [PATCH] Add generic support for "noinit" attribute
On Wed, 14 Aug 2019 at 17:59, Tamar Christina 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-ow...@gcc.gnu.org On Behalf
> Of Christophe Lyon
> Sent: Wednesday, August 14, 2019 2:18 PM
> To: Christophe Lyon ; Martin Sebor
> ; gcc Patches ; Richard Earnshaw
> ; ni...@redhat.com; Jozef Lawrynowicz
> ; Richard Sandiford
> Subject: Re: [PATCH] Add generic support for "noinit" attribute
>
> On Wed, 14 Aug 2019 at 14:14, Richard Sandiford
> wrote:
> >
> > Sorry for the slow response, I'd missed that there was an updated patch...
> >
> > Christophe Lyon writes:
> > > 2019-07-04 Christophe Lyon
> > >
> > > * 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,
> > > + intflags 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
> > > +s
RE: [PATCH] Add generic support for "noinit" attribute
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-ow...@gcc.gnu.org On Behalf
Of Christophe Lyon
Sent: Wednesday, August 14, 2019 2:18 PM
To: Christophe Lyon ; Martin Sebor
; gcc Patches ; Richard Earnshaw
; ni...@redhat.com; Jozef Lawrynowicz
; Richard Sandiford
Subject: Re: [PATCH] Add generic support for "noinit" attribute
On Wed, 14 Aug 2019 at 14:14, Richard Sandiford
wrote:
>
> Sorry for the slow response, I'd missed that there was an updated patch...
>
> Christophe Lyon writes:
> > 2019-07-04 Christophe Lyon
> >
> > * 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,
> > + intflags 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 000..ffcf8c6
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/noinit-attribute.c
> > @@ -
Re: [PATCH] Add generic support for "noinit" attribute
On Wed, 14 Aug 2019 at 14:14, Richard Sandiford
wrote:
>
> Sorry for the slow response, I'd missed that there was an updated patch...
>
> Christophe Lyon writes:
> > 2019-07-04 Christophe Lyon
> >
> > * 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,
> > + intflags 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 000..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
Re: [PATCH] Add generic support for "noinit" attribute
Sorry for the slow response, I'd missed that there was an updated patch...
Christophe Lyon writes:
> 2019-07-04 Christophe Lyon
>
> * 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,
> + intflags 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 000..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
Re: [PATCH] Add generic support for "noinit" attribute
Ping?
On Tue, 30 Jul 2019 at 15:35, Christophe Lyon
wrote:
>
> Hi,
>
> Thanks for the useful feedback.
>
>
> On Tue, 16 Jul 2019 at 11:54, Richard Sandiford
> wrote:
> >
> > Thanks for doing this in a generic way.
> >
> > Christophe Lyon 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,
> > > + intflags 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 000..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) __att
Re: [PATCH] Add generic support for "noinit" attribute
Hi,
On Tue, 30 Jul 2019 15:35:23 +0200
Christophe Lyon wrote:
> Hi,
>
> Thanks for the useful feedback.
>
>
> On Tue, 16 Jul 2019 at 11:54, Richard Sandiford
> wrote:
> >
> > Thanks for doing this in a generic way.
> >
> > Christophe Lyon 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,
> > > + intflags 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 000..f33c0d0
> > > --- /dev/null
>
Re: [PATCH] Add generic support for "noinit" attribute
Hi,
Thanks for the useful feedback.
On Tue, 16 Jul 2019 at 11:54, Richard Sandiford
wrote:
>
> Thanks for doing this in a generic way.
>
> Christophe Lyon 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,
> > + intflags 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 000..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"),
Re: [PATCH] Add generic support for "noinit" attribute
Thanks for doing this in a generic way.
Christophe Lyon 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,
> + intflags 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 000..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 o
Re: [PATCH] Add generic support for "noinit" attribute
Ping?
Le mar. 9 juil. 2019 à 00:04, Martin Sebor a écrit :
> On 7/8/19 5:10 AM, Christophe Lyon wrote:
> > On Sat, 6 Jul 2019 at 19:57, Martin Sebor 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
> >>>
> >>> * 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
> >>>
> >>> * c-attribs.c (c_common_attribute_table): Add "noinit" entry.
> >>> (handle_section_attribute): Add support for "noinit" attr
Re: [PATCH] Add generic support for "noinit" attribute
On 7/8/19 5:10 AM, Christophe Lyon wrote:
On Sat, 6 Jul 2019 at 19:57, Martin Sebor 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
* 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
* 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
* msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
gcc/testsuite/ChangeLog:
2019-07-04 Christophe Lyon
* gcc.target/arm/noinit-attribute.c: New test.
Re: [PATCH] Add generic support for "noinit" attribute
On Sat, 6 Jul 2019 at 19:57, Martin Sebor 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
> >
> > * 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
> >
> > * 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
> >
> > * msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-07-04 Christophe Lyon
> >
> > * gcc.target/arm/noinit-attribute.c: New test.
> >
>
commit 3a9dd81162eae36a87a067018a5e5bf1e7b978df
Author: Christophe Lyon
Date: Thu Jul 4 14:46:00 2019 +
Add generic support for noini
Re: [PATCH] Add generic support for "noinit" attribute
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
* 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
* 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
* msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
gcc/testsuite/ChangeLog:
2019-07-04 Christophe Lyon
* gcc.target/arm/noinit-attribute.c: New test.
Re: [PATCH] Add generic support for "noinit" attribute
On Fri, 5 Jul 2019 at 12:57, Jozef Lawrynowicz wrote: > > On Fri, 5 Jul 2019 11:26:20 +0200 > Christophe Lyon wrote: > > > On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz > > 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
Re: [PATCH] Add generic support for "noinit" attribute
On Fri, 5 Jul 2019 11:26:20 +0200 Christophe Lyon wrote: > On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz > 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
Re: [PATCH] Add generic support for "noinit" attribute
On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz 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
Re: [PATCH] Add generic support for "noinit" attribute
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
Re: [PATCH] Add generic support for "noinit" attribute
On Thu, 4 Jul 2019 17:27:28 +0200 Christophe Lyon 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
[PATCH] Add generic support for "noinit" attribute
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).
Thanks,
Christophe
gcc/ChangeLog:
2019-07-04 Christophe Lyon
* 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
* 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
* msp430/msp430.c (msp430_attribute_table): Remove "noinit" entry.
gcc/testsuite/ChangeLog:
2019-07-04 Christophe Lyon
* gcc.target/arm/noinit-attribute.c: New test.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 48819e7..3aefe84 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 *);
@@ -458,6 +459,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, NULL },
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
@@ -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");
+ goto fail;
+}
+
set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
return NULL_TREE;
@@ -2224,6 +2234,48 @@ 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. Th
