From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33981 invoked by alias); 16 Oct 2015 12:27: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 33967 invoked by uid 89); 16 Oct 2015 12:27:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 16 Oct 2015 12:27:54 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 5991D31AC83; Fri, 16 Oct 2015 12:27:53 +0000 (UTC) Received: from localhost.localdomain (vpn1-7-218.ams2.redhat.com [10.36.7.218]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9GCRqJd018047; Fri, 16 Oct 2015 08:27:52 -0400 Subject: Re: [PATCH] c/67882 - improve -Warray-bounds for invalid offsetof To: Martin Sebor , Gcc Patch List , Joseph Myers References: <56172C8C.2070202@gmail.com> From: Bernd Schmidt Message-ID: <5620ED47.2090009@redhat.com> Date: Fri, 16 Oct 2015 12:28:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <56172C8C.2070202@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg01580.txt.bz2 On 10/09/2015 04:55 AM, Martin Sebor wrote: > Gcc attempts to diagnose invalid offsetof expressions whose member > designator is an array element with an out-of-bounds index. The > logic in the function that does this detection is incomplete, leading > to false negatives. Since the result of the expression in these cases > can be surprising, this patch tightens up the logic to diagnose more > such cases. In the future, please explain more clearly in the patch submission what the false negatives are. That'll make the reviewer's job easier. > Tested by boostrapping and running c/c++ tests on x86_64 with no > regressions. Should run the full testsuite (standard practice, and library tests might have occurrences of offsetof). A ChangeLog is missing. (Not that I personally care about ChangeLogs, but apparently others do.) > +struct offsetof_ctx_t > +{ > + tree inx; /* The invalid array index or NULL_TREE. */ > + int maxinx; /* All indices to the array have the highest valid value. */ > +}; I think "idx" is commonly used, I've never seen the spelling "inx". Also, typically comments go on their own lines before the field. > + > + if (tree_int_cst_lt (upbound, t)) { > + pctx->inx = t; None-standard formatting. Elsewhere too. > + /* Index is considered valid when it's either less than > + the upper bound or equal to it and refers to the lowest > + rank. Since in the latter case it may not at this point > + in the recursive call to the function be known whether > + the element at the index is used to do more than to > + compute its offset (e.g., it can be used to designate > + a member of the type to which the just past-the-end > + element refers), point the INX variable at the index > + and leave it up to the caller to decide whether or not > + to diagnose it. Special handling is required for minor > + index values referring to the element just past the end > + of the array object. */ I admit to having trouble parsing this comment. Can you write that in a clearer way somehow? I'm still trying to make my mind up whether the logic in this patch could be simplified. > t = convert (sizetype, t); > off = size_binop (MULT_EXPR, TYPE_SIZE_UNIT (TREE_TYPE (expr)), t); > + > break; Spurious change, please remove. > +extern tree fold_offsetof_1 (tree, offsetof_ctx_t* = NULL); Space before *. > +// treatment since the offsetof exression yields the same result for "expression". > + // The following expression is silently accepted as an extension > + // because it simply forms the equivalent of a just-past-the-end > + // address. > + __builtin_offsetof (A, a1_1 [0][1]), // extension Hmm, do we really want to support any kind of multidimensional array for this extension? My guess would have been to warn here. So I checked and it looks like we accept flexible array member syntax like "int a[][2];", which suggests that the test might have the right idea, but has the indices swapped (the first one is the flexible one)? Ccing Joseph for a ruling. Bernd