[PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

Hi,

on s390 a warning test fails:

inline int ATTR ((cold, aligned (8)))
finline_hot_noret_align (int);

inline int ATTR ((warn_unused_result))
finline_hot_noret_align (int);

inline int ATTR ((aligned (4)))
   finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute 
.aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)."

This test actually uncovered two problems.  First, on s390 the default 
function alignment is 8 bytes.  When the second decl above is merged 
with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) > 
DECL_ALIGN (new).  Subsequently, when merging the third decl, no warning 
is emitted since DECL_USER_ALIGN is unset.

This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == DECL_ALIGN 
(new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)).


Then, while going through the related files I also noticed that we emit 
a wrong warning for:

inline int ATTR ((aligned (32)))
finline_align (int);

inline int ATTR ((aligned (4)))
   finline_align (int);  /* { dg-warning "ignoring attribute .aligned 
\\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */

What we emit is

warning: ignoring attribute ‘aligned (4)’ because it conflicts with 
attribute ‘aligned (8)’ [-Wattributes].

This is due to the short circuit evaluation in c-attribs.c:

            && ((curalign = DECL_ALIGN (decl)) > bitalign
                || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))

where lastalign is only initialized when curalign > bitalign.  On s390 
this is not the case and lastalign is used zero-initialized in the 
following code.

On top, the following condition
   else if (!warn_if_not_aligned_p
           && TREE_CODE (decl) == FUNCTION_DECL
           && DECL_ALIGN (decl) > bitalign)

seems to be fully covered by
     else if (TREE_CODE (decl) == FUNCTION_DECL
	   && ((curalign = DECL_ALIGN (decl)) > bitalign
	       || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))

and so is essentially dead. I therefore removed it as there does not 
seem to be a test anywhere for the error message ( "alignment for %q+D 
was previously specified as %d and may not be decreased") either.

Bootstrapped and regtested on s390, ppc64 and amd64.
Is it OK?

Regards
  Robin

--

gcc/c-family/ChangeLog:

         * c-attribs.c (common_handle_aligned_attribute): Re-init 
lastalign and remove case.

gcc/c/ChangeLog:

         * c-decl.c (merge_decls): Copy DECL_USER_ALIGN even for similar 
DECL_ALIGN.

gcc/cp/ChangeLog:

         * decl.c (duplicate_decls): Dito.

[-- Attachment #2: 0001-c-family-Copy-DECL_USER_ALIGN-even-if-DECL_ALIGN-is-.patch --]
[-- Type: text/x-patch, Size: 4030 bytes --]

From f3a9f8b9bd1e28c23f2efcdb061959852a91deb6 Mon Sep 17 00:00:00 2001
From: Robin Dapp <rdapp@linux.ibm.com>
Date: Tue, 20 Apr 2021 10:51:18 +0200
Subject: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is
 similar.

When re-declaring a function with differing attributes DECL_USER_ALIGN
is usually not merged/copied when DECL_ALIGN is similar.  On s390 this
will cause a warning message not to be shown.  Similarly, a note is not
shown when we short-circuit an alignment initialization in
common_handle_aligned_attribute ().

Fix this by copying DECL_USER_ALIGN even if DECL_ALIGN is similar as
well as reinitializing the short-circuited initialization.
---
 gcc/c-family/c-attribs.c | 23 ++++-------------------
 gcc/c/c-decl.c           |  3 +++
 gcc/cp/decl.c            |  5 +++++
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index c1f652d1dc9..d132b6fd3b6 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags,
 	   && ((curalign = DECL_ALIGN (decl)) > bitalign
 	       || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
     {
+      /* Re-init lastalign in case we short-circuit the condition,
+	 i.e.  curalign > bitalign.  */
+      lastalign = DECL_ALIGN (last_decl);
+
       /* Either a prior attribute on the same declaration or one
 	 on a prior declaration of the same function specifies
 	 stricter alignment than this attribute.  */
@@ -2366,25 +2370,6 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags,
       This formally comes from the c++11 specification but we are
       doing it for the GNU attribute syntax as well.  */
     *no_add_attrs = true;
-  else if (!warn_if_not_aligned_p
-	   && TREE_CODE (decl) == FUNCTION_DECL
-	   && DECL_ALIGN (decl) > bitalign)
-    {
-      /* Don't warn for function alignment here if warn_if_not_aligned_p
-	 is true.  It will be warned about later.  */
-      if (DECL_USER_ALIGN (decl))
-	{
-	  /* Only reject attempts to relax/override an alignment
-	     explicitly specified previously and accept declarations
-	     that appear to relax the implicit function alignment for
-	     the target.  Both increasing and increasing the alignment
-	     set by -falign-functions setting is permitted.  */
-	  error ("alignment for %q+D was previously specified as %d "
-		 "and may not be decreased", decl,
-		 DECL_ALIGN (decl) / BITS_PER_UNIT);
-	  *no_add_attrs = true;
-	}
-    }
   else if (warn_if_not_aligned_p
 	   && TREE_CODE (decl) == FIELD_DECL
 	   && !DECL_C_BIT_FIELD (decl))
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 3ea4708c507..2ea5051e9cd 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype)
 	  SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl));
 	  DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
 	}
