RFC: [PATCH] Add warn_if_not_aligned attribute

[Martin Sebor]

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

> 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.
>
>