From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65654 invoked by alias); 22 Aug 2017 08:46: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 62897 invoked by uid 89); 22 Aug 2017 08:45:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=sk:msebor@, sk:msebor, U*msebor, msebor@gmail.com X-HELO: mail-wr0-f174.google.com Received: from mail-wr0-f174.google.com (HELO mail-wr0-f174.google.com) (209.85.128.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 22 Aug 2017 08:45:13 +0000 Received: by mail-wr0-f174.google.com with SMTP id p8so54892154wrf.5 for ; Tue, 22 Aug 2017 01:45:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=gjzNm1UpyA2wCLorAL1Zs945LEofxf13k5Lo2qhyUCA=; b=LkkUvgL63T9zpqAvb2vSa49XJFLkN6hDYpiaKT7Ubpoj7uQRJk8bsZTFaP4ARAprc3 EdHaGFIA/v0t8HmpQF/M08e5kuThAlx15Prp8WumlkIsuuJFeCeF4TvRk+MNl+Xpuv/Y Ve8aIZWaoHh+y6jlXh4uqKpyZ9kw7e4/KdlmHn45D5D/KlXvxv8dDfk5wUY/S4F+l30v 3X0tqgQBXeY946ybpuCNTe6TBJAhYpNgOvCf5grALnm6b8xkt1aH/vRMIxglVAqU/jo2 v4YQ47DHBPjW5xEkyoe3qBGAM9hCzZeunz4xA9gk84LcE6tlWc6GyvBbV2vNi9n+wa7a ixSw== X-Gm-Message-State: AHYfb5h61OlwjdjVE2cBravGz4g+c1Am9p6w6YXz//JRz97RCcQg7B5e TAxBEiDGGKzo2PNMkpNp/IKDguwBVA== X-Received: by 10.80.163.212 with SMTP id t20mr6937648edb.292.1503391508685; Tue, 22 Aug 2017 01:45:08 -0700 (PDT) MIME-Version: 1.0 Received: by 10.80.180.249 with HTTP; Tue, 22 Aug 2017 01:45:08 -0700 (PDT) In-Reply-To: References: <8a0de82f-d68e-9c37-0406-f216e3f92884@gmail.com> <703424cb-db8c-8aae-9bd2-cece5883a5de@gmail.com> <0266f22c-6ac1-f676-123f-c028905519e5@redhat.com> From: Richard Biener Date: Tue, 22 Aug 2017 09:17:00 -0000 Message-ID: Subject: Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918) To: Martin Sebor Cc: Jeff Law , Gcc Patch List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg01248.txt.bz2 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. @@ -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. The ao_ref_init_from_ptr_and_size change misses a changelog entry. +detect_overlap (location_t loc, gimple *stmt, tree dst, tree src, tree size, + bool adjust /* = false */) +{ + ao_ref dstref, srcref; + unsigned HOST_WIDE_INT range[2]; + + /* Initialize and store the lower bound of a constant offset (in + bits), disregarding the offset for the destination. */ + ao_ref_init_from_ptr_and_size (&dstref, dst, size, range); + ao_ref_init_from_ptr_and_size (&srcref, src, size, range); just pass NULL range to the first call? - ref->ref = NULL_TREE; + + if (offrng) + offrng[0] = offrng[1] = 0; + + ref->ref = NULL_TREE; bogus indent + else if (offrng && TREE_CODE (offset) == SSA_NAME) + { + wide_int min, max; + value_range_type rng = get_range_info (offset, &min, &max); + if (rng == VR_RANGE && wi::fits_uhwi_p (min)) + { + ptr = gimple_assign_rhs1 (stmt); + offrng[0] = BITS_PER_UNIT * min.to_uhwi (); + offrng[1] = BITS_PER_UNIT * max.to_uhwi (); + + extra_offset = offrng[0]; you didnt' check whether max fits uhwi. The effect of passing offrng is not documented. + else + /* Offset range is indeterminate. */ + offrng[0] = offrng[1] = HOST_WIDE_INT_M1U; I believe a cleaner interface would be to do void ao_ref_init_from_ptr_and_size (ao_ref *ref, tree ptr, tree size, tree *var_byte_offset) and set *var_byte_offset to your 'offset' above, leaving 'ref' unchanged. The caller can then get at the range info of var_byte_offset and adjust ref->offset if desired. The indeterminate state is then much cleaner - NULL_TREE. +unsigned HOST_WIDE_INT +refs_overlap (ao_ref *ref1, ao_ref *ref2, unsigned HOST_WIDE_INT *aloff) +{ bool refs_must_overlap_p (ao_ref *, ao_ref *, unsigned HOST_WIDE_INT *off, unsinged HOST_WIDE_INT *size) your return values are in bytes thus + + // Compare pointers. + offset1 += mem_ref_offset (base1) << LOG2_BITS_PER_UNIT; + offset2 += mem_ref_offset (base2) << LOG2_BITS_PER_UNIT; + base1 = TREE_OPERAND (base1, 0); just do the intermediate computations in bytes as well. It looks like it is unspecified relative to which ref the offset is given, how's that useful information then -- the diagnostic doesn't seem too helpful? Why not keep it relative to the first ref and thus make aloff signed? detect_overlap doesn't belong to tree-ssa-alias.[ch]. (didn't look at the diagnostic parts) Thanks, Richard. > Martin