From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 607E83858C5F; Wed, 15 Mar 2023 14:39:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 607E83858C5F 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 18885219BC; Wed, 15 Mar 2023 14:39:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1678891184; 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=1ulrykVXwUAPiTPu1QqrRyd1bhS5UiOThj4zTjklnw0=; b=FM8yXdM6LRx30gVpsDrnslrcy9g6zwq5Dz1CBvMpBfIBrwH2xQ5QIgb6tUdSJPG8bVZ8AI KAJDaStCeP1vHORhgecyW/hRJT118n0+9UGH3uBwaLD2kvaPYWmC+k9qOPoBA7RqU69Ajc aljpsbJgRC5IgvVGbkBM6HiKEqmNUoE= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1678891184; 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=1ulrykVXwUAPiTPu1QqrRyd1bhS5UiOThj4zTjklnw0=; b=6qKr6iysRkEHCKoJ4bF6fFEaUlImJCwUWKJB6WnOi//gqy+Iw3DE5TteEtnj+snVQV1TAl NuPvygu6AFHSqyAg== 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 07EE32C141; Wed, 15 Mar 2023 14:39:44 +0000 (UTC) Date: Wed, 15 Mar 2023 14:39:43 +0000 (UTC) From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org, manu@gcc.gnu.org Subject: Re: [PATCH 2/2] tree-optimization/109123 - run -Wuse-afer-free only early In-Reply-To: Message-ID: References: <20230315104924.7F449385828D@sourceware.org> 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 Wed, 15 Mar 2023, Richard Biener wrote: > On Wed, 15 Mar 2023, Jakub Jelinek wrote: > > > On Wed, Mar 15, 2023 at 10:49:19AM +0000, Richard Biener via Gcc-patches wrote: > > > The following switches the -Wuse-after-free diagnostics from emitted > > > during the late access warning passes to the early access warning > > > passes to make sure we run before passes performing code motion run > > > which are the source of a lot of false positives on use-after-free > > > not involving memory operations. > > > > > > The patch also fixes issues in c-c++-common/Wuse-after-free-6.c and > > > g++.dg/warn/Wuse-after-free3.C. > > > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu (without 1/2 > > > sofar, but its testcase XFAILed). > > > > > > OK? > > > > > > Thanks, > > > Richard. > > > > > > PR tree-optimization/109123 > > > * gimple-ssa-warn-access.cc (pass_waccess::warn_invalid_pointer): > > > Do not emit -Wuse-after-free late. > > > (pass_waccess::check_call): Always check call pointer uses. > > > > > > * gcc.dg/Wuse-after-free-pr109123.c: New testcase. > > > * c-c++-common/Wuse-after-free-6.c: Un-XFAIL case. > > > * g++.dg/warn/Wuse-after-free3.C: Remove expected duplicate > > > diagnostic. > > > > I guess this is this is related to the never ending debate of what to > > do with the middle end late warnings, whether we should kill them > > altogether, move into analyzer framework and/or run early but perform > > IPA analysis for them. > > Yes. And for -Wuse-after-free also whether value-uses are considered > or not, but changing that has a much bigger testsuite fallout. > > > Doing the diagnostics in the early waccess pass is certainly fine > > no matter what, it will emit there more accurate diagnostics. > > A big question is whether to avoid diagnosing it late as well (other option > > is just make sure we don't diagnose same statements twice, once in early > > waccess and once in late waccess, which can be solved with warning > > suppressions (unless it is already properly suppressed)), especially for > > GCC 13. > > I see we have two copies of early waccess (so, we need to get the > > suppression right even if the warning is just moved to early waccess), > > one is very shortly after building ssa, so before early inlining, > > then there is another early one for -O+ non-Og shortly after IPA, > > and the late one is immediately before expansion. > > Yep, I think the suppression should already work. > > > So, the question is if the ccp after IPA is sufficient to uncover > > the most of the use after free issues we want to diagnose, if yes, > > then perhaps your patch is ok but we need to decide what to do about > > -Og, whether we shouldn't add > > NEXT_PASS (pass_warn_access, /*early=*/true); > > into -Og pass queue after > > NEXT_PASS (pass_object_sizes); > > or so. > > That would be possible for sure. But then there's not much > done between the late early and the late pass so the late pass > could run in both modes (but I don't feel like massaging that too much > at this point). > > There's also -O0 where the late pass catches all always-inlined > cases. > > > I think even the pre-IPA optimizations can do some harm for use after free > > false positives, but hopefully not as much as the post IPA optimizations. > > We do some very limited jump-threading but I don't think there's much > code-motion going on. I've tested and most of the -Wuse-after-free testcases work with -Og before and after the change (unfortunately adding -Og via RUNTESTFLAGS doesn't work since explicit -O2 in the testcases overrides that). We also do not have much test coverage here and I'm hesitant to change this now. That said, with the adjusted first patch and the split out fix for g++.dg/warn/Wuse-after-free3.C the following is what I propose now. There's extra adjustments in gcc.dg/Wuse-after-free-2.c as we now mention the pointer that is used. I do think not running these kind of diagnostics very late, at least a diagnostic like this that's prone to false positives due to code motion. But I'm also happy to leave the state of affairs as-is for GCC 13, but I don't promise to be able to pick up things during next stage1 again. Re-re-re-bootstrap & regtest running on x86_64-unknown-linux-gnu. Unless I get clear opinions I'm going to throw some dice whether to apply 1/2 and will "defer" this one. Thanks, Richard. >From c1ac6d319c76fea434080c0f5c2b779e5cea0446 Mon Sep 17 00:00:00 2001 From: Richard Biener Date: Wed, 15 Mar 2023 09:12:33 +0100 Subject: [PATCH] tree-optimization/109123 - run -Wuse-afer-free only early To: gcc-patches@gcc.gnu.org The following switches the -Wuse-after-free diagnostics from emitted during the late access warning passes to the early access warning passes to make sure we run before passes performing code motion run which are the source of a lot of false positives on use-after-free not involving memory operations. The patch also fixes an issue in c-c++-common/Wuse-after-free-6.c and causes the name of the unused pointer to appear in the diagnostic for extra cases in gcc.dg/Wuse-after-free-2.c PR tree-optimization/109123 * gimple-ssa-warn-access.cc (pass_waccess::warn_invalid_pointer): Do not emit -Wuse-after-free late. (pass_waccess::check_call): Always check call pointer uses. * gcc.dg/Wuse-after-free-pr109123.c: New testcase. * gcc.dg/Wuse-after-free-2.c: Amend expected diagnostic with the name of the pointer. * c-c++-common/Wuse-after-free-6.c: Un-XFAIL case. --- gcc/gimple-ssa-warn-access.cc | 28 ++++++------- .../c-c++-common/Wuse-after-free-6.c | 2 +- gcc/testsuite/gcc.dg/Wuse-after-free-2.c | 8 ++-- .../gcc.dg/Wuse-after-free-pr109123.c | 41 +++++++++++++++++++ 4 files changed, 60 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/Wuse-after-free-pr109123.c diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index fbb9b10fa75..88d44690ade 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -3907,7 +3907,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt, if (is_gimple_call (inval_stmt)) { - if ((equality && warn_use_after_free < 3) + if (!m_early_checks_p + || (equality && warn_use_after_free < 3) || (maybe && warn_use_after_free < 2) || warning_suppressed_p (use_stmt, OPT_Wuse_after_free)) return; @@ -4303,19 +4304,18 @@ pass_waccess::check_call (gcall *stmt) if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)) check_builtin (stmt); - if (!m_early_checks_p) - if (tree callee = gimple_call_fndecl (stmt)) - { - /* Check for uses of the pointer passed to either a standard - or a user-defined deallocation function. */ - unsigned argno = fndecl_dealloc_argno (callee); - if (argno < (unsigned) call_nargs (stmt)) - { - tree arg = call_arg (stmt, argno); - if (TREE_CODE (arg) == SSA_NAME) - check_pointer_uses (stmt, arg); - } - } + if (tree callee = gimple_call_fndecl (stmt)) + { + /* Check for uses of the pointer passed to either a standard + or a user-defined deallocation function. */ + unsigned argno = fndecl_dealloc_argno (callee); + if (argno < (unsigned) call_nargs (stmt)) + { + tree arg = call_arg (stmt, argno); + if (TREE_CODE (arg) == SSA_NAME) + check_pointer_uses (stmt, arg); + } + } check_call_access (stmt); check_call_dangling (stmt); diff --git a/gcc/testsuite/c-c++-common/Wuse-after-free-6.c b/gcc/testsuite/c-c++-common/Wuse-after-free-6.c index 581b1a0a024..0c17a2545f4 100644 --- a/gcc/testsuite/c-c++-common/Wuse-after-free-6.c +++ b/gcc/testsuite/c-c++-common/Wuse-after-free-6.c @@ -53,7 +53,7 @@ void* warn_cond_return_after_free (void *p, int c) free (p); // PHI handling not fully implemented. if (c) - return p; // { dg-warning "pointer 'p' may be used" "pr??????" { xfail *-*-* } } + return p; // { dg-warning "pointer 'p' may be used" } return 0; } diff --git a/gcc/testsuite/gcc.dg/Wuse-after-free-2.c b/gcc/testsuite/gcc.dg/Wuse-after-free-2.c index ebc051690db..ac174fdd104 100644 --- a/gcc/testsuite/gcc.dg/Wuse-after-free-2.c +++ b/gcc/testsuite/gcc.dg/Wuse-after-free-2.c @@ -76,7 +76,7 @@ void warn_cond_2_cst (char *p, int i) char *r = i ? p + 1 : p + 2; free (p); // { dg-message "call to 'free'" } - sink (r); // { dg-warning "pointer used after 'free'" } + sink (r); // { dg-warning "pointer 'r' used after 'free'" } } void warn_cond_2_var (char *p, int i, int j) @@ -84,7 +84,7 @@ void warn_cond_2_var (char *p, int i, int j) char *r = i ? p + i : p + j; free (p); // { dg-message "call to 'free'" } - sink (r); // { dg-warning "pointer used after 'free'" } + sink (r); // { dg-warning "pointer 'r' used after 'free'" } } void warn_cond_3_var (char *p0, int i, int j) @@ -92,7 +92,7 @@ void warn_cond_3_var (char *p0, int i, int j) char *r = i < 0 ? p0 - i : 0 < i ? p0 + j : p0 + i + j; free (p0); // { dg-message "call to 'free'" } - sink (r + 1); // { dg-warning "pointer used after 'free'" } + sink (r + 1); // { dg-warning "pointer 'r' used after 'free'" } } int warn_cond_4 (char *p0, char *q0, int i) @@ -100,7 +100,7 @@ int warn_cond_4 (char *p0, char *q0, int i) char *r = i < -1 ? p0 - 2 : i < 0 ? p0 - 1 : 1 < i ? p0 + 2 : p0 + 1; free (p0); // { dg-message "call to 'free'" } - return *r; // { dg-warning "pointer used after 'free'" } + return *r; // { dg-warning "pointer 'r' used after 'free'" } } int warn_cond_loop (char *p) diff --git a/gcc/testsuite/gcc.dg/Wuse-after-free-pr109123.c b/gcc/testsuite/gcc.dg/Wuse-after-free-pr109123.c new file mode 100644 index 00000000000..ece066dd28b --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wuse-after-free-pr109123.c @@ -0,0 +1,41 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wall" } */ + +typedef long unsigned int size_t; +extern void *realloc (void *__ptr, size_t __size) + __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__warn_unused_result__)) __attribute__ ((__alloc_size__ (2))); +struct vector_objective; +typedef struct vector_objective vector_objective; +struct vector_objective { double *_begin; double *_end; double *_capacity; }; +static inline size_t vector_objective_size(const vector_objective * v) { + return v->_end - v->_begin; /* { dg-bogus "used after" } */ +} +static inline size_t vector_objective_capacity(const vector_objective * v) { + return v->_capacity - v->_begin; +} +static inline void vector_objective_reserve(vector_objective * v, size_t n) { + size_t old_capacity = vector_objective_capacity(v); + size_t old_size = vector_objective_size(v); + if (n > old_capacity) { + v->_begin = realloc(v->_begin, sizeof(double) * n); + v->_end = v->_begin + old_size; + v->_capacity = v->_begin + n; + } +} +static inline void vector_objective_push_back(vector_objective * v, double x) { + if (v->_end == v->_capacity) + vector_objective_reserve (v, (vector_objective_capacity (v) == 0) ? 8 : 2 * vector_objective_capacity (v)); + *(v->_end) = x; + v->_end++; +} + +typedef struct { + vector_objective xy; +} eaf_polygon_t; + +int +rectangle_add(eaf_polygon_t * regions, double lx) +{ + vector_objective_push_back(®ions->xy, lx); + return 0; +} -- 2.35.3