From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100942 invoked by alias); 17 Dec 2018 13:34:56 -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 100926 invoked by uid 89); 17 Dec 2018 13:34:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=xx X-HELO: mail-lf1-f43.google.com Received: from mail-lf1-f43.google.com (HELO mail-lf1-f43.google.com) (209.85.167.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Dec 2018 13:34:53 +0000 Received: by mail-lf1-f43.google.com with SMTP id c16so9448741lfj.8 for ; Mon, 17 Dec 2018 05:34:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=M0nhCUXor0wMr6fGQ96qHz4ThNz/I6ELaFvrw/PsZvs=; b=Mg4YcqlW7dOgdzFzyreEW4aYI0TZegN721nCLtKbkqp/trHQzMI0xThSL3PRUWYs5J adhup8w4tnZHxklf6ulx9EsPtqi4e0IwUktqu5SzVGwJoD4l5orPEj2I6Zjukwuw0MwN uiXYRdcwi58ooUyznNh3nDDCDDNkFcRbbcQlcFDn+kUxysz38HKzpS5TwIKcpsfW6TgQ 3gWc0DsrfTIbBJK1LmHofAwtPVVdBnevnvN3/E9dbgMDoAB45mRa+37Ux9RqjWgd21RC Q6JuyQ0zyK9LKqu/XZ80hbm765UeYK89s4JKSdnzYDqNxcHo6UhEqYWrfVCbRr7HiCIN wL3g== MIME-Version: 1.0 References: <24f8a201-bb3e-3acb-b679-570a8796358d@redhat.com> In-Reply-To: From: Richard Biener Date: Mon, 17 Dec 2018 13:34:00 -0000 Message-ID: Subject: Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member To: "H. J. Lu" Cc: Jason Merrill , "Joseph S. Myers" , Martin Sebor , GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg01211.txt.bz2 On Mon, Dec 17, 2018 at 1:43 PM H.J. Lu wrote: > > On Mon, Dec 17, 2018 at 1:39 AM Richard Biener > wrote: > > > > On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote: > > > > > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrot= e: > > > > > > > > On 12/13/18 6:56 PM, H.J. Lu wrote: > > > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill = wrote: > > > > >> > > > > >> On 9/25/18 11:46 AM, H.J. Lu wrote: > > > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote: > > > > >>>> On 07/23/2018 05:24 PM, H.J. Lu wrote: > > > > >>>>> > > > > >>>>> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers > > > > >>>>> wrote: > > > > >>>>>> > > > > >>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote: > > > > >>>>>> > > > > >>>>>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers > > > > >>>>>>> wrote: > > > > >>>>>>>> > > > > >>>>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote: > > > > >>>>>>>> > > > > >>>>>>>>>> + if (TREE_CODE (rhs) =3D=3D COND_EXPR) > > > > >>>>>>>>>> + { > > > > >>>>>>>>>> + /* Check the THEN path first. */ > > > > >>>>>>>>>> + tree op1 =3D TREE_OPERAND (rhs, 1); > > > > >>>>>>>>>> + context =3D check_address_of_packed_member (type,= op1); > > > > >>>>>>>>> > > > > >>>>>>>>> > > > > >>>>>>>>> This should handle the GNU extension of re-using operand = 0 if operand > > > > >>>>>>>>> 1 is omitted. > > > > >>>>>>>> > > > > >>>>>>>> > > > > >>>>>>>> Doesn't that just use a SAVE_EXPR? > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> Hmm, I suppose it does, but many places in the compiler see= m to expect > > > > >>>>>>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TR= EE. > > > > >>>>>> > > > > >>>>>> > > > > >>>>>> Maybe that's used somewhere inside the C++ front end. For C= a SAVE_EXPR > > > > >>>>>> is produced directly. > > > > >>>>> > > > > >>>>> > > > > >>>>> Here is the updated patch. Changes from the last one: > > > > >>>>> > > > > >>>>> 1. Handle COMPOUND_EXPR. > > > > >>>>> 2. Fixed typos in comments. > > > > >>>>> 3. Combined warn_for_pointer_of_packed_member and > > > > >>>>> warn_for_address_of_packed_member into > > > > >>>>> warn_for_address_or_pointer_of_packed_member. > > > > >>>> > > > > >>>> > > > > >>>>> c.i:4:33: warning: converting a packed =E2=80=98struct C *=E2= =80=99 pointer increases the > > > > >>>>> alignment of =E2=80=98long int *=E2=80=99 pointer from 1 to 8= [-Waddress-of-packed-member] > > > > >>>> > > > > >>>> > > > > >>>> I think this would read better as > > > > >>>> > > > > >>>> c.i:4:33: warning: converting a packed =E2=80=98struct C *=E2= =80=99 pointer (alignment 1) to > > > > >>>> =E2=80=98long int *=E2=80=99 (alignment 8) may result in an un= aligned pointer value > > > > >>>> [-Waddress-of-packed-member] > > > > >>> > > > > >>> Fixed. > > > > >>> > > > > >>>>> + while (TREE_CODE (base) =3D=3D ARRAY_REF) > > > > >>>>> + base =3D TREE_OPERAND (base, 0); > > > > >>>>> + if (TREE_CODE (base) !=3D COMPONENT_REF) > > > > >>>>> + return NULL_TREE; > > > > >>>> > > > > >>>> > > > > >>>> Are you deliberately not handling the other handled_component_= p cases? If > > > > >>>> so, there should be a comment. > > > > >>> > > > > >>> I changed it to > > > > >>> > > > > >>> while (handled_component_p (base)) > > > > >>> { > > > > >>> enum tree_code code =3D TREE_CODE (base); > > > > >>> if (code =3D=3D COMPONENT_REF) > > > > >>> break; > > > > >>> switch (code) > > > > >>> { > > > > >>> case ARRAY_REF: > > > > >>> base =3D TREE_OPERAND (base, 0); > > > > >>> break; > > > > >>> default: > > > > >>> /* FIXME: Can it ever happen? */ > > > > >>> gcc_unreachable (); > > > > >>> break; > > > > >>> } > > > > >>> } > > > > >>> > > > > >>> Is there a testcase to trigger this ICE? I couldn't find one. > > > > >> > > > > >> You can take the address of an element of complex: > > > > >> > > > > >> __complex int i; > > > > >> int *p =3D &__real(i); > > > > >> > > > > >> You may get VIEW_CONVERT_EXPR with location wrappers. > > > > > > > > > > Fixed. I replaced gcc_unreachable with return NULL_TREE; > > > > > > > > Then we're back to my earlier question: are you deliberately not > > > > handling the other cases? Why not look through them as well? What= if > > > > e.g. the operand of __real is a packed field? > > > > > > > > > > Here is the updated patch with > > > > > > diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c > > > index 615134cfdac..f105742598e 100644 > > > --- a/gcc/c-family/c-warn.c > > > +++ b/gcc/c-family/c-warn.c > > > @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree= rhs) > > > switch (code) > > > { > > > case ARRAY_REF: > > > + case REALPART_EXPR: > > > + case IMAGPART_EXPR: > > > + case VIEW_CONVERT_EXPR: > > > base =3D TREE_OPERAND (base, 0); > > > break; > > > default: > > > > don't we have handled_component_p () for this? (you're still > > missing BIT_FIELD_REF which might be used for vector > > element accesses) > > > > Do you have a testcase? No, I suspect it might need some folding to trigger (IIRC I made the FEs use ARRAY_REFs but I'm not sure whether fully, esp. in the case of address-taking). My attempt: typedef int v4si __attribute__((vector_size(16))); struct X { v4si x; } __attribute__((packed)) x; int *foo() { return &x.x[1]; } that shows return &VIEW_CONVERT_EXPR(x.x)[1]; for both C and C++ (albeit checked GCC 8 here). Richard. > -- > H.J.