From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4830 invoked by alias); 12 May 2017 10:20:15 -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 4805 invoked by uid 89); 12 May 2017 10:20:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 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=business X-HELO: mail-oi0-f67.google.com Received: from mail-oi0-f67.google.com (HELO mail-oi0-f67.google.com) (209.85.218.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 12 May 2017 10:20:13 +0000 Received: by mail-oi0-f67.google.com with SMTP id w138so8081574oiw.3 for ; Fri, 12 May 2017 03:20:16 -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=oB1XpYDOls0uZzbTw6YzGwCyelBtkA/oEI/JRaypA0U=; b=VwURDQO5rUNRfsQVhie7fRfHOCq/SQTtzK7U9UFi4cqs3AV8LSR/TcLb+sRlGP4YOM tj9/woyKaPtHgnBmRxH+UBmWqYnawPaGO6gKkW58fndLqzgXJmKJmGJdOlRBwsTO5c8B uJA11oVNIeqPDcmLlGxHXGOXWM7/F1VApMFvKIZArckTJYopOdGK688v3/pV7HyNpxOP 1GtgLtGqfqM3BGhqZlzKr+Qm9MsHkq0JnGzPwSu9nL8heX4OfjYPaAi7HYMxnlJajdG3 AkEVgMQLibH+LW0GGTPorkEd/GQ9h9dHRaXnU2S1VS8t1jhUu2QjfcNq15sUJtPJsPwj 23uw== X-Gm-Message-State: AODbwcDJdEgzl6D3Jj2233PB9GZtb1RdrK9n0jFnzU3zTiYir1GLrQVC OzofHg1EcUmNbJfulFuLL63/Ji8IxQ== X-Received: by 10.157.45.231 with SMTP id g94mr1770268otb.229.1494584414777; Fri, 12 May 2017 03:20:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.157.51.83 with HTTP; Fri, 12 May 2017 03:20:14 -0700 (PDT) In-Reply-To: <20170512100908.GK4910@redhat.com> References: <20170511134951.GI4910@redhat.com> <20170512100908.GK4910@redhat.com> From: Richard Biener Date: Fri, 12 May 2017 10:26:00 -0000 Message-ID: Subject: Re: [PATCH] Don't fold in save_expr (PR sanitizer/80536, PR sanitizer/80386) To: Marek Polacek Cc: GCC Patches , Jakub Jelinek , Jason Merrill , Joseph Myers Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg01009.txt.bz2 On Fri, May 12, 2017 at 12:09 PM, Marek Polacek wrote: > On Fri, May 12, 2017 at 09:32:48AM +0200, Richard Biener wrote: >> On Thu, May 11, 2017 at 3:49 PM, Marek Polacek wrote: >> > I had an interesting time coming to grips with these two PRs. But it >> > essentially comes down to the fold call in save_expr. With that, we can call >> > fold() on an expression whose operands weren't folded yet, but that is what >> > code in fold_binary_loc assumes. Since fold() is not recursive, we could end >> > up there with expressions such as >> > i * ((unsigned long) -0 + 1) * 2 >> > causing various sorts of problems: turning valid code into invalid, infinite >> > recursion, etc. >> > >> > It's not clear why save_expr folds. I did some archeology but all I could >> > find was r67189 (that's 2003) which had: >> > >> > - tree t = expr; >> > + tree t = fold (expr); >> > tree inner; >> > >> > - /* Don't fold a COMPONENT_EXPR: if the operand was a CONSTRUCTOR (the >> > - only time it will fold), it can cause problems with PLACEHOLDER_EXPRs >> > - in Ada. Moreover, it isn't at all clear why we fold here at all. */ >> > - if (TREE_CODE (t) != COMPONENT_REF) >> > - t = fold (t); >> > >> > so it wasn't clear even 14 years ago. But now we know the call is harmful. > > I've come to believe this was purely to discover more invariants. > >> > Now, fold() doesn't recurse, but can it happen that it folds something anyway? >> > And maybe turn the expression into an invariant so that we don't need to wrap >> > it in a SAVE_EXPR? Turns out it can, but in the C FE, which either uses >> > c_save_expr that c_fully_folds, or calls save_expr when in_late_binary_op (so >> > the operands have already been folded), it's very rare, and can only be triggered >> > by code such as >> > >> > void >> > f (int i) >> > { >> > int (*a)[i]; >> > int x[sizeof (*a)]; >> > } >> > >> > so I'm not very worried about that. For C++, Jakub suggested to add SAVE_EXPR >> > handling to cp_fold. >> > >> > Future work: get rid of c_save_expr, only use save_expr, and teach c_fully_fold >> > how to fold SAVE_EXPR (once). Although there's this c_wrap_maybe_const >> > business... >> > >> > Bootstrapped/regtested on x86_64-linux, ok for trunk? >> >> Interesting - I tried this once (removing fold () from save_expr) but >> it failed miserably >> (don't remember the details). Maybe the cp/ part fixes it up. > > I think that must've been before C++ added all that constexpr bits. The > cp_fold part is just an optimization that triggers very rarely. > >> Did you include Ada in testing? > > Not before, so I've tested this now with Ada included -- looks fine still. > >> Otherwise the tree.c change is ok. > > Thanks. Jason/Joseph, any comments? > >> (yay, one less to go in the attempt to remove fold ()) > > Do we have any low hanging fruit here? Suppose not... Factoring out the quaternary cases to better handle fold (build4 (code, ... that happens in quite a few places. And then there's tree t = build3 (BIT_FIELD_REF, currop->type, genop0, op1, op2); REF_REVERSE_STORAGE_ORDER (t) = currop->reverse; return fold (t); for the lack of [fold_]buildN getting the REF_REVERSE_STORAGE_ORDER flag ... (I fully blame Eric for this ;)). Ok, now starts to be non-low-hanging. Richard. > Marek