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

[Jason Merrill <jason@redhat.com>]

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