From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10034 invoked by alias); 8 Jun 2017 19:13:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 9654 invoked by uid 89); 8 Jun 2017 19:13:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=up X-HELO: mail-qt0-f173.google.com Received: from mail-qt0-f173.google.com (HELO mail-qt0-f173.google.com) (209.85.216.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 08 Jun 2017 19:13:16 +0000 Received: by mail-qt0-f173.google.com with SMTP id u12so52974178qth.0 for ; Thu, 08 Jun 2017 12:13:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=PvYZZNTSl73ugu73SNcbyL7x4TGe+DEnl+WN6CbOjpY=; b=mOpC+RDVLUqLEadjXDLxY+u93pKcdczBpQzzVEWkUdfV5BhqZZ8apXUrDySlPaPb/w a5l/iHlvDE0UH3yrNAnn7z0F/Jv1RyZLKF1+CuMpcUR2mfOU1L4RaKMri6EbUNZJPQKp 6LM5uI4Qq6R60RMVoEXqov/VvgqLcm2R6ewDMIBF4H7SoGO6AC+A/6a0QJbi+jO0RWP4 28StUDD39Q62kuXmo2vI/2c1c1wCPqaP32LxM9WVuka9UbR4md3bJ85vetHjThSzS9iM AOU1yimJme+w1HtE93SF7J/GE6WCZzNmwVIgKv0esmu9vhqX/DQl6BbI2QhgLYF8mp6+ sJtg== X-Gm-Message-State: AKS2vOyOM9mUhdVuNTZAprdNdoBvTvYoqex2gSKd9lX6lesiSpjUIGh7 Ox380ylTU3onVIdL X-Received: by 10.55.64.133 with SMTP id n127mr42640870qka.145.1496949198887; Thu, 08 Jun 2017 12:13:18 -0700 (PDT) Received: from localhost.localdomain (71-218-22-76.hlrn.qwest.net. [71.218.22.76]) by smtp.gmail.com with ESMTPSA id t13sm3764976qtc.22.2017.06.08.12.13.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 08 Jun 2017 12:13:18 -0700 (PDT) Subject: Re: RFC: [PATCH] Add warn_if_not_aligned attribute To: "H.J. Lu" References: <20170604155212.GA8692@gmail.com> <2f2c702d-91e0-edc5-de77-04957b33969e@gmail.com> <6c6c268d-a40b-cfa9-574c-ad235f9205b7@gmail.com> Cc: Joseph Myers , GCC Patches From: Martin Sebor Message-ID: <4a4ac20f-9c27-4844-55dc-38ff56ac138b@gmail.com> Date: Thu, 08 Jun 2017 19:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00554.txt.bz2 On 06/08/2017 11:00 AM, H.J. Lu wrote: > 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. 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. > >