Re: RFC: [PATCH] Add warn_if_not_aligned attribute

[Martin Sebor <msebor at gmail dot com>]

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.

Martin

PS [*] On a related note, in the Clang discussion of
-Waddress-of-packed-member they briefly considered reusing
-Wcast-align for the same purpose, but decided against it
because it apparently involves an explicit cast.  That
doesn't seem to me like a string enough argument not to
change -Wcast-align to trigger on implicit conversions that
increase alignment.  (The option is essentially useless on
most targets so this extension would make it generally
useful.)