+      else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl)
+	       && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl))
+	DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
       if (DECL_WARN_IF_NOT_ALIGN (olddecl)
 	  > DECL_WARN_IF_NOT_ALIGN (newdecl))
 	SET_DECL_WARN_IF_NOT_ALIGN (newdecl,
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 60dc2bf182d..ad07b1a4e63 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2795,6 +2795,11 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden)
       SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl));
       DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
     }
+  else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl)
+      && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl))
+    {
+      DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
+    }
   DECL_USER_ALIGN (olddecl) = DECL_USER_ALIGN (newdecl);
   if (DECL_WARN_IF_NOT_ALIGN (olddecl)
       > DECL_WARN_IF_NOT_ALIGN (newdecl))
-- 
2.23.0

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

Ping.

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

> on s390 a warning test fails:
> 
> inline int ATTR ((cold, aligned (8)))
> finline_hot_noret_align (int);
> 
> inline int ATTR ((warn_unused_result))
> finline_hot_noret_align (int);
> 
> inline int ATTR ((aligned (4)))
>     finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute
> .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)."
> 
> This test actually uncovered two problems.  First, on s390 the default
> function alignment is 8 bytes.  When the second decl above is merged
> with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) >
> DECL_ALIGN (new).  Subsequently, when merging the third decl, no warning
> is emitted since DECL_USER_ALIGN is unset.

[..]

Ping.

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Martin Sebor]

