[Bug debug/55467] New: Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-1.c and gcc.dg/guality/pr36728-2.c

[Bug debug/55467] New: Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-1.c and gcc.dg/guality/pr36728-2.c

[hp at gcc dot gnu.org]

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

             Bug #: 55467
           Summary: Wrong value for optimized debug, variants of
                    gcc.dg/guality/pr36728-1.c and
                    gcc.dg/guality/pr36728-2.c
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: hp@gcc.gnu.org


While adjusting gcc.dg/guality/pr36728-1.c and -2.c for PR55030 (as suggested
in <http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01542.html>), the test-cases
started to fail with the "wrong" kind of failure: the gdb session showed wrong
values instead of values being optimized-out, which is the expected fallback
for shortcomings in gcc related to optimization and debug.  Attached will be
two such variants; I hope a single PR is sufficient for the both of them.

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-1.c and gcc.dg/guality/pr36728-2.c

[hp at gcc dot gnu.org]

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

--- Comment #1 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-11-26 00:30:10 UTC ---
Created attachment 28775
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28775
gcc.dg/guality/pr55467-2.c

Here's pr55467-2.c, a variant of gcc.dg/guality/pr36728-2.c, actually 
gcc.dg/guality/pr36728-2.c with the patch from
<http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01542.html>.  It fails at least
at r193349, r193583 and r193777 as follows (well, for the two first mentioned,
you'll see "ERROR" as explained and fixed in
<http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01998.html>):

Running /home/hp/gcctop/tmp/nbasg/gcc/gcc/testsuite/gcc.dg/guality/guality.exp
...
FAIL: gcc.dg/guality/pr55467-2.c  -O2  line 14 y == 2
FAIL: gcc.dg/guality/pr55467-2.c  -O3 -fomit-frame-pointer  line 14 y == 2
FAIL: gcc.dg/guality/pr55467-2.c  -O3 -g  line 14 y == 2
FAIL: gcc.dg/guality/pr55467-2.c  -Os  line 14 y == 2
FAIL: gcc.dg/guality/pr55467-2.c  -O2 -flto -fno-use-linker-plugin
-flto-partition=none  line 14 y == 2
FAIL: gcc.dg/guality/pr55467-2.c  -O2 -flto -fuse-linker-plugin
-fno-fat-lto-objects  line 14 y == 2

In gcc.log, you'll see that gdb reads some different value, like 0, 1,
or -23360.

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-1.c and gcc.dg/guality/pr36728-2.c

[hp at gcc dot gnu.org]

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

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |x86_64-linux

--- Comment #2 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-11-26 00:31:28 UTC ---
Right, target is x86_64-linux with -m32 or i686-linux.

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-1.c and gcc.dg/guality/pr36728-2.c

[hp at gcc dot gnu.org]

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

--- Comment #3 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-11-26 01:01:08 UTC ---
Created attachment 28776
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28776
gcc.dg/guality/pr55467-1.c

Here's pr55467-1.c, another variant of gcc.dg/guality/pr36728-2.c
which *also* fails at least at r193349, r193583 and r193777 as
follows, both with *and* without -m32.  The only difference to
pr55467-2.c is that x[0] isn't used after the second asm (in which it
is an input and output operand).  The problem is that x[0] is
displayed as 0 rather than 25 or optimized out.

Running /home/hp/gcctop/tmp/nbasg/gcc/gcc/testsuite/gcc.dg/guality/guality.exp
...
FAIL: gcc.dg/guality/pr55467-1.c  -O1  line 14 *x == (char) 25
FAIL: gcc.dg/guality/pr55467-1.c  -O2  line 14 *x == (char) 25
FAIL: gcc.dg/guality/pr55467-1.c  -O3 -fomit-frame-pointer  line 14 *x ==
(char) 25
FAIL: gcc.dg/guality/pr55467-1.c  -O3 -g  line 14 *x == (char) 25
FAIL: gcc.dg/guality/pr55467-1.c  -Os  line 14 *x == (char) 25
FAIL: gcc.dg/guality/pr55467-1.c  -O2 -flto -fno-use-linker-plugin
-flto-partition=none  line 14 *x == (char) 25
FAIL: gcc.dg/guality/pr55467-1.c  -O2 -flto -fuse-linker-plugin
-fno-fat-lto-objects  line 14 *x == (char) 25

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-2.c

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |jakub at gcc dot gnu.org
         Resolution|                            |INVALID

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-26 13:36:26 UTC ---
That is not a bug.  Without volatile and nothing using the result of the asm,
both the second inline asm and x[0] = 25 store are optimized away (by RTL
DSE1), there is not even anything on the line 14 anymore.
As it is optimized away quite late, the variable x is not optimized away, it
exists, just is completely uninitialized.  What you see in the debugger than is
exactly what you would see before the x[0] = 25; store then.

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-2.c

[hp at gcc dot gnu.org]

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

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
   Last reconfirmed|                            |2012-11-26
         Resolution|INVALID                     |
     Ever Confirmed|0                           |1

