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
