* [PATCH 0/3] Small testsuite updates @ 2018-04-09 15:15 Andrew Burgess 2018-04-09 15:15 ` [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case Andrew Burgess ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Andrew Burgess @ 2018-04-09 15:15 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess While working on additional riscv support I ran into a few minor annoyances in the testsuite, addressed in these patches. Only the second patch is really RiscV specific, one and three are pretty generic. --- Andrew Burgess (3): gdb/testsuite: Fix broken regexp in gdbstub case gdb/testsuite: Filter out some registers for riscv gdb/testsuite: Handle targets with lots of registers gdb/testsuite/ChangeLog | 15 +++++++++++++++ gdb/testsuite/gdb.base/callfuncs.exp | 10 ++++++++++ gdb/testsuite/gdb.base/maint.exp | 18 ++++++++++++++++-- gdb/testsuite/lib/mi-support.exp | 6 +++++- 4 files changed, 46 insertions(+), 3 deletions(-) -- 2.12.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case 2018-04-09 15:15 [PATCH 0/3] Small testsuite updates Andrew Burgess @ 2018-04-09 15:15 ` Andrew Burgess 2018-04-13 12:12 ` Pedro Alves 2018-04-09 15:15 ` [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv Andrew Burgess 2018-04-09 15:15 ` [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers Andrew Burgess 2 siblings, 1 reply; 16+ messages in thread From: Andrew Burgess @ 2018-04-09 15:15 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess When $use_gdb_stub is true then, when we start an MI target there's a regexp to match GDB's startup pattern. Unfortunately the pattern is broken, and we're also missing a timeout case in the match list (which would have helped point out that the regexp was broken). gdb/testsuite/ChangeLog: * lib/mi-support.exp (mi_run_cmd_full): Fix regexp and add a timeout. --- gdb/testsuite/ChangeLog | 5 +++++ gdb/testsuite/lib/mi-support.exp | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp index 2846da74e47..851e490f4de 100644 --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -979,7 +979,11 @@ proc mi_run_cmd_full {use_mi_command args} { send_gdb "jump *$start\n" warning "Using CLI jump command, expect run-to-main FAIL" gdb_expect { - -re "${run_match}&\"jump \\*${start}\\n\"\[\r\n\]+~\"Continuing at 0x\[0-9A-Fa-f\]+\\n.\"\[\r\n\]+\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\r\n${mi_gdb_prompt}" {} + -re "&\"jump \\*${start}\\\\n\"\[\r\n\]+~\"Continuing at 0x\[0-9A-Fa-f\]+\.\\\\n\"\[\r\n\]+\\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\[\r\n\]+${mi_gdb_prompt}" {} + timeout { + perror "Unable to start target" + return -1 + } } return 0 } -- 2.12.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case 2018-04-09 15:15 ` [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case Andrew Burgess @ 2018-04-13 12:12 ` Pedro Alves 2018-05-03 19:41 ` Andrew Burgess 0 siblings, 1 reply; 16+ messages in thread From: Pedro Alves @ 2018-04-13 12:12 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 04/09/2018 04:15 PM, Andrew Burgess wrote: > When $use_gdb_stub is true then, when we start an MI target there's a > regexp to match GDB's startup pattern. Unfortunately the pattern is > broken, and we're also missing a timeout case in the match list (which > would have helped point out that the regexp was broken). That seems to have been added by: https://sourceware.org/ml/gdb-patches/2015-12/msg00357.html curious how it seems to have worked back then. I wonder whether it was GDB's CLI-in-MI output that changed meanwhile? I'd have appreciated a bit more detail on what is actually broken in the pattern. The git log above gives no clue, and the diff is not exactly easy to read. I trust that it's now correct though. Thus, OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case 2018-04-13 12:12 ` Pedro Alves @ 2018-05-03 19:41 ` Andrew Burgess 2018-05-04 9:18 ` Pedro Alves 0 siblings, 1 reply; 16+ messages in thread From: Andrew Burgess @ 2018-05-03 19:41 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches * Pedro Alves <palves@redhat.com> [2018-04-13 13:12:30 +0100]: > On 04/09/2018 04:15 PM, Andrew Burgess wrote: > > When $use_gdb_stub is true then, when we start an MI target there's a > > regexp to match GDB's startup pattern. Unfortunately the pattern is > > broken, and we're also missing a timeout case in the match list (which > > would have helped point out that the regexp was broken). > > That seems to have been added by: > https://sourceware.org/ml/gdb-patches/2015-12/msg00357.html > > curious how it seems to have worked back then. I wonder whether > it was GDB's CLI-in-MI output that changed meanwhile? > > I'd have appreciated a bit more detail on what is actually broken in > the pattern. The git log above gives no clue, and the diff is not > exactly easy to read. I trust that it's now correct though. Thus, OK. I aim to please, so here's a revised commit message detailing what changed. The actual patch content hasn't changed. I'll wait a few days, but if I don't hear anything I'll assume your approval above still stands and apply the patch. Thanks, Andrew --- gdb/testsuite: Fix broken regexp in gdbstub case When $use_gdb_stub is true then, when we start an MI target there's a regexp to match GDB's startup pattern. Unfortunately the pattern is broken, and we're also missing a timeout case in the match list (which would have helped point out that the regexp was broken). The changes to the regexp are: 1. Remove '${run_match}' prefix, the issued command doesn't include '${run_prefix}' so expecting '${run_match}' is wrong. 2. Replaced '\\n' with '\\\\n' in order to match literal '\n' in GDBs output (that is, match a backslash followed by 'n', not a newline character). 3. Replaced a '.' (matching any character) with '\.' to match a '.' and moved the '\.' into the correct place in the regexp. 4. Replaced '\r\n' with '[\r\n]+' to match the end of a line. This change isn't esential, but matches the other end of line patterns within this regexp. Here's an example of the output that the regexp should match taken from a testfile log, the first line is the command sent to GDB, and the remaining lines are the response from GDB: jump *_start &"jump *_start\n" ~"Continuing at 0x10074.\n" ^running *running,thread-id="all" (gdb) gdb/testsuite/ChangeLog: * lib/mi-support.exp (mi_run_cmd_full): Fix regexp and add a timeout. --- gdb/testsuite/ChangeLog | 5 +++++ gdb/testsuite/lib/mi-support.exp | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp index 2846da74e47..851e490f4de 100644 --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -979,7 +979,11 @@ proc mi_run_cmd_full {use_mi_command args} { send_gdb "jump *$start\n" warning "Using CLI jump command, expect run-to-main FAIL" gdb_expect { - -re "${run_match}&\"jump \\*${start}\\n\"\[\r\n\]+~\"Continuing at 0x\[0-9A-Fa-f\]+\\n.\"\[\r\n\]+\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\r\n${mi_gdb_prompt}" {} + -re "&\"jump \\*${start}\\\\n\"\[\r\n\]+~\"Continuing at 0x\[0-9A-Fa-f\]+\.\\\\n\"\[\r\n\]+\\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\[\r\n\]+${mi_gdb_prompt}" {} + timeout { + perror "Unable to start target" + return -1 + } } return 0 } -- 2.14.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case 2018-05-03 19:41 ` Andrew Burgess @ 2018-05-04 9:18 ` Pedro Alves 0 siblings, 0 replies; 16+ messages in thread From: Pedro Alves @ 2018-05-04 9:18 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On 05/03/2018 08:41 PM, Andrew Burgess wrote: > * Pedro Alves <palves@redhat.com> [2018-04-13 13:12:30 +0100]: > >> On 04/09/2018 04:15 PM, Andrew Burgess wrote: >>> When $use_gdb_stub is true then, when we start an MI target there's a >>> regexp to match GDB's startup pattern. Unfortunately the pattern is >>> broken, and we're also missing a timeout case in the match list (which >>> would have helped point out that the regexp was broken). >> >> That seems to have been added by: >> https://sourceware.org/ml/gdb-patches/2015-12/msg00357.html >> >> curious how it seems to have worked back then. I wonder whether >> it was GDB's CLI-in-MI output that changed meanwhile? >> >> I'd have appreciated a bit more detail on what is actually broken in >> the pattern. The git log above gives no clue, and the diff is not >> exactly easy to read. I trust that it's now correct though. Thus, OK. > > I aim to please, :-) > so here's a revised commit message detailing what > changed. The actual patch content hasn't changed. I'll wait a few > days, but if I don't hear anything I'll assume your approval above > still stands and apply the patch. Perfect. This is OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv 2018-04-09 15:15 [PATCH 0/3] Small testsuite updates Andrew Burgess 2018-04-09 15:15 ` [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case Andrew Burgess @ 2018-04-09 15:15 ` Andrew Burgess 2018-04-09 21:28 ` Palmer Dabbelt 2018-04-09 15:15 ` [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers Andrew Burgess 2 siblings, 1 reply; 16+ messages in thread From: Andrew Burgess @ 2018-04-09 15:15 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess On riscv the cycle counter, and instructions retired counter CSRs are read only, this causes problems in the gdb.base/callfuncs.exp test, as the values in these CSRs change after an inferior call, the check that no target registers have been modified then fails. Luckily the test already has a mechanism in place for filtering out registers that are modified (and can't be restored) by an inferior call, so this commit adds the problem registers into this list for riscv. In the future we may end up needing to filter out more CSRs, but right now, for the targets I have access too, these are the only ones causing problems. gdb/testsuite/ChangeLog: * gdb.base/callfuncs.exp (fetch_all_registers): Add riscv register filter pattern. --- gdb/testsuite/ChangeLog | 5 +++++ gdb/testsuite/gdb.base/callfuncs.exp | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/gdb/testsuite/gdb.base/callfuncs.exp b/gdb/testsuite/gdb.base/callfuncs.exp index 94636938752..c5e39918c2a 100644 --- a/gdb/testsuite/gdb.base/callfuncs.exp +++ b/gdb/testsuite/gdb.base/callfuncs.exp @@ -285,6 +285,16 @@ proc fetch_all_registers {test} { } exp_continue } + -re "^\(?:cycle\|instret\)\[ \t\]+\[^\r\n\]+\r\n" { + if [istarget "riscv*-*-*"] { + # Filter out the cycle counter and instructions + # retired counter CSRs which are read-only, giving + # spurious differences. + } else { + lappend all_registers_lines $expect_out(0,string) + } + exp_continue + } -re "^\[^ \t\]+\[ \t\]+\[^\r\n\]+\r\n" { lappend all_registers_lines $expect_out(0,string) exp_continue -- 2.12.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv 2018-04-09 15:15 ` [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv Andrew Burgess @ 2018-04-09 21:28 ` Palmer Dabbelt 2018-04-09 22:26 ` Andrew Burgess 0 siblings, 1 reply; 16+ messages in thread From: Palmer Dabbelt @ 2018-04-09 21:28 UTC (permalink / raw) To: andrew.burgess; +Cc: gdb-patches, andrew.burgess On Mon, 09 Apr 2018 08:15:28 PDT (-0700), andrew.burgess@embecosm.com wrote: > On riscv the cycle counter, and instructions retired counter CSRs are > read only, this causes problems in the gdb.base/callfuncs.exp test, as > the values in these CSRs change after an inferior call, the check that > no target registers have been modified then fails. > > Luckily the test already has a mechanism in place for filtering out > registers that are modified (and can't be restored) by an inferior call, > so this commit adds the problem registers into this list for riscv. > > In the future we may end up needing to filter out more CSRs, but right > now, for the targets I have access too, these are the only ones causing > problems. > > gdb/testsuite/ChangeLog: > > * gdb.base/callfuncs.exp (fetch_all_registers): Add riscv register > filter pattern. > --- > gdb/testsuite/ChangeLog | 5 +++++ > gdb/testsuite/gdb.base/callfuncs.exp | 10 ++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/gdb/testsuite/gdb.base/callfuncs.exp b/gdb/testsuite/gdb.base/callfuncs.exp > index 94636938752..c5e39918c2a 100644 > --- a/gdb/testsuite/gdb.base/callfuncs.exp > +++ b/gdb/testsuite/gdb.base/callfuncs.exp > @@ -285,6 +285,16 @@ proc fetch_all_registers {test} { > } > exp_continue > } > + -re "^\(?:cycle\|instret\)\[ \t\]+\[^\r\n\]+\r\n" { > + if [istarget "riscv*-*-*"] { > + # Filter out the cycle counter and instructions > + # retired counter CSRs which are read-only, giving > + # spurious differences. > + } else { > + lappend all_registers_lines $expect_out(0,string) > + } > + exp_continue > + } > -re "^\[^ \t\]+\[ \t\]+\[^\r\n\]+\r\n" { > lappend all_registers_lines $expect_out(0,string) > exp_continue I think we only want to check the X and F registers here -- essentially every CSR is a special register where you can't really rely on the value not being changed somewhere by hardware. For example: * The interrupt pending bits could flip at any point, even if interrupts are disabled. * The floating-point dirty and exception state bits could change if a floating-point instruction executes. * The various trap CSRs (epc, badaddr, cause, etc) get set whenever a trap is executed. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv 2018-04-09 21:28 ` Palmer Dabbelt @ 2018-04-09 22:26 ` Andrew Burgess 2018-04-10 20:25 ` Palmer Dabbelt 0 siblings, 1 reply; 16+ messages in thread From: Andrew Burgess @ 2018-04-09 22:26 UTC (permalink / raw) To: Palmer Dabbelt; +Cc: gdb-patches * Palmer Dabbelt <palmer@sifive.com> [2018-04-09 14:28:33 -0700]: > On Mon, 09 Apr 2018 08:15:28 PDT (-0700), andrew.burgess@embecosm.com wrote: > > On riscv the cycle counter, and instructions retired counter CSRs are > > read only, this causes problems in the gdb.base/callfuncs.exp test, as > > the values in these CSRs change after an inferior call, the check that > > no target registers have been modified then fails. > > > > Luckily the test already has a mechanism in place for filtering out > > registers that are modified (and can't be restored) by an inferior call, > > so this commit adds the problem registers into this list for riscv. > > > > In the future we may end up needing to filter out more CSRs, but right > > now, for the targets I have access too, these are the only ones causing > > problems. > > > > gdb/testsuite/ChangeLog: > > > > * gdb.base/callfuncs.exp (fetch_all_registers): Add riscv register > > filter pattern. > > --- > > gdb/testsuite/ChangeLog | 5 +++++ > > gdb/testsuite/gdb.base/callfuncs.exp | 10 ++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/gdb/testsuite/gdb.base/callfuncs.exp b/gdb/testsuite/gdb.base/callfuncs.exp > > index 94636938752..c5e39918c2a 100644 > > --- a/gdb/testsuite/gdb.base/callfuncs.exp > > +++ b/gdb/testsuite/gdb.base/callfuncs.exp > > @@ -285,6 +285,16 @@ proc fetch_all_registers {test} { > > } > > exp_continue > > } > > + -re "^\(?:cycle\|instret\)\[ \t\]+\[^\r\n\]+\r\n" { > > + if [istarget "riscv*-*-*"] { > > + # Filter out the cycle counter and instructions > > + # retired counter CSRs which are read-only, giving > > + # spurious differences. > > + } else { > > + lappend all_registers_lines $expect_out(0,string) > > + } > > + exp_continue > > + } > > -re "^\[^ \t\]+\[ \t\]+\[^\r\n\]+\r\n" { > > lappend all_registers_lines $expect_out(0,string) > > exp_continue > > I think we only want to check the X and F registers here -- essentially > every CSR is a special register where you can't really rely on the value not > being changed somewhere by hardware. For example: > > * The interrupt pending bits could flip at any point, even if interrupts are > disabled. > * The floating-point dirty and exception state bits could change if a > floating-point instruction executes. > * The various trap CSRs (epc, badaddr, cause, etc) get set whenever a trap > is executed. I'm not sure about the second case. If GDB is stopped and we perform an inferior call, then ideally the entire floating point state, including contents of things like fcsr would be reset, otherwise, when we continue the behaviour might not be as we expect. I do agree with you that the two registers I've filtered so far probably aren't enough, but I'm really reluctant to _only_ check X and F registers. I think a better selection would be X, F, and read/write user CSRs. Which means I need to build the list of CSRs to filter, I was hoping to put that off for another day for now... Let me know how you'd feel about leaving this as it is for now, and extending the filter list at a later date. thanks, Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv 2018-04-09 22:26 ` Andrew Burgess @ 2018-04-10 20:25 ` Palmer Dabbelt 2018-04-13 12:55 ` Pedro Alves 0 siblings, 1 reply; 16+ messages in thread From: Palmer Dabbelt @ 2018-04-10 20:25 UTC (permalink / raw) To: andrew.burgess; +Cc: gdb-patches On Mon, 09 Apr 2018 15:26:07 PDT (-0700), andrew.burgess@embecosm.com wrote: > * Palmer Dabbelt <palmer@sifive.com> [2018-04-09 14:28:33 -0700]: > >> On Mon, 09 Apr 2018 08:15:28 PDT (-0700), andrew.burgess@embecosm.com wrote: >> > On riscv the cycle counter, and instructions retired counter CSRs are >> > read only, this causes problems in the gdb.base/callfuncs.exp test, as >> > the values in these CSRs change after an inferior call, the check that >> > no target registers have been modified then fails. >> > >> > Luckily the test already has a mechanism in place for filtering out >> > registers that are modified (and can't be restored) by an inferior call, >> > so this commit adds the problem registers into this list for riscv. >> > >> > In the future we may end up needing to filter out more CSRs, but right >> > now, for the targets I have access too, these are the only ones causing >> > problems. >> > >> > gdb/testsuite/ChangeLog: >> > >> > * gdb.base/callfuncs.exp (fetch_all_registers): Add riscv register >> > filter pattern. >> > --- >> > gdb/testsuite/ChangeLog | 5 +++++ >> > gdb/testsuite/gdb.base/callfuncs.exp | 10 ++++++++++ >> > 2 files changed, 15 insertions(+) >> > >> > diff --git a/gdb/testsuite/gdb.base/callfuncs.exp b/gdb/testsuite/gdb.base/callfuncs.exp >> > index 94636938752..c5e39918c2a 100644 >> > --- a/gdb/testsuite/gdb.base/callfuncs.exp >> > +++ b/gdb/testsuite/gdb.base/callfuncs.exp >> > @@ -285,6 +285,16 @@ proc fetch_all_registers {test} { >> > } >> > exp_continue >> > } >> > + -re "^\(?:cycle\|instret\)\[ \t\]+\[^\r\n\]+\r\n" { >> > + if [istarget "riscv*-*-*"] { >> > + # Filter out the cycle counter and instructions >> > + # retired counter CSRs which are read-only, giving >> > + # spurious differences. >> > + } else { >> > + lappend all_registers_lines $expect_out(0,string) >> > + } >> > + exp_continue >> > + } >> > -re "^\[^ \t\]+\[ \t\]+\[^\r\n\]+\r\n" { >> > lappend all_registers_lines $expect_out(0,string) >> > exp_continue >> >> I think we only want to check the X and F registers here -- essentially >> every CSR is a special register where you can't really rely on the value not >> being changed somewhere by hardware. For example: >> >> * The interrupt pending bits could flip at any point, even if interrupts are >> disabled. >> * The floating-point dirty and exception state bits could change if a >> floating-point instruction executes. >> * The various trap CSRs (epc, badaddr, cause, etc) get set whenever a trap >> is executed. > > I'm not sure about the second case. If GDB is stopped and we perform > an inferior call, then ideally the entire floating point state, > including contents of things like fcsr would be reset, otherwise, when > we continue the behaviour might not be as we expect. > > I do agree with you that the two registers I've filtered so far > probably aren't enough, but I'm really reluctant to _only_ check X and > F registers. I think a better selection would be X, F, and read/write > user CSRs. Which means I need to build the list of CSRs to filter, I > was hoping to put that off for another day for now... > > Let me know how you'd feel about leaving this as it is for now, and > extending the filter list at a later date. I think it's fine for now, we can fix it when another test fails :) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv 2018-04-10 20:25 ` Palmer Dabbelt @ 2018-04-13 12:55 ` Pedro Alves 0 siblings, 0 replies; 16+ messages in thread From: Pedro Alves @ 2018-04-13 12:55 UTC (permalink / raw) To: Palmer Dabbelt, andrew.burgess; +Cc: gdb-patches On 04/10/2018 09:25 PM, Palmer Dabbelt wrote: > On Mon, 09 Apr 2018 15:26:07 PDT (-0700), andrew.burgess@embecosm.com wrote: >> Let me know how you'd feel about leaving this as it is for now, and >> extending the filter list at a later date. > > I think it's fine for now, we can fix it when another test fails :) This is OK, then. For the register lists, I wonder whether a better option would be to instead let gdb determine which registers should be compared, instead of hard coding in the testcase. Like, look at the save/restore reggroups instead of all registers. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers 2018-04-09 15:15 [PATCH 0/3] Small testsuite updates Andrew Burgess 2018-04-09 15:15 ` [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case Andrew Burgess 2018-04-09 15:15 ` [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv Andrew Burgess @ 2018-04-09 15:15 ` Andrew Burgess 2018-04-12 23:40 ` Maciej W. Rozycki 2 siblings, 1 reply; 16+ messages in thread From: Andrew Burgess @ 2018-04-09 15:15 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess 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(-) 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. -- 2.12.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers 2018-04-09 15:15 ` [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers Andrew Burgess @ 2018-04-12 23:40 ` Maciej W. Rozycki 2018-04-13 13:10 ` Pedro Alves 0 siblings, 1 reply; 16+ messages in thread From: Maciej W. Rozycki @ 2018-04-12 23:40 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers 2018-04-12 23:40 ` Maciej W. Rozycki @ 2018-04-13 13:10 ` Pedro Alves 2018-04-13 13:57 ` Maciej W. Rozycki 0 siblings, 1 reply; 16+ messages in thread From: Pedro Alves @ 2018-04-13 13:10 UTC (permalink / raw) To: Maciej W. Rozycki, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers 2018-04-13 13:10 ` Pedro Alves @ 2018-04-13 13:57 ` Maciej W. Rozycki 2018-05-04 12:01 ` Andrew Burgess 0 siblings, 1 reply; 16+ messages in thread From: Maciej W. Rozycki @ 2018-04-13 13:57 UTC (permalink / raw) To: Pedro Alves; +Cc: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers 2018-04-13 13:57 ` Maciej W. Rozycki @ 2018-05-04 12:01 ` Andrew Burgess 2018-05-04 12:47 ` Pedro Alves 0 siblings, 1 reply; 16+ messages in thread From: Andrew Burgess @ 2018-05-04 12:01 UTC (permalink / raw) To: gdb-patches; +Cc: Maciej W. Rozycki, Pedro Alves 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers 2018-05-04 12:01 ` Andrew Burgess @ 2018-05-04 12:47 ` Pedro Alves 0 siblings, 0 replies; 16+ messages in thread From: Pedro Alves @ 2018-05-04 12:47 UTC (permalink / raw) To: Andrew Burgess, gdb-patches; +Cc: Maciej W. Rozycki 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-05-04 12:47 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-09 15:15 [PATCH 0/3] Small testsuite updates Andrew Burgess 2018-04-09 15:15 ` [PATCH 1/3] gdb/testsuite: Fix broken regexp in gdbstub case Andrew Burgess 2018-04-13 12:12 ` Pedro Alves 2018-05-03 19:41 ` Andrew Burgess 2018-05-04 9:18 ` Pedro Alves 2018-04-09 15:15 ` [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv Andrew Burgess 2018-04-09 21:28 ` Palmer Dabbelt 2018-04-09 22:26 ` Andrew Burgess 2018-04-10 20:25 ` Palmer Dabbelt 2018-04-13 12:55 ` Pedro Alves 2018-04-09 15:15 ` [PATCH 3/3] gdb/testsuite: Handle targets with lots of registers Andrew Burgess 2018-04-12 23:40 ` Maciej W. Rozycki 2018-04-13 13:10 ` Pedro Alves 2018-04-13 13:57 ` Maciej W. Rozycki 2018-05-04 12:01 ` Andrew Burgess 2018-05-04 12:47 ` 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).