This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: RFC: [PATCH] Add warn_if_not_aligned attribute
On Tue, Jun 6, 2017 at 5:11 PM, Martin Sebor <msebor@gmail.com> wrote:
> 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.
>
Since there is no agreement on -W options and changes
may touch many places, I will do
1. Add -Wwarn-if-not-aligned and enable it by default.
2. Add -Wpacked-not-aligned and disable it by default.
Once there is an agreement, I replace -Wpacked-not-aligned
with the new option.
--
H.J.