From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 120511 invoked by alias); 17 Dec 2018 13:53:39 -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 120494 invoked by uid 89); 17 Dec 2018 13:53:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=proof X-HELO: mail-qt1-f169.google.com Received: from mail-qt1-f169.google.com (HELO mail-qt1-f169.google.com) (209.85.160.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Dec 2018 13:53:35 +0000 Received: by mail-qt1-f169.google.com with SMTP id t13so14105076qtn.3 for ; Mon, 17 Dec 2018 05:53:35 -0800 (PST) Return-Path: Received: from [192.168.1.149] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id x57sm9052233qtx.96.2018.12.17.05.53.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Dec 2018 05:53:33 -0800 (PST) Subject: Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member To: "H.J. Lu" , Richard Guenther Cc: "Joseph S. Myers" , Martin Sebor , GCC Patches References: <24f8a201-bb3e-3acb-b679-570a8796358d@redhat.com> From: Jason Merrill Message-ID: Date: Mon, 17 Dec 2018 13:53:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 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: 2018-12/txt/msg01216.txt.bz2 On 12/17/18 7:42 AM, 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 wrote: >>>> >>>> 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) == COND_EXPR) >>>>>>>>>>>>>> + { >>>>>>>>>>>>>> + /* Check the THEN path first. */ >>>>>>>>>>>>>> + tree op1 = TREE_OPERAND (rhs, 1); >>>>>>>>>>>>>> + context = 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 seem to expect >>>>>>>>>>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 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 ‘struct C *’ pointer increases the >>>>>>>>> alignment of ‘long int *’ pointer from 1 to 8 [-Waddress-of-packed-member] >>>>>>>> >>>>>>>> >>>>>>>> I think this would read better as >>>>>>>> >>>>>>>> c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment 1) to >>>>>>>> ‘long int *’ (alignment 8) may result in an unaligned pointer value >>>>>>>> [-Waddress-of-packed-member] >>>>>>> >>>>>>> Fixed. >>>>>>> >>>>>>>>> + while (TREE_CODE (base) == ARRAY_REF) >>>>>>>>> + base = TREE_OPERAND (base, 0); >>>>>>>>> + if (TREE_CODE (base) != 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 = TREE_CODE (base); >>>>>>> if (code == COMPONENT_REF) >>>>>>> break; >>>>>>> switch (code) >>>>>>> { >>>>>>> case ARRAY_REF: >>>>>>> base = 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 = &__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 = 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? Is there a reason you only want to handle some component references and not others? If not, checking handled_component_p is simpler and more future proof than enumerating specific codes. Jason