Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]
On 25.03.22 12:34, Jakub Jelinek via Fortran wrote: What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i; }, is that applying the side-effects 11 times or once ? For side effects during the evaluation of expression, Fortran has a clear "if you depend on it, it's your fault" rule. In F 2018, it says 10.1.7 Evaluation of operands 1 It is not necessary for a processor to evaluate all of the operands of an expression, or to evaluate entirely each operand, if the value of the expression can be determined otherwise. Also, the semantics of a(a:b) = expr say that the expression on the LHS is evaluated only once before assignment. So, anything that looks like that should be translated to tmp = ++i; [0..10] = tmp;
[committed] analyzer: fix ICE on memset of untracked region [PR105057]
In r12-7809-g5f6197d7c197f9d2b7fb2e1a19dac39a023755e8 I added an optimization to avoid tracking the state of certain memory regions in the store. Unfortunately, I didn't cover every way in which store::get_or_create_cluster can be called for a base region, leading to assertion failure ICEs in -fanalyzer on certain function calls with certain params. I've worked through all uses of store::get_or_create_cluster and found four places where the assertion could fire. This patch fixes them, and adds regression tests where possible. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r12-7827-g8c8993c75309901e03418eba1d6239b9a39a43b7. gcc/analyzer/ChangeLog: PR analyzer/105057 * store.cc (binding_cluster::make_unknown_relative_to): Reject attempts to create a cluster for untracked base regions. (store::set_value): Likewise. (store::fill_region): Likewise. (store::mark_region_as_unknown): Likewise. gcc/testsuite/ChangeLog: PR analyzer/105057 * gcc.dg/analyzer/fread-2.c: New test, as a regression test for ICE in store::set_value on untracked base region. * gcc.dg/analyzer/memset-2.c: Likewise, for ICE in store::fill_region. * gcc.dg/analyzer/strcpy-2.c: Likewise, for ICE in store::mark_region_as_unknown. Signed-off-by: David Malcolm --- gcc/analyzer/store.cc| 17 ++--- gcc/testsuite/gcc.dg/analyzer/fread-2.c | 31 gcc/testsuite/gcc.dg/analyzer/memset-2.c | 27 + gcc/testsuite/gcc.dg/analyzer/strcpy-2.c | 27 + 4 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/fread-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/memset-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/strcpy-2.c diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index 62733b9777f..9aa7d690b04 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -1823,7 +1823,8 @@ binding_cluster::make_unknown_relative_to (const binding_cluster *other, { const region *base_reg = region_sval->get_pointee ()->get_base_region (); - if (!base_reg->symbolic_for_unknown_ptr_p ()) + if (base_reg->tracked_p () + && !base_reg->symbolic_for_unknown_ptr_p ()) { binding_cluster *c = out_store->get_or_create_cluster (base_reg); c->mark_as_escaped (); @@ -2384,11 +2385,17 @@ store::set_value (store_manager *mgr, const region *lhs_reg, mark_as_escaped (ptr_base_reg); } } - else + else if (lhs_base_reg->tracked_p ()) { lhs_cluster = get_or_create_cluster (lhs_base_reg); lhs_cluster->bind (mgr, lhs_reg, rhs_sval); } + else +{ + /* Reject attempting to bind values into an untracked region; +merely invalidate values below. */ + lhs_cluster = NULL; +} /* Bindings to a cluster can affect other clusters if a symbolic base region is involved. @@ -2564,7 +2571,8 @@ void store::fill_region (store_manager *mgr, const region *reg, const svalue *sval) { const region *base_reg = reg->get_base_region (); - if (base_reg->symbolic_for_unknown_ptr_p ()) + if (base_reg->symbolic_for_unknown_ptr_p () + || !base_reg->tracked_p ()) return; binding_cluster *cluster = get_or_create_cluster (base_reg); cluster->fill_region (mgr, reg, sval); @@ -2587,7 +2595,8 @@ store::mark_region_as_unknown (store_manager *mgr, const region *reg, uncertainty_t *uncertainty) { const region *base_reg = reg->get_base_region (); - if (base_reg->symbolic_for_unknown_ptr_p ()) + if (base_reg->symbolic_for_unknown_ptr_p () + || !base_reg->tracked_p ()) return; binding_cluster *cluster = get_or_create_cluster (base_reg); cluster->mark_region_as_unknown (mgr, reg, uncertainty); diff --git a/gcc/testsuite/gcc.dg/analyzer/fread-2.c b/gcc/testsuite/gcc.dg/analyzer/fread-2.c new file mode 100644 index 000..02a5e31cec6 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/fread-2.c @@ -0,0 +1,31 @@ +/* { dg-additional-options "-fdump-analyzer-untracked" } */ + +#include "analyzer-decls.h" + +struct S +{ + int i; +}; + +typedef __SIZE_TYPE__ size_t; + +extern size_t fread (void *, size_t, size_t, void *); + +/* fread of a static struct that never gets used. */ + +void +test_1 (void *fp) +{ + static struct S s; /* { dg-warning "track 's': no" } */ + fread (&s, sizeof (s), 1, fp); +} + +/* fread of a static struct that later gets used. */ + +int +test_2 (void *fp) +{ + static struct S s; /* { dg-warning "track 's': yes" } */ + fread (&s, sizeof (s), 1, fp); + return s.i; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/memset-2.c b/gcc/testsuite/gcc.dg/analyzer/memset-2.c new file mode 100644 index 000..de7c973b1fd --- /dev/null +++ b/gcc/testsui
Re: [PATCH] middle-end/100786 - constant folding from incompatible alias
Hi Richard, The patch for PR100786 introduced a testcase that systematically fails on darwin: FAIL: gcc.dg/torture/pr100786.c -O0 (test for excess errors) FAIL: gcc.dg/torture/pr100786.c -O1 (test for excess errors) FAIL: gcc.dg/torture/pr100786.c -O2 (test for excess errors) FAIL: gcc.dg/torture/pr100786.c -O2 -flto (test for excess errors) FAIL: gcc.dg/torture/pr100786.c -O2 -flto -flto-partition=none (test for excess errors) FAIL: gcc.dg/torture/pr100786.c -O3 -g (test for excess errors) FAIL: gcc.dg/torture/pr100786.c -Os (test for excess errors) see https://gcc.gnu.org/pipermail/gcc-testresults/2022-March/757882.html because pr100786.c:4:12: error: only weak aliases are supported in this configuration 4 | extern int b __attribute__((alias("a"))); |^ pr100786.c:8:15: error: only weak aliases are supported in this configuration 8 | extern double b2 __attribute__((alias("a2"))); | ^~ FX
Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]
> Am 26.03.2022 um 12:28 schrieb Thomas Koenig : > > On 25.03.22 12:34, Jakub Jelinek via Fortran wrote: >> What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i; >> }, is that applying the side-effects 11 times or once ? > > For side effects during the evaluation of expression, Fortran has a > clear "if you depend on it, it's your fault" rule. In F 2018, it says > > 10.1.7 Evaluation of operands > > 1 It is not necessary for a processor to evaluate all of the operands of > an expression, or to evaluate entirely each operand, if the value of the > expression can be determined otherwise. > > Also, the semantics of > > a(a:b) = expr > > say that the expression on the LHS is evaluated only once before > assignment. So, anything that looks like that should be translated > to > > tmp = ++i; > [0..10] = tmp; Note I was not trying to question middle-end semantic here but gfortran se_expr (?) one. Is there a Fortan input where Jakob’s patch would cause a side-effect to be dropped and is that valid? Richard. > >
RE: [PATCH] Ignore (possible) signed zeros in operands of FP comparisons.
Hi Aldy, The proposed frange implementation looks cool. The one technical tweak is that if x is not NaN and not +Inf/-Inf, then x*0.0 is [-0.0,0.0]. It's because this result is a range and not a constant that it can’t normally be constant folded, unless it appears in a context where the sign of zero isn't important (such as a comparison). I suspect that frange needs pos_zero_p, neg_zero_p and any_zero_p instead of just zero_p (and variants r.set_zero) to capture the subtleties. Cheers, Roger -- > -Original Message- > From: Aldy Hernandez > Sent: 21 March 2022 05:56 > To: Jeff Law > Cc: Andrew MacLeod ; Roger Sayle > ; Richard Biener > ; GCC Patches > Subject: Re: [PATCH] Ignore (possible) signed zeros in operands of FP > comparisons. > > On Fri, Mar 18, 2022 at 7:33 PM Aldy Hernandez wrote: > > > > > Consider the following interesting example: > > > > > > > > int foo(int x, double y) { > > > > return (x * 0.0) < y; > > > > } > > > > > > > > Although we know that x (when converted to double) can't be > > > > NaN or Inf, we still worry that for negative values of x that > > > > (x * 0.0) may be -0.0 and so perform the multiplication at > > > > run-time. But in this case, the result of the comparison (-0.0 > > > > < y) will be exactly the same as (+0.0 < y) for any y, hence > > > > the above may be safely constant folded to "0.0 < > > > y" > > > > avoiding the multiplication at run-time. > > Ok, once the "frange" infrastructure is in place, it's actually trivial. See > attached > patch and tests. We can do everything with small range-op entries and evrp / > ranger will handle everything else. > > Roger, I believe this is what you described: > > +// { dg-do compile } > +// { dg-options "-O2 -fno-tree-forwprop -fno-thread-jumps > -fdump-tree-evrp -fdump-tree-optimized" } > + > +extern void link_error (); > + > +int foo(int x, double y) > +{ > + return (x * 0.0) < y; > +} > + > +// The multiply should be gone by *vrp time. > +// { dg-final { scan-tree-dump-not " \\* " "evrp" } } > + > +// Ultimately, there sound be no references to "x". > +// { dg-final { scan-tree-dump-not "x_" "optimized" } } > > With the attached patch (and pending patches), we keep track that a cast from > int to float is guaranteed to not be NaN, which allows us to fold x*0.0 into > 0.0, > and DCE to remove x altogether. Also, as the other tests show, we are able to > resolve __builtin_isnan's for the operations implemented. It should be > straightforward for someone with floating point foo to extend this to all > operations. > > Aldy
Remove mysterious '-# Defining these options here in addition to common.opt is necessary' command-line option (was: [PATCH v2 1/2] add -Wuse-after-free)
Hi! On 2022-01-15T17:00:11-0700, Martin Sebor via Gcc-patches wrote: > On 1/11/22 15:40, Jason Merrill wrote: >> On 11/30/21 17:32, Martin Sebor via Gcc-patches wrote: >>> [default setting of the option] >> Let's put =2 in -Wall for now. > I've adjusted [...] and pushed r12-6605 [...] Pushed to master branch commit 43911ddd18b97d8ebd17d2959f36efa539d359b7 "Remove mysterious '-# Defining these options here in addition to common.opt is necessary' command-line option", see attached. ;'-) Grüße Thomas - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 >From 43911ddd18b97d8ebd17d2959f36efa539d359b7 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Thu, 24 Mar 2022 22:34:30 +0100 Subject: [PATCH] Remove mysterious '-# Defining these options here in addition to common.opt is necessary' command-line option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: $ [...]/gcc '-# Defining these options here in addition to common.opt is necessary' -S -x c /dev/null && echo MYSTERIOUS MYSTERIOUS After: $ [...]/gcc '-# Defining these options here in addition to common.opt is necessary' -S -x c /dev/null && echo MYSTERIOUS gcc: error: unrecognized command-line option ‘-# Defining these options here in addition to common.opt is necessary’ This commit changes: --- [...]/build-gcc/gcc/optionlist 2022-03-24 22:12:07.936746648 +0100 +++ [...]/build-gcc/gcc/optionlist 2022-03-24 22:30:06.976737341 +0100 @@ -1,4 +1,3 @@ -# Defining these options here in addition to common.opt is necessary# in order for the default -Wall setting of -Wuse-after-free=2 to take# effect. ###Driver -all-warningsAda AdaWhy AdaSCIL Alias(Wall) -all-warningsC ObjC C++ ObjC++ Warning Alias(Wall) [...] --- [...]/build-gcc/gcc/options.cc 2022-03-24 22:12:09.548727738 +0100 +++ [...]/build-gcc/gcc/options.cc 2022-03-24 22:30:08.904727249 +0100 @@ -3222,15 +3222,6 @@ const struct cl_option cl_options[] = { /* [0] = */ { -"-# Defining these options here in addition to common.opt is necessary", -"# effect.", -NULL, -NULL, -NULL, NULL, N_OPTS, N_OPTS, 68, /* .neg_idx = */ -1, -0, -0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, -(unsigned short) -1, 0, CLVC_INTEGER, 0, -1, -1 }, - /* [1] = */ { "-###", NULL, NULL, [...] ..., and re-numbers all following options. --- [...]/build-gcc/gcc/options.h 2022-03-21 23:24:25.894226828 +0100 +++ [...]/build-gcc/gcc/options.h 2022-03-24 22:30:07.288735708 +0100 @@ -9753,2118 +9753,2117 @@ enum opt_code { - OPT___Defining_these_options_here_in_addition_to_common_opt_is_necessary = 0,/* -# Defining these options here in addition to common.opt is necessary */ [...] ..., and likewise re-numbers all following options. Clean-up for commit 671a283636de75f7ed638ee6b01ed2d44361b8b6 "Add -Wuse-after-free [PR80532]". gcc/c-family/ * c.opt: Properly quote comment. --- gcc/c-family/c.opt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 9a4828ebe37..790d47caf0a 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1377,9 +1377,9 @@ Wunused-const-variable= C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_unused_const_variable) Warning LangEnabledBy(C ObjC,Wunused-variable, 1, 0) IntegerRange(0, 2) Warn when a const variable is unused. -# Defining these options here in addition to common.opt is necessary -# in order for the default -Wall setting of -Wuse-after-free=2 to take -# effect. +; Defining these options here in addition to common.opt is necessary +; in order for the default -Wall setting of -Wuse-after-free=2 to take +; effect. Wuse-after-free LangEnabledBy(C ObjC C++ LTO ObjC++) -- 2.35.1
Re: [PATCH RFC] Fix ICE due to shared BLOCK node in coroutine generation [PR103328]
On 3/17/22 07:37, Benno Evers via Gcc-patches wrote: The coroutine transformation moves the original function body into a newly created actor function, but the block of the `current_binding_level` still points into the original function, causing the block to be shared between the two functions if it is subsequently used. This may cause havoc later on, as subsequent compiler passes for one function will also implicitly modify the other. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103328#c19 for a more detailed writeup. This patch fixes the issue locally, but I'm not familiar with the GCC code base so I'm not sure if this is the right way to go about it or if there's a cleaner way to reset the current binding level. If this is the way to go I'm happy to extend the patch with a testcase and changelog entry. Please do, it looks like a good fix to me. Iain, does it make sense to you as well? --- gcc/cp/coroutines.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index b1bfdc767a4..eb5f80f499b 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -4541,6 +4541,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) BLOCK_VARS (top_block) = BIND_EXPR_VARS (ramp_bind); BLOCK_SUBBLOCKS (top_block) = NULL_TREE; + current_binding_level->blocks = top_block; + /* The decl_expr for the coro frame pointer, initialize to zero so that we can pass it to the IFN_CO_FRAME (since there's no way to pass a type, directly apparently). This avoids a "used uninitialized" warning. */