[Bug debug/50317] New: [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[Bug debug/50317] New: [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jan.kratochvil at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

             Bug #: 50317
           Summary: [4.7 Regression] missing DW_OP_GNU_implicit_pointer
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jan.kratochvil@redhat.com
            Target: i686-unknown-linux-gnu


PASS: gcc (GCC) 4.6.2 20110907 (prerelease)
 <2><5e>: Abbrev Number: 5 (DW_TAG_variable)
    <5f>   DW_AT_name        : p        
    <67>   DW_AT_location    : 16 byte block: f2 44 0 0 0 0 93 4 f2 51 0 0 0 0
93 4     (DW_OP_GNU_implicit_pointer: <0x44> 0; DW_OP_piece: 4;
DW_OP_GNU_implicit_pointer: <0x51> 0; DW_OP_piece: 4)
(gdb) print *p[1]
$1 = 23

FAIL: gcc (GCC) 4.7.0 20110907 (experimental)
 <2><5b>: Abbrev Number: 5 (DW_TAG_variable)
    <5c>   DW_AT_name        : p        
    <64>   DW_AT_location    : 10 byte block: f2 45 0 0 0 0 93 4 93 4  
(DW_OP_GNU_implicit_pointer: <0x45> 0; DW_OP_piece: 4; DW_OP_piece: 4)
(gdb) print *p[1]
Cannot access memory at address 0x0

GNU gdb (GDB) 7.3.50.20110907-cvs
(gdb) break foo
(gdb) run
(gdb) print *p[1]

static __attribute__((noinline, noclone)) int foo (int i)
{
  int j = i;
  int *p[2] = { &i, &j };
  return *p[0] + *p[1];
}
int main (void)
{
  return foo (23);
}

That p[1] content definition is missing.

There are many incompletele debug info cases around, filing it because it is a
regression (visible on GDB testcase gdb.dwarf2/implptr.exp).

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-09-07
                 CC|                            |jakub at gcc dot gnu.org
   Target Milestone|---                         |4.7.0
     Ever Confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-09-07 14:08:59 UTC ---
Seems this regressed with
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175916

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-09-07 14:21:51 UTC ---
The first difference is in fre1, just a better optimization:
@@ -22,7 +22,7 @@ foo (int i)
   p$1_9 = &j;
   # DEBUG p$1 => p$1_9
   D.2704_2 = p$0_10;
-  D.2705_3 = *D.2704_2;
+  D.2705_3 = i.0_1;
   D.2706_4 = p$1_9;
   D.2707_5 = i.0_1;
   D.2703_6 = D.2705_3 + D.2707_5;

In *.mergephi1 the difference is still pretty minor:
@@ -19,8 +19,7 @@ foo (int i)
   j = i.0_1;
   # DEBUG p$0 => &i
   # DEBUG p$1 => &j
-  D.2705_3 = i;
-  D.2703_6 = i.0_1 + D.2705_3;
+  D.2703_6 = i.0_1 + i.0_1;
   return D.2703_6;

 }

but in *.cddce1 the j variable is removed altogether:
 <bb 2>:
   i.0_1 = i;
-  j = i.0_1;
   # DEBUG p$0 => &i
   # DEBUG p$1 => &j
-  D.2705_3 = i;
-  D.2703_6 = i.0_1 + D.2705_3;
+  D.2703_6 = i.0_1 + i.0_1;
   return D.2703_6;

 }

in r175915 the j variable would survive in the IL until *.alias where it was
determined not address taken, gimplified and DEBUG j => j_11 added and during
copyprop2 the debug stmt was adjusted after j_11 removal.

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-27 10:16:54 UTC ---
So with the optimization it at least should not print

(gdb) print *p[1]
Cannot access memory at address 0x0

but instead

(gdb) print *p[1]
<optimized out>

