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

Reply via email to