public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 1/2] gdb: Minor cleanup in some gdb.arch/* tests
  2018-03-22 12:57 [PATCH 0/2] Test fix in gdb.arch/* tests Andrew Burgess
@ 2018-03-22 12:57 ` Andrew Burgess
  2018-03-22 13:51   ` Pedro Alves
  2018-03-22 12:57 ` [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp Andrew Burgess
  1 sibling, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2018-03-22 12:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

A small number of tests incorrectly tried to pass -Wa,-g through to
GCC as an extra compile time flag, either to gdb_compile or
prepare_for_testing.

There were two mistakes, first, the 'debug' flag was already being
passed, this will cause GCC to add a suitable -g flag, which should
then be propagated to the assembler.  Secondly, in order to pass
additional compiler flags, the syntax would be
'additional_flags=-Wa,-g'.  As it was, the flag was just being
ignored.

Given that all these tests pass 'debug', and the invalid flag has been
ignored for some time, I'm just removing the flags in this commit.

There should be no change in the test results after this commit.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-disp-step-avx.exp: Remove unneeded assembler
	flag option, syntax was wrong anyway.
	* gdb.arch/amd64-disp-step.exp: Likewise.
	* gdb.arch/arm-disp-step.exp: Likewise.
	* gdb.arch/i386-disp-step.exp: Likewise.
	* gdb.arch/sparc64-regs.exp: Likewise.
---
 gdb/testsuite/ChangeLog                        | 9 +++++++++
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp | 4 +---
 gdb/testsuite/gdb.arch/amd64-disp-step.exp     | 4 +---
 gdb/testsuite/gdb.arch/arm-disp-step.exp       | 4 +---
 gdb/testsuite/gdb.arch/i386-disp-step.exp      | 5 ++---
 gdb/testsuite/gdb.arch/sparc64-regs.exp        | 4 +---
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
index 5c20aeb35b4..362ed7b7b3a 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
@@ -25,10 +25,8 @@ if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
 if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
-	  [list debug $additional_flags]] } {
+	  [list debug]] } {
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step.exp b/gdb/testsuite/gdb.arch/amd64-disp-step.exp
index 84f7e69e40e..2731f04dc70 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step.exp
@@ -27,9 +27,7 @@ set newline "\[\r\n\]*"
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug]] != "" } {
     untested "failed to compile"
     return -1
 }
diff --git a/gdb/testsuite/gdb.arch/arm-disp-step.exp b/gdb/testsuite/gdb.arch/arm-disp-step.exp
index 760d392885e..7268105f2dc 100644
--- a/gdb/testsuite/gdb.arch/arm-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/arm-disp-step.exp
@@ -24,9 +24,7 @@ if {![is_aarch32_target]} then {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [prepare_for_testing "failed to prepare" $testfile $srcfile [list debug $additional_flags]] } {
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile [list debug]] } {
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.arch/i386-disp-step.exp b/gdb/testsuite/gdb.arch/i386-disp-step.exp
index ff0713cef96..c4bed320743 100644
--- a/gdb/testsuite/gdb.arch/i386-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/i386-disp-step.exp
@@ -25,9 +25,8 @@ if { ![is_x86_like_target] } then {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	  executable [list debug]] != "" } {
     untested "failed to compile"
     return -1
 }
diff --git a/gdb/testsuite/gdb.arch/sparc64-regs.exp b/gdb/testsuite/gdb.arch/sparc64-regs.exp
index 3e84de6dd01..9fece51dda2 100644
--- a/gdb/testsuite/gdb.arch/sparc64-regs.exp
+++ b/gdb/testsuite/gdb.arch/sparc64-regs.exp
@@ -25,10 +25,8 @@ if ![istarget "sparc64*-*-linux*"] then {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
 if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
-        [list debug $additional_flags]] } {
+	 [list debug]] } {
     return -1
 }
 
-- 
2.14.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp
  2018-03-22 12:57 [PATCH 0/2] Test fix in gdb.arch/* tests Andrew Burgess
  2018-03-22 12:57 ` [PATCH 1/2] gdb: Minor cleanup in some " Andrew Burgess
@ 2018-03-22 12:57 ` Andrew Burgess
  2018-03-22 13:44   ` Pedro Alves
  1 sibling, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2018-03-22 12:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

This test starts up and confirms that $xmm0 has the value 0, it then
modifies $xmm0 (in the inferior) and confirms that the new value can
be read (in GDB).

On some machines I was noticing that this test would occasionally
fail, and on investigation I believe that the reason for this is that
the test is linked as a dynamically linked executable and makes use of
the system libraries during startup.  The reason that this causes
problems is that both the runtime linker and the startup code run
before main can, and do (on at least some platforms) make use of the
XMM registers.

In this commit I modify the test program slightly to allow it to be
linked statically, without using the startup libraries.  Now by the
time GDB reaches the symbol main then we have only executed one 'nop'
instruction, and the XMM registers will all have the value 0.  I've
extended the existing check in the test script to cover $xmm0 through
to $xmm15 (originally only $xmm0 was checked).

The test program is still linked against libc in order that we can
call the exit function, however, we now call _exit rather than exit in
order to avoid all of the usual cleanup that exit does.  This clean up
tries to tear down things that are usually setup during the startup
code, but now this isn't called calling exit will just result in a
crash.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-disp-step-avx.S: Add '_start' label.
	(done): Call '_exit' not 'exit' to avoid atexit handlers.
	* gdb.arch/amd64-disp-step-avx.exp: Pass -static, and
	-nostartfiles when compiling the test.  Confirm that all registers
	xmm0 to xmm15 are initially 0.
---
 gdb/testsuite/ChangeLog                        |  8 ++++++++
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.S   |  8 +++++---
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp | 12 ++++++++----
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
index b56a552f6e6..1c06ceebab3 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
@@ -20,9 +20,11 @@
 
 	.text
 
-	.global main
-main:
+	.global _start,main
+_start:
 	nop
+main:
+        nop
 
 /***********************************************/
 
