gdb: make gdb.arch/amd64-disp-step-avx.exp actually test displaced stepping

Message ID 20200305222108.27194-1-simon.marchi@efficios.com
State New
Headers show
Series
  • gdb: make gdb.arch/amd64-disp-step-avx.exp actually test displaced stepping
Related show

Commit Message

Simon Marchi March 5, 2020, 10:21 p.m.
The test gdb.arch/amd64-disp-step-avx.exp is meant to test that doing a
displaced step of an AVX instruction works correctly.  However, I found
(by pure coincidence) that the test instructions are not actually
displaced stepped.  Rather, they are inline-stepped, so the test is not
actually testing what it's meat to test.

This is what a portion of the test binary looks like:

    0000000000400180 <_start>:
      400180:       90                      nop

    0000000000400181 <main>:
      400181:       90                      nop

    0000000000400182 <test_rip_vex2>:
      400182:       c5 fb 10 05 0e 00 00    vmovsd 0xe(%rip),%xmm0        # 400198 <ro_var>
      400189:       00

    000000000040018a <test_rip_vex2_end>:
      40018a:       90                      nop

The instruction at 0x400182 is the one we want to test a displaced step
for.  A breakpoint is placed at 0x400182 and ran to.  The execution is
then resumed from there, forcing a step-over (which should normally be a
displaced step) of the breakpoint.

However, the displaced stepping buffer is at the _start label, and that
means a breakpoint is present in the displaced stepping buffer.  The
breakpoint_in_range_p check in displaced_step_prepare_throw evaluates to
true, which makes displaced_step_prepare_throw fail, forcing GDB to fall
back on an in-line step.

This can be easily observed by placing a `gdb_assert (false)` inside the
breakpoint_in_range_p condition, in displaced_step_prepare_throw, and
running gdb.arch/amd64-disp-step-avx.exp.  The assertion will make the
test fail.

The proposed fix is to pad `_start` with a bunch of nops so that the
test instruction is out of the displaced step buffer.

I also think it would be good to enhance the test to make sure that we
are testing displaced stepping as intended.  I did that by enabling "set
debug displaced on" while we step over the interesting instruction, and
matching a message printed only when a displaced step is executed.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-disp-step-avx.S: Add nops after _start.
	* gdb.arch/amd64-disp-step-avx.exp: Enable "set debug displaced
	on" while stepping over the test instruction, match printed
	message.
---
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.S   | 5 +++++
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.25.1

Comments

Tom Tromey March 12, 2020, 6:19 p.m. | #1
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:


Simon> The test gdb.arch/amd64-disp-step-avx.exp is meant to test that doing a
Simon> displaced step of an AVX instruction works correctly.  However, I found
Simon> (by pure coincidence) that the test instructions are not actually
Simon> displaced stepped.  Rather, they are inline-stepped, so the test is not
Simon> actually testing what it's meat to test.

Typo "meat" -> "meant".


Nice catch here.  I think the patch looks good.

Tom
Simon Marchi March 12, 2020, 6:34 p.m. | #2
On 2020-03-12 2:19 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

> 

> Simon> The test gdb.arch/amd64-disp-step-avx.exp is meant to test that doing a

> Simon> displaced step of an AVX instruction works correctly.  However, I found

> Simon> (by pure coincidence) that the test instructions are not actually

> Simon> displaced stepped.  Rather, they are inline-stepped, so the test is not

> Simon> actually testing what it's meat to test.

> 

> Typo "meat" -> "meant".

> 

> 

> Nice catch here.  I think the patch looks good.

> 

> Tom

> 


Thanks, I pushed it.

Simon

Patch

diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
index 76747360b9..c72f6a5ca8 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
@@ -22,7 +22,12 @@ 
 
 	.global _start,main
 _start:
+	# The area at _start is used as the displaced stepping buffer.  Put
+	# more than enough nop instructions so that the instructions under test
+	# below don't conflict with it.
+	.rept 200
 	nop
+	.endr
 main:
         nop
 
diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
index 23282f6678..3144a147f3 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
@@ -92,10 +92,16 @@  proc disp_step_func { func } {
     set value "0xdeadbeefd3adb33f"
     set_regs $value
 
+    # Turn "debug displaced" on to make sure displaced step is actually
+    # executed, not an inline step.
+    gdb_test_no_output "set debug displaced on"
+
     gdb_test "continue" \
-	"Continuing.*Breakpoint.*, ${test_end_label} ().*" \
+	"Continuing.*displaced: displaced pc to.*Breakpoint.*, ${test_end_label} ().*" \
 	"continue to ${test_end_label}"
 
+    gdb_test_no_output "set debug displaced off"
+
     verify_regs $value
 }