[1/2] gdb: Minor cleanup in some gdb.arch/* tests

Message ID e07d3806010beef5c6f1a7a0ad97229e726ec174.1521722330.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • Test fix in gdb.arch/* tests
Related show

Commit Message

Andrew Burgess March 22, 2018, 12:57 p.m.
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(-)

-- 
2.14.3

Comments

Pedro Alves March 22, 2018, 1:51 p.m. | #1
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
Andrew Burgess March 22, 2018, 10:59 p.m. | #2
* 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
Pedro Alves March 23, 2018, 10:13 a.m. | #3
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

Patch

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
 }