On Thu, Jun 8, 2017 at 12:13 PM, Martin Sebor <mse...@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.to...@gmail.com> wrote:
>>>
>>> On Tue, Jun 6, 2017 at 5:11 PM, Martin Sebor <mse...@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 <mse...@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 <mse...@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
>>>>>>>>>> <jos...@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?
>
>> 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.
>>
>>
>
--
H.J.