On 5/3/21 8:53 AM, Robin Dapp via Gcc-patches wrote:
> Hi,
> 
> on s390 a warning test fails:
> 
> inline int ATTR ((cold, aligned (8)))
> finline_hot_noret_align (int);
> 
> inline int ATTR ((warn_unused_result))
> finline_hot_noret_align (int);
> 
> inline int ATTR ((aligned (4)))
>    finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute 
> .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)."
> 
> This test actually uncovered two problems.  First, on s390 the default 
> function alignment is 8 bytes.  When the second decl above is merged 
> with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) > 
> DECL_ALIGN (new).  Subsequently, when merging the third decl, no warning 
> is emitted since DECL_USER_ALIGN is unset.
> 
> This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == DECL_ALIGN 
> (new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)).
> 
> 
> Then, while going through the related files I also noticed that we emit 
> a wrong warning for:
> 
> inline int ATTR ((aligned (32)))
> finline_align (int);
> 
> inline int ATTR ((aligned (4)))
>    finline_align (int);  /* { dg-warning "ignoring attribute .aligned 
> \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */
> 
> What we emit is
> 
> warning: ignoring attribute ‘aligned (4)’ because it conflicts with 
> attribute ‘aligned (8)’ [-Wattributes].
> 
> This is due to the short circuit evaluation in c-attribs.c:
> 
>             && ((curalign = DECL_ALIGN (decl)) > bitalign
>                 || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
> 
> where lastalign is only initialized when curalign > bitalign.  On s390 
> this is not the case and lastalign is used zero-initialized in the 
> following code.
> 
> On top, the following condition
>    else if (!warn_if_not_aligned_p
>            && TREE_CODE (decl) == FUNCTION_DECL
>            && DECL_ALIGN (decl) > bitalign)
> 
> seems to be fully covered by
>      else if (TREE_CODE (decl) == FUNCTION_DECL
>         && ((curalign = DECL_ALIGN (decl)) > bitalign
>             || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
> 
> and so is essentially dead. I therefore removed it as there does not 
> seem to be a test anywhere for the error message ( "alignment for %q+D 
> was previously specified as %d and may not be decreased") either.

The removal of the dead code looks good to me.  The change to
"re-init lastalign" doesn't seem right.  When it's zero it means
the conflict is between two attributes on the same declaration,
in which case the note shouldn't be printed (it would just point
to the same location as the warning).

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index c1f652d1dc9..d132b6fd3b6 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree 
name, tree args, int flags,
  	   && ((curalign = DECL_ALIGN (decl)) > bitalign
  	       || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
      {
+      /* Re-init lastalign in case we short-circuit the condition,
+	 i.e.  curalign > bitalign.  */
+      lastalign = DECL_ALIGN (last_decl);
+
        /* Either a prior attribute on the same declaration or one
  	 on a prior declaration of the same function specifies
  	 stricter alignment than this attribute.  */

For the C/C++ FE changes:

diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 3ea4708c507..2ea5051e9cd 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree 
newtype, tree oldtype)
  	  SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl));
  	  DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
  	}
+      else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl)
+	       && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl))
+	DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);

This should be the same as

   DECL_USER_ALIGN (newdecl) = 1;

so I would suggest to use that for clarity.

Other than that, a maintainer needs to review and approve the work.

Martin

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Jason Merrill]

On 5/19/21 6:03 PM, Martin Sebor via Gcc-patches wrote:
> On 5/3/21 8:53 AM, Robin Dapp via Gcc-patches wrote:
>> Hi,
>>
>> on s390 a warning test fails:
>>
>> inline int ATTR ((cold, aligned (8)))
>> finline_hot_noret_align (int);
>>
>> inline int ATTR ((warn_unused_result))
>> finline_hot_noret_align (int);
>>
>> inline int ATTR ((aligned (4)))
>>    finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute 
>> .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)."
>>
>> This test actually uncovered two problems.  First, on s390 the default 
>> function alignment is 8 bytes.  When the second decl above is merged 
>> with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) 
>> > DECL_ALIGN (new).  Subsequently, when merging the third decl, no 
>> warning is emitted since DECL_USER_ALIGN is unset.
>>
>> This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == 
>> DECL_ALIGN (new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN 
>> (newdecl)).
>>
>>
>> Then, while going through the related files I also noticed that we 
>> emit a wrong warning for:
>>
>> inline int ATTR ((aligned (32)))
>> finline_align (int);
>>
>> inline int ATTR ((aligned (4)))
>>    finline_align (int);  /* { dg-warning "ignoring attribute .aligned 
>> \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */
>>
>> What we emit is
>>
>> warning: ignoring attribute ‘aligned (4)’ because it conflicts with 
>> attribute ‘aligned (8)’ [-Wattributes].
>>
>> This is due to the short circuit evaluation in c-attribs.c:
>>
>>             && ((curalign = DECL_ALIGN (decl)) > bitalign
>>                 || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
>>
>> where lastalign is only initialized when curalign > bitalign.  On s390 
>> this is not the case and lastalign is used zero-initialized in the 
>> following code.
>>
>> On top, the following condition
>>    else if (!warn_if_not_aligned_p
>>            && TREE_CODE (decl) == FUNCTION_DECL
>>            && DECL_ALIGN (decl) > bitalign)
>>
>> seems to be fully covered by
>>      else if (TREE_CODE (decl) == FUNCTION_DECL
>>         && ((curalign = DECL_ALIGN (decl)) > bitalign
>>             || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
>>
>> and so is essentially dead. I therefore removed it as there does not 
>> seem to be a test anywhere for the error message ( "alignment for %q+D 
>> was previously specified as %d and may not be decreased") either.
> 
> The removal of the dead code looks good to me.  The change to
> "re-init lastalign" doesn't seem right.  When it's zero it means
> the conflict is between two attributes on the same declaration,
> in which case the note shouldn't be printed (it would just point
> to the same location as the warning).

Agreed.

> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index c1f652d1dc9..d132b6fd3b6 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree 
> name, tree args, int flags,
>         && ((curalign = DECL_ALIGN (decl)) > bitalign
>             || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
>      {
> +      /* Re-init lastalign in case we short-circuit the condition,
> +     i.e.  curalign > bitalign.  */
> +      lastalign = DECL_ALIGN (last_decl);
> +
>        /* Either a prior attribute on the same declaration or one
>       on a prior declaration of the same function specifies
>       stricter alignment than this attribute.  */
> 
> For the C/C++ FE changes:
> 
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index 3ea4708c507..2ea5051e9cd 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree 
> newtype, tree oldtype)
>        SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl));
>        DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
>      }
> +      else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl)
> +           && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl))
> +    DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
> 
> This should be the same as
> 
>   DECL_USER_ALIGN (newdecl) = 1;
> 
> so I would suggest to use that for clarity.

