diff mbox series

aarch64: Handle attributes in the global namespace for aarch64_lookup_shared_state_flags [PR116598]

Message ID 20240904210521.3310478-1-quic_apinski@quicinc.com
State New
Headers
Series aarch64: Handle attributes in the global namespace for aarch64_lookup_shared_state_flags [PR116598] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed

Commit Message

Andrew Pinski Sept. 4, 2024, 9:05 p.m. UTC 
  The code in aarch64_lookup_shared_state_flags all C++11 attributes on the function type
had a namespace associated with them. But with the addition of reproducible/unsequenced,
this was no longer true.
This is the simple fix to ignore attributes in the global namespace since we are looking
for ones in the `arm` namespace instead.

Built and tested for aarch64-linux-gnu.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (aarch64_lookup_shared_state_flags): Ignore
	attributes in the global namespace.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/config/aarch64/aarch64.cc | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Marek Polacek Sept. 4, 2024, 9:35 p.m. UTC | #1 
On Wed, Sep 04, 2024 at 02:05:21PM -0700, Andrew Pinski wrote:
> The code in aarch64_lookup_shared_state_flags all C++11 attributes on the function type
> had a namespace associated with them. But with the addition of reproducible/unsequenced,
> this was no longer true.
> This is the simple fix to ignore attributes in the global namespace since we are looking
> for ones in the `arm` namespace instead.
> 
> Built and tested for aarch64-linux-gnu.
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/aarch64.cc (aarch64_lookup_shared_state_flags): Ignore
> 	attributes in the global namespace.
> 
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  gcc/config/aarch64/aarch64.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 27e24ba70ab..3f7bc572edc 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -597,6 +597,10 @@ aarch64_lookup_shared_state_flags (tree attrs, const char *state_name)
>        if (!cxx11_attribute_p (attr))
>  	continue;
>  
> +      /* Skip the attributes in the global namespace. */
> +      if (!TREE_PURPOSE (TREE_PURPOSE (attr)))
> +	continue;
> +
>        auto ns = IDENTIFIER_POINTER (TREE_PURPOSE (TREE_PURPOSE (attr)));
>        if (strcmp (ns, "arm") != 0)
>  	continue;

Take it or leave it, but I think the whole thing could be just

  tree ns = get_attribute_namespace (attr);
  if (!ns || !id_equal (ns, "arm"))
    continue;

You could also use get_attribute_name below for attr_name.

Marek
Andrew Pinski Sept. 4, 2024, 9:44 p.m. UTC | #2 
On Wed, Sep 4, 2024 at 2:36 PM Marek Polacek <polacek@redhat.com> wrote:
>
> On Wed, Sep 04, 2024 at 02:05:21PM -0700, Andrew Pinski wrote:
> > The code in aarch64_lookup_shared_state_flags all C++11 attributes on the function type
> > had a namespace associated with them. But with the addition of reproducible/unsequenced,
> > this was no longer true.
> > This is the simple fix to ignore attributes in the global namespace since we are looking
> > for ones in the `arm` namespace instead.
> >
> > Built and tested for aarch64-linux-gnu.
> >
> > gcc/ChangeLog:
> >
> >       * config/aarch64/aarch64.cc (aarch64_lookup_shared_state_flags): Ignore
> >       attributes in the global namespace.
> >
> > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> > ---
> >  gcc/config/aarch64/aarch64.cc | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > index 27e24ba70ab..3f7bc572edc 100644
> > --- a/gcc/config/aarch64/aarch64.cc
> > +++ b/gcc/config/aarch64/aarch64.cc
> > @@ -597,6 +597,10 @@ aarch64_lookup_shared_state_flags (tree attrs, const char *state_name)
> >        if (!cxx11_attribute_p (attr))
> >       continue;
> >
> > +      /* Skip the attributes in the global namespace. */
> > +      if (!TREE_PURPOSE (TREE_PURPOSE (attr)))
> > +     continue;
> > +
> >        auto ns = IDENTIFIER_POINTER (TREE_PURPOSE (TREE_PURPOSE (attr)));
> >        if (strcmp (ns, "arm") != 0)
> >       continue;
>
> Take it or leave it, but I think the whole thing could be just
>
>   tree ns = get_attribute_namespace (attr);
>   if (!ns || !id_equal (ns, "arm"))
>     continue;