@@ -59,7 +61,7 @@ ro_var:
 
 done:
 	mov $0,%rdi
-	call exit
+	call _exit
 	hlt
 
 /* RIP-relative data for VEX3 test above.  */
diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
index 362ed7b7b3a..d25f9692360 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
@@ -25,8 +25,10 @@ if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
 
 standard_testfile .S
 
-if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
-	  [list debug]] } {
+set options [list debug \
+		 additional_flags=-static \
+		 additional_flags=-nostartfiles]
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} $options] } {
     return -1
 }
 
@@ -103,8 +105,10 @@ proc disp_step_func { func } {
 with_test_prefix "vex2" {
     # This case writes to the 'xmm0' register.  Confirm the register's
     # value is what we believe it is before the AVX instruction runs.
-    gdb_test "p /x \$xmm0.uint128" " = 0x0" \
-	"xmm0 has expected value before"
+    for {set i 0 } { $i < 16 } { incr i } {
+	gdb_test "p /x \$xmm${i}.uint128" " = 0x0" \
+	    "xmm${i} has expected value before"
+    }
 
     disp_step_func "test_rip_vex2"
 
-- 
2.14.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 0/2] Test fix in gdb.arch/* tests
@ 2018-03-22 12:57 Andrew Burgess
  2018-03-22 12:57 ` [PATCH 1/2] gdb: Minor cleanup in some " Andrew Burgess
  2018-03-22 12:57 ` [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp Andrew Burgess
  0 siblings, 2 replies; 9+ messages in thread
From: Andrew Burgess @ 2018-03-22 12:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

While debugging some other patch I hit a random failure in
gdb.arch/amd64-disp-step-avx.exp.  I did manage to track down what was
causing the failure, but in the meantime something has changed / been
updated on my machine, and I can no longer recreate the original
failure.

Still, I think that the "fix" is still reasonable, and worth merging
if the list agrees, this is patch #2 of this series.

Patch #1 was just an associated cleanup that I spotted when preparing
patch #2.

Tested on x86-64 GNU/Linux, and on the buildbot with no regressions.

Thanks,
Andrewxs

---

Andrew Burgess (2):
  gdb: Minor cleanup in some gdb.arch/* tests
  gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp

 gdb/testsuite/ChangeLog                        | 17 +++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.S   |  8 +++++---
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp | 14 ++++++++------
 gdb/testsuite/gdb.arch/amd64-disp-step.exp     |  4 +---
 gdb/testsuite/gdb.arch/arm-disp-step.exp       |  4 +---
 gdb/testsuite/gdb.arch/i386-disp-step.exp      |  5 ++---
 gdb/testsuite/gdb.arch/sparc64-regs.exp        |  4 +---
 7 files changed, 35 insertions(+), 21 deletions(-)

-- 
2.14.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp
  2018-03-22 12:57 ` [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp Andrew Burgess
@ 2018-03-22 13:44   ` Pedro Alves
  2018-03-22 23:01     ` Andrew Burgess
  0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-03-22 13:44 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

Hi Andrew,

Sounds fine to me in principle.  A couple comments below.

On 03/22/2018 12:57 PM, Andrew Burgess wrote:

> @@ -103,8 +105,10 @@ proc disp_step_func { func } {
>  with_test_prefix "vex2" {
>      # This case writes to the 'xmm0' register.  Confirm the register's
>      # value is what we believe it is before the AVX instruction runs.
> -    gdb_test "p /x \$xmm0.uint128" " = 0x0" \
> -	"xmm0 has expected value before"
> +    for {set i 0 } { $i < 16 } { incr i } {
> +	gdb_test "p /x \$xmm${i}.uint128" " = 0x0" \
> +	    "xmm${i} has expected value before"
> +    }

This leaves a slight disconnect between the comment and the
code.  I.e., someone reading the comment may wonder why
we check more than xmm0?

Also, should we test xmm1-15 are still 0 after, too, for
completeness?

>  
>      disp_step_func "test_rip_vex2"
>  
> 

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gdb: Minor cleanup in some gdb.arch/* tests
  2018-03-22 12:57 ` [PATCH 1/2] gdb: Minor cleanup in some " Andrew Burgess
@ 2018-03-22 13:51   ` Pedro Alves
  2018-03-22 22:59     ` Andrew Burgess
  0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-03-22 13:51 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

On 03/22/2018 12:57 PM, Andrew Burgess wrote:
> A small number of tests incorrectly tried to pass -Wa,-g through to
> GCC as an extra compile time flag, either to gdb_compile or
> prepare_for_testing.
> 
> There were two mistakes, first, the 'debug' flag was already being
> passed, this will cause GCC to add a suitable -g flag, which should
> then be propagated to the assembler.  Secondly, in order to pass
> additional compiler flags, the syntax would be
> 'additional_flags=-Wa,-g'.  As it was, the flag was just being
> ignored.
> 
> Given that all these tests pass 'debug', and the invalid flag has been
> ignored for some time, I'm just removing the flags in this commit.
> 
> There should be no change in the test results after this commit.

OK.

> -
>  if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> -	  [list debug $additional_flags]] } {
> +	  [list debug]] } {
>      return -1

Note you could make these {debug} instead now.  "[list ...]" was only
necessary because of variable expansion.  (Don't know whether that
shortens the lines enough to avoid wrapping).  Actually, "debug" is
the default, so you could just remove them completely:

  proc prepare_for_testing { testname executable {sources ""} {options {debug}}} {

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gdb: Minor cleanup in some gdb.arch/* tests
  2018-03-22 13:51   ` Pedro Alves
@ 2018-03-22 22:59     ` Andrew Burgess
  2018-03-23 10:13       ` Pedro Alves
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2018-03-22 22:59 UTC (permalink / raw)
  To: gdb-patches; +Cc: Pedro Alves

* Pedro Alves <palves@redhat.com> [2018-03-22 13:51:30 +0000]:

> On 03/22/2018 12:57 PM, Andrew Burgess wrote:
> > A small number of tests incorrectly tried to pass -Wa,-g through to
> > GCC as an extra compile time flag, either to gdb_compile or
> > prepare_for_testing.
> > 
> > There were two mistakes, first, the 'debug' flag was already being
> > passed, this will cause GCC to add a suitable -g flag, which should
> > then be propagated to the assembler.  Secondly, in order to pass
> > additional compiler flags, the syntax would be
> > 'additional_flags=-Wa,-g'.  As it was, the flag was just being
> > ignored.
> > 
> > Given that all these tests pass 'debug', and the invalid flag has been
> > ignored for some time, I'm just removing the flags in this commit.
> > 
> > There should be no change in the test results after this commit.
> 
> OK.
> 
> > -
> >  if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> > -	  [list debug $additional_flags]] } {
> > +	  [list debug]] } {
> >      return -1
> 
> Note you could make these {debug} instead now.  "[list ...]" was only
> necessary because of variable expansion.  (Don't know whether that
> shortens the lines enough to avoid wrapping).  Actually, "debug" is
> the default, so you could just remove them completely:
> 
>   proc prepare_for_testing { testname executable {sources ""} {options {debug}}} {
> 

Thanks for the review.

I've updated inline with your suggestion.  I've also converted the
uses of gdb_compile to prepare_for_testing as I believe in these cases
the outcome is the same.  I compared the compile commands in the log
file, prepare_for_testing does split out the object file step into a
separate command, but otherwise I think the command are identical.

Anyway, how's this?

Thanks,
Andrew

---

gdb: Minor cleanup in some gdb.arch/* tests

A small number of tests incorrectly tried to pass -Wa,-g through to
GCC as an extra compile time flag, either to gdb_compile or
prepare_for_testing.

The problem is that the syntax used for passing the flags was
incorrect, and as a result these extra flags were being ignored.
Luckily, the 'debug' flag was being passed in each case anyway, which
means that the '-g' flag would already be added.

Given that all these tests pass 'debug', and the invalid flag has been
ignored for some time, I'm just removing the flags in this commit.

I've also changed the tests from using gdb_compile to
prepare_for_testing, which allows some extra code to be removed from a
couple of tests scripts.

There should be no change in the test results after this commit.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-disp-step-avx.exp: Remove unneeded assembler flag
	option, syntax was wrong anyway.
	* gdb.arch/arm-disp-step.exp: Likewise.
	* gdb.arch/sparc64-regs.exp: Likewise.
	* gdb.arch/amd64-disp-step.exp: Remove unneeded assembler flag
	option, syntax was wrong anyway, switch to use
	prepare_for_testing.
	* gdb.arch/i386-disp-step.exp: Likewise.
---
 gdb/testsuite/ChangeLog                        | 11 +++++++++++
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp |  5 +----
 gdb/testsuite/gdb.arch/amd64-disp-step.exp     | 12 +-----------
 gdb/testsuite/gdb.arch/arm-disp-step.exp       |  4 +---
 gdb/testsuite/gdb.arch/i386-disp-step.exp      | 12 +-----------
 gdb/testsuite/gdb.arch/sparc64-regs.exp        |  5 +----
 6 files changed, 16 insertions(+), 33 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
index 5c20aeb35b4..1f85fa77c1a 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
@@ -25,10 +25,7 @@ if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
-	  [list debug $additional_flags]] } {
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step.exp b/gdb/testsuite/gdb.arch/amd64-disp-step.exp
index 84f7e69e40e..782b75896cc 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step.exp
@@ -27,20 +27,10 @@ set newline "\[\r\n\]*"
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
-    untested "failed to compile"
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
     return -1
 }
 
-# Get things started.
-
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
-
 gdb_test "set displaced-stepping on" ""
 gdb_test "show displaced-stepping" ".* displaced stepping .* is on.*"
 
diff --git a/gdb/testsuite/gdb.arch/arm-disp-step.exp b/gdb/testsuite/gdb.arch/arm-disp-step.exp
index 760d392885e..0f33b37eaf1 100644
--- a/gdb/testsuite/gdb.arch/arm-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/arm-disp-step.exp
@@ -24,9 +24,7 @@ if {![is_aarch32_target]} then {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [prepare_for_testing "failed to prepare" $testfile $srcfile [list debug $additional_flags]] } {
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.arch/i386-disp-step.exp b/gdb/testsuite/gdb.arch/i386-disp-step.exp
index ff0713cef96..d15c54be74b 100644
--- a/gdb/testsuite/gdb.arch/i386-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/i386-disp-step.exp
@@ -25,20 +25,10 @@ if { ![is_x86_like_target] } then {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
-    untested "failed to compile"
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
     return -1
 }
 
-# Get things started.
-
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
-
 gdb_test "set displaced-stepping on" ""
 gdb_test "show displaced-stepping" ".* displaced stepping .* is on.*"
 
diff --git a/gdb/testsuite/gdb.arch/sparc64-regs.exp b/gdb/testsuite/gdb.arch/sparc64-regs.exp
index 3e84de6dd01..6b621bbd98c 100644
--- a/gdb/testsuite/gdb.arch/sparc64-regs.exp
+++ b/gdb/testsuite/gdb.arch/sparc64-regs.exp
@@ -25,10 +25,7 @@ if ![istarget "sparc64*-*-linux*"] then {
 
 standard_testfile .S
 
-set additional_flags "-Wa,-g"
-
-if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
-        [list debug $additional_flags]] } {
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
     return -1
 }
 
-- 
2.14.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp
  2018-03-22 13:44   ` Pedro Alves
@ 2018-03-22 23:01     ` Andrew Burgess
  2018-03-23 10:13       ` Pedro Alves
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2018-03-22 23:01 UTC (permalink / raw)
  To: gdb-patches; +Cc: Pedro Alves

* Pedro Alves <palves@redhat.com> [2018-03-22 13:44:51 +0000]:

> Hi Andrew,
> 
> Sounds fine to me in principle.  A couple comments below.
> 
> On 03/22/2018 12:57 PM, Andrew Burgess wrote:
> 
> > @@ -103,8 +105,10 @@ proc disp_step_func { func } {
> >  with_test_prefix "vex2" {
> >      # This case writes to the 'xmm0' register.  Confirm the register's
> >      # value is what we believe it is before the AVX instruction runs.
> > -    gdb_test "p /x \$xmm0.uint128" " = 0x0" \
> > -	"xmm0 has expected value before"
> > +    for {set i 0 } { $i < 16 } { incr i } {
> > +	gdb_test "p /x \$xmm${i}.uint128" " = 0x0" \
> > +	    "xmm${i} has expected value before"
> > +    }
> 
> This leaves a slight disconnect between the comment and the
> code.  I.e., someone reading the comment may wonder why
> we check more than xmm0?
> 
> Also, should we test xmm1-15 are still 0 after, too, for
> completeness?
> 
> >  
> >      disp_step_func "test_rip_vex2"
> >  

Thanks for the review.

I've updated the patch inline with your feedback, the comment has
been expanded to better match what we actually do, and we now confirm
that xmm1 -> xmm15 are still 0 after the test instruction.

Checked on x86-64 GNU/Linux and buildbot.

Thanks,
Andrew

---

gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp

This test starts up and confirms that $xmm0 has the value 0, it then
modifies $xmm0 (in the inferior) and confirms that the new value can
be read (in GDB).

On some machines I was noticing that this test would occasionally
fail, and on investigation I believe that the reason for this is that
the test is linked as a dynamically linked executable and makes use of
the system libraries during startup.  The reason that this causes
problems is that both the runtime linker and the startup code run
before main can, and do (on at least some platforms) make use of the
XMM registers.

In this commit I modify the test program slightly to allow it to be
linked statically, without using the startup libraries.  Now by the
time GDB reaches the symbol main we have only executed one 'nop'
instruction, and the XMM registers should all have the value 0.  I've
extended the test script to confirm that $xmm0 to $xmm15 are all
initially 0, and I also check that at the point after $xmm0 has been
modified, all the other XMM registers ($xmm1 to $xmm15) are still 0.

The test program is still linked against libc in order that we can
call the exit function, however, we now call _exit rather than exit in
order to avoid all of the usual cleanup that exit does.  This clean up
tries to tear down things that are usually setup during the startup
code, but now this isn't called calling exit will just result in a
crash.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-disp-step-avx.S: Add '_start' label.
	(done): Call '_exit' not 'exit' to avoid atexit handlers.
	* gdb.arch/amd64-disp-step-avx.exp: Pass -static, and
	-nostartfiles when compiling the test.  Confirm that all registers
	xmm0 to xmm15 are initially 0, and that xmm1 to xmm15 are 0 after.
---
 gdb/testsuite/ChangeLog                        |  8 ++++++++
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.S   |  8 +++++---
 gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp | 24 +++++++++++++++++++-----
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
index b56a552f6e6..1c06ceebab3 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.S
@@ -20,9 +20,11 @@
 
 	.text
 
-	.global main
-main:
+	.global _start,main
+_start:
 	nop
+main:
+        nop
 
 /***********************************************/
 
@@ -59,7 +61,7 @@ ro_var:
 
 done:
 	mov $0,%rdi
-	call exit
+	call _exit
 	hlt
 
 /* RIP-relative data for VEX3 test above.  */
diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
index 1f85fa77c1a..fe6eb6729be 100644
--- a/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
+++ b/gdb/testsuite/gdb.arch/amd64-disp-step-avx.exp
@@ -25,7 +25,10 @@ if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
 
 standard_testfile .S
 
-if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+set options [list debug \
+		 additional_flags=-static \
+		 additional_flags=-nostartfiles]
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} $options] } {
     return -1
 }
 
