From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70434 invoked by alias); 17 Nov 2017 13:58:47 -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 70174 invoked by uid 89); 17 Nov 2017 13:58:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,KB_WAM_FROM_NAME_SINGLEWORD,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=HCc:D*fr X-HELO: mail-io0-f172.google.com Received: from mail-io0-f172.google.com (HELO mail-io0-f172.google.com) (209.85.223.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 Nov 2017 13:58:44 +0000 Received: by mail-io0-f172.google.com with SMTP id v21so8830144ioi.4 for ; Fri, 17 Nov 2017 05:58:44 -0800 (PST) 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=g8ZDR8prq56IEcKqBdvew1GiQ/NAS9DDk6O2WAa3eZQ=; b=Z4CEEduesQWw36mEJIx2PFwc60XP5VQNh5KU1UHa8UpAWfmm4C0jJtfKM0FHZEgLlM tSBboDZP79IzNXB8adISx/FM97yNIzh4lbNnpH2YijYX/AeEqvfOCx9HgU7W9BU5yYfc tuc/xWtoi3fiHYL3JCREU04OZkKGBhlpYkoG0IMRLihG3DqZlPsIQ2McwGGXtLrVO3Tl jAqXBxt2Ovh6qdw7T8i8Cdw31CCoGes4ol7VeMvoMHSkb0yTWligKcqokMwnGsP/fTL8 6NwJpuUHwa9PhwAe31E8qBbcFhA2QlQZGh/YcXpnQDFgXgUV++KFZHdLNTee4XmQJ5Mf bALQ== X-Gm-Message-State: AJaThX4PDrDyYpDnZy/Z+bEcccAwv25VK1pfguuX0dhbH9gg7fRKBdKF tw0xIe7DBqAZWXUYsTfjSb4D3aCct+jazihe/y20jQ== X-Google-Smtp-Source: AGs4zMa4994s+HcmarpPmsWsGcYGV1dqpXhBSFX63Lr9sskWjW5OwutC9V5z9A8jMP0jWjrX+kzTGWYsit+oJZEUJzw= X-Received: by 10.107.178.145 with SMTP id b139mr2757942iof.52.1510927122531; Fri, 17 Nov 2017 05:58:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.171.196 with HTTP; Fri, 17 Nov 2017 05:58:22 -0800 (PST) In-Reply-To: References: From: Jason Merrill Date: Fri, 17 Nov 2017 14:06:00 -0000 Message-ID: Subject: Re: [RFTesting] New POINTER_DIFF_EXPR To: Richard Biener Cc: Marc Glisse , "Joseph S. Myers" , Jakub Jelinek , GCC Patches , Ulrich Weigand , Ilya Enkovich , DJ Delorie Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg01480.txt.bz2 On Fri, Nov 17, 2017 at 7:56 AM, Richard Biener wrote: > On Sat, Nov 11, 2017 at 12:44 AM, Marc Glisse wrote: >> Adding some random cc: to people who might be affected. Hopefully I am not >> breaking any of your stuff... >> >> Ulrich Weigand (address space) >> Ilya Enkovich (pointer bound check) >> DJ Delorie (target with 24-bit partial mode pointer) >> >> If you want to give it a try, or just take a quick look to check that you >> are obviously not affected, that would be nice, but don't feel forced. >> >> Here is an updated version of the patch, with just a few more >> transformations in match.pd, to match some MINUS_EXPR optimizations I missed >> the first time: -(A-B), X-Z> (A-B)+(C-A) >> >> The exact undefined-behavior status should probably be clarified more. >> First, I'd like to say that POINTER_DIFF_EXPR may only take 2 pointers into >> the same "object" (like in C/C++), so they differ by at most half the size >> of the address space, and a-b is not allowed to be the minimum negative >> value (so I can safely use b-a), and if we compute a-b and b-c, then a and c >> are in the same object so I can safely compute a-c, etc. Second, we probably >> need modes without undefined behavior, wrapping with either -fwrapv or a new >> -fwrapp, or sanitized. For the sanitized version, we could keep using >> POINTER_DIFF_EXPR and check TYPE_OVERFLOW_SANITIZED on the pointer type as >> we currently do for integers. For wrapping, either we say that >> POINTER_DIFF_EXPR has wrapping semantics when that option is in effect, or >> we do not use POINTER_DIFF_EXPR and instead cast to unsigned before applying >> a MINUS_EXPR (my preference). > > CCing C/C++ FE folks as well. > > + /* It is better if the caller provides the return type. */ > + if (code == POINTER_DIFF_EXPR) > + { > + offset_int res = wi::sub (wi::to_offset (arg1), > + wi::to_offset (arg2)); > + return force_fit_type (signed_type_for (TREE_TYPE (arg1)), res, 1, > + TREE_OVERFLOW (arg1) | TREE_OVERFLOW (arg2)); > + } > + > > there's an overload that provides the type... (we should probably go > over all callers > and use that). There is already a folding that is only done when the type is > available. So I'd simply remove the above from the non-type overload handling. > What breaks then? > > With the patch it's of course somewhat ugly in that we need to deal with both > MINUS_EXPR on pointers and POINTER_DIFF_EXPR - at least that's what > > + case POINTER_DIFF_EXPR: > case MINUS_EXPR: > + /* Fold &a[i] - &a[j] to i-j. */ > + if (TREE_CODE (arg0) == ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (arg0, 0)) == ARRAY_REF > + && TREE_CODE (arg1) == ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (arg1, 0)) == ARRAY_REF) > + { > + tree tem = fold_addr_of_array_ref_difference (loc, type, > + TREE_OPERAND (arg0, 0), > + TREE_OPERAND (arg1, 0), > + code > + == POINTER_DIFF_EXPR); > > suggests. But is that really so? The FEs today should never build > a MINUS_EXPR with pointer operands and your patch also doesn't. > I suppose we only arrive above because STRIP_NOPS strips the > pointer to integer conversion? > > + case POINTER_DIFF_EXPR: > + if (!POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (t, 0))) > + || !POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (t, 1)))) > + { > + error ("invalid operand to pointer diff, operand is not a pointer"); > + return t; > + } > + CHECK_OP (0, "invalid operand to pointer diff"); > + CHECK_OP (1, "invalid operand to pointer diff"); > + break; > > can you add > || TREE_CODE (TREE_TYPE (t)) != INTEGER_TYPE > || TYPE_UNSIGNED (TREE_TYPE (t)) > > ? Note that if the precision of the pointer type arguments are not equal to the > precision of the return value then foldings like > > (simplify > + (plus:c (pointer_diff @0 @1) (pointer_diff @2 @0)) > + (if (TYPE_OVERFLOW_UNDEFINED (type) > + && !TYPE_OVERFLOW_SANITIZED (TREE_TYPE (@0))) > + (pointer_diff @2 @1))) > > might not be correct as we drop a possible truncation here. Shall we > require (and document) that the result of the pointer difference in > a POINTER_DIFF_EXPR has to be representable in the type of the > expression, otherwise > the behavior is undefined? Shall we allow an unsigned return type for > differences known to be representable? Probably not... Does the behavior > depend on TYPE_OVERFLOW_UNDEFINED? It would be nice to amend > the generic.texi docs somewhat here. > > Ah, the gimple checking adds some more bits here that clarify things: > > + case POINTER_DIFF_EXPR: > + { > + if (!POINTER_TYPE_P (rhs1_type) > + || !POINTER_TYPE_P (rhs2_type) > + || !types_compatible_p (rhs1_type, rhs2_type) > + || TREE_CODE (lhs_type) != INTEGER_TYPE > + || TYPE_UNSIGNED (lhs_type) > + || TYPE_PRECISION (lhs_type) != TYPE_PRECISION (rhs1_type)) > + { > + error ("type mismatch in pointer diff expression"); > + debug_generic_stmt (lhs_type); > + debug_generic_stmt (rhs1_type); > + debug_generic_stmt (rhs2_type); > + return true; > + } > > > I think the patch is ok for trunk, the FE bits should get approval > from their maintainers. > Joseph may have an idea about the address-space issue. > > With this in place we should be able to fix some of the issues Jakub ran into > with pointer sanitizing? > > Also with this in place it would be nice to do the corresponding adjustment to > POINTER_PLUS_EXPR, that is, require a _signed_ offset type that matches > the precision of the other argument. > > Richard. > >> >> >> On Sat, 28 Oct 2017, Marc Glisse wrote: >> >>> Hello, >>> >>> first, if you are doing anything unusual with pointers (address spaces, >>> pointer/sizetype with weird sizes, instrumentation, etc), it would be great >>> if you could give this patch a try. It was bootstrapped and regtested on >>> powerpc64le-unknown-linux-gnu (gcc112), and a slightly older version on >>> x86_64-pc-linux-gnu (skylake laptop). I also built bare cross-compilers (no >>> sysroot or anything) for avr, m32c, alpha64-vms, s390-linux, and visium to >>> check that on trivial examples it behaves as expected (by the way, m32c >>> seems broken for unrelated reasons at the moment), but I wouldn't count that >>> as complete testing. >>> >>> This was previously discussed in the thread "Fix pointer diff (was: >>> -fsanitize=pointer-overflow support (PR sanitizer/80998))" ( >>> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02128.html for the latest >>> message). >>> >>> Front-ends other than C/C++ can be changed later (I took a quick look at >>> fortran and ada, but they are way too unfamiliar to me), I did not remove >>> any handling for the other representations. >>> >>> 2017-10-28 Marc Glisse >>> >>> gcc/cp/ >>> * constexpr.c (cxx_eval_constant_expression, >>> potential_constant_expression_1): Handle POINTER_DIFF_EXPR. >>> * cp-gimplify.c (cp_fold): Likewise. >>> * error.c (dump_expr): Likewise. >>> * typeck.c (cp_build_binary_op): Likewise. It's not clear to me that cp_build_binary_op needs to handle POINTER_DIFF_EXPR, it should get MINUS_EXPR and produce POINTER_DIFF_EXPR. >>> * tree.def: New tree code POINTER_DIFF_EXPR. The comment might mention that the value is the same for all pointer types, not divided by the size of the pointed-to type. Jason