From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119394 invoked by alias); 7 Dec 2017 22:23:45 -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 119380 invoked by uid 89); 7 Dec 2017 22:23:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=has, snip, sk:Warray, pr79223c 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 ESMTP; Thu, 07 Dec 2017 22:23:42 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 96E517E42F; Thu, 7 Dec 2017 22:23:41 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-2.rdu2.redhat.com [10.10.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 71EF960C4E; Thu, 7 Dec 2017 22:23:40 +0000 (UTC) Subject: Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918) To: Martin Sebor , Richard Biener Cc: Gcc Patch List References: <8a0de82f-d68e-9c37-0406-f216e3f92884@gmail.com> <703424cb-db8c-8aae-9bd2-cece5883a5de@gmail.com> <0266f22c-6ac1-f676-123f-c028905519e5@redhat.com> <1574f108-5456-711e-6533-73c4711c26f2@gmail.com> <40984eff-b156-3315-7bb5-558e9e83bf6c@gmail.com> <86344144-d7de-9a92-2e65-9d43536c312b@gmail.com> <11adcf4e-2ed0-c441-2ea7-538fb453b956@gmail.com> <5ffed8d2-4957-898f-590f-b09a3d9ca719@redhat.com> <7b05b1da-da02-1385-5ef6-ba0eee6fc7b1@gmail.com> From: Jeff Law Message-ID: <7a5fae23-f63e-98b5-7368-a7cf992ebe20@redhat.com> Date: Thu, 07 Dec 2017 22:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00421.txt.bz2 On 11/29/2017 04:36 PM, Martin Sebor wrote: > I've finished reimplementing the patch as a standalone pass. > In the attached revision I also addressed your comments below > as well as Richard's to allowing the strlen optimizations even > for overlapping accesses. > > While beefing up the tests I found a few minor issues that > I also fixed (false negatives). > > The fallout wasn't quite as bad as I thought, mainly thanks > to the narrow API for the checker. > > Syncing up with the latest trunk has led to some more changes > in tree-ssa-strlen. > > I've retested the patch with GDB and Glibc with the same results > as before. > > The patch seems sizable (over 3KLOC without tests) but it's worth > noting that most of the complexity is actually not in determining > whether or not an overlap exists (that's quite simple) but rather > in computing its offset and size to mention in the warnings and > making sure the information is meaningful to the user even when > ranges are involved.  All the subtly different forms of warnings > also contribute substantially to the overall size. > > Martin [ Huge snip. ] > > gcc-78918.diff > > > PR tree-optimization/78918 - missing -Wrestrict on memcpy copying over self > > gcc/c-family/ChangeLog: > > PR tree-optimization/78918 > * c-common.c (check_function_restrict): Avoid checking built-ins. > * c.opt (-Wrestrict): Include in -Wall. > > gcc/ChangeLog: > > PR tree-optimization/78918 > * Makefile.in (OBJS): Add gimple-ssa-warn-restrict.o. > * builtins.c (check_sizes): Rename... > (check_access): ...to this. Rename function arguments for clarity. > (check_memop_sizes): Adjust names. > (expand_builtin_memchr, expand_builtin_memcpy): Same. > (expand_builtin_memmove, expand_builtin_mempcpy): Same. > (expand_builtin_strcat, expand_builtin_stpncpy): Same. > (check_strncat_sizes, expand_builtin_strncat): Same. > (expand_builtin_strncpy, expand_builtin_memset): Same. > (expand_builtin_bzero, expand_builtin_memcmp): Same. > (expand_builtin_memory_chk, maybe_emit_chk_warning): Same. > (maybe_emit_sprintf_chk_warning): Same. > (expand_builtin_strcpy): Adjust. > (expand_builtin_stpcpy): Same. > (expand_builtin_with_bounds): Detect out-of-bounds accesses > in pointer-checking forms of memcpy, memmove, and mempcpy. > (gcall_to_tree_minimal, max_object_size): Define new functions. > * builtins.h (max_object_size): Declare. > * calls.c (alloc_max_size): Call max_object_size instead of > hardcoding ssizetype limit. > (get_size_range): Handle new argument. > * calls.h (get_size_range): Add a new argument. > * cfgexpand.c (expand_call_stmt): Propagate no-warning bit. > * doc/invoke.texi (-Wrestrict): Adjust, add example. > * gimple-fold.c (gimple_fold_builtin_memory_op): Detect overlapping > operations. > (gimple_fold_builtin_memory_chk): Same. > (gimple_fold_builtin_stxcpy_chk): New function. > * gimple-ssa-warn-restrict.c: New source. > * gimple-ssa-warn-restrict.h: New header. > * gimple.c (gimple_build_call_from_tree): Propagate location. > * passes.def (pass_warn_restrict): Add new pass. > * tree-pass.h (make_pass_warn_restrict): Declare. > * tree-ssa-strlen.c (handle_builtin_strcpy): Detect overlapping > operations. > (handle_builtin_strcat): Same. > (strlen_optimize_stmt): Rename... > (strlen_check_and_optimize_stmt): ...to this. Handle strncat, > stpncpy, strncpy, and their checking forms. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/78918 > * c-c++-common/Warray-bounds.c: New test. > * c-c++-common/Warray-bounds-2.c: New test. > * c-c++-common/Warray-bounds-3.c: New test. > * c-c++-common/Wrestrict-2.c: New test. > * c-c++-common/Wrestrict.c: New test. > * c-c++-common/Wrestrict.s: New test. > * c-c++-common/Wsizeof-pointer-memaccess1.c: Adjust > * c-c++-common/Wsizeof-pointer-memaccess2.c: Same. > * g++.dg/torture/Wsizeof-pointer-memaccess1.C: Same. > * g++.dg/torture/Wsizeof-pointer-memaccess2.C: Same. > * gcc.dg/memcpy-6.c: New test. > * gcc.dg/pr69172.c: Adjust. > * gcc.dg/pr79223.c: Same. > * gcc.dg/Wrestrict-2.c: New test. > * gcc.dg/Wrestrict.c: New test. > * gcc.dg/Wsizeof-pointer-memaccess1.c > * gcc.target/i386/chkp-stropt-17.c: New test. > * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Adjust. > > @@ -3874,32 +3885,32 @@ check_strncat_sizes (tree exp, tree objsize) > size_one_node) > : NULL_TREE); > > - /* Strncat copies at most MAXLEN bytes and always appends the terminating > + /* Strncat copies at most MAXREAD bytes and always appends the terminating Nit. Use "strncat" rather than "Strncat", even when starting a sentence. I saw this elsewhere. You can fix these in a follow-up which you can consider pre-approved. So please correct me if I'm wrong. WRT overall structure, you've got a pass which walks the IL looking for suitable calls and potentially warns. Additionally, that pass exports check_bounds_or_overlap which we use to varying degress in the folder and strlen optimization pass. It doesn't really query any pass specific state, it's just an exported interface into the meat of your optimization pass. Presumably you have those exported interfaces so that you can diagnose things prior to folding or other transformations that potentially obfuscate the calls you want to diagnose? Right? The main pass (which runs after VRP) presumably runs to capture more stuff that is exposed by the optimizers. ie, the optimizers may const/copy propagate things which are necessary to expose some of the cases we want to capture. Assuming I'm correct on those issues, this starts to look a lot like how we structure early/late warnings. -- I believe standard practice is to put the pass class into the anonymous namespace -- pass_data_wstrict & pass_wrestrict as well as any methods you define for them. I'm sure there's a good reason why we do that, though I find it annoying given GDB's poor support for anonymous namespaces. BUt please follow convention here. Consider walking the blocks in dominator order. If the pass can benefit from range data it'll make adding the evrp analyzer easier. As a reference see how I changed the sprintf warnings bits to use a dominator walk order. On pass_wrestrict::check_call, put its return value type onto a separate line. It looks like you have to create an expression for each and every builtin call statement you check. Is there any way to defer creating that CALL_EXPR until you know you need it in a diagnostic? Can the uses prior to the actual diagnostic get their data from elsewhere? It looks like you're doing a lot more than just restrict checking in the new pass. I see array bounds checking and what appears to be general pointer-out-of-bounds checking in the new pass. Can you comment on that? Overall it looks pretty good. I want to make sure I understand the overall structure and reasoning behind that structure. Jeff