From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id B0F793858D37 for ; Tue, 21 Mar 2023 08:21:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B0F793858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 1BFD421A6B; Tue, 21 Mar 2023 08:21:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1679386879; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=RpKmwtFSX8Xd0om3FNwdB8IQ2knWyn19q3ZB0KAVHHo=; b=LpnWMBYtPnDfaepij6hBMD5FXG76y+t7sl2RP6KAuk3rjIG4lwJPxKU6VVVJef8QC9wF5b sxp5DrTnCvkkj/mRD37bjo0GIPDgF1JkVkEgAIRIELtUOjpAix1TRk9PyORug4ti6zMFoh Je8L2Lzu8kFov5AgeI8lK2qa71jWTkg= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1679386879; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=RpKmwtFSX8Xd0om3FNwdB8IQ2knWyn19q3ZB0KAVHHo=; b=rJgLuXrEr3u9FymkcKVuUIm4h9cmM8fKCmwg369n7Mzcnt9GQ+G4WeW05QzWfKfPLp2mRO Cp9OAuCoyaweYRBA== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 021D92C141; Tue, 21 Mar 2023 08:21:18 +0000 (UTC) Date: Tue, 21 Mar 2023 08:21:18 +0000 (UTC) From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org, Jan Hubicka Subject: Re: [PATCH] tree-optimization/109170 - bogus use-after-free with __builtin_expect In-Reply-To: Message-ID: References: <20230317121833.16A961346F@imap2.suse-dmz.suse.de> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, 20 Mar 2023, Jakub Jelinek wrote: > On Mon, Mar 20, 2023 at 12:12:14PM +0000, Richard Biener wrote: > > PR tree-optimization/109170 > > * gimple-range-op.cc (cfn_pass_through_arg1): New. > > (gimple_range_op_handler::maybe_builtin_call): Handle > > __builtin_expect and similar via cfn_pass_through_arg1 > > and inspecting the calls fnspec. > > * builtins.cc (builtin_fnspec): Handle BUILT_IN_EXPECT > > and BUILT_IN_EXPECT_WITH_PROBABILITY. > > I'm still worried about this builtins.cc change, can't we defer > that part till GCC 14 where there will be enough time to see if it > doesn't result in some undesirable problems (__builtin_expect* being > optimized away when it still shouldn't etc.)? Sure. I've retested and pushed the following. Richard. >From 02face8ff38e5a7942cfcb8c7444e6cca35d7523 Mon Sep 17 00:00:00 2001 From: Richard Biener Date: Fri, 17 Mar 2023 13:14:49 +0100 Subject: [PATCH] tree-optimization/109170 - bogus use-after-free with __builtin_expect To: gcc-patches@gcc.gnu.org The following adds a missing range-op for __builtin_expect which helps -Wuse-after-free to detect the case a realloc original pointer is used when the result was NULL. The implementation should handle all argument one pass-through builtins we handle in the fnspec machinery, but that's defered to GCC 14. The gcc.dg/tree-ssa/ssa-lim-21.c testcase needs adjustment because for (int j = 0; j < m; j++) if (__builtin_expect (m, 0)) for (int i = 0; i < m; i++) is now correctly optimized to a unconditional jump by EVRP - m cannot be zero when the outer loop is entered. I've adjusted the outer loop to iterate 'n' times which makes us apply store-motion to 'count' and 'q->data1' but only out of the inner loop and as expected not apply store motion to 'q->data' at all. The gcc.dg/predict-20.c testcase relies on broken behavior of profile estimation when trying to handle __builtin_expect values flowing into PHI nodes. I have opened PR109210 and removed the expected matching from the testcase. PR tree-optimization/109170 * gimple-range-op.cc (cfn_pass_through_arg1): New. (gimple_range_op_handler::maybe_builtin_call): Handle __builtin_expect via cfn_pass_through_arg1. * gcc.dg/Wuse-after-free-pr109170.c: New testcase. * gcc.dg/tree-ssa/ssa-lim-21.c: Adjust. * gcc.dg/predict-20.c: Likewise. --- gcc/gimple-range-op.cc | 27 +++++++++++++++++++ .../gcc.dg/Wuse-after-free-pr109170.c | 15 +++++++++++ gcc/testsuite/gcc.dg/predict-20.c | 3 ++- gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-21.c | 7 ++--- 4 files changed, 48 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc index a5d625387e7..c7c546caf43 100644 --- a/gcc/gimple-range-op.cc +++ b/gcc/gimple-range-op.cc @@ -309,6 +309,26 @@ public: } } op_cfn_constant_p; +// Implement range operator for integral/pointer functions returning +// the first argument. +class cfn_pass_through_arg1 : public range_operator +{ +public: + using range_operator::fold_range; + virtual bool fold_range (irange &r, tree, const irange &lh, + const irange &, relation_trio) const + { + r = lh; + return true; + } + virtual bool op1_range (irange &r, tree, const irange &lhs, + const irange &, relation_trio) const + { + r = lhs; + return true; + } +} op_cfn_pass_through_arg1; + // Implement range operator for CFN_BUILT_IN_SIGNBIT. class cfn_signbit : public range_operator_float { @@ -966,6 +986,13 @@ gimple_range_op_handler::maybe_builtin_call () m_int = &op_cfn_parity; break; + case CFN_BUILT_IN_EXPECT: + case CFN_BUILT_IN_EXPECT_WITH_PROBABILITY: + m_valid = true; + m_op1 = gimple_call_arg (call, 0); + m_int = &op_cfn_pass_through_arg1; + break; + default: break; } diff --git a/gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c b/gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c new file mode 100644 index 00000000000..14f1350aa29 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wuse-after-free=2" } */ + +unsigned long bufmax = 0; +unsigned long __open_catalog_bufmax; +void *realloc(void *, __SIZE_TYPE__); +void free(void *); + +void __open_catalog(char *buf) +{ + char *old_buf = buf; + buf = realloc (buf, bufmax); + if (__builtin_expect ((buf == ((void *)0)), 0)) + free (old_buf); /* { dg-bogus "used after" } */ +} diff --git a/gcc/testsuite/gcc.dg/predict-20.c b/gcc/testsuite/gcc.dg/predict-20.c index 31d01835b80..7bb0d411f88 100644 --- a/gcc/testsuite/gcc.dg/predict-20.c +++ b/gcc/testsuite/gcc.dg/predict-20.c @@ -16,8 +16,9 @@ c () break; } int d = b < 0; + /* We fail to apply __builtin_expect heuristics here. Se PR109210. */ if (__builtin_expect (d, 0)) asm(""); } -/* { dg-final { scan-tree-dump-times "__builtin_expect heuristics of edge" 3 "profile_estimate"} } */ +/* { dg-final { scan-tree-dump-times "__builtin_expect heuristics of edge" 2 "profile_estimate" } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-21.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-21.c index ffe6f8f699d..fe29e841f28 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-21.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-21.c @@ -17,7 +17,7 @@ void func (int m, int n, int k, struct obj *a) { struct obj *q = a; - for (int j = 0; j < m; j++) + for (int j = 0; j < n; j++) if (__builtin_expect (m, 0)) for (int i = 0; i < m; i++) { @@ -31,5 +31,6 @@ func (int m, int n, int k, struct obj *a) } } -/* { dg-final { scan-tree-dump-not "Executing store motion of" "lim2" } } */ - +/* { dg-final { scan-tree-dump "Executing store motion of count from loop 2" "lim2" } } */ +/* { dg-final { scan-tree-dump "Executing store motion of \[^ \]*data1 from loop 2" "lim2" } } */ +/* { dg-final { scan-tree-dump-times "Executing store motion of" 2 "lim2" } } */ -- 2.35.3