Bug 29962 - FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
Summary: FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
Status: NEW
Alias: None
Product: gdb
Classification: Unclassified
Component: gdb (show other bugs)
Version: unknown
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-04 10:09 UTC by Tom de Vries
Modified: 2023-01-12 16:30 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
log file including debug infrun output (gcc 9.4.0 case) (1.24 KB, text/plain)
2023-01-11 10:06 UTC, Tom de Vries
Details
log file including debug infrun output (gcc 12.2.1 case) (1.60 KB, text/plain)
2023-01-11 10:08 UTC, Tom de Vries
Details
Tentative patch (2.69 KB, patch)
2023-01-12 16:30 UTC, Tom de Vries
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2023-01-04 10:09:17 UTC
[ Likewise for gdb.reverse/step-precsave.exp, which uses the same source file. ]

On ubuntu 20.04.5 x86_64 (using gcc 9.4.0), I run into:
...
(gdb) PASS: gdb.reverse/step-reverse.exp: simple reverse stepi
step^M
78         a[5] = a[3] - a[4]; /* FINISH TEST */^M
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
step^M
...

The basic problem here is that the forward execution goes like:
...
$ ./build/gdb/gdb -q -batch ./build/gdb/testsuite/outputs/gdb.reverse/step-reverse/step-reverse -ex "b 78" -ex run -ex "set trace-commands on" -ex stepi -ex "stepi" -ex "stepi" -ex "stepi" -ex "stepi"
Breakpoint 1 at 0x123b: file /home/vries/gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c, line 78.

Breakpoint 1, main () at /home/vries/gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:78
78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
0x0000555555555243	78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
79	   callee();	/* STEPI TEST */
$ 
...
because we have:
...
INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END 
26     75     0x0000555555555231 Y                    
27     78     0x000055555555523b Y                    
28     78     0x000055555555523e Y                    
29     78     0x0000555555555241 Y                    
30     78     0x0000555555555245 Y                    
31     79     0x0000555555555248 Y                    
...
and when doing the backward execution, we stop at 0x0000555555555245 rather stepi-ing to 0x000055555555523b as is the intention.

This simple test-case patch fixes it:
...
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index 27e4b175274..8ed4f6d0269 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -72,6 +72,8 @@ gdb_test_multiple "finish" "$test_message" {
     }
 }
 
+set pc_finish_test [get_valueof /x {$pc} ""]
+
 # stepi over flat code (no calls)
 
 set test_message "simple stepi"
@@ -216,7 +218,13 @@ gdb_test_multiple "stepi" "$test_message" {
 	exp_continue
     }
     -re "$stepi_location.* FINISH TEST.*$gdb_prompt $" {
-	pass "$test_message"
+	set pc [get_valueof /x {$pc} ""]
+	if { $pc != $pc_finish_test } {
+	    send_gdb "stepi\n"
+	    exp_continue
+	} else {
+	    pass "$test_message"
+	}
     }
     -re "STEP INTO THIS CALL.*$gdb_prompt $" {
 	fail "$test_message (too far)"
...
(though it introduces a duplicate for the get_valueof).

OTOH, this can be fixed by:
...
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 851852404eb..0e6ee8fe04d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -20205,11 +20205,6 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
     return 1;
   if (line != last_line)
     return 1;
-  /* Same line for the same file that we've seen already.
-     As a last check, for pr 17276, only record the line if the line
-     has never had a non-zero discriminator.  */
-  if (!line_has_non_zero_discriminator)
-    return 1;
   return 0;
 } 
...
which gets us:
...
25     75     0x0000555555555231 Y                    
26     78     0x000055555555523b Y                    
27     79     0x0000555555555248 Y                    
...
and:
...
Breakpoint 1, main () at step-reverse.c:78
78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
0x000055555555523e	78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
0x0000555555555241	78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
0x0000555555555243	78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
0x0000555555555245	78	   a[5] = a[3] - a[4]; /* FINISH TEST */
+stepi
79	   callee();	/* STEPI TEST */
...

So in that sense, it's similar to PR25911.
Comment 1 Tom de Vries 2023-01-04 13:30:42 UTC
Hmm, I'm starting to wonder if I mis-analyzed this.

Looking at:
...
stepi^M
78         a[5] = a[3] - a[4]; /* FINISH TEST */^M
(gdb) PASS: gdb.reverse/step-reverse.exp: simple reverse stepi
step^M
78         a[5] = a[3] - a[4]; /* FINISH TEST */^M
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
...
we see a step from line 78 to line ... 78.  That looks like the problem.
Comment 2 Tom de Vries 2023-01-04 17:18:11 UTC
The decision to stop stepping happens here in this code in process_event_stop_test:
...
     /* When stepping backward, stop at beginning of line range                                                  
         (unless it's the function entry point, in which case                                                     
         keep going back to the call point).  */
      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
      if (stop_pc == ecs->event_thread->control.step_range_start
          && stop_pc != ecs->stop_func_start
          && execution_direction == EXEC_REVERSE)
        end_stepping_range (ecs);
      else
        keep_going (ecs);

      return;
    }

  /* We stepped out of the stepping range.  */
