On Wed, Jun 7, 2017 at 6:30 AM, H.J. Lu wrote: > On Tue, Jun 6, 2017 at 5:11 PM, Martin Sebor wrote: >> On 06/06/2017 04:57 PM, H.J. Lu wrote: >>> >>> On Tue, Jun 6, 2017 at 10:34 AM, Martin Sebor wrote: >>>> >>>> On 06/06/2017 10:59 AM, H.J. Lu wrote: >>>>> >>>>> >>>>> On Tue, Jun 6, 2017 at 9:10 AM, Martin Sebor 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 >>>>>>>> >>>>>>>> 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. > > 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.