From patchwork Fri May 5 09:04:09 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 90373 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp253790vqo; Fri, 5 May 2023 02:05:17 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6Sa9x/67XbdhOLbiYSLUyo/Bz+I+Y5RkXB6iJzDwEd6K/O5tfTpJGfWeTKPje4KEkhqQog X-Received: by 2002:aa7:d355:0:b0:50b:d5d1:7409 with SMTP id m21-20020aa7d355000000b0050bd5d17409mr731640edr.23.1683277516584; Fri, 05 May 2023 02:05:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683277516; cv=none; d=google.com; s=arc-20160816; b=lPQdiY2Oeilfs6oDLpvpm4M8+1KORAirZBFkm9gmVn+Mds8AdOvb92hfbXJDEa5SZo 2TxhncFyQUwBjTqss4Oqfu+6yvKCBbSWVq+1S6vJY/o0a4QjEODFn220kEwzFue0OcRK 7J8Z3oTSegNfXwEY9EHGbn0aMY9RnoYgtSEmDxGQXo6SbAEPTp5aQigQXg3TEyFE2q2h RlbLVRYOcm3Vqj4CtMi6OQWiMLGW7lf4PBkl1YATlneL42+d6BwSh4ZqqgM2TUghl83x oRmJ2gCO7aDftfxxEIgUgoVtAIbvKFHYhwx0zxaLcvpyWUJCXoWdUd2ny60SLgrCya3e zbHA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-disposition:mime-version:message-id:subject:cc:to:date :dmarc-filter:delivered-to:dkim-signature:dkim-filter; bh=jJhLT7nfuQmoZHsCNjVG6qxx0dr/BtgasNklolKDXlg=; b=lorMduELIkKlx6KuOkjW7jlSfluqdUPe+TvUCWgP8ysEdtfbKbWYqnZMl8rDr0n/9h tEE0TkRId2fqcv0uszWSFR2ti4JFCLqrCWaK3lkaZy9GbSgZwL7vWUQzfy3qsHFLCD3m wty0KvH/TRK4TkcGF3BaKu2T+CTYAaiS1W6XAKa1QscTHDOwsukqL/fywrzlL2go3BK7 a6HmkGTiEjUSa5lStJgCbKO4OHqc9/tTGorTVUa+0i62HkGL1dW/e0jrVc47B+sm/IJr U4ntA5nrDYmCWQdilwiMj6mI985S6hzhUzxGShcD2bzucSmWIerrxE4ulOaw5U0U6iEP eMdg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=J8GHIMgk; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id w8-20020aa7cb48000000b00508aa5d0efbsi4098204edt.62.2023.05.05.02.05.16 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 May 2023 02:05:16 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=J8GHIMgk; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1EC333856944 for ; Fri, 5 May 2023 09:05:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1EC333856944 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1683277515; bh=jJhLT7nfuQmoZHsCNjVG6qxx0dr/BtgasNklolKDXlg=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=J8GHIMgkQG3X7vwv9yDT3KYGDXKfTWhjebSjV6gBIQC2+lbIw43xgUDSkX1rJK7Uq YsxqANYmzeo42ykSt/5PUawt3SHDaPSKJIOevHM/cRgSI7krkMDNDiy0Zs0AgPE1Vd KP6B/WWnYAXw6no1P+bAsgYi+J5OCut/cczhbhXM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id C95AF3858D33 for ; Fri, 5 May 2023 09:04:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C95AF3858D33 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-325-C_h1XkHCMgWz4GuTjEbLfA-1; Fri, 05 May 2023 05:04:15 -0400 X-MC-Unique: C_h1XkHCMgWz4GuTjEbLfA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AB8DC802666; Fri, 5 May 2023 09:04:14 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.194.156]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 503981121331; Fri, 5 May 2023 09:04:14 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 34594Bbg1360808 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 5 May 2023 11:04:11 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 345949bq1360807; Fri, 5 May 2023 11:04:09 +0200 Date: Fri, 5 May 2023 11:04:09 +0200 To: Richard Biener , "Joseph S. Myers" , Jason Merrill , Eric Botcazou Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] tree: Fix up save_expr [PR52339] Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jakub Jelinek via Gcc-patches From: Jakub Jelinek Reply-To: Jakub Jelinek Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1765044404988682608?= X-GMAIL-MSGID: =?utf-8?q?1765044404988682608?= Hi! As mentioned in the PR, save_expr seems to be very optimistic when some expression is invariant, which can result in various wrong-code issues. The problem is with the TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t) case in tree_invariant_p_1. TREE_READONLY (t) in that case says that the object shouldn't be modified during its lifetime and !TREE_SIDE_EFFECTS (t) that it can be evaluated safely multiple times, but that doesn't mean we can avoid wrapping the expression into SAVE_EXPR say for a TREE_READONLY COMPONENT_REF with INDIRECT_REF as first operand - either the lifetime of the TREE_READONLY object could end earlier than when we need to reevaluate the object (that happens in the pr52339-1.c case where save_expr is called on p->a and then free (p) is done or pr52339.C where delete a->b when calling ~B () dtor deallocates a), or e.g. the pointer could change as in pr52339-2.c (so evaluating p->a again after ++p yields a possibly different value than originally and again we need a SAVE_EXPR). Attached are two patches which fix this, unfortunately both regress FAIL: gnat.dg/loop_optimization21.adb scan-tree-dump-times optimized "Index_Check" 1 FAIL: gnat.dg/vect1.adb scan-tree-dump-times vect "vectorized 1 loops" 15 FAIL: gnat.dg/vect2.adb scan-tree-dump-times vect "vectorized 1 loops" 15 FAIL: gnat.dg/vect3.adb scan-tree-dump-times vect "vectorized 1 loops" 15 FAIL: gnat.dg/vect4.adb scan-tree-dump-times vect "vectorized 1 loops" 15 FAIL: gnat.dg/vect5.adb scan-tree-dump-times vect "vectorized 1 loops" 15 FAIL: gnat.dg/vect6.adb scan-tree-dump-times vect "vectorized 1 loops" 15 on x86_64-linux (the first scan triggers 2 times rather than once, the next 3 13 times rather than 15 and the last 3 14 times rather than 15 times). The first patch has been otherwise successfully bootstrapped/regtested on x86_64-linux and i686-linux (with that above regressions), the second one is probably better but has been so far tested just on the new testcases and verified to also cause the above Ada regressions. Ok for trunk (which version), what to do about the regressions, shall we just adjust the expected counts or something else? E.g. in the vect6.adb case it is the procedure Add (X : Varray; Y : Long_Float; R : out Varray) is begin for I in X'Range loop R(I) := X(I) + Y; end loop; end; function that is no longer vectorized. Both patches lead to lots of former r.P_BOUNDS->LB0 r.P_BOUNDS->UB0 x.P_BOUNDS->LB0 x.P_BOUNDS->UB0 expressions to be wrapped into SAVE_EXPRs. Jakub 2023-05-05 Jakub Jelinek PR c++/52339 * tree.cc (tree_invariant_p_2): New function, copied from tree_invariant_p. (contains_indirect_refs): New function. (tree_invariant_p): Rewritten as wrapper around tree_invariant_p_2. Return false if expression contains INDIRECT_REFs or MEM_REFs which actually dereference some pointer. (save_expr): Use SAVE_EXPR if tree_invariant_p_1 expression contains INDIRECT_REFs or MEM_REfs which actually dereference some pointer. (skip_simple_arithmetic): Use tree_invariant_p_2 instead of tree_invariant_p. * g++.dg/opt/pr52339.C: New test. * gcc.c-torture/execute/pr52339-1.c: New test. * gcc.c-torture/execute/pr52339-2.c: New test. 2023-05-05 Jakub Jelinek PR c++/52339 * tree.cc (tree_invariant_p_1): For TREE_READONLY (t) without side-effects, only return true if DECL_P (get_base_address (t)). * g++.dg/opt/pr52339.C: New test. * gcc.c-torture/execute/pr52339-1.c: New test. * gcc.c-torture/execute/pr52339-2.c: New test. --- gcc/tree.cc.jj 2023-05-01 09:59:46.686293833 +0200 +++ gcc/tree.cc 2023-05-05 10:19:19.061827355 +0200 @@ -3876,10 +3876,21 @@ tree_invariant_p_1 (tree t) { tree op; - if (TREE_CONSTANT (t) - || (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))) + if (TREE_CONSTANT (t)) return true; + if (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t)) + { + /* Return true for const qualified vars, but for members or array + elements without side-effects return true only if the base + object is a decl. If the base is e.g. a pointer dereference, + what the pointer points to could be deallocated or the pointer + could be changed. See PR52339. */ + tree base = get_base_address (t); + if (DECL_P (base)) + return true; + } + switch (TREE_CODE (t)) { case SAVE_EXPR: --- gcc/testsuite/g++.dg/opt/pr52339.C.jj 2023-05-04 15:23:20.459935705 +0200 +++ gcc/testsuite/g++.dg/opt/pr52339.C 2023-05-04 15:22:35.640578681 +0200 @@ -0,0 +1,19 @@ +// PR c++/52339 +// { dg-do run { target c++11 } } + + +struct B; +struct A { B *b; }; +struct B { + A *a; + B () : a(new A{this}) {} + ~B () { delete a; } +}; + +int +main () +{ + B *b = new B; + const A *a = b->a; + delete a->b; +} --- gcc/testsuite/gcc.c-torture/execute/pr52339-1.c.jj 2023-05-04 15:22:59.177241023 +0200 +++ gcc/testsuite/gcc.c-torture/execute/pr52339-1.c 2023-05-04 15:20:19.820527142 +0200 @@ -0,0 +1,29 @@ +/* PR c++/52339 */ + +struct S { int a; }; + +void +bar (int *p, struct S *q) +{ + __builtin_free (q); +} + +int +foo (const struct S *p, struct S *q) +{ + int b[p->a]; + bar (b, q); + return sizeof (b); +} + +int +main () +{ + struct S *p = __builtin_malloc (sizeof (struct S)); + if (!p) + return 0; + p->a = 42; + if (foo (p, p) != 42 * sizeof (int)) + __builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.c-torture/execute/pr52339-2.c.jj 2022-11-21 10:04:00.210677046 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr52339-2.c 2023-05-04 19:34:08.581686806 +0200 @@ -0,0 +1,20 @@ +/* PR c++/52339 */ + +struct S { int a; }; + +int +foo (const struct S *p) +{ + int b[p->a]; + ++p; + return sizeof (b); +} + +int +main () +{ + struct S s[] = { { 42 }, { 43 } }; + if (foo (s) != 42 * sizeof (int)) + __builtin_abort (); + return 0; +} --- gcc/tree.cc.jj 2023-05-01 09:59:46.686293833 +0200 +++ gcc/tree.cc 2023-05-04 15:40:58.684762277 +0200 @@ -3920,13 +3920,43 @@ tree_invariant_p_1 (tree t) /* Return true if T is function-invariant. */ -bool -tree_invariant_p (tree t) +static bool +tree_invariant_p_2 (tree t) { tree inner = skip_simple_arithmetic (t); return tree_invariant_p_1 (inner); } +/* Return non-NULL if *TP is INDIRECT_REF or MEM_REF with first operand + other than address of a decl. */ + +static tree +contains_indirect_refs (tree *tp, int *, void *) +{ + tree t = *tp; + if (TREE_CODE (t) == INDIRECT_REF) + return t; + else if (TREE_CODE (t) == MEM_REF + && (TREE_CODE (TREE_OPERAND (t, 0)) != ADDR_EXPR + || !DECL_P (TREE_OPERAND (TREE_OPERAND (t, 0), 0)))) + return t; + else + return NULL_TREE; +} + +/* Return true if T is function-invariant. Return false for expressions + containing pointer/reference dereferences. */ + +bool +tree_invariant_p (tree t) +{ + if (!tree_invariant_p_2 (t)) + return false; + return (TREE_CONSTANT (t) + || !walk_tree_without_duplicates (&t, contains_indirect_refs, + NULL)); +} + /* Wrap a SAVE_EXPR around EXPR, if appropriate. Do this to any expression which may be used in more than one place, but must be evaluated only once. @@ -3963,7 +3993,13 @@ save_expr (tree expr) if (TREE_CODE (inner) == ERROR_MARK) return inner; - if (tree_invariant_p_1 (inner)) + if (tree_invariant_p_1 (inner) + && (TREE_CONSTANT (expr) + /* Use SAVE_EXPR if there are any pointer dereferences, evaluating + such expressions again and again might lead to use-after-free. + See PR52339. */ + || !walk_tree_without_duplicates (&expr, contains_indirect_refs, + NULL))) return expr; /* If INNER contains a PLACEHOLDER_EXPR, we must evaluate it each time, since @@ -4008,9 +4044,9 @@ skip_simple_arithmetic (tree expr) expr = TREE_OPERAND (expr, 0); else if (BINARY_CLASS_P (expr)) { - if (tree_invariant_p (TREE_OPERAND (expr, 1))) + if (tree_invariant_p_2 (TREE_OPERAND (expr, 1))) expr = TREE_OPERAND (expr, 0); - else if (tree_invariant_p (TREE_OPERAND (expr, 0))) + else if (tree_invariant_p_2 (TREE_OPERAND (expr, 0))) expr = TREE_OPERAND (expr, 1); else break; --- gcc/testsuite/g++.dg/opt/pr52339.C.jj 2023-05-04 15:23:20.459935705 +0200 +++ gcc/testsuite/g++.dg/opt/pr52339.C 2023-05-04 15:22:35.640578681 +0200 @@ -0,0 +1,19 @@ +// PR c++/52339 +// { dg-do run { target c++11 } } + + +struct B; +struct A { B *b; }; +struct B { + A *a; + B () : a(new A{this}) {} + ~B () { delete a; } +}; + +int +main () +{ + B *b = new B; + const A *a = b->a; + delete a->b; +} --- gcc/testsuite/gcc.c-torture/execute/pr52339-1.c.jj 2023-05-04 15:22:59.177241023 +0200 +++ gcc/testsuite/gcc.c-torture/execute/pr52339-1.c 2023-05-04 15:20:19.820527142 +0200 @@ -0,0 +1,29 @@ +/* PR c++/52339 */ + +struct S { int a; }; + +void +bar (int *p, struct S *q) +{ + __builtin_free (q); +} + +int +foo (const struct S *p, struct S *q) +{ + int b[p->a]; + bar (b, q); + return sizeof (b); +} + +int +main () +{ + struct S *p = __builtin_malloc (sizeof (struct S)); + if (!p) + return 0; + p->a = 42; + if (foo (p, p) != 42 * sizeof (int)) + __builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.c-torture/execute/pr52339-2.c.jj 2022-11-21 10:04:00.210677046 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr52339-2.c 2023-05-04 19:34:08.581686806 +0200 @@ -0,0 +1,20 @@ +/* PR c++/52339 */ + +struct S { int a; }; + +int +foo (const struct S *p) +{ + int b[p->a]; + ++p; + return sizeof (b); +} + +int +main () +{ + struct S s[] = { { 42 }, { 43 } }; + if (foo (s) != 42 * sizeof (int)) + __builtin_abort (); + return 0; +}