I'm not sure that's clearer, though it certainly is equivalent.  I'm 
happy either way.

The braces around this statement in the parallel C++ change are unnecessary.

I'm a little nervous about the front-end changes given that we were 
specifically not setting *_USER_ALIGN in this case before, but I'm not 
aware of any rationale for that, so I'm content to make the change and 
see if anything breaks.

Jason

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

Hi Martin and Jason,

>> The removal of the dead code looks good to me.  The change to
>> "re-init lastalign" doesn't seem right.  When it's zero it means
>> the conflict is between two attributes on the same declaration,
>> in which case the note shouldn't be printed (it would just point
>> to the same location as the warning).
> 
> Agreed.

Did I get it correctly that you refer to printing a note in e.g. the 
following case?

  inline int __attribute__ ((aligned (16), aligned (4)))
  finline_align (int);

I indeed missed this but it could be fixed by checking (on top of the patch)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 98c98944405..7349da73f14 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2324,7 +2324,7 @@ common_handle_aligned_attribute (tree *node, tree 
name, tree args, int flags,
        /* Either a prior attribute on the same declaration or one
          on a prior declaration of the same function specifies
          stricter alignment than this attribute.  */
-      bool note = lastalign != 0;
+      bool note = last_decl != decl && lastalign != 0;

As there wasn't any FAIL, I would add another test which checks for this.

I find the whole logic here a bit convoluted but when there is no real 
last_decl, then last_decl = decl.  A note would not be printed before 
the patch because we erroneously warned about the "conflict" of the 
function's default alignment (8) vs the requested alignment (4).

Regards
  Robin

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Martin Sebor]

On 5/25/21 4:38 AM, Robin Dapp wrote:
> Hi Martin and Jason,
> 
>>> The removal of the dead code looks good to me.  The change to
>>> "re-init lastalign" doesn't seem right.  When it's zero it means
>>> the conflict is between two attributes on the same declaration,
>>> in which case the note shouldn't be printed (it would just point
>>> to the same location as the warning).
>>
>> Agreed.
> 
> Did I get it correctly that you refer to printing a note in e.g. the 
> following case?
> 
>   inline int __attribute__ ((aligned (16), aligned (4)))
>   finline_align (int);

Yes, that's what I was referring to.

