Re: RFC: [PATCH] Add warn_if_not_aligned attribute

[Martin Sebor <msebor@gmail.com>]

On 06/06/2017 04:57 PM, H.J. Lu wrote:
> On Tue, Jun 6, 2017 at 10:34 AM, Martin Sebor <msebor@gmail.com> wrote:
>> On 06/06/2017 10:59 AM, H.J. Lu wrote:
>>>
>>> On Tue, Jun 6, 2017 at 9:10 AM, Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> On 06/06/2017 10:07 AM, Martin Sebor wrote:
>>>>>
>>>>>
>>>>> On 06/05/2017 11:45 AM, H.J. Lu wrote:
>>>>>>
>>>>>>
>>>>>> On Mon, Jun 5, 2017 at 8:11 AM, Joseph Myers <joseph@codesourcery.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> The new attribute needs documentation.  Should the test be in
>>>>>>> c-c++-common
>>>>>>
>>>>>>
>>>>>>
>>>>>> This feature does support C++.  But C++ compiler issues a slightly
>>>>>> different warning at a different location.
>>>>>>
>>>>>>> or does this feature not support C++?
>>>>>>>
>>>>>>
>>>>>> Here is the updated patch with documentation and a C++ test.  This
>>>>>> patch caused a few testsuite failures:
>>>>>>
>>>>>> FAIL: gcc.dg/compat/struct-align-1 c_compat_x_tst.o compile
>>>>>>
>>>>>>
>>>>>>
>>>>>> /export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/compat//struct-align-1.h:169:1:
>>>>>>
>>>>>> warning: alignment 1 of 'struct B2_m_inner_p_outer' is less than 16
>>>>>>
>>>>>> FAIL: g++.dg/torture/pr80334.C   -O0  (test for excess errors)
>>>>>>
>>>>>>
>>>>>>
>>>>>> /export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/torture/pr80334.C:4:8:
>>>>>>
>>>>>> warning: alignment 1 of 'B' is less than 16
>>>>>>
>>>>>
>>>>> Users often want the ability to control a warning, even when it
>>>>> certainly indicates a bug.  I would suggest to add an option to
>>>>> make it possible for this warning as well.
>>>>>
>>>>> Btw., a bug related to some of those this warning is meant to
>>>>> detect is assigning the address of an underaligned object to
>>>>> a pointer of a natively aligned type.  Clang has an option
>>>>> to detect this problem: -Waddress-of-packed-member.  It might
>>>>> make a nice follow-on enhancement to add support for the same
>>>>> thing.  I mention this because I think it would make sense to
>>>>> consider this when choosing the name of the GCC option (i.e.,
>>>>> rather than having two distinct but closely related warnings,
>>>>> have one that detects both of these alignment type of bugs.
>>>>
>>>>
>>>>
>>>> A bug that has some additional context on this is pr 51628.
>>>> A possible name for the new option suggested there is -Wpacked.
>>>>
>>>> Martin
>>>
>>>
>>> Isn't -Waddress-of-packed-member a subset of or the same as
>>> -Wpacked?
>>
>>
>> In Clang it's neither.  -Waddress-of-packed-member only triggers
>> when the address of a packed member is taken but not for the cases
>> in bug 53037 (i.e., reducing the alignment of a member).  It's
>> also enabled by default, while -Wpacked needs to be specified
>> explicitly (i.e., it's in neither -Wall or -Wextra).
>>
>> FWIW, I don't really have a strong opinion about the names of
>> the options.  My input is that the proliferation of fine-grained
>> warning options for closely related problems tends to make it
>> tricky to get their interactions right (both in the compiler
>> and for users).  Enabling both/all such options can lead to
>> multiple warnings for what boils down to essentially the same
>> bug in the same expression, overwhelming the user in repetitive
>> diagnostics.
>>
>
> There is already -Wpacked.  Should I overload it for this?

I'd say yes if -Wpacked were at least in -Wall.  But it's
an opt-in kind of warning that's not even in -Wextra, and
relaxing an explicitly specified alignment seems more like
a bug than just something that might be nice to know about.
I would expect warn_if_not_aligned to trigger a warning even
without -Wall (i.e., as you have it in your patch, but with
an option to control it).  That would suggest three levels
of warnings:

1) warn by default (warn_if_not_aligned violation)
2) warn with -Wall (using a type with attribute aligned to
    define a member of a packed struct)
3) warn if requested (current -Wpacked)

So one way to deal with it would be to change -Wpacked to
take an argument between 0 and 3, set the default to
correspond to the (1) above, and have -Wall bump it up to
(2).

If the equivalent of -Waddress-of-packed-member were to be
implemented in GCC it would probably be a candidate to add
to the (2) above.(*)

This might be more involved than you envisioned.  A slightly
simpler alternative would be to add a different option, say
something like -Walign=N, and have it handle just (1) and
(2) above, leaving -Wpacked unchanged.

Martin

PS [*] On a related note, in the Clang discussion of
-Waddress-of-packed-member they briefly considered reusing
-Wcast-align for the same purpose, but decided against it
because it apparently involves an explicit cast.  That
doesn't seem to me like a string enough argument not to
change -Wcast-align to trigger on implicit conversions that
increase alignment.  (The option is essentially useless on
most targets so this extension would make it generally
useful.)