not sure if that is a gdb bug though ('p' is optimized out - does the
debug info say that 'p' is zero?).  Jan, can you investigate that at
the gdb side?

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jan.kratochvil at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #4 from Jan Kratochvil <jan.kratochvil at redhat dot com> 2011-10-27 13:57:22 UTC ---
(In reply to comment #3)
> not sure if that is a gdb bug though ('p' is optimized out - does the
> debug info say that 'p' is zero?).  Jan, can you investigate that at
> the gdb side?

GDB in general considers <optimized out> values as zero, as it misses changes
for optimized out values.  Maybe there should be implemented partially
optimized out values first before fixing all the operations on top of it.

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 13:11:49 UTC ---
Created attachment 25933
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25933
gcc47-pr50317-1.patch

One way to fix this regression is schedule another update_addresses_taken
before cddce.  Already in *copyprop1 the var has address taken only solely in
debug stmts, and by dropping TREE_ADDRESSABLE on it the right thing happens
(except that {CLOBBER} stuff messes it up, see patch I'm going to attach
momentarily).

I've tried:

--- gcc/tree-ssa-dce.c.jj 2011-11-08 23:35:12.000000000 +0100
+++ gcc/tree-ssa-dce.c      2011-11-28 12:35:55.745657320 +0100
@@ -1215,6 +1215,25 @@ remove_dead_stmt (gimple_stmt_iterator *
          ei_next (&ei);
     }

+  /* If this is a store into a variable that is being optimized away,
+     add a debug bind stmt if possible.  */
+  if (MAY_HAVE_DEBUG_STMTS
+      && gimple_assign_single_p (stmt)
+      && is_gimple_val (gimple_assign_rhs1 (stmt)))
+    {
+      tree lhs = gimple_assign_lhs (stmt);
+      if ((TREE_CODE (lhs) == VAR_DECL || TREE_CODE (lhs) == PARM_DECL)
+         && !DECL_IGNORED_P (lhs)
+         && is_gimple_reg_type (TREE_TYPE (lhs))
+         && !DECL_HAS_VALUE_EXPR_P (lhs))
+       {
+         tree rhs = gimple_assign_rhs1 (stmt);
+         gimple note
+           = gimple_build_debug_bind (lhs, unshare_expr (rhs), stmt);
+         gsi_insert_after (i, note, GSI_SAME_STMT);
+       }
+    }
+
   unlink_stmt_vdef (stmt);
   gsi_remove (i, true);
   release_defs (stmt);

as an alternative, but leads to ICEs, at some later point we'd need to
somewhere determine if the var mentioned in the debug stmts on the RHS which is
still TREE_ADDRESSABLE is ever referenced in non-debug stmts or not.  If not,
we'd want to drop TREE_ADDRESSABLE bit from it, otherwise e.g. at expansion
time we could just ignore the debug stmt, because it is not
target_for_debug_bind when it is TREE_ADDRESSABLE.

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 13:17:02 UTC ---
Created attachment 25934
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25934
gcc47-pr50317-2.patch

Patch to drop ={v} {CLOBBER} stmts when the lhs lost TREE_ADDRESSABLE bit
during execute_update_addresses_taken.  If I remember well, Micha said that
CLOBBERs for SSA_NAMEs are undesirable and indeed for debug stmts they are just
harmful - the end of the scope is better to be determined from where the scope
in the debug info ends, when we add DEBUG var => {CLOBBER}, it might be still
before a few stmts such as GIMPLE_RETURN in this testcase where it is still
valid.

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[matz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #7 from Michael Matz <matz at gcc dot gnu.org> 2011-11-28 13:46:45 UTC ---
(In reply to comment #6)
> Patch to drop ={v} {CLOBBER} stmts when the lhs lost TREE_ADDRESSABLE bit
> during execute_update_addresses_taken.

Actually it's not only the loss of TREE_ADDRESSABLE, but more specifically
that the symbol is marked for renaming now (i.e. will become a SSA name).
There are other decls (not of gimple_reg type) that aren't marked addressed
but still aren't renamed (and therefore the clobbers are still useful).

I see that your patch does the right thing for this, but just wanted to
mention it because it's really the renaming, not the loss of TREE_ADDRESSABLE
that makes removing the clobber a good idea.  I think your condition is a bit
too verbose.  marked_for_renaming implies !TREE_ADDRESSABLE, and a clobber
statement should always have a gimple_vdef.

Then, adding another IL walk just for this?  Why does your first patch ICE?
Can't that be fixed up somehow?

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 14:10:40 UTC ---
Perhaps we could drop the var ={v} {CLOBBER} stmts when renaming the var into
SSA instead.

As for gcc47-pr50317-1.patch, the another walk isn't because of the CLOBBERs,
what it tries to solve is drop the ADDRESSABLE bit before CDDCE decides to
instead optimize all the stores away as unnecessary (the former does the right
thing for debug info, the latter does not).

The reason why the gcc/tree-ssa-dce.c change doesn't work is that currently we
rely on the var in debug stmts to be target_for_debug_bind (which among other
conditions means is_gimple_reg).  That patch emits debug stmts even for cases
where the var is not is_gimple_reg because of TREE_ADDRESSABLE.  As nothing
drops the TREE_ADDRESSABLE bit later on (on this testcase we could do it
somewhere, but not generally, consider e.g. where the first store into some
addressable var is necessary, but last store is not).  DCE at this point
doesn't know if it is removing all references of the addressable var from the
IL.
Passes that are upset about the !target_for_debug_bind var in debug stmts/debug
insns are expansion (it doesn't ICE, but doesn't create the implicit pointer
because the var whose address is taken is addressable) and var-tracking (for
which it leads to ICEs because VALUE is propagated through into the notes).

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 14:36:53 UTC ---
Created attachment 25936
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25936
gcc47-pr50317-3.patch

Alternative to gcc47-pr50317-3.patch (everything untested), which adds some
hacks so that the DCE hunk works on this testcase.  I'm a little bit worried
that it could create too many debug bind stmts which might turn out to be
useless (as not all of references to the var will be removed).

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[matz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #10 from Michael Matz <matz at gcc dot gnu.org> 2011-11-28 14:52:50 UTC ---
(In reply to comment #8)
> Perhaps we could drop the var ={v} {CLOBBER} stmts when renaming the var
> into SSA instead.

I think your current patch is better, no use in slowing down the SSA renamer,
there aren't that many points where a var can suddenly become renamed
when it couldn't before.  I think you should send it to gcc-patches
independent if it fixes this one or not.

> As for gcc47-pr50317-1.patch, the another walk isn't because of the
> CLOBBERs, what it tries to solve is drop the ADDRESSABLE bit before
> CDDCE decides to instead optimize all the stores away as unnecessary
> (the former does the right thing for debug info, the latter does not).

But conceivably we can have other passes than CDDCE which can remove
stores, including the last one, so IMO the latter should better be made to
work.  As in, update_address_taken should purely be an optimization, it should
never be required for correct operation.

> The reason why the gcc/tree-ssa-dce.c change doesn't work is that currently we
> rely on the var in debug stmts to be target_for_debug_bind (which among other
> conditions means is_gimple_reg).  That patch emits debug stmts even for cases
> where the var is not is_gimple_reg because of TREE_ADDRESSABLE.  As nothing
> drops the TREE_ADDRESSABLE bit later on (on this testcase we could do it
> somewhere, but not generally, consider e.g. where the first store into some
> addressable var is necessary, but last store is not).  DCE at this point
> doesn't know if it is removing all references of the addressable var from the
> IL.
> Passes that are upset about the !target_for_debug_bind var in debug stmts/debug
> insns are expansion (it doesn't ICE, but doesn't create the implicit pointer
> because the var whose address is taken is addressable) and var-tracking (for
> which it leads to ICEs because VALUE is propagated through into the notes).

Okay, so there are multiple ways out:
1) accept also TREE_ADDRESSABLES in target_for_debug_bind;
   why is it rejecting the needs_to_live_in_memory objects?
2) drop TREE_ADDRESSABLE for unreferenced variables
   (remove_unused_locals, but that again should only be an optimization,
   not a correctness measure)
3) Accept variables in DEBUG_BIND unconditionally if they are
   target_for_debug_bind or not.  Presumably the one creating the debug
   bind will have known what he was doing.

I think (3) looks most sensible?

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 15:32:18 UTC ---
(In reply to comment #10)
> (In reply to comment #8)
> > Perhaps we could drop the var ={v} {CLOBBER} stmts when renaming the var
> > into SSA instead.
> 
> I think your current patch is better, no use in slowing down the SSA renamer,
> there aren't that many points where a var can suddenly become renamed
> when it couldn't before.  I think you should send it to gcc-patches
> independent if it fixes this one or not.

Yeah, I played with it and doing it in the renamer wouldn't be very trivial and
very surprising for various passes if the renamer removes stmts.
Will bootstrap/regtest it.

> > As for gcc47-pr50317-1.patch, the another walk isn't because of the
> > CLOBBERs, what it tries to solve is drop the ADDRESSABLE bit before
> > CDDCE decides to instead optimize all the stores away as unnecessary
> > (the former does the right thing for debug info, the latter does not).
> 
> But conceivably we can have other passes than CDDCE which can remove
> stores, including the last one, so IMO the latter should better be made to
> work.  As in, update_address_taken should purely be an optimization, it should
> never be required for correct operation.

Well, the another scheduled in updates_address_taken wasn't a correctness
issue, just a nice to have thing which improves debug info in that case.  The
debug info isn't incorrect, just less complete (and the regression is in the
debug info quality).

> Okay, so there are multiple ways out:
> 1) accept also TREE_ADDRESSABLES in target_for_debug_bind;
>    why is it rejecting the needs_to_live_in_memory objects?
> 2) drop TREE_ADDRESSABLE for unreferenced variables
>    (remove_unused_locals, but that again should only be an optimization,
>    not a correctness measure)
> 3) Accept variables in DEBUG_BIND unconditionally if they are
>    target_for_debug_bind or not.  Presumably the one creating the debug
>    bind will have known what he was doing.
> 
> I think (3) looks most sensible?

