[3/3] gdb/testsuite: Handle targets with lots of registers

Message ID f708d6f5e4f81f3f08c27916fb8d6a18d753cde8.1523286728.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • Small testsuite updates
Related show

Commit Message

Andrew Burgess April 9, 2018, 3:15 p.m.
In gdb.base/maint.exp a test calls 'maint print registers'.  If the
target has lots of registers this may overflow expect's buffers,
causing the test to fail.

After this commit we process the output line at a time until we get back
to the GDB prompt, this should prevent buffer overrun while still
testing that the command works as required.

gdb/testsuite/ChangeLog:

	* gdb.base/maint.exp: Process output from 'maint print registers'
	line at a time.
---
 gdb/testsuite/ChangeLog          |  5 +++++
 gdb/testsuite/gdb.base/maint.exp | 18 ++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.12.2

Comments

Maciej W. Rozycki April 12, 2018, 11:39 p.m. | #1
On Mon, 9 Apr 2018, Andrew Burgess wrote:

>  # Test for a regression where this command would internal-error if the

> -# program wasn't running.

> -gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*"

> +# program wasn't running.  If there's a lot of registers then this

> +# might overflow expect's buffers, so process the output line at a

> +# time.

> +send_gdb "maint print registers\n"

> +gdb_expect {

> +    -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" {

> +	exp_continue

> +    }


 I think this changes the meaning of the test; you want to preserve the 
heading match pattern at the very least.  Also `gdb_test' handles various 
error cases gracefully (which matters for the avoidance of excessive 
timeouts with some test boards), whereas your simple matcher does not.

 Also how many is "a lot"?  Perhaps you could take the path of least 
resistance instead and simply increase the size of the buffer, like with 
commit ff604a674771 ("gdb/testsuite: Bump up `match_max'").  This could be 
done temporarily for this test only, so as to avoid slowing down `expect' 
throughout the test suite.

  Maciej
Pedro Alves April 13, 2018, 1:10 p.m. | #2
On 04/13/2018 12:39 AM, Maciej W. Rozycki wrote:
> On Mon, 9 Apr 2018, Andrew Burgess wrote:

> 

>>  # Test for a regression where this command would internal-error if the

>> -# program wasn't running.

>> -gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*"

>> +# program wasn't running.  If there's a lot of registers then this

>> +# might overflow expect's buffers, so process the output line at a

>> +# time.

>> +send_gdb "maint print registers\n"

>> +gdb_expect {

>> +    -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" {

>> +	exp_continue

>> +    }

> 

>  I think this changes the meaning of the test; you want to preserve the 

> heading match pattern at the very least.  Also `gdb_test' handles various 

> error cases gracefully (which matters for the avoidance of excessive 

> timeouts with some test boards), whereas your simple matcher does not.


Yeah, there's no good reason to use gdb_expect directly here, AFAICT.
You can do the same thing with gdb_test_multiple, which handles
the timeout already, as well as other error conditions, including
the internal-error the comment the test above mentions.

> 

>  Also how many is "a lot"?  Perhaps you could take the path of least 

> resistance instead and simply increase the size of the buffer, like with 

> commit ff604a674771 ("gdb/testsuite: Bump up `match_max'").  This could be 

