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