Actually I think it could be reduced further down to just:
if (!is_attribute_namespace_p ("arm", attr))
  continue;

In this case.

If I get some time this weekend I will submit a patch to clean up this code.

Thanks,
Andrew Pinski

>
> You could also use get_attribute_name below for attr_name.
>
> Marek
>
Andrew Pinski Sept. 5, 2024, 5:11 a.m. UTC | #3 
On Wed, Sep 4, 2024 at 2:44 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Wed, Sep 4, 2024 at 2:36 PM Marek Polacek <polacek@redhat.com> wrote:
> >
> > On Wed, Sep 04, 2024 at 02:05:21PM -0700, Andrew Pinski wrote:
> > > The code in aarch64_lookup_shared_state_flags all C++11 attributes on the function type
> > > had a namespace associated with them. But with the addition of reproducible/unsequenced,
> > > this was no longer true.
> > > This is the simple fix to ignore attributes in the global namespace since we are looking
> > > for ones in the `arm` namespace instead.
> > >
> > > Built and tested for aarch64-linux-gnu.
> > >
> > > gcc/ChangeLog:
> > >
> > >       * config/aarch64/aarch64.cc (aarch64_lookup_shared_state_flags): Ignore
> > >       attributes in the global namespace.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> > > ---
> > >  gcc/config/aarch64/aarch64.cc | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > > index 27e24ba70ab..3f7bc572edc 100644
> > > --- a/gcc/config/aarch64/aarch64.cc
> > > +++ b/gcc/config/aarch64/aarch64.cc
> > > @@ -597,6 +597,10 @@ aarch64_lookup_shared_state_flags (tree attrs, const char *state_name)
> > >        if (!cxx11_attribute_p (attr))
> > >       continue;
> > >
> > > +      /* Skip the attributes in the global namespace. */
> > > +      if (!TREE_PURPOSE (TREE_PURPOSE (attr)))
> > > +     continue;
> > > +
> > >        auto ns = IDENTIFIER_POINTER (TREE_PURPOSE (TREE_PURPOSE (attr)));
> > >        if (strcmp (ns, "arm") != 0)
> > >       continue;
> >
> > Take it or leave it, but I think the whole thing could be just
> >
> >   tree ns = get_attribute_namespace (attr);
> >   if (!ns || !id_equal (ns, "arm"))
> >     continue;
>
> Actually I think it could be reduced further down to just:
> if (!is_attribute_namespace_p ("arm", attr))
>   continue;
>
> In this case.
>
> If I get some time this weekend I will submit a patch to clean up this code.

In the end I did it today.
New patch:
https://gcc.gnu.org/pipermail/gcc-patches/2024-September/662325.html

Thanks again for the suggestion and the review.

Thanks,
Andrew

>
> Thanks,
> Andrew Pinski
>
> >
> > You could also use get_attribute_name below for attr_name.
> >
> > Marek
> >
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 27e24ba70ab..3f7bc572edc 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -597,6 +597,10 @@  aarch64_lookup_shared_state_flags (tree attrs, const char *state_name)
       if (!cxx11_attribute_p (attr))
 	continue;
 
+      /* Skip the attributes in the global namespace. */
+      if (!TREE_PURPOSE (TREE_PURPOSE (attr)))
+	continue;
+
       auto ns = IDENTIFIER_POINTER (TREE_PURPOSE (TREE_PURPOSE (attr)));
       if (strcmp (ns, "arm") != 0)
 	continue;