From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104063 invoked by alias); 6 Nov 2019 20:27:16 -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 104055 invoked by uid 89); 6 Nov 2019 20:27:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-12.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=anti, H*f:sk:6f6bdef, H*MI:sk:6f6bdef, H*i:sk:6f6bdef X-HELO: mail-yw1-f65.google.com Received: from mail-yw1-f65.google.com (HELO mail-yw1-f65.google.com) (209.85.161.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 06 Nov 2019 20:27:13 +0000 Received: by mail-yw1-f65.google.com with SMTP id p128so1190823ywc.11 for ; Wed, 06 Nov 2019 12:27:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=qwdRDAxrZeEr+yb6QovMbOPZEdtBHAPDkUVrZsXguNg=; b=hJ0CjYmNlrTA0Zl56452aL3d4eLwfAjbkZBgICJh4IURpH/AqS14lySuiG3qxbFn0/ va8BoYRE+qfvVb6m4j8Z40tVJeRU9MUwgFly0FdByb9NjO5im6TKkggDBo0a4MIqtnD/ ZJoSlKR++43s3+E0FX9e7r12OgpDmsgDDyOabDe3EDVJ0JDEIQo15P4w4sh+xuBY0USH yzvVWFa00lIfwmqqPj5oVN01RHGwKeub+oztkNRZZBTRYoLBSv3L1M00dlyVI0EIM2y9 zRRFj57+XKDVPSALsew5+YUqYX/3/gM4g3dW11KqKMQHELN6gqt+7r1E1kccHmbj9TPO dOnw== Return-Path: Received: from [192.168.0.41] (97-118-98-145.hlrn.qwest.net. [97.118.98.145]) by smtp.gmail.com with ESMTPSA id q131sm4271483ywh.22.2019.11.06.12.27.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 06 Nov 2019 12:27:10 -0800 (PST) Subject: Re: [PATCH] include size and offset in -Wstringop-overflow To: Jeff Law , gcc-patches References: <9d3210bf-e1bd-81ac-b1a0-5c66c8ec6db7@gmail.com> <6f6bdef6-0d0d-0303-b870-10ff2b036444@redhat.com> From: Martin Sebor Message-ID: <41e6f241-2361-487d-1bb0-c376df10a116@gmail.com> Date: Wed, 06 Nov 2019 20:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <6f6bdef6-0d0d-0303-b870-10ff2b036444@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg00438.txt.bz2 On 11/6/19 11:55 AM, Jeff Law wrote: > On 11/6/19 11:00 AM, Martin Sebor wrote: >> The -Wstringop-overflow warnings for single-byte and multi-byte >> stores mention the amount of data being stored and the amount of >> space remaining in the destination, such as: >> >> warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] >>   123 |   *p = 0; >>       |   ~~~^~~ >> note: destination object declared here >>    45 |   char b[N]; >>       |        ^ >> >> A warning like this can take some time to analyze.  First, the size >> of the destination isn't mentioned and may not be easy to tell from >> the sources.  In the note above, when N's value is the result of >> some non-trivial computation, chasing it down may be a small project >> in and of itself.  Second, it's also not clear why the region size >> is zero.  It could be because the offset is exactly N, or because >> it's negative, or because it's in some range greater than N. >> >> Mentioning both the size of the destination object and the offset >> makes the existing messages clearer, are will become essential when >> GCC starts diagnosing overflow into allocated buffers (as my >> follow-on patch does). >> >> The attached patch enhances -Wstringop-overflow to do this by >> letting compute_objsize return the offset to its caller, doing >> something similar in get_stridx, and adding a new function to >> the strlen pass to issue this enhanced warning (eventually, I'd >> like the function to replace the -Wstringop-overflow handler in >> builtins.c).  With the change, the note above might read something >> like: >> >> note: at offset 11 to object ‘b’ with size 8 declared here >>    45 |   char b[N]; >>       |        ^ >> >> Tested on x86_64-linux. >> >> Martin >> >> gcc-store-offset.diff >> >> gcc/ChangeLog: >> >> * builtins.c (compute_objsize): Add an argument and set it to offset >> into destination. >> * builtins.h (compute_objsize): Add an argument. >> * tree-object-size.c (addr_object_size): Add an argument and set it >> to offset into destination. >> (compute_builtin_object_size): Same. >> * tree-object-size.h (compute_builtin_object_size): Add an argument. >> * tree-ssa-strlen.c (get_addr_stridx): Add an argument and set it >> to offset into destination. >> (maybe_warn_overflow): New function. >> (handle_store): Call maybe_warn_overflow to issue warnings. >> >> gcc/testsuite/ChangeLog: >> >> * c-c++-common/Wstringop-overflow-2.c: Adjust text of expected messages. >> * g++.dg/warn/Wstringop-overflow-3.C: Same. >> * gcc.dg/Wstringop-overflow-17.c: Same. >> > >> Index: gcc/tree-ssa-strlen.c >> =================================================================== >> --- gcc/tree-ssa-strlen.c (revision 277886) >> +++ gcc/tree-ssa-strlen.c (working copy) >> @@ -189,6 +189,52 @@ struct laststmt_struct >> static int get_stridx_plus_constant (strinfo *, unsigned HOST_WIDE_INT, tree); >> static void handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *); >> >> +/* Sets MINMAX to either the constant value or the range VAL is in >> + and returns true on success. */ >> + >> +static bool >> +get_range (tree val, wide_int minmax[2], const vr_values *rvals = NULL) >> +{ >> + if (tree_fits_uhwi_p (val)) >> + { >> + minmax[0] = minmax[1] = wi::to_wide (val); >> + return true; >> + } >> + >> + if (TREE_CODE (val) != SSA_NAME) >> + return false; >> + >> + if (rvals) >> + { >> + gimple *def = SSA_NAME_DEF_STMT (val); >> + if (gimple_assign_single_p (def) >> + && gimple_assign_rhs_code (def) == INTEGER_CST) >> + { >> + /* get_value_range returns [0, N] for constant assignments. */ >> + val = gimple_assign_rhs1 (def); >> + minmax[0] = minmax[1] = wi::to_wide (val); >> + return true; >> + } > Umm, something seems really off with this hunk. If the SSA_NAME is set > via a simple constant assignment, then the range ought to be a singleton > ie [CONST,CONST]. Is there are particular test were this is not true? > > The only way offhand I could see this happening is if originally the RHS > wasn't a constant, but due to optimizations it either simplified into a > constant or a constant was propagated into an SSA_NAME appearing on the > RHS. This would have to happen between the last range analysis and the > point where you're making this query. Yes, I think that's right. Here's an example where it happens: void f (void) { char s[] = "1234"; unsigned n = strlen (s); char vla[n]; // or malloc (n) vla[n] = 0; // n = [4, 4] ... } The strlen call is folded to 4 but that's not propagated to the access until sometime after the strlen pass is done. >> + // FIXME: handle anti-ranges? >> + return false; > Please don't if we can avoid them. anti-ranges are on the chopping > block, so I'd prefer not to add cases where we're trying to handle them > if we can reasonably avoid it. It's mostly a reminder that there may be room for improvement here. Maybe not for ranges of sizes but possibly for ranges of offsets (e.g., if an offset's range is the union of [-4, -1] and [7, 9] and the destination array is 4 byes big the access is invalid). I've been thinking about how to handle multiple ranges when the new range info makes them available. I'm not sure I see how it will be possible to retrofit the existing code to make use of them. It seems that even code that tries to put anti- ranges to use today will need to change. It will be a fun exercise. Martin > > > No objections elsewhere. So I think we just need to figure out what's > going on with the ranges when you've got an INTEGER_CST on the RHS of an > assignment. > > jeff >