Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Aug 25, 2017 at 2:38 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 08/25/2017 03:05 PM, H.J. Lu wrote:
>>
>> On Fri, Aug 25, 2017 at 1:14 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 08/25/2017 01:35 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Fri, Aug 25, 2017 at 12:24 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On 08/25/2017 11:31 AM, H.J. Lu wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor <msebor@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 08/21/2017 06:21 AM, H.J. Lu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> When warn_if_not_aligned_p is true, a warning will be issued on
>>>>>>>> function
>>>>>>>> declaration later.  There is no need to warn function alignment when
>>>>>>>> warn_if_not_aligned_p is true.
>>>>>>>>
>>>>>>>> OK for trunk?
>>>>>>>>
>>>>>>>> H.J.
>>>>>>>> --
>>>>>>>>         * c-attribs.c (common_handle_aligned_attribute): Don't warn
>>>>>>>>         function alignment if warn_if_not_aligned_p is true.
>>>>>>>> ---
>>>>>>>>  gcc/c-family/c-attribs.c | 5 ++++-
>>>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>>>>>>>> index 5f79468407f..78969532543 100644
>>>>>>>> --- a/gcc/c-family/c-attribs.c
>>>>>>>> +++ b/gcc/c-family/c-attribs.c
>>>>>>>> @@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node,
>>>>>>>> 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 (TREE_CODE (decl) == FUNCTION_DECL
>>>>>>>> +  else if (!warn_if_not_aligned_p
>>>>>>>> +          && TREE_CODE (decl) == FUNCTION_DECL
>>>>>>>>            && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
>>>>>>>>      {
>>>>>>>> +      /* Don't warn function alignment here if
>>>>>>>> warn_if_not_aligned_p
>>>>>>>> is
>>>>>>>> +        true.  It will be warned later.  */
>>>>>>>>        if (DECL_USER_ALIGN (decl))
>>>>>>>>         error ("alignment for %q+D was previously specified as %d "
>>>>>>>>                "and may not be decreased", decl,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Your comment refers to warning but the code here uses error().
>>>>>>> That raises two questions for me: a) will the later diagnostic
>>>>>>> really be a warning or an error, and if a warning, under what
>>>>>>> option will it be issued? and b) why is an error appropriate
>>>>>>> here when a warning is appropriate elsewhere (most other
>>>>>>> attribute conflicts are at present diagnosed with -Wattributes).
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> It can be changed to warning.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Okay, that's goo to hear (it validates what I did in the patch
>>>>> I referenced).
>>>>>
>>>>> As for your patch, I don't quite see ho the block can ever be
>>>>> entered.  In fact, I had to make changes to the function in
>>>>> my patch below to let it detect and diagnose the condition
>>>>> (attempting to reduce the alignment of a function).  The
>>>>> details are in bug 81566.  Did I miss something?
>>>>
>>>>
>>>>
>>>> Try this:
>>>>
>>>> __attribute__((warn_if_not_aligned(8)))
>>>> void
>>>> foo2 (void)
>>>> {
>>>> }
>>>
>>>
>>>
>>> For me the top of trunk prints the same error both with and
>>> without your patch:
>>>
>>>   error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’
>>
>>
>> Please try it with Linux/alpha cross compiler.
>
>
> I get the same error (both with and without your patch). Do
> I need to be using some specific options?  (-Wif-not-aligned
> makes no difference.)

Ooops.  It is ia64-linux:

[hjl@gnu-tools-1 gcc]$ cat /tmp/x.i
__attribute__((warn_if_not_aligned(8)))
void
foo2 (void)
{
}
[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ /tmp/x.i -S -O2
/tmp/x.i:3:1: error: alignment for ‘foo2’ must be at least 16
 foo2 (void)
 ^~~~
/tmp/x.i:3:1: error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’
[hjl@gnu-tools-1 gcc]$ cat x.i
__attribute__((aligned(32)))
void
foo2 (void)
{
}
[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -O2 x.i -S
[hjl@gnu-tools-1 gcc]$ cat x.s
.file "x.i"
.pred.safe_across_calls p1-p5,p16-p63
.text
.align 32 <<<<<<<<<<<<<< Aligned at 32 bytes.
.global foo2#
.type foo2#, @function
.proc foo2#
foo2:
.prologue
.body
.mib
nop 0
nop 0
br.ret.sptk.many b0
.endp foo2#
.ident "GCC: (GNU) 8.0.0 20170825 (experimental)"
[hjl@gnu-tools-1 gcc]$

> I'm also not sure I understand how the target affects this
> case (is there supposed to be some other attribute on the
> declaration above? attribute aligned).  I thought the
> handling of function declarations with the warn_if_not_aligned
> attribute was target independent.
>
> In any case, I think it would be easier if the patch included
> a test showing what the problem is and how the change prevents
> it.
>
> Martin



-- 
H.J.