What I have in the last patch is roughly accepting those TREE_ADDRESSABLES in
there while in GIMPLE IL (== 1) ), dropping TREE_ADDRESSABLE in
remove_unused_locals as an optimization (== 2) ) and avoiding creation of the
debug insns during expansion if they don't still satisfy target_for_debug_bind
(correctness issue).

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 21:04:49 UTC ---
Author: jakub
Date: Mon Nov 28 21:04:45 2011
New Revision: 181788

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181788
Log:
    PR debug/50317
    * tree-ssa.c (execute_update_addresses_taken): Remove
    var ={v} {CLOBBER} stmts instead of rewriting them into
    var_N ={v} {CLOBBER}.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa.c

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-12-01 19:12:58 UTC ---
Author: jakub
Date: Thu Dec  1 19:12:54 2011
New Revision: 181890

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181890
Log:
    PR debug/50317
    * tree-ssa-dce.c (remove_dead_stmt): Add a debug stmt when removing
    as unnecessary a store to a variable with gimple reg type.
    * tree-ssa-live.c (remove_unused_locals): Clear TREE_ADDRESSABLE bit
    on local unreferenced variables.
    * cfgexpand.c (expand_gimple_basic_block): Don't emit DEBUG_INSNs
    for !target_for_debug_bind variables.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c
    trunk/gcc/tree-ssa-dce.c
    trunk/gcc/tree-ssa-live.c

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-12-01 19:16:20 UTC ---
Fixed.

[Bug debug/50317] [4.7 Regression] missing DW_OP_GNU_implicit_pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50317

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-12-03 16:40:05 UTC ---
Author: jakub
Date: Sat Dec  3 16:39:56 2011
New Revision: 181971

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181971
Log:
    PR debug/50317
    * tree-ssa.c (target_for_debug_bind): Also allow is_gimple_reg_type
    vars that aren't referenced.
    (tree-ssa-live.c (remove_unused_locals): Don't clear TREE_ADDRESSABLE
    of unreferenced local vars.
    * cfgexpand.c (expand_debug_expr): For DEBUG_IMPLICIT_PTR allow also
    TREE_ADDRESSABLE vars that satisfy target_for_debug_bind.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c
    trunk/gcc/tree-ssa-live.c
    trunk/gcc/tree-ssa.c