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