From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71083 invoked by alias); 7 Jun 2017 00:11:13 -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 69809 invoked by uid 89); 7 Jun 2017 00:11:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=briefly X-HELO: mail-qk0-f175.google.com Received: from mail-qk0-f175.google.com (HELO mail-qk0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Jun 2017 00:11:09 +0000 Received: by mail-qk0-f175.google.com with SMTP id p66so72282797qkf.3 for ; Tue, 06 Jun 2017 17:11:13 -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=pygGrqdHNPQB+qOgDDpY06NljmYII7YoHtVgnTw8+pQ=; b=NqvogdCka1hBUe/NiYOIDqNliC4ow15ctkjZjZ1EI+1+O3OLIQn2i7cI0hMgNUilqQ 3GLIBc8IZXQozNNPqspGHZqvm+f+z5RueZBcg0S9fvmKOt/ZZv8a597IzK0ytxN7xPtF DPNzq0V3eYrYywVYPPIXe0XGTFREXMtoKCxZ6zJ0gxHhglhpGQ1TOsZcU+JVj8gPGhCB MmJ0lbStlnmH0i36TCrnLVA16yhhmHvdmT9fcTqHfoKwbmwxJ2Cn8ilBxCdrabqpNEQs Ffy6DfDq1kG06BRstuKC4tWXEeTSjJVLOwpHdK6UpSjLw+AynOsumss46W63T45zSKnK wUGA== X-Gm-Message-State: AKS2vOybULTv5vObJME03fcBIHOQ9NCtNG9UJJm2wU9gVZR4+FLFktlp VCwyyE+ZpHDfmRdq X-Received: by 10.55.24.11 with SMTP id j11mr13938804qkh.209.1496794271338; Tue, 06 Jun 2017 17:11:11 -0700 (PDT) Received: from localhost.localdomain (71-218-22-76.hlrn.qwest.net. [71.218.22.76]) by smtp.gmail.com with ESMTPSA id u9sm1701qtb.29.2017.06.06.17.11.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 06 Jun 2017 17:11:10 -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> Cc: Joseph Myers , GCC Patches From: Martin Sebor Message-ID: <6c6c268d-a40b-cfa9-574c-ad235f9205b7@gmail.com> Date: Wed, 07 Jun 2017 00:11: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: 7bit X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00385.txt.bz2 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. 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.)