> 
> I indeed missed this but it could be fixed by checking (on top of the 
> patch)
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 98c98944405..7349da73f14 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -2324,7 +2324,7 @@ common_handle_aligned_attribute (tree *node, tree 
> name, tree args, int flags,
>         /* Either a prior attribute on the same declaration or one
>           on a prior declaration of the same function specifies
>           stricter alignment than this attribute.  */
> -      bool note = lastalign != 0;
> +      bool note = last_decl != decl && lastalign != 0;
> 
> As there wasn't any FAIL, I would add another test which checks for this.

That would be great, thank you!

Martin

> 
> I find the whole logic here a bit convoluted but when there is no real 
> last_decl, then last_decl = decl.  A note would not be printed before 
> the patch because we erroneously warned about the "conflict" of the 
> function's default alignment (8) vs the requested alignment (4).
> 
> Regards
>   Robin

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Jason Merrill]

On 5/25/21 11:15 AM, Martin Sebor wrote:
> On 5/25/21 4:38 AM, Robin Dapp wrote:
>> Hi Martin and Jason,
>>
>>>> The removal of the dead code looks good to me.  The change to
>>>> "re-init lastalign" doesn't seem right.  When it's zero it means
>>>> the conflict is between two attributes on the same declaration,
>>>> in which case the note shouldn't be printed (it would just point
>>>> to the same location as the warning).
>>>
>>> Agreed.
>>
>> Did I get it correctly that you refer to printing a note in e.g. the 
>> following case?
>>
>>   inline int __attribute__ ((aligned (16), aligned (4)))
>>   finline_align (int);
> 
> Yes, that's what I was referring to.
> 
>>
>> I indeed missed this but it could be fixed by checking (on top of the 
>> patch)
>>
>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>> index 98c98944405..7349da73f14 100644
>> --- a/gcc/c-family/c-attribs.c
>> +++ b/gcc/c-family/c-attribs.c
>> @@ -2324,7 +2324,7 @@ common_handle_aligned_attribute (tree *node, 
>> tree name, tree args, int flags,
>>         /* Either a prior attribute on the same declaration or one
>>           on a prior declaration of the same function specifies
>>           stricter alignment than this attribute.  */
>> -      bool note = lastalign != 0;
>> +      bool note = last_decl != decl && lastalign != 0;
>>
>> As there wasn't any FAIL, I would add another test which checks for this.
> 
> That would be great, thank you!
> 
>> I find the whole logic here a bit convoluted but when there is no real 
>> last_decl, then last_decl = decl.  A note would not be printed before 
>> the patch because we erroneously warned about the "conflict" of the 
>> function's default alignment (8) vs the requested alignment (4).

Ah, the problem is that we only give this warning because of 
DECL_USER_ALIGN on last_decl, but then don't use the alignment of last_decl.

As you say, the logic is convoluted.  Let's simplify it rather than make 
it more convoluted.  One possibility would be to change || to | to avoid 
the shortcut, and then

bool note = lastalign > curalign;
if (note)
   curalign = lastalign;

Jason

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

> As you say, the logic is convoluted.  Let's simplify it rather than make
> it more convoluted.  One possibility would be to change || to | to avoid
> the shortcut, and then
> 
> bool note = lastalign > curalign;
> if (note)
>     curalign = lastalign;

I went with your suggestion in the attached v2.  Regtested and 
bootstrapped on s390x, x86 and ppc64le.

Regards
  Robin

[-- Attachment #2: wattr-v2-p1.diff --]
[-- Type: text/x-patch, Size: 3326 bytes --]

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index ccf9e4ccf0b..f9d1c17f8ef 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2314,14 +2314,14 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags,
       *no_add_attrs = true;
     }
   else if (TREE_CODE (decl) == FUNCTION_DECL
-	   && ((curalign = DECL_ALIGN (decl)) > bitalign
-	       || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
+	   && (((curalign = DECL_ALIGN (decl)) > bitalign)
+	       | ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
     {
       /* Either a prior attribute on the same declaration or one
 	 on a prior declaration of the same function specifies
 	 stricter alignment than this attribute.  */
-      bool note = lastalign != 0;
-      if (lastalign)
+      bool note = lastalign > curalign;
+      if (note)
 	curalign = lastalign;
 
       curalign /= BITS_PER_UNIT;
@@ -2366,25 +2366,6 @@ common_handle_aligned_attribute (tree *node, tree name, tree args, int flags,
       This formally comes from the c++11 specification but we are
       doing it for the GNU attribute syntax as well.  */
     *no_add_attrs = true;
-  else if (!warn_if_not_aligned_p
-	   && TREE_CODE (decl) == FUNCTION_DECL
-	   && DECL_ALIGN (decl) > bitalign)
-    {
-      /* Don't warn for function alignment here if warn_if_not_aligned_p
-	 is true.  It will be warned about later.  */
-      if (DECL_USER_ALIGN (decl))
-	{
-	  /* Only reject attempts to relax/override an alignment
-	     explicitly specified previously and accept declarations
-	     that appear to relax the implicit function alignment for
-	     the target.  Both increasing and increasing the alignment
-	     set by -falign-functions setting is permitted.  */
-	  error ("alignment for %q+D was previously specified as %d "
-		 "and may not be decreased", decl,
-		 DECL_ALIGN (decl) / BITS_PER_UNIT);
-	  *no_add_attrs = true;
-	}
-    }
   else if (warn_if_not_aligned_p
 	   && TREE_CODE (decl) == FIELD_DECL
 	   && !DECL_C_BIT_FIELD (decl))
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 53b2b5b637d..40488585052 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype)
 	  SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl));
 	  DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
 	}
+      else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl)
+	       && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl))
+	DECL_USER_ALIGN (newdecl) = 1;
       if (DECL_WARN_IF_NOT_ALIGN (olddecl)
 	  > DECL_WARN_IF_NOT_ALIGN (newdecl))
 	SET_DECL_WARN_IF_NOT_ALIGN (newdecl,
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index e7268d5ad18..f774c75228d 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2794,6 +2794,10 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden)
       SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl));
       DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl);
     }