> done temporarily for this test only, so as to avoid slowing down `expect' 

> throughout the test suite.


I think the exp_continue trick is better for getting rid of the
problem for good.  We can still use it, with gdb_test_multiple.

A few comments more:

> +    -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" {


Nit: \n\r instead of \r\n kind of reads like a typo to me:

$ grep -rn "\\\\r\\\\n\\\\]" | wc -l
1036
$ grep -rn "\\\\n\\\\r\\\\]" | wc -l
28

I'd suggest flipping it around to the more usual form
just to avoid causing pause when people read the regexp.

> +	exp_continue

> +    }

> +    -re ".*$gdb_prompt $" {


No need for leading .*, it's implied.

> +	pass "maint print registers"

> +    }

> +    timeout {

> +	fail "maint print registers (timeout)"

> +    }

> +}

> +


So I'd suggest something like this:

set saw_registers 0
set test "maint print registers"
gdb_test_multiple $test $test {
    -re "^\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[\r\n\]+" {
        set saw_registers 1
	exp_continue
    }
    -re "$gdb_prompt $" {
	gdb_assert $saw_registers $test
    }
}

The "saw_registers" bit ends up serving as replacement for
seeing the heading, though you can also add a pattern to 
match the heading and check it in the gdb_assert instead if
you'd like.

Thanks,
Pedro Alves
Maciej W. Rozycki April 13, 2018, 1:55 p.m. | #3
On Fri, 13 Apr 2018, Pedro Alves wrote:

> So I'd suggest something like this:

> 

> set saw_registers 0

> set test "maint print registers"

> gdb_test_multiple $test $test {

>     -re "^\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[\r\n\]+" {

>         set saw_registers 1

> 	exp_continue

>     }

>     -re "$gdb_prompt $" {

> 	gdb_assert $saw_registers $test

>     }

> }

> 

> The "saw_registers" bit ends up serving as replacement for

> seeing the heading, though you can also add a pattern to 

> match the heading and check it in the gdb_assert instead if

> you'd like.


 FWIW I think we should keep the heading check.

  Maciej
Andrew Burgess May 4, 2018, 12:01 p.m. | #4
Thanks for the review feedback.

I've updated the patch below inline with Pedro's feedback, and
included a header check as requested by Maciej.  I tested this on
x86-64 and against RiscV which is the target that was originally
causing me problems.

Thanks,
Andrew

---

[PATCH 1/2] gdb/testsuite: Handle targets with lots of registers

In gdb.base/maint.exp a test calls 'maint print registers'.  If the
target has lots of registers this may overflow expect's buffers,
causing the test to fail.

After this commit we process the output line at a time until we get back
to the GDB prompt, this should prevent buffer overrun while still
testing that the command works as required.

gdb/testsuite/ChangeLog:

	* gdb.base/maint.exp: Process output from 'maint print registers'
	line at a time.
---
 gdb/testsuite/ChangeLog          |  5 +++++
 gdb/testsuite/gdb.base/maint.exp | 25 +++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp
index 93221085653..1fe36405df3 100644
--- a/gdb/testsuite/gdb.base/maint.exp
+++ b/gdb/testsuite/gdb.base/maint.exp
@@ -62,8 +62,29 @@ gdb_test_no_output "set height 0"
 gdb_file_cmd ${binfile}
 
 # Test for a regression where this command would internal-error if the
-# program wasn't running.
-gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*"
+# program wasn't running.  If there's a lot of registers then this
+# might overflow expect's buffers, so process the output line at a
+# time.
+set saw_registers 0
+set saw_headers 0
+set test "maint print registers"
+gdb_test_multiple $test $test {
+    -re "\[^\r\n\]+Name\[^\r\n\]+Nr\[^\r\n\]+Rel\[^\r\n\]+Offset\[^\r\n\]+Size\[^\r\n\]+Type\[^\r\n\]+\[\r\n\]+" {
+	set saw_headers 1
+	exp_continue
+    }
+    -re "^\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[\r\n\]+" {
+        set saw_registers 1
+        exp_continue
+    }
+    -re "^\\*\[0-9\]+\[^\r\n\]+\[\r\n\]+" {
+        exp_continue
+    }
+    -re "$gdb_prompt $" {
+        gdb_assert $saw_registers "$test: saw some registers"
+        gdb_assert $saw_headers "$test: saw header line"
+    }
+}
 
 # Test "mt expand-symtabs" here as it's easier to verify before we
 # run the program.
-- 
2.14.3
Pedro Alves May 4, 2018, 12:47 p.m. | #5
On 05/04/2018 01:01 PM, Andrew Burgess wrote:
> Thanks for the review feedback.

> 

> I've updated the patch below inline with Pedro's feedback, and

> included a header check as requested by Maciej.  I tested this on

> x86-64 and against RiscV which is the target that was originally

> causing me problems.


Thanks.

>  # Test for a regression where this command would internal-error if the

> -# program wasn't running.

> -gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*"

> +# program wasn't running.  If there's a lot of registers then this

> +# might overflow expect's buffers, so process the output line at a

> +# time.

> +set saw_registers 0

> +set saw_headers 0

> +set test "maint print registers"

> +gdb_test_multiple $test $test {

> +    -re "\[^\r\n\]+Name\[^\r\n\]+Nr\[^\r\n\]+Rel\[^\r\n\]+Offset\[^\r\n\]+Size\[^\r\n\]+Type\[^\r\n\]+\[\r\n\]+" {

> +	set saw_headers 1

> +	exp_continue

> +    }

> +    -re "^\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[0-9\]+\[^\r\n\]+\[\r\n\]+" {

> +        set saw_registers 1

> +        exp_continue

> +    }

> +    -re "^\\*\[0-9\]+\[^\r\n\]+\[\r\n\]+" {

> +        exp_continue

> +    }

> +    -re "$gdb_prompt $" {

> +        gdb_assert $saw_registers "$test: saw some registers"

> +        gdb_assert $saw_headers "$test: saw header line"


We try to avoid the potential for different FAIL / PASS
names/messages.  I.e., if the test times out, or gdb crashes,
you'll get "FAIL: $test" only , while if it reaches the prompt,
you'll get two tests recorded in gdb.sum.  The idea of matching
FAIL/PASS is to make it easier for scripts to distinguish
regressions/progressions vs new failures/passes.  (Text in
trailing ()s, like " (timeout)" is considered informational, and
can/should be stripped for test-matching purposes).  If you want to
record that the register and headers were seen, better put it in gdb.log,
with send_log or verbose -log, and do:

        gdb_assert {$saw_registers && $saw_headers} $test

OK with that change.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp
index 93221085653..f73495faa5b 100644
--- a/gdb/testsuite/gdb.base/maint.exp
+++ b/gdb/testsuite/gdb.base/maint.exp
@@ -62,8 +62,22 @@  gdb_test_no_output "set height 0"
 gdb_file_cmd ${binfile}
 
 # Test for a regression where this command would internal-error if the
-# program wasn't running.
-gdb_test "maint print registers" "Name.*Nr.*Rel.*Offset.*Size.*Type.*"
+# program wasn't running.  If there's a lot of registers then this
+# might overflow expect's buffers, so process the output line at a
+# time.
+send_gdb "maint print registers\n"
+gdb_expect {
+    -re "^\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[0-9\]+\[^\n\r\]+\[\n\r\]+" {
+	exp_continue
+    }
+    -re ".*$gdb_prompt $" {
+	pass "maint print registers"
+    }
+    timeout {
+	fail "maint print registers (timeout)"
+    }
+}
+
 
 # Test "mt expand-symtabs" here as it's easier to verify before we
 # run the program.