[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