+  else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl)
+      && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl))
+    DECL_USER_ALIGN (newdecl) = 1;
+
   DECL_USER_ALIGN (olddecl) = DECL_USER_ALIGN (newdecl);
   if (DECL_WARN_IF_NOT_ALIGN (olddecl)
       > DECL_WARN_IF_NOT_ALIGN (newdecl))

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

[-- Attachment #1: Type: text/plain, Size: 149 bytes --]

This is the revised testsuite change with v2 adding a check for no 
separate note for

  __attribute__((aligned (32), aligned (4)).

Regards
  Robin

[-- Attachment #2: wattr-v2-p2.diff --]
[-- Type: text/x-patch, Size: 6522 bytes --]

diff --git a/gcc/testsuite/c-c++-common/Wattributes.c b/gcc/testsuite/c-c++-common/Wattributes.c
index 4ad90441b4d..a97e5ad5f74 100644
--- a/gcc/testsuite/c-c++-common/Wattributes.c
+++ b/gcc/testsuite/c-c++-common/Wattributes.c
@@ -97,6 +97,8 @@ fnoinline1 (void);       /* { dg-message "previous declaration here" } */
 /* Verify a warning for always_inline conflict.  */
 void ATTR ((always_inline))
 fnoinline1 (void) { }    /* { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." } */
+			 /* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */
+			 /* { dg-message "note: previous definition" "" { target *-*-* } .-2 } */
 
 /* Verify a warning for gnu_inline conflict.  */
 inline void ATTR ((gnu_inline))
@@ -364,13 +366,15 @@ inline int ATTR ((cold))
 finline_cold_noreturn (int);
 
 inline int ATTR ((noreturn))
-finline_cold_noreturn (int);
+finline_cold_noreturn (int);	/* { dg-message "note: previous declaration here" } */
 
 inline int ATTR ((noinline))
 finline_cold_noreturn (int);    /* { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." } */
+				/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */
 
 inline int ATTR ((hot))
 finline_cold_noreturn (int);    /* { dg-warning "ignoring attribute .hot. because it conflicts with attribute .cold." } */
+				/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */
 
 inline int ATTR ((warn_unused_result))
 finline_cold_noreturn (int);    /* { dg-warning "ignoring attribute .warn_unused_result. because it conflicts with attribute .noreturn." } */
@@ -389,23 +393,24 @@ finline_cold_noreturn (int i) { (void)&i; __builtin_abort (); }
    and some on distinct declarations.  */
 
 inline int ATTR ((always_inline, hot))
-finline_hot_noret_align (int);
+finline_hot_noret_align (int);	/* { dg-message "note: previous declaration here" } */
 
 inline int ATTR ((noreturn, noinline))
 finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." } */
+				/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */
 
 inline int ATTR ((cold, aligned (8)))
 finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .cold. because it conflicts with attribute .hot." } */
+				/* { dg-message "note: previous declaration here" "" { target *-*-* } .-1 } */
 
 inline int ATTR ((warn_unused_result))
 finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .warn_unused_result. because it conflicts with attribute .noreturn." } */
 
 inline int ATTR ((aligned (4)))
-  finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." "" { target { ! { hppa*64*-*-* s390*-*-* } } } } */
-/* { dg-error "alignment for '.*finline_hot_noret_align.*' must be at least 8" "" { target s390*-*-* } .-1 } */
+  finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." "" { target { ! { hppa*64*-*-* } } } } */
 
 inline int ATTR ((aligned (8)))
-finline_hot_noret_align (int);
+finline_hot_noret_align (int);  /* { dg-message "note: previous declaration here" } */
 
 inline int ATTR ((const))
 finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .const. because it conflicts with attribute .noreturn." } */
@@ -416,6 +421,26 @@ inline int ATTR ((noreturn))
 finline_hot_noret_align (int i) { (void)&i; __builtin_abort (); }
 
 
+/* Expect a warning about conflicting alignment but without
+   other declarations inbetween.  */
+inline int ATTR ((aligned (32)))
+finline_align (int);	        /* { dg-message "note: previous declaration here" } */
+
+inline int ATTR ((aligned (4)))
+finline_align (int);  /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */
+
+inline int ATTR ((noreturn))
+finline_align (int i) { (void)&i; __builtin_abort (); }
+
+
+/* Expect no note that would refer to the same declaration.  */
+inline int ATTR ((aligned (32), aligned (4)))
+finline_double_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." } */
+
+inline int ATTR ((noreturn))
+finline_double_align (int i) { (void)&i; __builtin_abort (); }
+
+
 /* Exercise variable attributes.  */
 
 extern int ATTR ((common))
diff --git a/gcc/testsuite/gcc.dg/Wattributes-6.c b/gcc/testsuite/gcc.dg/Wattributes-6.c
index 4ba59bf2806..c6b2225943d 100644
--- a/gcc/testsuite/gcc.dg/Wattributes-6.c
+++ b/gcc/testsuite/gcc.dg/Wattributes-6.c
@@ -401,8 +401,7 @@ inline int ATTR ((warn_unused_result))
 finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .warn_unused_result. because it conflicts with attribute .noreturn." } */
 
 inline int ATTR ((aligned (4)))
-  finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." "" { target { ! { hppa*64*-*-* s390*-*-* } } } } */
-/* { dg-error "alignment for 'finline_hot_noret_align' must be at least 8" "" { target s390*-*-* } .-1 } */
+  finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." "" { target { ! { hppa*64*-*-* } } } } */
 
 inline int ATTR ((aligned (8)))
 finline_hot_noret_align (int);
@@ -416,6 +415,26 @@ inline int ATTR ((noreturn))
 finline_hot_noret_align (int i) { (void)&i; __builtin_abort (); }
 
 
+/* Expect a warning about conflicting alignment but without
+   other declarations inbetween.  */
+inline int ATTR ((aligned (32)))
+finline_align (int);
+
+inline int ATTR ((aligned (4)))
+  finline_align (int);  /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */
+
+inline int ATTR ((noreturn))
+finline_align (int i) { (void)&i; __builtin_abort (); }
+
+
+/* Expect no note that would refer to the same declaration.  */
+inline int ATTR ((aligned (32), aligned (4)))
+finline_double_align (int); /* { dg-warning "ignoring attribute .aligned \\(4\\). because it conflicts with attribute .aligned \\(32\\)." } */
+
+inline int ATTR ((noreturn))
+finline_double_align (int i) { (void)&i; __builtin_abort (); }
+
+
 /* Exercise variable attributes.  */
 
 extern int ATTR ((common))

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Jason Merrill]

On 6/1/21 9:20 AM, Robin Dapp wrote:
>> As you say, the logic is convoluted.  Let's simplify it rather than make
>> it more convoluted.  One possibility would be to change || to | to avoid
>> the shortcut, and then
>>
>> bool note = lastalign > curalign;
>> if (note)
>>     curalign = lastalign;
> 
> I went with your suggestion in the attached v2.  Regtested and 
> bootstrapped on s390x, x86 and ppc64le.

OK.

Jason

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Robin Dapp]

>>> As you say, the logic is convoluted.  Let's simplify it rather than make
>>> it more convoluted.  One possibility would be to change || to | to avoid
>>> the shortcut, and then
>>>
>>> bool note = lastalign > curalign;
>>> if (note)
>>>      curalign = lastalign;
>>
>> I went with your suggestion in the attached v2.  Regtested and
>> bootstrapped on s390x, x86 and ppc64le.
> 
> OK.

urg, I did not commit yet because I fiddled around with my tests again. 
Using dg-notes checks (and thus being "forced" to handle all cases 
individually) I realized that the above is not enough as
it reintroduces the same problem that I was originally trying to avoid:

We want to warn if lastalign == curalign but DECL_USER_ALIGN (last_decl) 
!= DECL_USER_ALIGN (decl), i.e. when the user explicitly specified 
__attribute__ ((aligned (8))) on s390 (see example in my first mail).

What about checking

  lastalign > curalign || (lastalign == curalign && DECL_USER_ALIGN 
(last_decl) != DECL_USER_ALIGN (decl))

instead and changing the associated comment?

Even then it's not really satisfactory to emit a note that complains
about "aligned (8)" at a declaration where it was not even explicitly
specified.  But then, the situation is similar for other attributes as 
well and we would need to explicitly store the location where an 
attribute was specified first.

Regards
  Robin

Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar.

[Jason Merrill]

On 6/9/21 4:47 AM, Robin Dapp wrote:
>>>> As you say, the logic is convoluted.  Let's simplify it rather than 
>>>> make
>>>> it more convoluted.  One possibility would be to change || to | toavoid
>>>> the shortcut, and then
>>>>
>>>> bool note = lastalign > curalign;
>>>> if (note)
>>>>      curalign = lastalign;
>>>
>>> I went with your suggestion in the attached v2.  Regtested and
>>> bootstrapped on s390x, x86 and ppc64le.
>>
>> OK.
> 
> urg, I did not commit yet because I fiddled around with my tests again. 
> Using dg-notes checks (and thus being "forced" to handle all cases 
> individually) I realized that the above is not enough as
> it reintroduces the same problem that I was originally trying to avoid:
> 
> We want to warn if lastalign == curalign but DECL_USER_ALIGN (last_decl) 
> != DECL_USER_ALIGN (decl), i.e. when the user explicitly specified 
> __attribute__ ((aligned (8))) on s390 (see example in my first mail).
> 
> What about checking
> 
> lastalign > curalign || (lastalign == curalign && DECL_USER_ALIGN 
> (last_decl) != DECL_USER_ALIGN (decl))
> 
> instead and changing the associated comment?

I might use > instead of !=, but sure.

> Even then it's not really satisfactory to emit a note that complains
> about "aligned (8)" at a declaration where it was not even explicitly
> specified.  But then, the situation is similar for other attributes as 
> well and we would need to explicitly store the location where an 
> attribute was specified first.