Re: RFC: [PATCH] Add warn_if_not_aligned attribute

[Martin Sebor <msebor@gmail.com>]

On 06/09/2017 07:31 AM, H.J. Lu wrote:
> On Thu, Jun 8, 2017 at 12:13 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 06/08/2017 11:00 AM, H.J. Lu wrote:
>>>
>>> On Wed, Jun 7, 2017 at 6:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> 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.
>>
>>
>> Okay.  I can't approve the patch but thank you for enhancing your
>> patch to handle the additional case I brought up!
>>
>> Martin
>
> Where do we go from here?

Other than the C and C++ maintainers needing to approve the patch
I can't think of anything else.

If you think it's worthwhile to merge the two options into one
(or perhaps even incorporate them into -Wpacked) that can be done
afterwards.

Martin

>
>>
>>> Here is the updated patch.
>>>
>>> Add warn_if_not_aligned attribute as well as  command line options:
>>> -Wif-not-aligned and -Wpacked-not-aligned.
>>>
>>> __attribute__((warn_if_not_aligned(N))) causes compiler to issue a
>>> warning if the field in a struct or union is not aligned to N:
>>>
>>> typedef unsigned long long __u64
>>>   __attribute__((aligned(4),warn_if_not_aligned(8)));
>>>
>>> struct foo
>>> {
>>>   int i1;
>>>   int i2;
>>>   __u64 x;
>>> };
>>>
>>> __u64 is aligned to 4 bytes.  But inside struct foo, __u64 should be
>>> aligned at 8 bytes.  It is used to define struct foo in such a way that
>>> struct foo has the same layout and x has the same alignment when __u64
>>> is aligned at either 4 or 8 bytes.
>>>
>>> Since struct foo is normally aligned to 4 bytes, a warning will be issued:
>>>
>>> warning: alignment 4 of ‘struct foo’ is less than 8
>>>
>>> Align struct foo to 8 bytes:
>>>
>>> struct foo
>>> {
>>>   int i1;
>>>   int i2;
>>>   __u64 x;
>>> } __attribute__((aligned(8)));
>>>
>>> silences the warning.  It also warns the field with misaligned offset:
>>>
>>> struct foo
>>> {
>>>   int i1;
>>>   int i2;
>>>   int i3;
>>>   __u64 x;
>>> } __attribute__((aligned(8)));
>>>
>>> warning: ‘x’ offset 12 in ‘struct foo’ isn't aligned to 8
>>>
>>> This warning is controlled by -Wif-not-aligned and is enabled by default.
>>>
>>> When -Wpacked-not-aligned is used, the same warning is also issued for
>>> the field with explicitly specified alignment in a packed struct or union:
>>>
>>> struct __attribute__ ((aligned (8))) S8 { char a[8]; };
>>> struct __attribute__ ((packed)) S {
>>>   struct S8 s8;
>>> };
>>>
>>> warning: alignment 1 of ‘struct S’ is less than 8
>>>
>>> This warning is disabled by default and enabled by -Wall.
>>>
>>> gcc/
>>>
>>> PR c/53037
>>> * print-tree.c (print_node): Support DECL_WARN_IF_NOT_ALIGN
>>> and TYPE_WARN_IF_NOT_ALIGN.
>>> * stor-layout.c (do_type_align): Merge DECL_WARN_IF_NOT_ALIGN.
>>> (handle_warn_if_not_align): New.
>>> (place_union_field): Call handle_warn_if_not_align.
>>> (place_field): Call handle_warn_if_not_align.  Copy
>>> TYPE_WARN_IF_NOT_ALIGN.
>>> (finish_builtin_struct): Copy TYPE_WARN_IF_NOT_ALIGN.
>>> (layout_type): Likewise.
>>> * tree-core.h (tree_type_common): Add warn_if_not_align.  Set
>>> spare to 18.
>>> (tree_decl_common): Add warn_if_not_align.
>>> * tree.c (build_range_type_1): Likewise.
>>> * tree.h (TYPE_WARN_IF_NOT_ALIGN): New.
>>> (SET_TYPE_WARN_IF_NOT_ALIGN): Likewise.
>>> (DECL_WARN_IF_NOT_ALIGN): Likewise.
>>> (SET_DECL_WARN_IF_NOT_ALIGN): Likewise.
>>> * doc/extend.texi: Document warn_if_not_aligned attribute.
>>> * doc/invoke.texi: Document -Wif-not-aligned and
>>> -Wpacked-not-aligned.
>>>
>>> gcc/c-family/
>>>
>>> PR c/53037
>>> * c-attribs.c (handle_warn_if_not_aligned_attribute): New.
>>> (c_common_attribute_table): Add warn_if_not_aligned.
>>> (handle_aligned_attribute): Renamed to ...
>>> (common_handle_aligned_attribute): Remove argument, name, and add
>>> argument, warn_if_not_aligned.  Handle warn_if_not_aligned.
>>> (handle_aligned_attribute): New.
>>> * c.opt: Add -Wif-not-aligned and -Wpacked-not-aligned.
>>>
>>> gcc/c/
>>>
>>> PR c/53037
>>> * c-decl.c (merge_decls): Also merge TYPE_WARN_IF_NOT_ALIGN.
>>>
>>> gcc/cp/
>>>
>>> PR c/53037
>>> * decl.c (duplicate_decls): Also merge TYPE_WARN_IF_NOT_ALIGN.
>>>
>>> gcc/testsuite/
>>>
>>> PR c/53037
>>> * c-c++-common/pr53037-4.c: New test.
>>> * g++.dg/pr53037-1.C: Likewise.
>>> * g++.dg/pr53037-2.C: Likewise.
>>> * g++.dg/pr53037-3.C: Likewise.
>>> * gcc.dg/pr53037-1.c: Likewise.
>>> * gcc.dg/pr53037-2.c: Likewise.
>>> * gcc.dg/pr53037-3.c: Likewise.
>>>
>>>
>>
>
>
>