From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66413 invoked by alias); 24 Aug 2017 22:09:33 -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 66404 invoked by uid 89); 24 Aug 2017 22:09:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=popped, weed, hardly, Theyre 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, 24 Aug 2017 22:09:30 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3302B81DF8; Thu, 24 Aug 2017 22:09:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3302B81DF8 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=law@redhat.com Received: from localhost.localdomain (ovpn-116-59.phx2.redhat.com [10.3.116.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id D74EF61348; Thu, 24 Aug 2017 22:09:28 +0000 (UTC) Subject: Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918) To: Richard Biener , Martin Sebor 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> From: Jeff Law Message-ID: Date: Thu, 24 Aug 2017 22:36:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 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-08/txt/msg01479.txt.bz2 On 08/22/2017 02:45 AM, Richard Biener wrote: > On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor wrote: >> On 08/09/2017 10:14 AM, Jeff Law wrote: >>> >>> On 08/06/2017 05:08 PM, Martin Sebor wrote: >>> >>>>> >>>>> Well, simply because the way as implemented isn't a must-alias query >>>>> but maybe one that's good enough for warnings (reduces false positives >>>>> but surely doesn't eliminate them). >>>> >>>> >>>> I'm very interested in reducing the rate of false positives in >>>> these and all other warnings. As I mentioned in my comments, >>>> I did my best to weed them out of the implementation by building >>>> GDB, Glibc, Busybox, and the Linux kernel. That of course isn't >>>> a guarantee that there aren't any. But the first implementation >>>> of any non-trivial feature is never perfect, and hardly any >>>> warning of sufficient complexity is free of false positives, no >>>> matter here it's implemented (the middle-end, front-end, or >>>> a standalone static analysis tool). If you spotted some cases >>>> I had missed I'd certainly be grateful for examples. Otherwise, >>>> they will undoubtedly be reported as more software is exposed >>>> to the warning and, if possible, fixed, as happens with all >>>> other warnings. >>> >>> I think Richi is saying that the must alias query you've built isn't >>> proper/correct. It's less about false positives for Richi and more >>> about building a proper must alias query if I understand him correctly. >>> >>> I suspect he's also saying that you can't reasonably build must-alias on >>> top of a may-alias query framework. They're pretty different queries. >>> >>> If you need something that is close to, but not quite a must alias >>> query, then you're going to have to make a argument for that and you >>> can't call it a must alias query. >> >> >> Attached is an updated and simplified patch that avoids making >> changes to any of the may-alias functions. It turns out that >> all the information the logic needs to determine the overlap >> is already in the ao_ref structures populated by >> ao_ref_init_from_ptr_and_size. The only changes to the pass >> are the enhancement to ao_ref_init_from_ptr_and_size to make >> use of range info and the addition of the two new functions >> used by the -Wrestrict clients outside the pass. > > Warning for memcpy (p, p, ...) is going to fire false positives all around > given the C++ FE emits those in some cases and optimization can > expose that we are dealing with self-assignments. And *p = *p is > valid. Correct. I wound my way through this mess a while back. Essentially Red Hat had a customer with code that had overlapped memcpy arguments. We had them use the memstomp interposition library to start tracking these problems down. One of the things that popped up was structure/class copies which were implemented via calls to memcpy. In the case of self assignment, the interposition library would note the overlap and (rightly IMHO) complain. One could argue that GCC should emit memmove by default for structure assignments, only using memcpy when it knows its not doing self assignment (which may be hard to determine). Furthermore, GCC should eliminate self structure/class assignment. > > @@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi, > } > } > > + /* Avoid folding the call if overlap is detected. */ > + if (check_overlap && detect_overlap (loc, stmt, dest, src, len)) > + return false; > + > > no, please not. You diagnosed the call (which might be a false positive) > so why keep it undefined? The folded stmt will either have the same > semantics (aggregate copies expanded as memcpy) or have all reads > performed before writes. So can we distinguish here between overlap and the self-copy case? A self-copy should just be folded away. It's no different than x = x on scalars except that we've dropped it to a memcpy in the IL. Doing so makes the code more efficient and removes false positives from tools like the memstomp interposition library, making those tools more useful. Jeff