...

AFAICT, there just isn't enough information at this point to decide to stop stepping.

This code is part of an early-out that tries to do:
...
 /* If stepping through a line, keep going if still within it.                                                   
...
Comment 3 Tom de Vries 2023-01-11 09:39:55 UTC
As mentioned before, this failure shows up with gcc 9.4.0.

It doesn't show up with gcc 12.2.1, because of a difference in generated code.

In the 12.2.1 case we have 4 insns:
...
  401208:       8b 55 cc                mov    -0x34(%rbp),%edx
  40120b:       8b 45 d0                mov    -0x30(%rbp),%eax
  40120e:       29 c2                   sub    %eax,%edx
  401210:       89 55 d4                mov    %edx,-0x2c(%rbp)
...
each with their line number entry:
... 
step-reverse.c         78            0x401208               x
step-reverse.c         78            0x40120b               x
step-reverse.c         78            0x40120e               x
step-reverse.c         78            0x401210               x
...
Comment 4 Tom de Vries 2023-01-11 10:03:37 UTC
(In reply to Tom de Vries from comment #3)
> As mentioned before, this failure shows up with gcc 9.4.0.
> 
> It doesn't show up with gcc 12.2.1, because of a difference in generated
> code.
> 
> In the 12.2.1 case we have 4 insns:
> ...
>   401208:       8b 55 cc                mov    -0x34(%rbp),%edx
>   40120b:       8b 45 d0                mov    -0x30(%rbp),%eax
>   40120e:       29 c2                   sub    %eax,%edx
>   401210:       89 55 d4                mov    %edx,-0x2c(%rbp)
> ...
> each with their line number entry:
> ... 
> step-reverse.c         78            0x401208               x
> step-reverse.c         78            0x40120b               x
> step-reverse.c         78            0x40120e               x
> step-reverse.c         78            0x401210               x
> ...

In the gcc 9.4.0 case, we have 5 insns:
...
  401228:       8b 55 cc                mov    -0x34(%rbp),%edx
  40122b:       8b 45 d0                mov    -0x30(%rbp),%eax
  40122e:       29 c2                   sub    %eax,%edx
  401230:       89 d0                   mov    %edx,%eax
  401232:       89 45 d4                mov    %eax,-0x2c(%rbp)
...
but still 4 line number entries:
...
step-reverse.c           78            0x401228               x
step-reverse.c           78            0x40122b               x
step-reverse.c           78            0x40122e               x
step-reverse.c           78            0x401232               x
...
Note that the insn at 0x401230 doesn't have it's own line number entry.

So, we are at line 79:
...
(gdb) stepi^M
79         callee();    /* STEPI TEST */^M
...
and do a stepi, which lands us, as expected at insn 0x401232:
...
stepi^M
78         a[5] = a[3] - a[4]; /* FINISH TEST */^M
(gdb) PASS: gdb.reverse/step-reverse.exp: simple reverse stepi
print /x $pc^M
$2 = 0x401232^M
(gdb) PASS: gdb.reverse/step-reverse.exp: print /x $pc
...

Then we do a step, which lands us, unexpectedly, at insn 0x40122e:
...
step^M
78         a[5] = a[3] - a[4]; /* FINISH TEST */^M
(gdb) FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
print /x $pc^M
$3 = 0x40122e^M
(gdb) PASS: gdb.reverse/step-reverse.exp: print /x $pc
...

It's stepping over the 0x401230 insn without line number entry that triggers the problem.
Comment 5 Tom de Vries 2023-01-11 10:06:17 UTC
Created attachment 14575 [details]
log file including debug infrun output (gcc 9.4.0 case)

$ gdb -q -batch outputs/gdb.reverse/step-reverse/step-reverse -ex "set trace-commands on" -ex start -ex record -ex "break 79" -ex continue -ex reverse-stepi -ex "set debug infrun 1" -ex reverse-step 2>&1 | tee log.txt
Comment 6 Tom de Vries 2023-01-11 10:08:14 UTC
Created attachment 14576 [details]
log file including debug infrun output (gcc 12.2.1 case)
Comment 7 Tom de Vries 2023-01-12 11:55:29 UTC
This passes gdb.reverse/*.exp:
...
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 181d961d80d..ba0b10de965 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6900,11 +6900,12 @@ process_event_stop_test (struct execution_control_state *ecs)
       if (stop_pc == ecs->event_thread->control.step_range_start
          && stop_pc != ecs->stop_func_start
          && execution_direction == EXEC_REVERSE)
-       end_stepping_range (ecs);
+       ;
       else
-       keep_going (ecs);
-
-      return;
+       {
+         keep_going (ecs);
+         return;
+       }
     }
 
   /* We stepped out of the stepping range.  */
@@ -7401,6 +7402,8 @@ process_event_stop_test (struct execution_control_state *ecs)
                               "it's not the start of a statement");
        }
     }
+  else if (execution_direction == EXEC_REVERSE)
+    refresh_step_info = false;
 
   /* We aren't done stepping.
 
...
Comment 8 Tom de Vries 2023-01-12 16:30:40 UTC
Created attachment 14588 [details]
Tentative patch