[obvious, PATCH 4/3][doc] -Wuninitialized doesn't do -Wclobbered's job

[Vladislav Ivanishin]

Oh, and this old documentation bug.  (AFAICT this is a historical
artifact from the pre-tree-ssa times when both -Wclobbered and
-Wuninitialized were implemented on RTL.)

This one is actually independent of the others and, I believe, is
obvious enough.  I am going to install tomorrow.

>From 1ddc87c39b5bf938fd4838c86d19ceeb20d518d8 Mon Sep 17 00:00:00 2001
From: Vladislav Ivanishin 
Date: Thu, 3 Oct 2019 17:01:48 +0300
Subject: [PATCH 4/4] [doc] -Wuninitialized doesn't do -Wclobbered's job

* gcc/doc/invoke.texi (-Wuninitialized): Don't mention the clobbered by
setjmp situation here.  Fix a verb's ending: "the exact variables or
elements for which there are warnings depends" -> "... depend".
---
 gcc/doc/invoke.texi | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 77a2d561e38..36997d9fc7d 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -5576,21 +5576,20 @@ either specify @option{-Wextra -Wunused} (note that @option{-Wall} implies
 @item -Wuninitialized
 @opindex Wuninitialized
 @opindex Wno-uninitialized
-Warn if an automatic variable is used without first being initialized
-or if a variable may be clobbered by a @code{setjmp} call. In C++,
-warn if a non-static reference or non-static @code{const} member
-appears in a class without constructors.
+Warn if an automatic variable is used without first being initialized.
+In C++, warn if a non-static reference or non-static @code{const}
+member appears in a class without constructors.
 
 If you want to warn about code that uses the uninitialized value of the
 variable in its own initializer, use the @option{-Winit-self} option.
 
-These warnings occur for individual uninitialized or clobbered
-elements of structure, union or array variables as well as for
-variables that are uninitialized or clobbered as a whole.  They do
-not occur for variables or elements declared @code{volatile}.  Because
-these warnings depend on optimization, the exact variables or elements
-for which there are warnings depends on the precise optimization
-options and version of GCC used.
+These warnings occur for individual uninitialized elements of
+structure, union or array variables as well as for variables that are
+uninitialized as a whole.  They do not occur for variables or elements
+declared @code{volatile}.  Because these warnings depend on
+optimization, the exact variables or elements for which there are
+warnings depend on the precise optimization options and version of GCC
+used.
 
 Note that there may be no warning about a variable that is used only
 to compute a value that itself is never used, because such
-- 
2.22.0

[PATCH 3/3] Implementation of -Wclobbered on tree-ssa

[Vladislav Ivanishin]

What it does


A fundamental limitation of the new approach is that it requires phi
nodes for variables to perform the analysis it needs to issue the
warning for them.  No phis - no warning.  In particular, it doesn't deal
with register asm variables (see gcc.target/i386/attr-returns_twice-1.c
which I xfailed) because of how they are currently implemented in GCC.
Also, in theory there can be struct members that SRA doesn't scalarize,
but some other compiler might.

Another "downgrade" from the old implementation is that currently, the
new pass is placed in such a position in the pipeline that it requires
optimize > 0 in order to be run.  But there's nothing fundamental about
it; that is something I just haven't experimented with.

I placed the new pass in tree-ssa-uninit.c, because I used static
functions defined there.  The pass ended up using only struct pred_info
and is_pred_expr_subset_of().

The main PR tracking the problems of the old implementation is 21161.
I also went through the "See also" list of PRs for it.  This new
implementation addresses them all, and the only failing test I saw for
x86_64-pc-linux-gnu is the already mentioned register asm test.

The existing RTL implementation doesn't check return values of
setjmp/vfork and thus does not distinguish the paths only realizable
upon a non-abnormal [*] return (this is what Wclobbered-pr21161-2.c is
about).  The new implementation does deal with it.

How it does it
==
Though I think, the comments for the new pass are extensive, they are
scattered throughout the code, so here's a brief overview of the patch.

The pass considers all returns-twice functions, though {,_,__}setjmp,
{,_,__}sigsetjmp, and vfork are special in that we know the meaning of
their return values and that allows to suppress false positives (see
below [fp]).  I refer to the "setjmp-like (returns-twice) function call
site" as the setjmp_bb or simply "setjmp" in the code and the comments.
Without losing generality, let's do the same for the purpose of this
discussion.

The representation currently used for setjmp calls is convenient enough
for the analysis.  There is one ABNORMAL_DISPATCHER block per function
and abnormal edges from it to all the setjmps in the function. (A setjmp
call is the first non-debug instruction in its block).  Also, there is
an abnormal edge for every function call leading from the point right
after the call to the abnormal dispatcher.

Now let me walk you through the code in the order of execution (leading
to producing a warning).

First, the gate() method checks whether calls_setjmp is set for the
function.  No need to do anything if it isn't.

Then the main driver - warn_clobbered_setjmp() - is run.  It iterates
over all the setjmps.  For a setjmp, all phi functions in its block are
considered.  The basic idea is to warn when there's a redefinition of a
variable on the path from setjmp to a longjmp call (i.e. to the abnormal
dispatcher).  That redefinition is exactly the thing we intend to warn
about: its effect might not be visible (the value might get clobbered)
after an abnormal return from setjmp - when some register values get
restored from a stash.  A warning is warranted in such a situation
(given that there's also an initial definition flowing into the phi),
because the value of the variable upon an abnormal return from setjmp is
potentially ambiguous (unspecified).  phi_opnd_phantom_redefinition()
discovers such redefinitions.

It does so by "recursively" (via a worklist of phis) walking the phi's
operands.  So when we find an actual non-phi definition we already know
that it flows into the phi in the setjmp's basic block.  We also need to
check that the redefinition is actually reachable from the setjmp and
that it is actually a *re*definition flowing into the setjmp bb via an
abnormal edge (what we actually check is that it flows into the abnormal
dispatcher).

[fp] Now back to the driver.  In some cases such a redefinition may not
do any real harm: if the unspecified value is never used.  We now walk
all ultimate uses of the phi in all_uses_reference_specified_values_p()
and check that all SSA values that reach a use (via phis) are not
unspecified in all_phi_args_are_specified_p().  This is where the
knowledge of the meaning of return values comes into play.  In case of
the setjmp family and vfork, the return value indicates whether this is
the first (normal) or a subsequent (abnormal) return.  And the value may
only be unspecified upon an abnormal return.  So along with plain
reachability we check for compatibility of predicates and thus suppress
false positives on programs such as Wclobbered-pr21161-2.c.

I don't know how to check that a function is one of *setjmp or vfork
reliably, so I copied some code from special_function_p that uses string
comparison (see setjmp_or_vfork_p in the patch).  One option would be to
have builtins for all kinds of setjmp and vfork and compare against
enum built_in_function 

[PATCH 2/3] Remove the old implementation of -Wclobbered

[Vladislav Ivanishin]

This patch removes the old implementation of the warning on RTL.

Note: regstat_get_setjmp_crosses() and the associated data seems to be
needed for the RA itself, so I am not touching that.

>From c86a8fdc3f91d3cd33d73c7063bb9be2a7ae7e1f Mon Sep 17 00:00:00 2001
From: Vladislav Ivanishin 
Date: Thu, 29 Aug 2019 15:14:59 +0300
Subject: [PATCH 2/3] Remove the old implementation of -Wclobbered

gcc/

* function.c (regno_clobbered_at_setjmp): Remove function.
(setjmp_vars_warning): Remove function.
(setjmp_args_warning): Remove function.
(generate_setjmp_warnings): Remove function.
* gcc/ira.c (ira): Remove the call to generate_setjmp_warnings.
---
 gcc/function.c | 76 --
 gcc/ira.c  |  6 
 2 files changed, 82 deletions(-)

diff --git a/gcc/function.c b/gcc/function.c
index 2a0061cad35..8a8e548c303 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4181,82 +4181,6 @@ pad_below (struct args_size *offset_ptr, machine_mode passed_mode, tree sizetree
 }
 
 
-/* True if register REGNO was alive at a place where `setjmp' was
-   called and was set more than once or is an argument.  Such regs may
-   be clobbered by `longjmp'.  */
-
-static bool
-regno_clobbered_at_setjmp (bitmap setjmp_crosses, int regno)
-{
-  /* There appear to be cases where some local vars never reach the
- backend but have bogus regnos.  */
-  if (regno >= max_reg_num ())
-return false;
-
-  return ((REG_N_SETS (regno) > 1
-	   || REGNO_REG_SET_P (df_get_live_out (ENTRY_BLOCK_PTR_FOR_FN (cfun)),
-			   regno))
-	  && REGNO_REG_SET_P (setjmp_crosses, regno));
-}
-
-/* Walk the tree of blocks describing the binding levels within a
-   function and warn about variables the might be killed by setjmp or
-   vfork.  This is done after calling flow_analysis before register
-   allocation since that will clobber the pseudo-regs to hard
-   regs.  */
-
-static void
-setjmp_vars_warning (bitmap setjmp_crosses, tree block)
-{
-  tree decl, sub;
-
-  for (decl = BLOCK_VARS (block); decl; decl = DECL_CHAIN (decl))
-{
-  if (VAR_P (decl)
-	  && DECL_RTL_SET_P (decl)
-	  && REG_P (DECL_RTL (decl))
-	  && regno_clobbered_at_setjmp (setjmp_crosses, REGNO (DECL_RTL (decl
-	warning (OPT_Wclobbered, "variable %q+D might be clobbered by"
- " % or %", decl);
-}
-
-  for (sub = BLOCK_SUBBLOCKS (block); sub; sub = BLOCK_CHAIN (sub))
-setjmp_vars_warning (setjmp_crosses, sub);
-}
-
-/* Do the appropriate part of setjmp_vars_warning
-   but for arguments instead of local variables.  */
-
-static void
-setjmp_args_warning (bitmap setjmp_crosses)
-{
-  tree decl;
-  for (decl = DECL_ARGUMENTS (current_function_decl);
-   decl; decl = DECL_CHAIN (decl))
-if (DECL_RTL (decl) != 0
-	&& REG_P (DECL_RTL (decl))
-	&& regno_clobbered_at_setjmp (setjmp_crosses, REGNO (DECL_RTL (decl
-  warning (OPT_Wclobbered,
-   "argument %q+D might be clobbered by % or %",
-	   decl);
-}
-
-/* Generate warning messages for variables live across setjmp.  */
-
-void
-generate_setjmp_warnings (void)
-{
-  bitmap setjmp_crosses = regstat_get_setjmp_crosses ();
-
-  if (n_basic_blocks_for_fn (cfun) == NUM_FIXED_BLOCKS
-  || bitmap_empty_p (setjmp_crosses))
-return;
-
-  setjmp_vars_warning (setjmp_crosses, DECL_INITIAL (current_function_decl));
-  setjmp_args_warning (setjmp_crosses);
-}
-
-
 /* Reverse the order of elements in the fragment chain T of blocks,
and return the new head of the chain (old last element).
In addition to that clear BLOCK_SAME_RANGE flags when needed
diff --git a/gcc/ira.c b/gcc/ira.c
index c58daba6e79..e21778c2c9e 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5293,12 +5293,6 @@ ira (FILE *f)
   regstat_init_n_sets_and_refs ();
   regstat_compute_ri ();
 
-  /* If we are not optimizing, then this is the only place before
- register allocation where dataflow is done.  And that is needed
- to generate these warnings.  */
-  if (warn_clobbered)
-generate_setjmp_warnings ();
-
   if (resize_reg_info () && flag_ira_loop_pressure)
 ira_set_pseudo_classes (true, ira_dump_file);
 
-- 
2.22.0

[PATCH 1/3] Make gsi_next_nonvirtual_phi do what one expects

[Vladislav Ivanishin]

This refactoring makes gsi_next_nonvirtual_phi() always advance the
gimple phi iterator (regardless of whether the current phi is virtual or
not).  It matches the behavior of other gsi_next functions.

It can be applied independently of the other patches (but patch 3
depends on this one).

I regstrapped this patch separately on x86_64-pc-linux-gnu.  Ok?

>From cbf3fa8408f54baffbec79d304d930e17aa7231c Mon Sep 17 00:00:00 2001
From: Vladislav Ivanishin 
Date: Thu, 29 Aug 2019 14:41:06 +0300
Subject: [PATCH 1/3] Make gsi_next_nonvirtual_phi do what one expects

gcc/:

* gimple-iterator.h (gsi_next_nonvirtual_phi): Change the semantics to
match that of other gsi_next_* functions.  Adjust the comment.
(gsi_start_nonvirtual_phis): New function.
* ipa-icf.c (sem_function::compare_phi_node): Update uses of
gsi_next_nonvirtual_phi accordingly.  (No functional change.)
---
 gcc/gimple-iterator.h | 33 ++---
 gcc/ipa-icf.c | 11 ---
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h
index ee6f5b1f54d..ccd93d936be 100644
--- a/gcc/gimple-iterator.h
+++ b/gcc/gimple-iterator.h
@@ -325,28 +325,31 @@ gsi_one_nondebug_before_end_p (gimple_stmt_iterator i)
   return gsi_end_p (i);
 }
 
-/* Iterates I statement iterator to the next non-virtual statement.  */
+/* Advance I statement iterator to the next non-virtual GIMPLE_PHI
+   statement.  */
 
 static inline void
 gsi_next_nonvirtual_phi (gphi_iterator *i)
 {
-  gphi *phi;
-
-  if (gsi_end_p (*i))
-return;
-
-  phi = i->phi ();
-  gcc_assert (phi != NULL);
-
-  while (virtual_operand_p (gimple_phi_result (phi)))
+  do
 {
   gsi_next (i);
-
-  if (gsi_end_p (*i))
-	return;
-
-  phi = i->phi ();
 }
+  while (!gsi_end_p (*i) && virtual_operand_p (gimple_phi_result (i->phi (;
+}
+
+/* Return a new iterator pointing to the first non-virtual phi statement in
+   basic block BB.  */
+
+static inline gphi_iterator
+gsi_start_nonvirtual_phis (basic_block bb)
+{
+  gphi_iterator i = gsi_start_phis (bb);
+
+  if (!gsi_end_p (i) && virtual_operand_p (gimple_phi_result (i.phi (
+gsi_next_nonvirtual_phi ();
+
+  return i;
 }
 
 /* Return the basic block associated with this iterator.  */
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 2174fb7494c..24d5b0e6e6c 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1678,13 +1678,10 @@ sem_function::compare_phi_node (basic_block bb1, basic_block bb2)
   gcc_assert (bb1 != NULL);
   gcc_assert (bb2 != NULL);
 
-  si2 = gsi_start_phis (bb2);
-  for (si1 = gsi_start_phis (bb1); !gsi_end_p (si1);
-   gsi_next ())
+  si2 = gsi_start_nonvirtual_phis (bb2);
+  for (si1 = gsi_start_nonvirtual_phis (bb1); !gsi_end_p (si1);
+   gsi_next_nonvirtual_phi ())
 {
-  gsi_next_nonvirtual_phi ();
-  gsi_next_nonvirtual_phi ();
-
   if (gsi_end_p (si1) && gsi_end_p (si2))
 	break;
 
@@ -1721,7 +1718,7 @@ sem_function::compare_phi_node (basic_block bb1, basic_block bb2)
 	return return_false ();
 	}
 
-  gsi_next ();
+  gsi_next_nonvirtual_phi ();
 }
 
   return true;
-- 
2.22.0


Thanks,
Vlad

Reimplementation of -Wclobbered on GIMPLE SSA

[Vladislav Ivanishin]

Hi!

This series implements the -Wclobbered warning on GIMPLE in place of the
old RTL implementation.

This cover letter contains a high-level explanation of what is going on
and why.  The new implementation itself is in the patch 3.  I will post
the details in the accompanying blurb.

The most obvious benefit of doing it on GIMPLE is making this warning
independent of the specific register allocation decisions the RA makes.
I.e. if there is a possibility of a different compiler allocating a
variable to a register live through the setjmp call, but in the current
compilation it got spilled, the existing implementation does not give a
warning.

So the new implementation strives to indicate as many potential errors
in the code as it can, not limited to the problems pertinent to this
compilation with this particular compiler.  This is in line with
e.g. what Tom Lane (of PostgreSQL) expressed interest in seeing [1].

Also, in order to suppress spurious warnings, it is sometimes useful to
distinguish paths realizable after the second (and subsequent) returns
from setjmp vs. realizable only after the first return (see PR 21161).
The new implementation does that using a simple heuristic.

All comments are appreciated.  Is this OK for trunk?

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1185673#c5

Thanks,
Vlad

Ping: [PATCH 1/2] gdbhooks.py: use strip_typedefs to simplify matching type names

[Vladislav Ivanishin]

Vladislav Ivanishin  writes:

> David Malcolm  writes:
>
>> On Tue, 2019-07-02 at 14:29 +0300, Vladislav Ivanishin wrote:
>>> David Malcolm  writes:
>>> 
>>> > On Mon, 2019-07-01 at 12:50 +0300, Vladislav Ivanishin wrote:
>>> > > Hi!
>>> > > 
>>> > > GDB's Python API provides strip_typedefs method that can be
>>> > > instrumental for writing DRY code.  Using it at least partially
>>> > > obviates the need for the add_printer_for_types method we have in
>>> > > gdbhooks.py (it takes a list of typenames to match and is usually
>>> > > used to deal with typedefs).
>>> > > 
>>> > > I think, the code can be simplified further (there's still
>>> > > ['tree_node *', 'const tree_node *'], which is a little awkward)
>>> > > if we deal with pointer types in a uniform fashion (I'll touch on
>>> > > this in the description of the second patch).  But that can be
>>> > > improved separately.
>>> > > 
>>> > > The gimple_statement_cond, etc. part has been dysfunctional for a
>>> > > while (namely since gimple-classes-v2-option-3 branch was
>>> > > merged).  I updated it to use the new classes' names.  That seems
>>> > > to work (though it doesn't output much info anyway: pretty
>>> > > vs. 
>>> > >(gphi *) 0x778c0200
>>> > > in the raw version).
>>> > > 
>>> > > I changed the name passed to pp.add_printer_for_types() for
>>> > > scalar_mode and friends -- so they all share the same name now --
>>> > > but I don't believe the name is used in any context where it
>>> > > would matter.
>>> > 
>>> > IIRC there's a gdb command to tell you what the registered pretty-
>>> > printers are;
>>> 
>>> Yeah, it's (gdb) info pretty-printer
>>> 
>>> > I think the name is only ever used in that context (or maybe for
>>> > fine-grained control of pretty-printing) -
>>> 
>>> From the gdb info manual:
>>> 'disable pretty-printer [OBJECT-REGEXP [NAME-REGEXP]]'
>>>  Disable pretty-printers matching OBJECT-REGEXP and NAME-REGEXP.
>>> 
>>> In our case, this is how to do it:
>>> (gdb) disable pretty-printer global gcc;scalar.*
>>> 
>>> So, that would be a regression, if different modes were given
>>> different names on purpose (starts with r251467).  Looks
>>> unintentional though, and the subprinter is very simple.
>>> 
>>> I think, these scenarios exhaust the uses for the name attribute of a
>>> pretty printer.
>>> 
>>> > and I don't know of anyone who uses that.  I don't think changing
>>> > the name matters.
>>> 
>>> Good.  Neither do I :) Except after writing this I started using this
>>> feature myself.  It provides a convenient way to isolate a faulty
>>> pretty-printer, or continue debugging without it.  The manual says
>>> that "The consequences of a broken pretty-printer are severe enough
>>> that GDB provides support for enabling and disabling individual
>>> printers".  Oh, were they right ...  (see below).
>>
>> In any case, I have no objection to the change-of-names part of the
>> patch.
>>
>>> > > This is just a clean up of gdbhooks.py.  OK to commit?
>>> > 
>>> > The only area I wasn't sure about were the removal hunks relating
>>> > to machine modes: is that all covered now by the looking-through-
>>> > typedefs?
>>> 
>>> Yes, I checked (using debug output) that all the modes for which I
>>> removed explicit handling are correctly expanded to either
>>> opt_mode<>, or pod_mode<>.
>>> 
>>> > Otherwise, looks good to me.  I assume you've tested the patch by
>>> > debugging with it.
>>> 
>>> Yeah, I thought my debugging with the patch should have given it
>>> reasonable coverage.
>>> 
>>> However, I had not previously actually tested debugging code
>>> involving modes, so I went ahead and did it.  And I am seeing a
>>> segfault with RtxPrinter now (-ex "b reg_nonzero_bits_for_combine"
>>> -ex "r" -ex "bt").  Delving in...
>>> 
>>> The rtx type (for which the unqualified form is also 'rtx') was not
>>> being matched by any of the printers.  The typedef-stripped form is
&g

Representative returns and location info (Re: [RFC, PATCH] Display inlining context for uninitialized warnings)

[Vladislav Ivanishin]

Jeff Law  writes:
> On 6/19/19 8:57 AM, Martin Sebor wrote:
>> On 6/19/19 5:11 AM, Vladislav Ivanishin wrote:
>>> Hi,
>>>
>>> This patch (partially) adds displaying inlining context for
>>> -W{maybe,}uninitialized warnings.  This is not as trivial to enable as
>>> simply supplying the "%G" format specifier, so I have some questions
>>> below.
>>>
>>> I need this hunk
>>>
>>>   void
>>>   percent_K_format (text_info *text, location_t loc, tree block)
>>>   {
>>> -  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>>> +  if (loc != UNKNOWN_LOCATION)
>>> +    text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>>>    for two reasons:
>>>
>>> - The gimple return stmt has UNKNOWN_LOCATION due to this code in
>>>    lower_gimple_return ():
>>>
>>>    if (gimple_return_retval (stmt) == gimple_return_retval (tmp_rs.stmt))
>>>  {
>>>    /* Remove the line number from the representative return
>>> statement.
>>>   It now fills in for many such returns.  Failure to remove this
>>>   will result in incorrect results for coverage analysis.  */
>>>    gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);
>> 
>> This code is also causing quite a few non-trivial instances of
>> -Wreturn-local-addr to have no location after two (or more) such
>> statements have been merged (I raised PR 90735 for it), independent
>> of inlining.  I wonder if using the location of the closing curly
>> brace of the function definition (if it's available somewhere)
>> would work without messing up the coverage analysis.
>> 
>>>
>>>    (In case you are wondering, the code and the comment were
>>>    added in 2004.)
[... snip ...]
> You might also want to bring in Andreas K who added the original bits to
> help with debug info generation and Eric B who extended that code in
> 2014 due to impacts on profiling.
>
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00769.html
> https://gcc.gnu.org/ml/gcc-patches/2011-02/msg00421.html

(Let's focus on location info and representative returns in this
subthread.)

AFAIU, a return statement may be chosen as a representative return for a
function.  The representative return's location is set to
UNKNOWN_LOCATION, because otherwise the results of coverage analysis are
skewed.  What is the difficulty preventing us from having both the
location info for the return and faithful coverage?

Andreas, Eric, would you have any comments?

-- 
Vlad

Ping: [RFC, PATCH] Display inlining context for uninitialized warnings

[Vladislav Ivanishin]

Hi,

I'm pinging .

I think, there are two subtopics to it that can be discussed separately.
I would like to focus on the patch itself here.  I am going to also
start a subthread dedicated to dealing with representative returns.

I still have the same questions I had when I was sending the patch.
I am going to reiterate them, but in a more structured/focused fashion.

> When percent_K_format is called, TEXT might already hold precise
> location information in case its (indirect) caller is
> warning_at/error_at.  So it seems to me, this function lacks
> distinguishing the two cases: being called from plain warning/error
> functions vs. their *_at counterparts (the location shouldn't be
> updated in the latter case).

David, do you agree?  Should a discriminator of some sort be introduced?

> Sometimes inlining context is still lost (with the current patch), as
> can be seen e.g. with a version of the test with 's/unsigned yo/int yo/'
> substitution applied.  (The chain of block = BLOCK_SUPERCONTEXT (block)
> is broken - it ends with 0 rather than a FUNCTION_DECL.)  Is this known
> and expected (e.g. because pass_late_warn_uninitialized runs very late),
> or something to be investigated and fixed?

I don't know whom to address this question to.  I guess I'll just go
ahead and investigate if no expert shows up.

> The patch was bootstrapped and regtested on x86_64-pc-linux-gnu.
> Shall it be applied?

Addressing the two points above would make the solution more complete.
However, I think, the patch is still beneficial as-is, and it is not
hacky.  Re-tested, no regressions.

-- 
Vlad

[committed] Re: [PATCH] make gdbhooks.py idempotent with respect to reloading

[Vladislav Ivanishin]

Vladislav Ivanishin  writes:

> Vladislav Ivanishin  writes:
>
>> Hi!
>>
>> It is nice to be able to reload the pretty printers and convenience
>> functions from gdbhooks.py without exiting GDB: reloading cc1 takes
>> several seconds (plus, the debugging session is lost).
>>
>> Previously:
>>
>>(gdb) python import imp; imp.reload(gdbhooks);
>>RuntimeError: pretty-printer already registered: gcc
>>
>> Fixing this turned out easier than I expected.
>> (gdb) py help (gdb.printing)
>> revealed, that we can pass replace parameter to register_pretty_printer
>> (which is False by default).
>>
>> With the patch:
>>
>> (gdb) python import imp; imp.reload(gdbhooks);
>> Successfully loaded GDB hooks for GCC
>>
>> gcc/
>>
>> * gdbhooks.py: Pass replace=True to
>> gdb.printing.register_pretty_printer.
>> ---
>>  gcc/gdbhooks.py | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
>> index 15a5ceaa56f..26a09749aa3 100644
>> --- a/gcc/gdbhooks.py
>> +++ b/gcc/gdbhooks.py
>> @@ -602,7 +602,8 @@ def build_pretty_printer():
>>  
>>  gdb.printing.register_pretty_printer(
>>  gdb.current_objfile(),
>> -build_pretty_printer())
>> +build_pretty_printer(),
>> +replace=True)
>>  
>>  def find_gcc_source_dir():
>>  # Use location of global "g" to locate the source tree
>> -- 
>> 2.22.0
>>
>>
>> OK?
>
> I actually think, that change is obvious.  It has proven useful and I've
> not run into any issues with it.
>
>> BTW, perhaps I should also add a convenience function for 'import imp;
>> imp.reload(gdbhooks)' or something to that effect?
>
> I added a user-defined gdb command and a short alias for it.  I think,
> this is obvious too, but would feel more comfortable if someone OKs it.
> Dave?
>
> [PATCH] gdbinit.in: add reload-gdbhooks (rh) command
>
> gcc/
> * gdbinit.in (reload-gdbhooks): New command with an attached doc 
> string.
> (rh): New alias for it.
> ---
>  gcc/gdbinit.in | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/gcc/gdbinit.in b/gcc/gdbinit.in
> index cec55f86749..2454441f859 100644
> --- a/gcc/gdbinit.in
> +++ b/gcc/gdbinit.in
> @@ -216,6 +216,16 @@ is emitted (as opposed to those warnings that are 
> suppressed by
>  command-line options).
>  end
>  
> +define reload-gdbhooks
> +python import imp; imp.reload(gdbhooks)
> +end
> +
> +document reload-gdbhooks
> +Load the gdbhooks.py module again in order to pick up any changes made to it.
> +end
> +
> +alias rh = reload-gdbhooks
> +
>  # Define some macros helpful to gdb when it is expanding macros.
>  macro define __FILE__ "gdb"
>  macro define __LINE__ 1

Committed both patches as obvious, revisions 273737 and 273738
respectively.

-- 
Vlad

[PATCH] gdbhooks.py: dump-fn, dot-fn: cast ret values of fopen/fclose

[Vladislav Ivanishin]

Hi,

Without the patch, I see these error messages with gdb 8.3:

(gdb) Python Exception  'fclose@@GLIBC_2.2.5' has
unknown return type; cast the call to its declared return type:
(gdb) Error occurred in Python: 'fclose@@GLIBC_2.2.5' has unknown
return type; cast the call to its declared return type

One doesn't have to use python to reproduce that: start debugging cc1
and issue

(gdb) call fopen ("", "")

This actually looks like a GDB bug: from looking at cc1's (built with
either -g, or -ggdb3) DWARF with either dwarfdump, or readelf I see that
there is info about the return type (for fopen it's FILE *, and `ptype
FILE` in gdb gives the full struct).

Tom, you contributed the {dot,dump}-fn functions.  Do they still work
for you without the patch?  (And if so, do you happen to have debuginfo
for libc installed on you machine?)

I think, the patch itself is obvious (as a workaround).  I've only
tested it with the version of GDB I have (8.3, which is the latest
release), but expect this to work for older versions as well.

(Comparisons of gdb.Value's returned from parse_and_eval, like fp == 0
and their conversion to python strings in "%s" % fp work automagically.)

* gdbhooks.py (DumpFn.invoke): Add explicit casts of return values of
fopen and fclose to their respective types.
(DotFn.invoke): Ditto.
---
 gcc/gdbhooks.py | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
index 15f9738aeee..c68bffb4d1a 100644
--- a/gcc/gdbhooks.py
+++ b/gcc/gdbhooks.py
@@ -773,18 +773,17 @@ class DumpFn(gdb.Command):
 f.close()
 
 # Open file
-fp = gdb.parse_and_eval("fopen (\"%s\", \"w\")" % filename)
+fp = gdb.parse_and_eval("(FILE *) fopen (\"%s\", \"w\")" % filename)
 if fp == 0:
 print ("Could not open file: %s" % filename)
 return
-fp = "(FILE *)%u" % fp
 
 # Dump function to file
 _ = gdb.parse_and_eval("dump_function_to_file (%s, %s, %u)" %
(func, fp, flags))
 
 # Close file
-ret = gdb.parse_and_eval("fclose (%s)" % fp)
+ret = gdb.parse_and_eval("(int) fclose (%s)" % fp)
 if ret != 0:
 print ("Could not close file: %s" % filename)
 return
@@ -843,11 +842,10 @@ class DotFn(gdb.Command):
 
 # Close and reopen temp file to get C FILE*
 f.close()
-fp = gdb.parse_and_eval("fopen (\"%s\", \"w\")" % filename)
+fp = gdb.parse_and_eval("(FILE *) fopen (\"%s\", \"w\")" % filename)
 if fp == 0:
 print("Cannot open temp file")
 return
-fp = "(FILE *)%u" % fp
 
 # Write graph to temp file
 _ = gdb.parse_and_eval("start_graph_dump (%s, \"\")" % fp)
@@ -856,7 +854,7 @@ class DotFn(gdb.Command):
 _ = gdb.parse_and_eval("end_graph_dump (%s)" % fp)
 
 # Close temp file
-ret = gdb.parse_and_eval("fclose (%s)" % fp)
+ret = gdb.parse_and_eval("(int) fclose (%s)" % fp)
 if ret != 0:
 print("Could not close temp file: %s" % filename)
 return
-- 
2.22.0

-- 
Vlad

Re: [PATCH] make gdbhooks.py idempotent with respect to reloading

[Vladislav Ivanishin]

Vladislav Ivanishin  writes:

> Hi!
>
> It is nice to be able to reload the pretty printers and convenience
> functions from gdbhooks.py without exiting GDB: reloading cc1 takes
> several seconds (plus, the debugging session is lost).
>
> Previously:
>
>(gdb) python import imp; imp.reload(gdbhooks);
>RuntimeError: pretty-printer already registered: gcc
>
> Fixing this turned out easier than I expected.
> (gdb) py help (gdb.printing)
> revealed, that we can pass replace parameter to register_pretty_printer
> (which is False by default).
>
> With the patch:
>
> (gdb) python import imp; imp.reload(gdbhooks);
> Successfully loaded GDB hooks for GCC
>
> gcc/
>
> * gdbhooks.py: Pass replace=True to
> gdb.printing.register_pretty_printer.
> ---
>  gcc/gdbhooks.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
> index 15a5ceaa56f..26a09749aa3 100644
> --- a/gcc/gdbhooks.py
> +++ b/gcc/gdbhooks.py
> @@ -602,7 +602,8 @@ def build_pretty_printer():
>  
>  gdb.printing.register_pretty_printer(
>  gdb.current_objfile(),
> -build_pretty_printer())
> +build_pretty_printer(),
> +replace=True)
>  
>  def find_gcc_source_dir():
>  # Use location of global "g" to locate the source tree
> -- 
> 2.22.0
>
>
> OK?

I actually think, that change is obvious.  It has proven useful and I've
not run into any issues with it.

> BTW, perhaps I should also add a convenience function for 'import imp;
> imp.reload(gdbhooks)' or something to that effect?

I added a user-defined gdb command and a short alias for it.  I think,
this is obvious too, but would feel more comfortable if someone OKs it.
Dave?

[PATCH] gdbinit.in: add reload-gdbhooks (rh) command

gcc/
* gdbinit.in (reload-gdbhooks): New command with an attached doc string.
(rh): New alias for it.
---
 gcc/gdbinit.in | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/gcc/gdbinit.in b/gcc/gdbinit.in
index cec55f86749..2454441f859 100644
--- a/gcc/gdbinit.in
+++ b/gcc/gdbinit.in
@@ -216,6 +216,16 @@ is emitted (as opposed to those warnings that are suppressed by
 command-line options).
 end
 
+define reload-gdbhooks
+python import imp; imp.reload(gdbhooks)
+end
+
+document reload-gdbhooks
+Load the gdbhooks.py module again in order to pick up any changes made to it.
+end
+
+alias rh = reload-gdbhooks
+
 # Define some macros helpful to gdb when it is expanding macros.
 macro define __FILE__ "gdb"
 macro define __LINE__ 1
-- 
2.22.0


Thanks,
Vlad

Re: [PATCH 1/2] gdbhooks.py: use strip_typedefs to simplify matching type names

[Vladislav Ivanishin]

David Malcolm  writes:

> On Tue, 2019-07-02 at 14:29 +0300, Vladislav Ivanishin wrote:
>> David Malcolm  writes:
>> 
>> > On Mon, 2019-07-01 at 12:50 +0300, Vladislav Ivanishin wrote:
>> > > Hi!
>> > > 
>> > > GDB's Python API provides strip_typedefs method that can be
>> > > instrumental for writing DRY code.  Using it at least partially
>> > > obviates the need for the add_printer_for_types method we have in
>> > > gdbhooks.py (it takes a list of typenames to match and is usually
>> > > used to deal with typedefs).
>> > > 
>> > > I think, the code can be simplified further (there's still
>> > > ['tree_node *', 'const tree_node *'], which is a little awkward)
>> > > if we deal with pointer types in a uniform fashion (I'll touch on
>> > > this in the description of the second patch).  But that can be
>> > > improved separately.
>> > > 
>> > > The gimple_statement_cond, etc. part has been dysfunctional for a
>> > > while (namely since gimple-classes-v2-option-3 branch was
>> > > merged).  I updated it to use the new classes' names.  That seems
>> > > to work (though it doesn't output much info anyway: pretty
>> > > vs. 
>> > >(gphi *) 0x778c0200
>> > > in the raw version).
>> > > 
>> > > I changed the name passed to pp.add_printer_for_types() for
>> > > scalar_mode and friends -- so they all share the same name now --
>> > > but I don't believe the name is used in any context where it
>> > > would matter.
>> > 
>> > IIRC there's a gdb command to tell you what the registered pretty-
>> > printers are;
>> 
>> Yeah, it's (gdb) info pretty-printer
>> 
>> > I think the name is only ever used in that context (or maybe for
>> > fine-grained control of pretty-printing) -
>> 
>> From the gdb info manual:
>> 'disable pretty-printer [OBJECT-REGEXP [NAME-REGEXP]]'
>>  Disable pretty-printers matching OBJECT-REGEXP and NAME-REGEXP.
>> 
>> In our case, this is how to do it:
>> (gdb) disable pretty-printer global gcc;scalar.*
>> 
>> So, that would be a regression, if different modes were given
>> different names on purpose (starts with r251467).  Looks
>> unintentional though, and the subprinter is very simple.
>> 
>> I think, these scenarios exhaust the uses for the name attribute of a
>> pretty printer.
>> 
>> > and I don't know of anyone who uses that.  I don't think changing
>> > the name matters.
>> 
>> Good.  Neither do I :) Except after writing this I started using this
>> feature myself.  It provides a convenient way to isolate a faulty
>> pretty-printer, or continue debugging without it.  The manual says
>> that "The consequences of a broken pretty-printer are severe enough
>> that GDB provides support for enabling and disabling individual
>> printers".  Oh, were they right ...  (see below).
>
> In any case, I have no objection to the change-of-names part of the
> patch.
>
>> > > This is just a clean up of gdbhooks.py.  OK to commit?
>> > 
>> > The only area I wasn't sure about were the removal hunks relating
>> > to machine modes: is that all covered now by the looking-through-
>> > typedefs?
>> 
>> Yes, I checked (using debug output) that all the modes for which I
>> removed explicit handling are correctly expanded to either
>> opt_mode<>, or pod_mode<>.
>> 
>> > Otherwise, looks good to me.  I assume you've tested the patch by
>> > debugging with it.
>> 
>> Yeah, I thought my debugging with the patch should have given it
>> reasonable coverage.
>> 
>> However, I had not previously actually tested debugging code
>> involving modes, so I went ahead and did it.  And I am seeing a
>> segfault with RtxPrinter now (-ex "b reg_nonzero_bits_for_combine"
>> -ex "r" -ex "bt").  Delving in...
>> 
>> The rtx type (for which the unqualified form is also 'rtx') was not
>> being matched by any of the printers.  The typedef-stripped form is
>> 'rtx_def *'. It matches now and uncovers a bug in the subprinter(?)
>> 
>> I dug into it and the impression I have so far is that the
>> print_inline_rtx call upsets gdb's frame unwinders... Upon the "bt"
>> command, GDB itself produces more than 157000 frames before hitting
>> the segfault.
>
> Sounds like an infinite recursion (possibly a mut

Re: [PATCH 1/2] gdbhooks.py: use strip_typedefs to simplify matching type names

[Vladislav Ivanishin]

David Malcolm  writes:

> On Mon, 2019-07-01 at 12:50 +0300, Vladislav Ivanishin wrote:
>> Hi!
>> 
>> GDB's Python API provides strip_typedefs method that can be
>> instrumental
>> for writing DRY code.  Using it at least partially obviates the need
>> for
>> the add_printer_for_types method we have in gdbhooks.py (it takes a
>> list
>> of typenames to match and is usually used to deal with typedefs).
>> 
>> I think, the code can be simplified further (there's still
>> ['tree_node *', 'const tree_node *'], which is a little awkward) if
>> we
>> deal with pointer types in a uniform fashion (I'll touch on this in
>> the
>> description of the second patch).  But that can be improved
>> separately.
>> 
>> The gimple_statement_cond, etc. part has been dysfunctional for a
>> while
>> (namely since gimple-classes-v2-option-3 branch was merged).  I
>> updated
>> it to use the new classes' names.  That seems to work (though it
>> doesn't
>> output much info anyway: pretty
>> vs. 
>>(gphi *) 0x778c0200
>> in the raw version).
>> 
>> I changed the name passed to pp.add_printer_for_types() for
>> scalar_mode
>> and friends -- so they all share the same name now -- but I don't
>> believe the name is used in any context where it would matter.
>
> IIRC there's a gdb command to tell you what the registered pretty-
> printers are;

Yeah, it's (gdb) info pretty-printer

> I think the name is only ever used in that context (or maybe for
> fine-grained control of pretty-printing) -

>From the gdb info manual:
'disable pretty-printer [OBJECT-REGEXP [NAME-REGEXP]]'
 Disable pretty-printers matching OBJECT-REGEXP and NAME-REGEXP.

In our case, this is how to do it:
(gdb) disable pretty-printer global gcc;scalar.*

So, that would be a regression, if different modes were given different
names on purpose (starts with r251467).  Looks unintentional though, and
the subprinter is very simple.

I think, these scenarios exhaust the uses for the name attribute of a
pretty printer.

> and I don't know of anyone who uses that.  I don't think changing the
> name matters.

Good.  Neither do I :) Except after writing this I started using this
feature myself.  It provides a convenient way to isolate a faulty
pretty-printer, or continue debugging without it.  The manual says that
"The consequences of a broken pretty-printer are severe enough that GDB
provides support for enabling and disabling individual printers".  Oh,
were they right ...  (see below).

>> This is just a clean up of gdbhooks.py.  OK to commit?
>
> The only area I wasn't sure about were the removal hunks relating to
> machine modes: is that all covered now by the looking-through-typedefs?

Yes, I checked (using debug output) that all the modes for which I
removed explicit handling are correctly expanded to either opt_mode<>,
or pod_mode<>.

> Otherwise, looks good to me.  I assume you've tested the patch by
> debugging with it.

Yeah, I thought my debugging with the patch should have given it
reasonable coverage.

However, I had not previously actually tested debugging code involving
modes, so I went ahead and did it.  And I am seeing a segfault with
RtxPrinter now (-ex "b reg_nonzero_bits_for_combine" -ex "r" -ex "bt").
Delving in...

The rtx type (for which the unqualified form is also 'rtx') was not
being matched by any of the printers.  The typedef-stripped form is
'rtx_def *'. It matches now and uncovers a bug in the subprinter(?)

I dug into it and the impression I have so far is that the
print_inline_rtx call upsets gdb's frame unwinders... Upon the "bt"
command, GDB itself produces more than 157000 frames before hitting the
segfault.

Have you seen anything like that before?  It would be nice to understand
the root cause of this bug.  I am going to spend some more time on it,
but I've no prior experience in debugging gdb internals.

As a workaround, I'd propose perhaps removing the kludge, but the output
looks nice, while the alternative implementation (after return) is not
as informative.

Thanks,
Vlad

> Thanks
> Dave

[PATCH 2/2] gdbhooks.py: extend vec support in pretty printers

[Vladislav Ivanishin]

This change is threefold:
- enable pretty printing of vec<>, not just vec<>*
- generalize 'vec<(\S+), (\S+), (\S+)>' regex, which is limiting
- extend to work for vl_ptr layout (only vl_embed was supported)

The motivating example for all three is a vector of vectors in
tree-ssa-uninit.c:

typedef vec pred_chain;
typedef vec pred_chain_union;

(This predictably expands to
  vec, va_heap, vl_ptr>
in gdb, if we use strip_typedefs() on it -- otherwise it's just
pred_chain_union.  The first patch of the series takes care of that.)

This example features "direct" vecs themselves rather than pointers to
vecs, which is not a rare case; in fact, a quick grep showed that the
number of declarations of direct vecs is about the same as that of
pointers to vec.

The GDB's Python API seems to do dereferencing automatically (i.e. it
detects whether to use '->', or '.' for field access), allowing to use
the same code to access fields from a struct directly as well as through
a pointer.

This makes it possible to reuse VecPrinter (meant for vec<> *) for plain
vec<>, but perhaps it would be beneficial to handle all pointers in a
generic way e.g. print the address and then print the dereferenced
object, letting the appropriate pretty printer to pick it up and do its
thing, if one is provided for the type.

The output I have for direct vecs now:

(gdb) p *bb->succs
$94 = @0x776569b0 = { 5)>,  3)>}

Compare that to

(gdb) p bb->succs
$70 = 0x776569b0 = { 5)>,  3)>}

Hope the choice of the '@' sign to represent the address of an object
taken by the pretty printer is not confusing?  (GDB itself uses this
notation for references, like so: (edge_def *&) @0x77656c38.)

OK?

gcc/

* gdbhooks.py: Adjust toplevel comment.  Add a new example.
(VecPrinter.__init__): Distinguish between pointer and direct types.
(VecPrinter.to_string): Print pointer value differently based on that
distinction.
(VecPrinter.children): Adjust accordingly, and account for vl_ptr
layout, add a comment about that.
(build_pretty_printer): Generalize regex for the regex based subprinter
for vec<>*.  Add a new regex subprinter for vec<>.
---
 gcc/gdbhooks.py | 47 +++
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
index b036704b86a..a7e665cadb9 100644
--- a/gcc/gdbhooks.py
+++ b/gcc/gdbhooks.py
@@ -128,12 +128,18 @@ and here's a length 1 vec:
   (gdb) p bb->succs
   $19 = 0x70428bb8 = { EXIT)>}
 
-You cannot yet use array notation [] to access the elements within the
-vector: attempting to do so instead gives you the vec itself (for vec[0]),
-or a (probably) invalid cast to vec<> for the memory after the vec (for
-vec[1] onwards).
+Direct instances of vec<> are also supported (their addresses are
+prefixed with the '@' sign).
 
-Instead (for now) you must access m_vecdata:
+In case you have a vec<> pointer, to use array notation [] to access the
+elements within the vector you must first dereference the pointer, just
+as you do in C:
+  (gdb) p (*bb->succs)[0]
+  $50 = (edge_def *&) @0x77656c38:  10)>
+  (gdb) p bb->succs[0][0]
+  $51 = (edge_def *&) @0x77656c38:  10)>
+
+Another option is to access m_vecdata:
   (gdb) p bb->preds->m_vecdata[0]
   $20 =  5)>
   (gdb) p bb->preds->m_vecdata[1]
@@ -441,6 +447,10 @@ class VecPrinter:
 #-ex "up" -ex "p bb->preds"
 def __init__(self, gdbval):
 self.gdbval = gdbval
+if self.gdbval.type.code == gdb.TYPE_CODE_PTR:
+self.ptr = intptr(self.gdbval)
+else:
+self.ptr = None
 
 def display_hint (self):
 return 'array'
@@ -448,14 +458,25 @@ class VecPrinter:
 def to_string (self):
 # A trivial implementation; prettyprinting the contents is done
 # by gdb calling the "children" method below.
-return '0x%x' % intptr(self.gdbval)
+if self.ptr:
+return '0x%x' % self.ptr
+else:
+return '@0x%x' % intptr(self.gdbval.address)
 
 def children (self):
-if intptr(self.gdbval) == 0:
+if self.ptr == 0:
 return
-m_vecpfx = self.gdbval['m_vecpfx']
+m_vec = self.gdbval
+# There are 2 kinds of vec layouts: vl_embed, and vl_ptr.  The latter
+# is implemented with the former stored in the m_vec field.  Sadly,
+# template_argument(2) won't work: `gdb.error: No type named vl_embed`.
+try:
+m_vec = m_vec['m_vec']
+except:
+pass
+m_vecpfx = m_vec['m_vecpfx']
 m_num = m_vecpfx['m_num']
-m_vecdata = self.gdbval['m_vecdata']
+m_vecdata = m_vec['m_vecdata']
 for i in range(m_num):
 yield ('[%d]' % i, m_vecdata[i])
 
@@ -572,9 +593,11 @@ def build_pretty_printer():
 pp.add_printer_for_types(['rtx_def *'], 'rtx_def', RtxPrinter)
 

[PATCH 1/2] gdbhooks.py: use strip_typedefs to simplify matching type names

[Vladislav Ivanishin]

Hi!

GDB's Python API provides strip_typedefs method that can be instrumental
for writing DRY code.  Using it at least partially obviates the need for
the add_printer_for_types method we have in gdbhooks.py (it takes a list
of typenames to match and is usually used to deal with typedefs).

I think, the code can be simplified further (there's still
['tree_node *', 'const tree_node *'], which is a little awkward) if we
deal with pointer types in a uniform fashion (I'll touch on this in the
description of the second patch).  But that can be improved separately.

The gimple_statement_cond, etc. part has been dysfunctional for a while
(namely since gimple-classes-v2-option-3 branch was merged).  I updated
it to use the new classes' names.  That seems to work (though it doesn't
output much info anyway: pretty
vs. 
   (gphi *) 0x778c0200
in the raw version).

I changed the name passed to pp.add_printer_for_types() for scalar_mode
and friends -- so they all share the same name now -- but I don't
believe the name is used in any context where it would matter.

This is just a clean up of gdbhooks.py.  OK to commit?

gcc/

* gdbhooks.py (GdbPrettyPrinters.__call__): canonicalize any variants of
a type name using strip_typdefs.
(build_pretty_printer): Use canonical names for types and otherwise
simplify the code.  Clean up gimple_sth, gimple_statement_sth cruft.
---
 gcc/gdbhooks.py | 48 +++-
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
index 26a09749aa3..b036704b86a 100644
--- a/gcc/gdbhooks.py
+++ b/gcc/gdbhooks.py
@@ -528,7 +528,7 @@ class GdbPrettyPrinters(gdb.printing.PrettyPrinter):
 self.subprinters.append(GdbSubprinterRegex(regex, name, class_))
 
 def __call__(self, gdbval):
-type_ = gdbval.type.unqualified()
+type_ = gdbval.type.unqualified().strip_typedefs()
 str_type = str(type_)
 for printer in self.subprinters:
 if printer.enabled and printer.handles_type(str_type):
@@ -540,7 +540,7 @@ class GdbPrettyPrinters(gdb.printing.PrettyPrinter):
 
 def build_pretty_printer():
 pp = GdbPrettyPrinters('gcc')
-pp.add_printer_for_types(['tree', 'const_tree'],
+pp.add_printer_for_types(['tree_node *', 'const tree_node *'],
  'tree', TreePrinter)
 pp.add_printer_for_types(['cgraph_node *', 'varpool_node *', 'symtab_node *'],
  'symtab_node', SymtabNodePrinter)
@@ -548,32 +548,25 @@ def build_pretty_printer():
  'cgraph_edge', CgraphEdgePrinter)
 pp.add_printer_for_types(['ipa_ref *'],
  'ipa_ref', IpaReferencePrinter)
-pp.add_printer_for_types(['dw_die_ref'],
+pp.add_printer_for_types(['die_struct *'],
  'dw_die_ref', DWDieRefPrinter)
 pp.add_printer_for_types(['gimple', 'gimple *',
 
   # Keep this in the same order as gimple.def:
-  'gimple_cond', 'const_gimple_cond',
-  'gimple_statement_cond *',
-  'gimple_debug', 'const_gimple_debug',
-  'gimple_statement_debug *',
-  'gimple_label', 'const_gimple_label',
-  'gimple_statement_label *',
-  'gimple_switch', 'const_gimple_switch',
-  'gimple_statement_switch *',
-  'gimple_assign', 'const_gimple_assign',
-  'gimple_statement_assign *',
-  'gimple_bind', 'const_gimple_bind',
-  'gimple_statement_bind *',
-  'gimple_phi', 'const_gimple_phi',
-  'gimple_statement_phi *'],
+  'gcond', 'gcond *',
+  'gdebug', 'gdebug *',
+  'glabel', 'glabel *',
+  'gswitch', 'gswitch *',
+  'gassign', 'gassign *',
+  'gbind', 'gbind *',
+  'gphi', 'gphi *'],
 
  'gimple',
  GimplePrinter)
-pp.add_printer_for_types(['basic_block', 'basic_block_def *'],
+pp.add_printer_for_types(['basic_block_def *'],
  'basic_block',
  BasicBlockPrinter)
-pp.add_printer_for_types(['edge', 'edge_def *'],
+pp.add_printer_for_types(['edge_def *'],
  'edge',
  CfgEdgePrinter)
 pp.add_printer_for_types(['rtx_def *'], 'rtx_def', RtxPrinter)
@@ -585,18 +578,15 @@ def build_pretty_printer():
 
 pp.add_printer_for_regex(r'opt_mode<(\S+)>',
   

[PATCH] make gdbhooks.py idempotent with respect to reloading

[Vladislav Ivanishin]

Hi!

It is nice to be able to reload the pretty printers and convenience
functions from gdbhooks.py without exiting GDB: reloading cc1 takes
several seconds (plus, the debugging session is lost).

Previously:

   (gdb) python import imp; imp.reload(gdbhooks);
   RuntimeError: pretty-printer already registered: gcc

Fixing this turned out easier than I expected.
(gdb) py help (gdb.printing)
revealed, that we can pass replace parameter to register_pretty_printer
(which is False by default).

With the patch:

(gdb) python import imp; imp.reload(gdbhooks);
Successfully loaded GDB hooks for GCC

gcc/

* gdbhooks.py: Pass replace=True to
gdb.printing.register_pretty_printer.
---
 gcc/gdbhooks.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
index 15a5ceaa56f..26a09749aa3 100644
--- a/gcc/gdbhooks.py
+++ b/gcc/gdbhooks.py
@@ -602,7 +602,8 @@ def build_pretty_printer():
 
 gdb.printing.register_pretty_printer(
 gdb.current_objfile(),
-build_pretty_printer())
+build_pretty_printer(),
+replace=True)
 
 def find_gcc_source_dir():
 # Use location of global "g" to locate the source tree
-- 
2.22.0


OK?

BTW, perhaps I should also add a convenience function for 'import imp;
imp.reload(gdbhooks)' or something to that effect?

-- 
Vlad

[PATCH, obvious] gdbhooks.py: rename parameters to match usage

[Vladislav Ivanishin]

Hi,

The parameter names here were in the wrong order (which doesn't matter
to the interpreter, but confuses the humans reading the calling code).

I am going to install the following patch soon.

gcc/ChangeLog:

* gdbhooks.py (GdbPrettyPrinters.add_printer_for_types): Reorder
parameter names in accord with usage (no functional change).
(GdbPrettyPrinters.add_printer_for_regex): Ditto.
---
 gcc/gdbhooks.py | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
index 7b1a7be0002..15a5ceaa56f 100644
--- a/gcc/gdbhooks.py
+++ b/gcc/gdbhooks.py
@@ -521,11 +521,11 @@ class GdbPrettyPrinters(gdb.printing.PrettyPrinter):
 def __init__(self, name):
 super(GdbPrettyPrinters, self).__init__(name, [])
 
-def add_printer_for_types(self, name, class_, types):
-self.subprinters.append(GdbSubprinterTypeList(name, class_, types))
+def add_printer_for_types(self, types, name, class_):
+self.subprinters.append(GdbSubprinterTypeList(types, name, class_))
 
-def add_printer_for_regex(self, name, class_, regex):
-self.subprinters.append(GdbSubprinterRegex(name, class_, regex))
+def add_printer_for_regex(self, regex, name, class_):
+self.subprinters.append(GdbSubprinterRegex(regex, name, class_))
 
 def __call__(self, gdbval):
 type_ = gdbval.type.unqualified()
-- 
2.22.0


-- 
Vlad

[RFC, PATCH] Display inlining context for uninitialized warnings

[Vladislav Ivanishin]

Hi, 

This patch (partially) adds displaying inlining context for
-W{maybe,}uninitialized warnings.  This is not as trivial to enable as
simply supplying the "%G" format specifier, so I have some questions
below.

I need this hunk 

 void
 percent_K_format (text_info *text, location_t loc, tree block)
 {
-  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
+  if (loc != UNKNOWN_LOCATION)
+text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
  
for two reasons:

- The gimple return stmt has UNKNOWN_LOCATION due to this code in
  lower_gimple_return ():

  if (gimple_return_retval (stmt) == gimple_return_retval (tmp_rs.stmt))
{
  /* Remove the line number from the representative return statement.
 It now fills in for many such returns.  Failure to remove this
 will result in incorrect results for coverage analysis.  */
  gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);

  (In case you are wondering, the code and the comment were
  added in 2004.)

- When percent_K_format is called, TEXT might already hold precise
  location information in case its (indirect) caller is
  warning_at/error_at.  So it seems to me, this function lacks
  distinguishing the two cases: being called from plain warning/error
  functions vs. their *_at counterparts (the location shouldn't be
  updated in the latter case).

  Martin, you did the necessary work for percent_G_format to accept
  arbitrary gimple statements rather than just calls for PR86650, but
  left the PR open.  Do you remember running into that sort of problem,
  or was it something else?
  
Sometimes inlining context is still lost (with the current patch), as
can be seen e.g. with a version of the test with 's/unsigned yo/int yo/'
substitution applied.  (The chain of block = BLOCK_SUPERCONTEXT (block)
is broken - it ends with 0 rather than a FUNCTION_DECL.)  Is this known
and expected (e.g. because pass_late_warn_uninitialized runs very late),
or something to be investigated and fixed?

The patch was bootstrapped and regtested on x86_64-pc-linux-gnu.  Shall
it be applied?

* tree-ssa-uninit.c (warn_uninit): Pass context (stmt of the uninit use)
to warning_at.
(warn_uninitialized_vars): Add %G format specifier.
(warn_uninitialized_phi): Ditto.
* tree-pretty-print.c (percent_K_format): Don't re-set location of TEXT
to UNKNOWN_LOCATION.

testsuite/
* gcc.dg/uninit-inlined.c: New test.
---
 gcc/testsuite/gcc.dg/uninit-inlined.c | 25 +
 gcc/tree-pretty-print.c   |  3 ++-
 gcc/tree-ssa-uninit.c |  8 
 3 files changed, 31 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/uninit-inlined.c

diff --git a/gcc/testsuite/gcc.dg/uninit-inlined.c b/gcc/testsuite/gcc.dg/uninit-inlined.c
new file mode 100644
index 000..231a0b6b7c2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-inlined.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wmaybe-uninitialized" } */
+
+extern unsigned bar (int);
+
+extern int f;
+
+static int
+foo (int num)
+{
+  unsigned yo;
+  if (f)
+yo = bar (num);
+  return yo; /* { dg-warning "may be used uninitialized" }  */
+}
+int
+test (int num)
+{
+  if (num == 42 || !num)
+return foo (num);
+  return 0;
+}
+
+/* { dg-regexp "In function 'foo'," "In foo" } */
+/* { dg-regexp ".*inlined from 'test'.*" "Inilned from" } */
diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
index 329cc6fceeb..db1a00a6e49 100644
--- a/gcc/tree-pretty-print.c
+++ b/gcc/tree-pretty-print.c
@@ -4196,7 +4196,8 @@ newline_and_indent (pretty_printer *pp, int spc)
 void
 percent_K_format (text_info *text, location_t loc, tree block)
 {
-  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
+  if (loc != UNKNOWN_LOCATION)
+text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
   gcc_assert (pp_ti_abstract_origin (text) != NULL);
   *pp_ti_abstract_origin (text) = NULL;
 
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index fe8f8f0bc28..4d6f3773a87 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -179,7 +179,7 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
   xloc = expand_location (location);
   floc = expand_location (cfun_loc);
   auto_diagnostic_group d;
-  if (warning_at (location, wc, gmsgid, expr))
+  if (warning_at (location, wc, gmsgid, context, expr))
 {
   TREE_NO_WARNING (expr) = 1;
 
@@ -257,12 +257,12 @@ warn_uninitialized_vars (bool warn_possibly_uninitialized)
 	  if (always_executed)
 		warn_uninit (OPT_Wuninitialized, use, SSA_NAME_VAR (use),
 			 SSA_NAME_VAR (use),
-			 "%qD is used uninitialized in this function", stmt,
+			 "%G%qD is used uninitialized in this function", stmt,
 			 UNKNOWN_LOCATION);
 	  else if (warn_possibly_uninitialized)
 		warn_uninit (OPT_Wmaybe_uninitialized, use, SSA_NAME_VAR (use),
 			 SSA_NAME_VAR (use),
-			 "%qD may be used uninitialized in 

[committed, obvious] gcov-tool: Mark {merge,rewrite}_usage with noreturn attribute

[Vladislav Ivanishin]

This makes similar functions merge_usage, rewrite_usage, and
overlap_usage all have ATTRIBUTE_NORETURN.  (The latter was marked with
it in r264562, the other two were not).

Checked in as r272119.

Index: gcc/ChangeLog
===
--- gcc/ChangeLog   (revision 272118)
+++ gcc/ChangeLog   (working copy)
@@ -1,3 +1,8 @@
+2019-06-10  Vladislav Ivanishin  
+
+   * gcov-tool.c (merge_usage, rewrite_usage): Mark with
+   ATTRIBUTE_NORETURN thus making consistent with overlap_usage.
+
 2019-06-10  Jakub Jelinek  

* tree.def (OMP_SCAN): New tree code.
Index: gcc/gcov-tool.c
===
--- gcc/gcov-tool.c (revision 272118)
+++ gcc/gcov-tool.c (working copy)
@@ -188,7 +188,7 @@

 /* Print merge usage and exit.  */

-static void
+static void ATTRIBUTE_NORETURN
 merge_usage (void)
 {
   fnotice (stderr, "Merge subcomand usage:");
@@ -284,7 +284,7 @@

 /* Print profile rewrite usage and exit.  */

-static void
+static void ATTRIBUTE_NORETURN
 rewrite_usage (void)
 {
   fnotice (stderr, "Rewrite subcommand usage:");


-- 
Vlad

Re: [PATCH] tree-ssa-uninit: suppress more spurious warnings

[Vladislav Ivanishin]

Iain Sandoe  writes:

>> On 22 May 2019, at 16:19, Jeff Law  wrote:
>> 
>> On 5/22/19 8:44 AM, Vladislav Ivanishin wrote:
>>> Christophe, Rainer,
>>> 
>>> Rainer Orth  writes:
>>> 
>>>> Hi Christophe,
>>>> 
>>>>> On Fri, 17 May 2019 at 10:12, Vladislav Ivanishin  wrote:
>>>>>> 
>>>>> As you have probably noticed already, the new test uninit-28.c fails:
>>>>> /gcc/testsuite/gcc.dg/uninit-28-gimple.c:9:16: warning: 'undef' may be
>>>>> used uninitialized in this function [-Wmaybe-uninitialized]
>>>>> FAIL: gcc.dg/uninit-28-gimple.c  (test for bogus messages, line 9)
>>>>> at least on arm & aarch64
>>> 
>>> Thanks for spotting.  I managed to reproduce the failure on x86_64 (I
>>> started from revision and configure options in one of H.J.'s test
>>> results [1]) and it seems, another check-in is to blame.  The stage1
>>> compiler is always fine w.r.t. the result on uninit-28-gimple.c.  The
>>> stage2 compiler seems to be miscompiled.
>>> 
>>> r271460 is already bad, yes, but the problem starts earlier (better to
>>> pick another test as an indicator, or bisect just the stage1 compiler,
>>> compiling pseudo-stage2 with it from newer sources).
>>> 
>>> I'm going to bisect the regression and report to the appropriate thread
>>> unless someone beats me to it.

OK, this is the right thread after all.  Failure on uninit-28-gimple.c
is fixed by Martin's patch for PR90587 (checked on x84_64-pc-linux-gnu).

Thank you!

If the problems persist on other tests and targets, please let me know.
I'll check gcc-testresults when testers pick up the fix.

Vlad

>>> 
>>> [1]: https://gcc.gnu.org/ml/gcc-testresults/2019-05/msg02436.html
>>> 
>>>> I'm seeing it on sparc-sun-solaris2.11, and there are gcc-testresults
>>>> reports for i?86, mips64el, powerpc*, s390x, and x86_64.
>> FWIW I'm also seeing uninit-18 failing on the ppc targets.
>
> uninit-18, 19 are failing on x86_64-darwin16 (m32 and m64) at least, too
> (although uninit-28 is passing there).
>
> Iain
>
>> 
>> And I'll reiterate my concern that these are likely masking issues
>> earlier in the optimizer pipeline.  For example uninit-18 really should
>> be fixed by threading in either DOM or VRP.
>> 
>> Jeff

Re: [PATCH] tree-ssa-uninit: suppress more spurious warnings

[Vladislav Ivanishin]

Christophe, Rainer,

Rainer Orth  writes:

> Hi Christophe,
>
>> On Fri, 17 May 2019 at 10:12, Vladislav Ivanishin  wrote:
>>>
>> As you have probably noticed already, the new test uninit-28.c fails:
>> /gcc/testsuite/gcc.dg/uninit-28-gimple.c:9:16: warning: 'undef' may be
>> used uninitialized in this function [-Wmaybe-uninitialized]
>> FAIL: gcc.dg/uninit-28-gimple.c  (test for bogus messages, line 9)
>> at least on arm & aarch64

Thanks for spotting.  I managed to reproduce the failure on x86_64 (I
started from revision and configure options in one of H.J.'s test
results [1]) and it seems, another check-in is to blame.  The stage1
compiler is always fine w.r.t. the result on uninit-28-gimple.c.  The
stage2 compiler seems to be miscompiled.

r271460 is already bad, yes, but the problem starts earlier (better to
pick another test as an indicator, or bisect just the stage1 compiler,
compiling pseudo-stage2 with it from newer sources).

I'm going to bisect the regression and report to the appropriate thread
unless someone beats me to it.

[1]: https://gcc.gnu.org/ml/gcc-testresults/2019-05/msg02436.html

> I'm seeing it on sparc-sun-solaris2.11, and there are gcc-testresults
> reports for i?86, mips64el, powerpc*, s390x, and x86_64.
>
>   Rainer

-- 
Vlad

[PATCH, committed] Add myself to MAINTAINERS

[Vladislav Ivanishin]

===
--- ChangeLog   (revision 271424)
+++ ChangeLog   (working copy)
@@ -1,3 +1,7 @@
+2019-05-20  Vladislav Ivanishin  
+
+   * MAINTAINERS (Write After Approval): Add myself.
+
 2019-05-19  Peter Bergner  

* MAINTAINERS: Update my email address.
Index: MAINTAINERS
===
--- MAINTAINERS (revision 271424)
+++ MAINTAINERS (working copy)
@@ -429,6 +429,7 @@
 Roland Illig   
 Meador Inge
 Bernardo Innocenti 
+Vladislav Ivanishin
 Alexander Ivchenko 
 Balaji V. Iyer 
 Daniel Jacobowitz  

-- 
Vlad

Re: [RFC, PATCH] Don't introduce useless edge splits unless in PRE

[Vladislav Ivanishin]

Richard Biener  writes:

> On Tue, May 14, 2019 at 3:58 PM Vladislav Ivanishin  wrote:
>>
>> Hi!
>>
>> The split_critical_edges() function has multiple uses and it seems, a
>> portion of its code was added to work only when called from tree-ssa-pre
>> but right now it is executed regardless of the caller.
>>
>> The below patch survives bootstrap and regression testing on
>> x86_64-pc-linux-gnu.  Does it make sense?
>
> Yeah.  Please rename the in_pre_p parameter to for_edge_insertion_p
> since that is what it ensures.  Note that the use in tree-ssa-sink.c
> also requires this since it looks for places to sink code to.  Similar
> the use in tree-tail-merge.c (where I'm not sure why it needs split
> critical edges at all...).
>
> So, it seems more safe to have the default of the parameter to true,
> or rather have no default but adjust all few cases effectively only
> changing the one before uninit.
>
> Better (source) documentation would be using an overload,
> split_edges_for_insertion?

Thanks for the feedback. 

Here is a safer version of the patch.

gcc/ChangeLog:

* tree-cfg.h (split_critical_edges): Add for_edge_insertion_p
	parameter with default value false to declaration.
(split_edges_for_insertion): New inline function.  Wrapper for
	split_critical_edges with for_edge_insertion_p = true.
* tree-cfg.c (split_critical_edges): Don't split non-critical
	edges if for_edge_insertion_p is false.  Fix whitespace.
* tree-ssa-pre.c (pass_pre::execute): Call
	split_edges_for_insertion instead of split_critical_edges.
* gcc/tree-ssa-tail-merge.c (tail_merge_optimize): Ditto.
* gcc/tree-ssa-sink.c (pass_sink_code::execute): Ditto.
	(pass_data_sink_code): Update function name in the comment.
---
 gcc/tree-cfg.c| 14 --
 gcc/tree-cfg.h|  9 -
 gcc/tree-ssa-pre.c|  2 +-
 gcc/tree-ssa-sink.c   |  4 ++--
 gcc/tree-ssa-tail-merge.c |  2 +-
 5 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index c6a70c8f10b..eacf494d45f 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -8899,10 +8899,11 @@ struct cfg_hooks gimple_cfg_hooks = {
 };
 
 
-/* Split all critical edges.  */
+/* Split all critical edges.  Split some extra (not necessarily critical) edges
+   if FOR_EDGE_INSERTION_P is true.  */
 
 unsigned int
-split_critical_edges (void)
+split_critical_edges (bool for_edge_insertion_p /* = false */)
 {
   basic_block bb;
   edge e;
@@ -8925,11 +8926,12 @@ split_critical_edges (void)
 	 end by control flow statements, such as RESX.
 	 Go ahead and split them too.  This matches the logic in
 	 gimple_find_edge_insert_loc.  */
-	  else if ((!single_pred_p (e->dest)
-	|| !gimple_seq_empty_p (phi_nodes (e->dest))
-		|| e->dest == EXIT_BLOCK_PTR_FOR_FN (cfun))
+	  else if (for_edge_insertion_p
+		   && (!single_pred_p (e->dest)
+		   || !gimple_seq_empty_p (phi_nodes (e->dest))
+		   || e->dest == EXIT_BLOCK_PTR_FOR_FN (cfun))
 		   && e->src != ENTRY_BLOCK_PTR_FOR_FN (cfun)
-	   && !(e->flags & EDGE_ABNORMAL))
+		   && !(e->flags & EDGE_ABNORMAL))
 	{
 	  gimple_stmt_iterator gsi;
 
diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
index 212f5ff5919..836f8e8af51 100644
--- a/gcc/tree-cfg.h
+++ b/gcc/tree-cfg.h
@@ -105,7 +105,7 @@ extern void extract_true_false_edges_from_block (basic_block, edge *, edge *);
 extern tree find_case_label_for_value (const gswitch *switch_stmt, tree val);
 extern edge find_taken_edge_switch_expr (const gswitch *switch_stmt, tree val);
 extern unsigned int execute_fixup_cfg (void);
-extern unsigned int split_critical_edges (void);
+extern unsigned int split_critical_edges (bool for_edge_insertion_p = false);
 extern basic_block insert_cond_bb (basic_block, gimple *, gimple *,
    profile_probability);
 extern bool gimple_find_sub_bbs (gimple_seq, gimple_stmt_iterator *);
@@ -128,4 +128,11 @@ should_remove_lhs_p (tree lhs)
 	  && !TREE_ADDRESSABLE (TREE_TYPE (lhs)));
 }
 
+
+inline unsigned int
+split_edges_for_insertion ()
+{
+  return split_critical_edges (/*for_edge_insertion_p=*/true);
+}
+
 #endif /* _TREE_CFG_H  */
diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
index 469199fa213..086f8c6 100644
--- a/gcc/tree-ssa-pre.c
+++ b/gcc/tree-ssa-pre.c
@@ -4183,7 +4183,7 @@ pass_pre::execute (function *fun)
   /* This has to happen before VN runs because
  loop_optimizer_init may create new phis, etc.  */
   loop_optimizer_init (LOOPS_NORMAL);
-  split_critical_edges ();
+  split_edges_for_insertion ();
   scev_initialize ();
   calculate_dominance_info (CDI_DOMINATORS);
 
diff --git a/gcc/tree-ssa-sink.c b/gcc/tree-ssa-sink.c
index fe762f54d96..77abe3aa4b6 100644
--- a/gcc/tree-ssa-sink.c
+++ b/gcc/tree-ssa-sink.c

[PATCH] tree-ssa-uninit: suppress more spurious warnings

[Vladislav Ivanishin]

Hi!

Without the patch, two of the newly added tests fail with bogus warnings:

- gcc.dg/uninit-28-gimple.c (Definition guarded with NE_EXPR, use with
  BIT_AND_EXPR.  This is an FP my previous patch [1] knowingly
  overlooks.)
- gcc.dg/uninit-30-gimple.c (EQ_EXPR in the predicate guarding use.
  This is what I spotted while refactoring.  It never worked, and is
  easy to handle).

Bootstraped with `BOOT_CFLAGS="-O -Wno-error=maybe-uninitialized
-Wuninitialized"` and regtested on x86_64-pc-linux-gnu.  OK for trunk?

Also, Richard, would you mind being a sponsor for my svn account?

[1]: https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00896.html

	* tree-ssa-uninit.c (value_sat_pred_p): This new function is a wrapper
around is_value_included_in that knows how to handle BIT_AND_EXPR.
(is_pred_expr_subset_of): Use the new function.  Handle more cases where
code1 == EQ_EXPR and where code1 == BIT_AND_EXPR and thus fix some false
positives.

testsuite/
* gcc.dg/uninit-28-gimple.c: New test.
* gcc.dg/uninit-29-gimple.c: New test.
* gcc.dg/uninit-30-gimple.c: New test.
* gcc.dg/uninit-31-gimple.c: New test.
---
 gcc/testsuite/gcc.dg/uninit-28-gimple.c | 47 
 gcc/testsuite/gcc.dg/uninit-29-gimple.c | 45 +++
 gcc/testsuite/gcc.dg/uninit-30-gimple.c | 43 ++
 gcc/testsuite/gcc.dg/uninit-31-gimple.c | 48 +
 gcc/tree-ssa-uninit.c   | 37 +--
 5 files changed, 210 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/uninit-28-gimple.c
 create mode 100644 gcc/testsuite/gcc.dg/uninit-29-gimple.c
 create mode 100644 gcc/testsuite/gcc.dg/uninit-30-gimple.c
 create mode 100644 gcc/testsuite/gcc.dg/uninit-31-gimple.c

diff --git a/gcc/testsuite/gcc.dg/uninit-28-gimple.c b/gcc/testsuite/gcc.dg/uninit-28-gimple.c
new file mode 100644
index 000..0648b8a4aa7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-28-gimple.c
@@ -0,0 +1,47 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O -Wmaybe-uninitialized" } */
+
+unsigned int __GIMPLE (ssa,startwith("uninit1"))
+foo (unsigned int v)
+{
+  /* Uninit warning here would be bogus, because (16 & 3) == 0 and therefore
+ if v == 16, the uninit value is not used (the use is properly guarded).  */
+  unsigned int undef;/* { dg-bogus "may be used uninitialized" } */
+  unsigned int _2;
+  unsigned int _9;
+  unsigned int _10;
+  unsigned pred;
+
+  __BB(2):
+  if (v_4(D) != 16u)
+goto __BB3;
+  else
+goto __BB4;
+
+  /* 'undef' is defined conditionally (under 'v != 16' predicate)  */
+  __BB(3):
+  undef_8 = 8u;
+  goto __BB4;
+
+  /* An undef value flows into a phi.  */
+  __BB(4):
+  undef_1 = __PHI (__BB2: undef_5(D), __BB3: undef_8);
+  pred = v_4(D) & 3u;
+  if (pred != 0u)
+goto __BB5;
+  else
+goto __BB6;
+
+  /* The phi value is used here (under 'v & 3' predicate).  */
+  __BB(5):
+  _9 = undef_1;
+  goto __BB7;
+
+  __BB(6):
+  _10 = v_4(D);
+  goto __BB7;
+
+  __BB(7):
+  _2 = __PHI (__BB5: _9, __BB6: _10);
+  return _2;
+}
diff --git a/gcc/testsuite/gcc.dg/uninit-29-gimple.c b/gcc/testsuite/gcc.dg/uninit-29-gimple.c
new file mode 100644
index 000..cb5bc97164e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-29-gimple.c
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O -Wmaybe-uninitialized" } */
+
+unsigned int __GIMPLE (ssa,startwith("uninit1"))
+foo (unsigned int v)
+{
+  unsigned int undef;/* { dg-warning "may be used uninitialized" } */
+  unsigned int _2;
+  unsigned int _9;
+  unsigned int _10;
+  unsigned pred;
+
+  __BB(2):
+  pred = v_4(D) & 3u;
+  if (pred != 0u)
+goto __BB3;
+  else
+goto __BB4;
+
+  /* 'undef' is defined conditionally (under 'v & 3' predicate)  */
+  __BB(3):
+  undef_8 = 8u;
+  goto __BB4;
+
+  /* An undef value flows into a phi.  */
+  __BB(4):
+  undef_1 = __PHI (__BB2: undef_5(D), __BB3: undef_8);
+  if (v_4(D) != 16u)
+goto __BB5;
+  else
+goto __BB6;
+
+  /* The phi value is used here (under 'v != 16' predicate).  */
+  __BB(5):
+  _9 = undef_1;
+  goto __BB7;
+
+  __BB(6):
+  _10 = v_4(D);
+  goto __BB7;
+
+  __BB(7):
+  _2 = __PHI (__BB5: _9, __BB6: _10);
+  return _2;
+}
diff --git a/gcc/testsuite/gcc.dg/uninit-30-gimple.c b/gcc/testsuite/gcc.dg/uninit-30-gimple.c
new file mode 100644
index 000..8c91f79d509
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-30-gimple.c
@@ -0,0 +1,43 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O -Wmaybe-uninitialized" } */
+
+unsigned int __GIMPLE (ssa,startwith("uninit1"))
+foo (unsigned int v)
+{
+  unsigned int undef;/* { dg-bogus "may be used uninitialized" } */
+  unsigned int _2;
+  unsigned int _9;
+  unsigned int _10;
+
+  __BB(2):
+  if (v_4(D) < 100u)
+goto __BB3;
+  else
+goto __BB4;
+
+  /* 'undef' is defined conditionally (under 'v < 100' predicate).  */
+  __BB(3):
+  

[PATCH] Fix PR90394, [10 Regression] ICE in is_value_included_in, at tree-ssa-uninit.c:1055

[Vladislav Ivanishin]

Hi!

Here is a simple patch fixing the regression introduced in r270660.

gcc/testsuite/ChangeLog:

	  PR tree-optimization/90394
	  * gcc.dg/uninit-pr90394-1-gimple.c: New test.
	  * gcc.dg/uninit-pr90394.c: New test.

gcc/ChangeLog:

	  PR tree-optimization/90394
	  * tree-ssa-uninit.c (is_pred_expr_subset_of): Potentially give false
	  positives rather than ICE for cases where (code2 == NE_EXPR
	  && code1 == BIT_AND_EXPR).
---
 .../gcc.dg/uninit-pr90394-1-gimple.c  | 47 +++
 gcc/testsuite/gcc.dg/uninit-pr90394.c | 33 +
 gcc/tree-ssa-uninit.c |  2 +-
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c
 create mode 100644 gcc/testsuite/gcc.dg/uninit-pr90394.c

diff --git a/gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c b/gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c
new file mode 100644
index 000..f8feb6b8967
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c
@@ -0,0 +1,47 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O -Wmaybe-uninitialized" } */
+
+unsigned int __GIMPLE (ssa,startwith("uninit1"))
+foo (unsigned int v)
+{
+  /* The warning is not bogus, because (5 & 3) != 0 and therefore if v == 5,
+ the value of undef is used without being initialized.  */
+  unsigned int undef;/* { dg-warning "may be used uninitialized" } */
+  unsigned int _2;
+  unsigned int _9;
+  unsigned int _10;
+  unsigned pred;
+
+  __BB(2):
+  if (v_4(D) != 5u)
+goto __BB3;
+  else
+goto __BB4;
+
+  /* 'undef' is defined conditionally (under 'v != 5' predicate)  */
+  __BB(3):
+  undef_8 = 8u;
+  goto __BB4;
+
+  /* An undef value flows into a phi.  */
+  __BB(4):
+  undef_1 = __PHI (__BB2: undef_5(D), __BB3: undef_8);
+  pred = v_4(D) & 3u;
+  if (pred != 0u)
+goto __BB5;
+  else
+goto __BB6;
+
+  /* The phi value is used here (under 'v & 3' predicate).  */
+  __BB(5):
+  _9 = undef_1;
+  goto __BB7;
+
+  __BB(6):
+  _10 = v_4(D);
+  goto __BB7;
+
+  __BB(7):
+  _2 = __PHI (__BB5: _9, __BB6: _10);
+  return _2;
+}
diff --git a/gcc/testsuite/gcc.dg/uninit-pr90394.c b/gcc/testsuite/gcc.dg/uninit-pr90394.c
new file mode 100644
index 000..16e750d6b33
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr90394.c
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -fno-tree-fre -Wuninitialized" } */
+int pz;
+int zi;
+
+void
+uk (void)
+{
+  int th = 1;
+  int *gw = 
+
+  for (zi = 0; zi < 2; ++zi)
+{
+  int a2 = 0;
+
+  for (zi = 0; zi < 1; ++zi)
+{
+  th = a2 * 2;
+
+ og:
+  for (pz = 0; pz < 1; ++pz)
+{
+}
+}
+
+  pz = !!*gw ? *gw : pz;
+  pz = (!!th ? (pz & 1) : 0);
+  if (pz == 0)
+++a2;
+}
+
+  goto og;
+}
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 7362e374dea..b89da4017e8 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -1471,7 +1471,7 @@ is_pred_expr_subset_of (pred_info expr1, pred_info expr2)
   if (code2 == NE_EXPR && code1 == NE_EXPR)
 return false;
 
-  if (code2 == NE_EXPR)
+  if (code2 == NE_EXPR && code1 != BIT_AND_EXPR)
 return !is_value_included_in (expr2.pred_rhs, expr1.pred_rhs, code1);
 
   if ((code1 == EQ_EXPR || code1 == BIT_AND_EXPR) && code2 == BIT_AND_EXPR)
-- 
2.20.1


Regtested and bootstrapped with `BOOT_CFLAGS="-O
-Wno-error=maybe-uninitialized -Wuninitialized"` on x86_64-pc-linux-gnu.
OK for trunk?

It is not the ideal solution, but it is the simplest (and safest).  So
let's roll with it and then I've already prepared an improvement, but
need your input.

The case where code2 == NE_EXPR and code1 == BIT_AND_EXPR is now (with
this patch) caught by

  if (code1 != code2)
return false;

This means a false positive in some cases: e.g. if we change 5u to 16u
in the gimple test case above, we'll have

  if (v != 16)
u = sth;
   ...
  if (v & 3)
use (u);

for which a warning will be generated (and it's an FP: u is uninit iff
v == 16, but in that case u is not used).

This can be fixed with something like this:

 if (code2 == NE_EXPR)
  {
if (code1 == BIT_AND_EXPR)
  return !((wi::to_wide (expr1.pred_rhs) & wi::to_wide (expr2.pred_rhs))
   .to_uhwi());
else
  return !is_value_included_in (expr2.pred_rhs, expr1.pred_rhs, code1);
  }

But actually I would rather extend is_value_included_in to handle
BIT_AND_EXPR.  OTOH, its other callers don't seem to need it.  My
concern is that handling BIT_AND_EXPR in is_value_included_in as I would
like to do would make the assertion in is_value_included_in weaker.

Frankly, I don't completely understand where BIT_AND_EXPR can and cannot
appear.

Would you have any suggestions?

Thank you.
Vlad

[PATCH] Refactor away an obsolete is_unsigned distinction in is_value_included_in

[Vladislav Ivanishin]

Hi!

This is just a clean-up.  During the course of past refactorings, the
then and else branches of `if (is_unsigned)` in is_value_included_in()
became semantically equivalent and almost textually identical.  This
patch removes the conditional altogether.

Regtested and bootstrapped with `BOOT_CFLAGS="-O
-Wno-error=maybe-uninitialized -Wuninitialized"` on x86_64-pc-linux-gnu.

OK for trunk?

gcc/Changelog:

* tree-ssa-uninit.c (is_value_included_in): remove is_unsigned and merge
semantically equivalent branches (left over after prior refactorings).
---
 gcc/tree-ssa-uninit.c | 31 ++-
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 831587854c6..7362e374dea 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -1017,45 +1017,26 @@ static bool
 is_value_included_in (tree val, tree boundary, enum tree_code cmpc)
 {
   bool inverted = false;
-  bool is_unsigned;
   bool result;
 
   /* Only handle integer constant here.  */
   if (TREE_CODE (val) != INTEGER_CST || TREE_CODE (boundary) != INTEGER_CST)
 return true;
 
-  is_unsigned = TYPE_UNSIGNED (TREE_TYPE (val));
-
   if (cmpc == GE_EXPR || cmpc == GT_EXPR || cmpc == NE_EXPR)
 {
   cmpc = invert_tree_comparison (cmpc, false);
   inverted = true;
 }
 
-  if (is_unsigned)
-{
-  if (cmpc == EQ_EXPR)
-	result = tree_int_cst_equal (val, boundary);
-  else if (cmpc == LT_EXPR)
-	result = tree_int_cst_lt (val, boundary);
-  else
-	{
-	  gcc_assert (cmpc == LE_EXPR);
-	  result = tree_int_cst_le (val, boundary);
-	}
-}
+  if (cmpc == EQ_EXPR)
+result = tree_int_cst_equal (val, boundary);
+  else if (cmpc == LT_EXPR)
+result = tree_int_cst_lt (val, boundary);
   else
 {
-  if (cmpc == EQ_EXPR)
-	result = tree_int_cst_equal (val, boundary);
-  else if (cmpc == LT_EXPR)
-	result = tree_int_cst_lt (val, boundary);
-  else
-	{
-	  gcc_assert (cmpc == LE_EXPR);
-	  result = (tree_int_cst_equal (val, boundary)
-		|| tree_int_cst_lt (val, boundary));
-	}
+  gcc_assert (cmpc == LE_EXPR);
+  result = tree_int_cst_le (val, boundary);
 }
 
   if (inverted)
-- 
2.21.0


-- 
Vlad

[RFC, PATCH] Don't introduce useless edge splits unless in PRE

[Vladislav Ivanishin]

Hi!

The split_critical_edges() function has multiple uses and it seems, a
portion of its code was added to work only when called from tree-ssa-pre
but right now it is executed regardless of the caller.

The below patch survives bootstrap and regression testing on
x86_64-pc-linux-gnu.  Does it make sense?

>From d6d843653b82f277e780f19f2d2b3cc3125db8b5 Mon Sep 17 00:00:00 2001
From: Vladislav Ivanishin 
Date: Wed, 8 May 2019 20:29:34 +0300
Subject: [PATCH] Don't introduce useless edge splits unless in pre

gcc/Changelog:

* tree-cfg.h (split_critical_edges): Add in_pre_p parameter with default
value false to declaration.
* tree-cfg.c (split_critical_edges): Don't split non-critical edges if
in_pre_p is false.  Fix whitespace.
* tree-ssa-pre.c (pass_pre::execute): Pass in_pre_p = true to
split_critical_edges.
---
 gcc/tree-cfg.c | 14 --
 gcc/tree-cfg.h |  2 +-
 gcc/tree-ssa-pre.c |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 587408150fb..11683e63cd1 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -8870,10 +8870,11 @@ struct cfg_hooks gimple_cfg_hooks = {
 };
 
 
-/* Split all critical edges.  */
+/* Split all critical edges.  Split some extra edges to help PRE if IN_PRE_P is
+   true.  */
 
 unsigned int
-split_critical_edges (void)
+split_critical_edges (bool in_pre_p /* = false */)
 {
   basic_block bb;
   edge e;
@@ -8896,11 +8897,12 @@ split_critical_edges (void)
 	 end by control flow statements, such as RESX.
 	 Go ahead and split them too.  This matches the logic in
 	 gimple_find_edge_insert_loc.  */
-	  else if ((!single_pred_p (e->dest)
-	|| !gimple_seq_empty_p (phi_nodes (e->dest))
-		|| e->dest == EXIT_BLOCK_PTR_FOR_FN (cfun))
+	  else if (in_pre_p
+		   && (!single_pred_p (e->dest)
+		   || !gimple_seq_empty_p (phi_nodes (e->dest))
+		   || e->dest == EXIT_BLOCK_PTR_FOR_FN (cfun))
 		   && e->src != ENTRY_BLOCK_PTR_FOR_FN (cfun)
-	   && !(e->flags & EDGE_ABNORMAL))
+		   && !(e->flags & EDGE_ABNORMAL))
 	{
 	  gimple_stmt_iterator gsi;
 
diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
index 212f5ff5919..3fbf983b36a 100644
--- a/gcc/tree-cfg.h
+++ b/gcc/tree-cfg.h
@@ -105,7 +105,7 @@ extern void extract_true_false_edges_from_block (basic_block, edge *, edge *);
 extern tree find_case_label_for_value (const gswitch *switch_stmt, tree val);
 extern edge find_taken_edge_switch_expr (const gswitch *switch_stmt, tree val);
 extern unsigned int execute_fixup_cfg (void);
-extern unsigned int split_critical_edges (void);
+extern unsigned int split_critical_edges (bool in_pre_p = false);
 extern basic_block insert_cond_bb (basic_block, gimple *, gimple *,
    profile_probability);
 extern bool gimple_find_sub_bbs (gimple_seq, gimple_stmt_iterator *);
diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
index 09335faa6a9..2c3645b5301 100644
--- a/gcc/tree-ssa-pre.c
+++ b/gcc/tree-ssa-pre.c
@@ -4195,7 +4195,7 @@ pass_pre::execute (function *fun)
   /* This has to happen before VN runs because
  loop_optimizer_init may create new phis, etc.  */
   loop_optimizer_init (LOOPS_NORMAL);
-  split_critical_edges ();
+  split_critical_edges (/*in_pre_p=*/true);
   scev_initialize ();
   calculate_dominance_info (CDI_DOMINATORS);
 
-- 
2.21.0


If it does, then it uncovers what I think might be a latent bug in the
late uninit pass.

Without the patch, the crited pass inserts an empty basic block that
almost magically prevents the uninit pass from reporting a false
positive.

$ gcc -c -fdump-tree-all -fgimple -O -Wuninitialized gimple-fp-hfs.c

int __GIMPLE (ssa,startwith("uninit1"))
probe_hfsplus ()
{
  int D_1913;
  unsigned int ext_block_start;
  int ext;
  unsigned int _1;
  int _14;

  __BB(2):
  ext_7 = 0;
  goto __BB4;

  __BB(3):
  ext_block_start_11 = 1u;
  _1 = 0u;
  ext_13 = ext_4 + 1;
  goto __BB4;

  __BB(4,loop_header(1)):
  ext_block_start_2 = __PHI (__BB2: ext_block_start_8(D), __BB3: 
ext_block_start_11);
  ext_4 = __PHI (__BB2: 0, __BB3: ext_13);
  if (ext_4 <= 7)
goto __BB3;
  else
goto __BB5;

  __BB(5):
  ext_block_start_3 = __PHI (__BB4: ext_block_start_2);
  _14 = (int) ext_block_start_3;
  return _14;

}



(no warning)

--- gimple-fp-hfs.c.170t.veclower21
+++ gimple-fp-hfs.c.194t.crited1
@@ -24,10 +24,12 @@
   if (ext_4 <= 7)
 goto ; [INV]
   else
-goto ; [INV]
+goto ; [INV]
+
+   :

:
-  # ext_block_start_3 = PHI 
+  # ext_block_start_3 = PHI 
   _14 = (int) ext_block_start_3;
   return _14;

And with the patch, the useless bb 6 is not inserted and the following
is produced:

gimple-fp-hfs.c: In function 'probe_hfsplus':
gimple-fp-hfs.c:5:16: warning: 'ext_block_start' may be used uninitialized in 
this function [-Wmaybe-uninitialized]
5 |   unsigned int ext_block_start;
  |

Re: [PATCH] Fix a missed case in predicate analysis of the late uninit pass

[Vladislav Ivanishin]

Richard Biener  writes:

> On Thu, Apr 4, 2019 at 4:05 PM Vladislav Ivanishin  wrote:
>>
>> Richard Biener  writes:
>>
>> > On Mon, Apr 1, 2019 at 5:36 PM Vladislav Ivanishin  wrote:
>> >>
>> >> Hi!
>> >>
>> >> This is a fairly trivial change fixing a false negative in
>> >> -Wmaybe-uninitialized.  I am pretty sure this is simply an overlooked
>> >> case (is_value_included_in() is not meant to deal with the case where
>> >> both compare codes are NE_EXPRs, neither does it need to, since their
>> >> handling is trivial).
>> >>
>> >> In a nutshell, what happens is values of v restricted by (v != 2) are
>> >> incorrectly considered a subset of values of v restricted by (v != 1).
>> >> As if "v != 2, therefore v != 1".
>> >>
>> >> This is by no means a gcc-9 regression; I'll ping the patch once stage4
>> >> is over, if needed.
>> >>
>> >> This came up when I was experimenting with moving the uninit passes
>> >> around.  On mainline, the late uninit pass runs very late, so reliably
>> >> triggering the affected path is a bit tricky.  So I created a GIMPLE
>> >> test (it reproduces the behavior precisely, but might be fragile
>> >> w.r.t. future versions of the textual representation) and then with a
>> >> hint from Alexander managed to produce a simple C test.  [By the way,
>> >> the first take was to insert an asm with a lot of newlines to prevent
>> >> the dom pass from rewriting the CFG due to high cost of duplicating
>> >> instructions.  This didn't work out; I think the dom pass does not
>> >> respect sizes of inline asms.  I plan to create a testcase and file a
>> >> bug later.]
>> >>
>> >> I ran regression testing on x86_64-pc-linux-gnu and saw no new
>> >> regressions modulo a handful of flaky UNRESOLVED tests under
>> >> gcc.dg/tree-prof.  `BOOT_CFLAGS="-O -Wno-error=maybe-uninitialized
>> >> -Wmaybe-uninitialized" bootstrap` also succeeded producing no new
>> >> warnings.
>> >>
>> >> OK for stage1?
>> >
>> > Hum.  While definitely two NE_EXPR do not work correctly I'd like
>> > to see a positive test since LT_EXPR doesn't work either?
>>
>> Right, thanks.  The case that was not covered well is actually when
>> cond2 == NE_EXPR (arbitrary cond1).  I created 2 new tests: uninit-26.c
>> demonstrates a false negative, and uninit-27-gimple.c a false positive
>> with trunk.
>>
>> > Specifically the code falls through to test is_value_included_in which
>> > seems to assume code1 == code2.
>>
>> The function is_value_included_in itself only cares about one condition
>> code (I'll expound on this below).  I agree though that the second part
>> of the comment seems to assume that.
>>
>> Please take a look at the updated patch.  The rationale is as follows.
>>
>> When we have 2 potentially comparable predicates in
>> is_pred_expr_subset_of, there are basically two things we want to check.
>>
>> 1) Whether two ranges with identical condition codes are nested.  This
>> is done straightforwardly with is_value_included_in.
>>
>> 2) Whether X CMPC VAL1 is nested in X != VAL2.  Which is easy: this
>> happens iff the set (a.k.a "range") {X: X CMPC VAL1 is true} doesn't
>> contain ("cover") VAL2, in other words iff VAL2 CMPC VAL1 is false.
>>
>> Only, the logic of 2) is faulty when X CMPC VAL1 is not a range bounded
>> on one end (this happens when, and only when CMPC is NE_EXPR; the range
>> is then unbounded on both ends and can only be nested in X != VAL2, if
>> VAL1 == VAL2).
>>
>> 3) Whether X != VAL1 is nested in X CMPC VAL2, but that is always false
>> unless CMPC is NE_EXPR, which is already considered.
>
> OK.  Your patch does
>
> +  if (code2 == NE_EXPR && code1 == NE_EXPR)
> +return false;
>
> but it should instead return operand_equal_p (expr1.pred_rhs,
> expr2.pred_rhs, 0)?

This doesn't change the semantics, because the case with equal rhs's is
already considered at the beginning of this function:

static bool
is_pred_expr_subset_of (pred_info expr1, pred_info expr2)
{
  enum tree_code code1, code2;

  if (pred_equal_p (expr1, expr2))
return true;

So I think, leaving this part of the patch as is results in less
localized/explicit code, but better matches the overall style of this
function.  Or perhaps add a checking assert?

  if (code1 == code2)
gcc_c

Re: [PATCH] Fix a missed case in predicate analysis of the late uninit pass

[Vladislav Ivanishin]

Richard Biener  writes:

> On Mon, Apr 1, 2019 at 5:36 PM Vladislav Ivanishin  wrote:
>>
>> Hi!
>>
>> This is a fairly trivial change fixing a false negative in
>> -Wmaybe-uninitialized.  I am pretty sure this is simply an overlooked
>> case (is_value_included_in() is not meant to deal with the case where
>> both compare codes are NE_EXPRs, neither does it need to, since their
>> handling is trivial).
>>
>> In a nutshell, what happens is values of v restricted by (v != 2) are
>> incorrectly considered a subset of values of v restricted by (v != 1).
>> As if "v != 2, therefore v != 1".
>>
>> This is by no means a gcc-9 regression; I'll ping the patch once stage4
>> is over, if needed.
>>
>> This came up when I was experimenting with moving the uninit passes
>> around.  On mainline, the late uninit pass runs very late, so reliably
>> triggering the affected path is a bit tricky.  So I created a GIMPLE
>> test (it reproduces the behavior precisely, but might be fragile
>> w.r.t. future versions of the textual representation) and then with a
>> hint from Alexander managed to produce a simple C test.  [By the way,
>> the first take was to insert an asm with a lot of newlines to prevent
>> the dom pass from rewriting the CFG due to high cost of duplicating
>> instructions.  This didn't work out; I think the dom pass does not
>> respect sizes of inline asms.  I plan to create a testcase and file a
>> bug later.]
>>
>> I ran regression testing on x86_64-pc-linux-gnu and saw no new
>> regressions modulo a handful of flaky UNRESOLVED tests under
>> gcc.dg/tree-prof.  `BOOT_CFLAGS="-O -Wno-error=maybe-uninitialized
>> -Wmaybe-uninitialized" bootstrap` also succeeded producing no new
>> warnings.
>>
>> OK for stage1?
>
> Hum.  While definitely two NE_EXPR do not work correctly I'd like
> to see a positive test since LT_EXPR doesn't work either?

Right, thanks.  The case that was not covered well is actually when
cond2 == NE_EXPR (arbitrary cond1).  I created 2 new tests: uninit-26.c
demonstrates a false negative, and uninit-27-gimple.c a false positive
with trunk.

> Specifically the code falls through to test is_value_included_in which
> seems to assume code1 == code2.

The function is_value_included_in itself only cares about one condition
code (I'll expound on this below).  I agree though that the second part
of the comment seems to assume that.

Please take a look at the updated patch.  The rationale is as follows.

When we have 2 potentially comparable predicates in
is_pred_expr_subset_of, there are basically two things we want to check.

1) Whether two ranges with identical condition codes are nested.  This
is done straightforwardly with is_value_included_in.

2) Whether X CMPC VAL1 is nested in X != VAL2.  Which is easy: this
happens iff the set (a.k.a "range") {X: X CMPC VAL1 is true} doesn't
contain ("cover") VAL2, in other words iff VAL2 CMPC VAL1 is false.

Only, the logic of 2) is faulty when X CMPC VAL1 is not a range bounded
on one end (this happens when, and only when CMPC is NE_EXPR; the range
is then unbounded on both ends and can only be nested in X != VAL2, if
VAL1 == VAL2).

3) Whether X != VAL1 is nested in X CMPC VAL2, but that is always false
unless CMPC is NE_EXPR, which is already considered.

> To me it looks like is_value_includeds comment should be clarified to
> say
>
> /* Returns true if all values X satisfying X CMPC VAL satisfy
>X CMPC BOUNDARY.  */

This is indeed more clear than the current comment, and the meaning is
the same.  I suggest however to not discuss nestedness of ranges in this
comment at all.

> That is, "all values in the range" in the current comment is
> under-specified since VAL is just a single value.

The range is implied, since only one CMPC is passed.  (If CMPC is
NE_EXPR, however, then I guess the range would only be known in the
caller, but the function does not work correctly for this purpose --
hence the patch to not call it then.)  But is_value_included_in actually
only cares about one value and one set anyway, as the name suggests.

So I think the second part of the comment is supposed to help to think
about applying this function for its main use-case (this function is
used twice, actually: in the case we are discussing there is a range
whose nestedness the calling code is testing, in the other case there is
just a constant), whereas the first part simply states what the function
does.  I'd say, the second part of the comment should be rewritten or
discarded, while the first should be kept.  OTOH, it might have been
helpful to the person who wrote this code.

> So I wonder what testcases regress if we remove the && code2 != NE_EXPR
> case?  T

[PATCH] Fix a missed case in predicate analysis of the late uninit pass

[Vladislav Ivanishin]

Hi!

This is a fairly trivial change fixing a false negative in
-Wmaybe-uninitialized.  I am pretty sure this is simply an overlooked
case (is_value_included_in() is not meant to deal with the case where
both compare codes are NE_EXPRs, neither does it need to, since their
handling is trivial).

In a nutshell, what happens is values of v restricted by (v != 2) are
incorrectly considered a subset of values of v restricted by (v != 1).
As if "v != 2, therefore v != 1".

This is by no means a gcc-9 regression; I'll ping the patch once stage4
is over, if needed.

This came up when I was experimenting with moving the uninit passes
around.  On mainline, the late uninit pass runs very late, so reliably
triggering the affected path is a bit tricky.  So I created a GIMPLE
test (it reproduces the behavior precisely, but might be fragile
w.r.t. future versions of the textual representation) and then with a
hint from Alexander managed to produce a simple C test.  [By the way,
the first take was to insert an asm with a lot of newlines to prevent
the dom pass from rewriting the CFG due to high cost of duplicating
instructions.  This didn't work out; I think the dom pass does not
respect sizes of inline asms.  I plan to create a testcase and file a
bug later.]

I ran regression testing on x86_64-pc-linux-gnu and saw no new
regressions modulo a handful of flaky UNRESOLVED tests under
gcc.dg/tree-prof.  `BOOT_CFLAGS="-O -Wno-error=maybe-uninitialized
-Wmaybe-uninitialized" bootstrap` also succeeded producing no new
warnings.

OK for stage1?

gcc/ChangeLog:

* tree-ssa-uninit.c (is_pred_expr_subset_of): Correctly handle a case
where the operand of both simple predicates is the same, both compare
codes are NE_EXPR, and the integer values are different (e.g. neither of
the predicate expressions 'v != 1' and 'v != 2' should be considered a
subset of the other).

gcc/testsuite/ChangeLog:

* gcc.dg/uninit-25-gimple.c: New test.
* gcc.dg/uninit-25.c: New test.
---
 gcc/testsuite/gcc.dg/uninit-25-gimple.c | 41 +
 gcc/testsuite/gcc.dg/uninit-25.c| 23 ++
 gcc/tree-ssa-uninit.c   |  3 ++
 3 files changed, 67 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/uninit-25-gimple.c
 create mode 100644 gcc/testsuite/gcc.dg/uninit-25.c

diff --git a/gcc/testsuite/gcc.dg/uninit-25-gimple.c b/gcc/testsuite/gcc.dg/uninit-25-gimple.c
new file mode 100644
index 000..0c0acd6b83e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-25-gimple.c
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O -Wmaybe-uninitialized" } */
+
+unsigned int __GIMPLE (ssa,startwith("uninit1"))
+foo (unsigned int v)
+{
+  unsigned int undef;/* { dg-warning "may be used uninitialized" } */
+  unsigned int _2;
+  unsigned int _9;
+  unsigned int _10;
+
+  __BB(2):
+  if (v_4(D) != 1u)
+goto __BB3;
+  else
+goto __BB4;
+
+  __BB(3):
+  undef_8 = 8u; // 'undef' is defined conditionally (under 'v != 1' predicate)
+  goto __BB4;
+
+  __BB(4):
+  // An undef value flows into a phi.
+  undef_1 = __PHI (__BB2: undef_5(D), __BB3: undef_8);
+  if (v_4(D) != 2u) // This condition is neither a superset nor a subset of 'v != 1'.
+goto __BB5;
+  else
+goto __BB6;
+
+  __BB(5):
+  _9 = undef_1; // The phi value is used here (under 'v != 2' predicate).
+  goto __BB7;
+
+  __BB(6):
+  _10 = v_4(D);
+  goto __BB7;
+
+  __BB(7):
+  _2 = __PHI (__BB5: _9, __BB6: _10);
+  return _2;
+}
diff --git a/gcc/testsuite/gcc.dg/uninit-25.c b/gcc/testsuite/gcc.dg/uninit-25.c
new file mode 100644
index 000..ce3f91c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-25.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wmaybe-uninitialized" } */
+
+extern unsigned bar (void);
+extern void quux (void);
+
+unsigned foo (unsigned v)
+{
+  unsigned u;
+  if (v != 1)
+u = bar ();
+
+  // Prevent the "dom" pass from changing the CFG layout based on the inference
+  // 'if (v != 1) is false then (v != 2) is true'.  (Now it would have to
+  // duplicate the loop in order to do so, which is deemed expensive.)
+  for (int i = 0; i < 10; i++)
+quux ();
+
+  if (v != 2)
+return u;   /* { dg-warning "may be used uninitialized" } */
+
+  return 0;
+}
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 55a55a05c96..1de7d21d2eb 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -1488,6 +1488,9 @@ is_pred_expr_subset_of (pred_info expr1, pred_info expr2)
   if (expr2.invert)
 code2 = invert_tree_comparison (code2, false);
 
+  if (code1 == NE_EXPR && code2 == NE_EXPR)
+return false;
+
   if ((code1 == EQ_EXPR || code1 == BIT_AND_EXPR) && code2 == BIT_AND_EXPR)
 return (wi::to_wide (expr1.pred_rhs)
 	== (wi::to_wide (expr1.pred_rhs) & wi::to_wide (expr2.pred_rhs)));
-- 
2.20.1


-- 
Vlad