@@ -100,10 +103,15 @@ proc disp_step_func { func } {
 
 # Test a VEX2-encoded RIP-relative instruction.
 with_test_prefix "vex2" {
-    # This case writes to the 'xmm0' register.  Confirm the register's
-    # value is what we believe it is before the AVX instruction runs.
-    gdb_test "p /x \$xmm0.uint128" " = 0x0" \
-	"xmm0 has expected value before"
+    # This test writes to the 'xmm0' register.  As the test is
+    # statically linked, we know that the XMM registers should all
+    # have the default value of 0 at this point in time.  We're about
+    # to run an AVX instruction that will modify $xmm0, but lets first
+    # confirm that all XMM registers are 0.
+    for {set i 0 } { $i < 16 } { incr i } {
+	gdb_test "p /x \$xmm${i}.uint128" " = 0x0" \
+	    "xmm${i} has expected value before"
+    }
 
     disp_step_func "test_rip_vex2"
 
@@ -111,6 +119,12 @@ with_test_prefix "vex2" {
     # modified xmm0.
     gdb_test "p /x \$xmm0.uint128" " = 0x1122334455667788" \
 	"xmm0 has expected value after"
+
+    # And all of the other XMM register should still be 0.
+    for {set i 1 } { $i < 16 } { incr i } {
+	gdb_test "p /x \$xmm${i}.uint128" " = 0x0" \
+	    "xmm${i} has expected value after"
+    }
 }
 
 # Test a VEX3-encoded RIP-relative instruction.
-- 
2.14.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp
  2018-03-22 23:01     ` Andrew Burgess
@ 2018-03-23 10:13       ` Pedro Alves
  0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2018-03-23 10:13 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

On 03/22/2018 11:01 PM, Andrew Burgess wrote:

> Thanks for the review.
> 
> I've updated the patch inline with your feedback, the comment has
> been expanded to better match what we actually do, and we now confirm
> that xmm1 -> xmm15 are still 0 after the test instruction.
> 
> Checked on x86-64 GNU/Linux and buildbot.

Perfect, OK.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] gdb: Minor cleanup in some gdb.arch/* tests
  2018-03-22 22:59     ` Andrew Burgess
@ 2018-03-23 10:13       ` Pedro Alves
  0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2018-03-23 10:13 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

On 03/22/2018 10:59 PM, Andrew Burgess wrote:

> I've updated inline with your suggestion.  I've also converted the
> uses of gdb_compile to prepare_for_testing as I believe in these cases
> the outcome is the same.  I compared the compile commands in the log
> file, prepare_for_testing does split out the object file step into a
> separate command, but otherwise I think the command are identical.
> 
> Anyway, how's this?

Great, OK.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-03-23 10:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-22 12:57 [PATCH 0/2] Test fix in gdb.arch/* tests Andrew Burgess
2018-03-22 12:57 ` [PATCH 1/2] gdb: Minor cleanup in some " Andrew Burgess
2018-03-22 13:51   ` Pedro Alves
2018-03-22 22:59     ` Andrew Burgess
2018-03-23 10:13       ` Pedro Alves
2018-03-22 12:57 ` [PATCH 2/2] gdb: Fix testsuite issue in gdb.arch/amd64-disp-step-avx.exp Andrew Burgess
2018-03-22 13:44   ` Pedro Alves
2018-03-22 23:01     ` Andrew Burgess
2018-03-23 10:13       ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).