--- Comment #5 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-11-26 14:13:24 UTC ---
(In reply to comment #4)
> That is not a bug.  Without volatile and nothing using the result of the asm,
> both the second inline asm and x[0] = 25 store are optimized away (by RTL
> DSE1), there is not even anything on the line 14 anymore.
> As it is optimized away quite late, the variable x is not optimized away, it
> exists, just is completely uninitialized.  What you see in the debugger than is
> exactly what you would see before the x[0] = 25; store then.

Your reply matches only for the attachment named pr55467-1.c.  I'd hope to see
the value being optimized out, but ok.

A bit too quick on closing, methinks: see the (first) attachment, named
gcc.dg/guality/pr55467-2.c, where x[0] is used at the return (after being
magically changed by the asm).  So, reopened.

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-2.c

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-26 14:37:48 UTC ---
I disagree.  You can't see <optimized out> for aggregate var in memory which
actually has been allocated on the stack, VTA doesn't value track those (and
can't really), there you just have a variable with description where in memory
it is and whatever is on the stack there is what you get.

As for the other testcase, that is just a testcase problem you've introduced by
removing the volatile from the asm.  Without volatile the compiler is free to
schedule the second asm before the first one (and does for e.g. -m32 -O2), but
at that point y obviously isn't initialized.  If volatile can't be used there,
you'd need something like:

@@ -2,6 +2,9 @@
 /* { dg-do run } */
 /* { dg-options "-g" } */

+#include "../nop.h"
+
+int a;
 int __attribute__((noinline))
 foo (int arg1, int arg2, int arg3, int arg4, int arg5, int arg6, int arg7)
 {
@@ -9,9 +12,9 @@ foo (int arg1, int arg2, int arg3, int a
   int __attribute__ ((aligned(32))) y;

   y = 2;
-  asm ("" : "=m" (y) : "m" (y));
+  asm (NOP : "=m" (y), "=m" (a) : "m" (y));
   x[0] = 25;
-  asm ("" : "=m" (x[0]) : "m" (x[0]));
+  asm (NOP : "=m" (x[0]) : "m" (x[0]), "m" (a));
   return y + x[0];
 }

plus adjust line numbers in gdb-test lines.  The "" -> NOP change is desirable
in any way, as the test wants to stop on the inline asm, if the inline asm is
empty, it stops on the following insn rather than on the inline asm.

[Bug debug/55467] Wrong value for optimized debug, variants of gcc.dg/guality/pr36728-2.c

[hp at gcc dot gnu.org]

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

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |INVALID

--- Comment #7 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-11-26 18:02:41 UTC ---
(In reply to comment #6)
> Without volatile the compiler is free to
> schedule the second asm before the first one (and does for e.g. -m32 -O2),

Oh, that's what's happening.  Wicked.

> ... The "" -> NOP change is desirable
> in any way, as the test wants to stop on the inline asm, if the inline asm is
> empty, it stops on the following insn rather than on the inline asm.

Yes.  That break-on-empty-asm-line bothered me a little, but I shrugged it off,
thinking the author probably intended what was there. ;)  Nice with the nop.h. 
Ok, re-closing this.

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |
   Target Milestone|---                         |4.8.0
            Summary|Wrong value for optimized   |[4.8 Regression] Debug info
                   |debug, variants of          |issues with
                   |gcc.dg/guality/pr36728-2.c  |gcc.dg/guality/pr36728-*.c

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-27 09:09:43 UTC ---
I'm going to reuse this PR for issues caused or made no longer latent by the
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193802
on the pr36728-*.c testcases, given that it already contains some discussion
about them.

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-27 09:27:06 UTC ---
Created attachment 28789
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28789
gcc48-pr55467.patch

The following patch tweaks the testcases to have explicit dependency between
the two asms and use NOP from nop.h.  Additionally it adds two modified
testcases, which contain volatile asm everywhere but on the first occurrence in
foo.  That shouldn't cause any debug info degradation, some information will be
lost after the volatile asm, but the breakpoint is still where the information
should be present.
But it seems e.g. on i686-linux, all the arg[1-7] vars lose their locations
already much earlier, on the fp setter instruction.  I've tracked it down to
cselib_reset_table not actually resetting the REG/MEM locations in the table
for values that aren't dropped completely from the hash table.

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aoliva at gcc dot gnu.org

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-27 09:44:32 UTC ---
Or perhaps better just follow what we do at the end of basic blocks in
var-tracking:
--- gcc/cselib.c.jj    2012-11-26 10:14:26.000000000 +0100
+++ gcc/cselib.c    2012-11-27 10:29:23.524991980 +0100
@@ -2632,6 +2632,8 @@ cselib_process_insn (rtx insn)
       || (NONJUMP_INSN_P (insn)
       && volatile_insn_p (PATTERN (insn))))
     {
+      if (cselib_preserve_constants)
+    cselib_preserve_only_values ();
       cselib_reset_table (next_uid);
       cselib_current_insn = NULL_RTX;
       return;
That will also clear REG/MEMs from VALUEs that we drop from the hash table.

But there is another, unrelated, issue, that regressed with the r193802 commit.
In main of the new testcases, or even just
int
foo (int arg1, int arg2, int arg3, int arg4, int arg5, int arg6, int arg7);

int
main ()
{
  int l = 0;
  asm volatile ("" : "=r" (l) : "0" (l));
  foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30);
  return 0;
}
alone for e.g. -g -O2 -m32, since that commit we now emit wrong call site info,
saying that e.g. the first argument has call_site_value of %eax + 1, which is
wrong, as %eax is call clobbered register.
I'd have expected that
  case MO_CALL:
    dataflow_set_clear_at_call (set);
should have cleared the %eax location from all the VALUEs, as %eax is call
clobbered.
With the cselib.c patch in this comment as opposed to the cselib.c in the
attached patch the call site info is also wrong, but differently (instead of
saying that the value is say %eax + 1 it now says it is 1 (it is correct only
because the inline asm hasn't changed it to non-zero, but the compiler can't
rely on that).

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-27 09:56:12 UTC ---
Note it isn't just call arg info, also NOTE_INSN_VAR_LOCATION for variable l.
Without any of the patches we have (testcase from previous comment):
(note 55 2 41 2 (var_location l (const_int 0 [0])) NOTE_INSN_VAR_LOCATION)
...
(note 56 7 35 2 (var_location l (reg:SI 0 ax [orig:68 l ] [68]))
NOTE_INSN_VAR_LOCATION) ! right after asm volatile
...
(insn 22 40 57 2 (parallel [
            (set (reg:SI 0 ax [orig:76 D.1390 ] [76])
                (plus:SI (reg:SI 0 ax [orig:68 l ] [68])
                    (const_int 1 [0x1])))
            (clobber (reg:CC 17 flags))
        ]) ./pr36728-4.c:9 271 {*addsi_1}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (expr_list:REG_EQUIV (mem:SI (reg/f:SI 7 sp) [0 S4 A32])
            (nil))))
(note 57 22 21 2 (var_location l (reg:SI 0 ax [orig:68 l ] [68]))
NOTE_INSN_VAR_LOCATION)

note 57 is wrong, obviously l doesn't have at that point value %eax, but %eax -
1.
With the patch from the previous comment, 
(note 57 22 21 2 (var_location l (const_int 0 [0])) NOTE_INSN_VAR_LOCATION)
instead (also wrong, the compiler can't assume the inline asm hasn't changed
the register).  What if it is
asm volatile ("incl %0" : "=r" (l) : "0" (l));
instead.

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-27 10:38:13 UTC ---
I'd bet the problem is that for the volatile asms (and setjmp call) cselib.c
doesn't call the hook at all.  var-tracking.c doesn't call cselib on
CODE_LABELs (another spot that resetted the table), and previously as volatile
asms we considered only volatile asms with no outputs and no clobbers.

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28789|0                           |1
        is obsolete|                            |
             Status|REOPENED                    |ASSIGNED
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-27 10:42:46 UTC ---
Created attachment 28791
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28791
gcc48-pr55467.patch

Actually, I think for var-tracking it is completely nonsensical to reset the
table and not call the hook on volatile insns.  Var-tracking isn't performing
any CSE or similar across the volatile insns, all it cares about are what
values are preserved and what values are clobbered across them.  And that is
independent on whether they are volatile or not, even volatile asm can't
clobber registers it doesn't describe to be clobbered, and memory it doesn't
describe to be clobbered.  So, instead of adding lots of code to somehow call
the hook anyway for these instructions (even for setjmp we e.g. want to
generate NOTE_INSN_CALL_ARG_LOCATION note), I think it is better just to reset
everything only when cselib isn't used in var-tracking.

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[hp at gcc dot gnu.org]

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

--- Comment #14 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2012-11-27 11:39:25 UTC ---
(In reply to comment #13)
> even volatile asm can't
> clobber registers it doesn't describe to be clobbered, and memory it doesn't
> describe to be clobbered.

Wrong regarding registers.  Since you think so, please reply to the discussion
at <http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01542.html> (cc:ed again now).

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-28 23:01:21 UTC ---
Author: jakub
Date: Wed Nov 28 23:01:03 2012
New Revision: 193911

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193911
Log:
    PR debug/36728
    PR debug/55467
    PR middle-end/55507
    PR bootstrap/55511
    * cselib.c (cselib_process_insn): If cselib_preserve_constants,
    don't reset table and exit early on volatile insns and setjmp.
    Reset table afterwards on setjmp.

    * gcc.dg/guality/pr36728-1.c: Include "../nop.h", make sure the asm
    are non-empty and add dependency between the first and second asm.
    * gcc.dg/guality/pr36728-2.c: Likewise.
    * gcc.dg/guality/pr36728-3.c: New test.
    * gcc.dg/guality/pr36728-4.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/guality/pr36728-3.c
    trunk/gcc/testsuite/gcc.dg/guality/pr36728-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cselib.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/guality/pr36728-1.c
    trunk/gcc/testsuite/gcc.dg/guality/pr36728-2.c

[Bug debug/55467] [4.8 Regression] Debug info issues with gcc.dg/guality/pr36728-*.c

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-29 08:10:11 UTC ---
Fixed.