* [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info
@ 2021-06-22 17:24 Simon Marchi
2021-06-22 21:52 ` Andrew Burgess
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Simon Marchi @ 2021-06-22 17:24 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom de Vries, Simon Marchi
On Ubuntu 20.04, when the debug info package for libc is not installed,
I get:
FAIL: gdb.base/info-types-c++.exp: info types
FAIL: gdb.base/info-types-c.exp: info types
The reason is that the output of info types is exactly:
(gdb) info types
All defined types:
File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c:
52: typedef enum {...} anon_enum_t;
45: typedef struct {...} anon_struct_t;
68: typedef union {...} anon_union_t;
28: typedef struct baz_t baz;
31: typedef struct baz_t * baz_ptr;
21: struct baz_t;
double
33: enum enum_t;
float
int
38: typedef enum enum_t my_enum_t;
17: typedef float my_float_t;
16: typedef int my_int_t;
54: typedef enum {...} nested_anon_enum_t;
47: typedef struct {...} nested_anon_struct_t;
70: typedef union {...} nested_anon_union_t;
30: typedef struct baz_t nested_baz;
29: typedef struct baz_t nested_baz_t;
39: typedef enum enum_t nested_enum_t;
19: typedef float nested_float_t;
18: typedef int nested_int_t;
62: typedef union union_t nested_union_t;
56: union union_t;
unsigned int
(gdb)
The lines we expect in the test contain an empty line at the end:
...
"18:\[\t \]+typedef int nested_int_t;" \
"62:\[\t \]+typedef union_t nested_union_t;" \
"--optional" "\[\t \]+unsigned int" \
""]
This is written with the supposition that other files will be listed, so
an empty line will be included to separate the symbols from this file
from the next one. This empty line is not included when info-types.c is
the only file listed.
This patch fixes the issue by not requiring an empty line. I don't
think it's a very good solution, however: the empty line ensures that
there is no additional unexpected items after "unsigned int".
I don't think that making the empty line optional (with --optional)
would achieve anything, since the lines would still match if there was
some unexpected output.
So, I'll think about it a bit more, but any suggestions are appreciated.
gdb/testsuite/ChangeLog:
* gdb.base/info-types.exp.tcl (run_test): Remove empty line at
end of expected regexps.
Change-Id: I078127e271e2eb628b4805129bdb6f3f9c795629
---
gdb/testsuite/gdb.base/info-types.exp.tcl | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/gdb/testsuite/gdb.base/info-types.exp.tcl b/gdb/testsuite/gdb.base/info-types.exp.tcl
index c820adc4ac1..a24899c243d 100644
--- a/gdb/testsuite/gdb.base/info-types.exp.tcl
+++ b/gdb/testsuite/gdb.base/info-types.exp.tcl
@@ -75,8 +75,7 @@ proc run_test { lang } {
"19:\[\t \]+typedef float nested_float_t;" \
"18:\[\t \]+typedef int nested_int_t;" \
"62:\[\t \]+typedef union_t nested_union_t;" \
- "--optional" "\[\t \]+unsigned int" \
- ""]
+ "--optional" "\[\t \]+unsigned int"]
} else {
set output_lines \
[list \
@@ -106,8 +105,7 @@ proc run_test { lang } {
"18:\[\t \]+typedef int nested_int_t;" \
"62:\[\t \]+typedef union union_t nested_union_t;" \
"56:\[\t \]+union union_t;" \
- "--optional" "\[\t \]+unsigned int" \
- ""]
+ "--optional" "\[\t \]+unsigned int"]
}
gdb_test_lines "info types" "" $output_lines
--
2.31.1
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-22 17:24 [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info Simon Marchi @ 2021-06-22 21:52 ` Andrew Burgess 2021-06-23 11:35 ` Andrew Burgess 2021-06-23 12:04 ` Tom de Vries 2021-06-23 11:33 ` Tom de Vries 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 2 siblings, 2 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-22 21:52 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-22 13:24:47 -0400]: > On Ubuntu 20.04, when the debug info package for libc is not installed, > I get: > > FAIL: gdb.base/info-types-c++.exp: info types > FAIL: gdb.base/info-types-c.exp: info types > > The reason is that the output of info types is exactly: > > (gdb) info types > All defined types: > > File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: > 52: typedef enum {...} anon_enum_t; > 45: typedef struct {...} anon_struct_t; > 68: typedef union {...} anon_union_t; > 28: typedef struct baz_t baz; > 31: typedef struct baz_t * baz_ptr; > 21: struct baz_t; > double > 33: enum enum_t; > float > int > 38: typedef enum enum_t my_enum_t; > 17: typedef float my_float_t; > 16: typedef int my_int_t; > 54: typedef enum {...} nested_anon_enum_t; > 47: typedef struct {...} nested_anon_struct_t; > 70: typedef union {...} nested_anon_union_t; > 30: typedef struct baz_t nested_baz; > 29: typedef struct baz_t nested_baz_t; > 39: typedef enum enum_t nested_enum_t; > 19: typedef float nested_float_t; > 18: typedef int nested_int_t; > 62: typedef union union_t nested_union_t; > 56: union union_t; > unsigned int > (gdb) > > The lines we expect in the test contain an empty line at the end: > > ... > "18:\[\t \]+typedef int nested_int_t;" \ > "62:\[\t \]+typedef union_t nested_union_t;" \ > "--optional" "\[\t \]+unsigned int" \ > ""] > > This is written with the supposition that other files will be listed, so > an empty line will be included to separate the symbols from this file > from the next one. This empty line is not included when info-types.c is > the only file listed. > > This patch fixes the issue by not requiring an empty line. I don't > think it's a very good solution, however: the empty line ensures that > there is no additional unexpected items after "unsigned int". > > I don't think that making the empty line optional (with --optional) > would achieve anything, since the lines would still match if there was > some unexpected output. > > So, I'll think about it a bit more, but any suggestions are appreciated. What about the patch below? Instead of using the builtin gdb_test_lines, I grab the type lines (but just for $srcfile) using gdb_test_multiple, then manually check for each type in turn. We no longer care about the order of the types in the output, but I think that is fine, the order was always pretty random anyway. I don't have a ChangeLog or commit message for this, but feel free to take this patch if you like the approach, or let me know and I can add the missing bits. Thanks, Andrew --- diff --git a/gdb/testsuite/gdb.base/info-types.exp.tcl b/gdb/testsuite/gdb.base/info-types.exp.tcl index c820adc4ac1..729ae23f367 100644 --- a/gdb/testsuite/gdb.base/info-types.exp.tcl +++ b/gdb/testsuite/gdb.base/info-types.exp.tcl @@ -39,13 +39,57 @@ proc run_test { lang } { } set file_re "File .*[string_to_regexp $srcfile]:" + set file_types {} + set collect_types False + set seen_header False + # Run 'info types' and collect all of the types associated with + # SRCFILE into the list FILE_TYPES. + gdb_test_multiple "info types" "" { + -re "^info types\r\n" { + exp_continue + } + -re "^\r\n" { + set collect_types False + exp_continue + } + -re "^All defined types:\r\n" { + set seen_header True + exp_continue + } + -re "^(File \[^\r\n\]+:)\r\n" { + set line $expect_out(1,string) + if {[regexp ${file_re} ${line}]} { + set collect_types True + } else { + set collect_types False + } + exp_continue + } + -re "^(\\d+:\\s+\[^\r\n\]+)\r\n" { + if { $collect_types } { + lappend file_types $expect_out(1,string) + } + exp_continue + } + -re "^(\\s+\[^\r\n\]+)\r\n" { + if { $collect_types } { + lappend file_types $expect_out(1,string) + } + exp_continue + } + -re "^$::gdb_prompt $" { + gdb_assert { $seen_header } + } + } + + # Get the list of all the types we expect to find in SRCFILE, as + # well as any optional types that might appear in SRCFILE. + # Optional types will depend on the compiler, these are just the + # ones we've seen so far. if { $lang == "c++" } { - set output_lines \ + set required_types \ [list \ - "All defined types:" \ - "--any" \ - $file_re \ "98:\[\t \]+CL;" \ "42:\[\t \]+anon_struct_t;" \ "65:\[\t \]+anon_union_t;" \ @@ -74,15 +118,14 @@ proc run_test { lang } { "39:\[\t \]+typedef enum_t nested_enum_t;" \ "19:\[\t \]+typedef float nested_float_t;" \ "18:\[\t \]+typedef int nested_int_t;" \ - "62:\[\t \]+typedef union_t nested_union_t;" \ - "--optional" "\[\t \]+unsigned int" \ - ""] + "62:\[\t \]+typedef union_t nested_union_t;"] + + set optional_types \ + [list \ + "\[\t \]+unsigned int"] } else { - set output_lines \ + set required_types \ [list \ - "All defined types:" \ - "--any" \ - $file_re \ "52:\[\t \]+typedef enum {\\.\\.\\.} anon_enum_t;" \ "45:\[\t \]+typedef struct {\\.\\.\\.} anon_struct_t;" \ "68:\[\t \]+typedef union {\\.\\.\\.} anon_union_t;" \ @@ -105,12 +148,33 @@ proc run_test { lang } { "19:\[\t \]+typedef float nested_float_t;" \ "18:\[\t \]+typedef int nested_int_t;" \ "62:\[\t \]+typedef union union_t nested_union_t;" \ - "56:\[\t \]+union union_t;" \ - "--optional" "\[\t \]+unsigned int" \ - ""] + "56:\[\t \]+union union_t;"] + + set optional_types \ + [list \ + "\[\t \]+unsigned int"] + } + + # Check that every required type was present. As we find each + # type we remove it from FILE_TYPES. + foreach type_pattern $required_types { + set idx [lsearch -regexp $file_types ${type_pattern}] + gdb_assert { $idx != -1 } \ + "found type '${type_pattern}'" + set file_types [lreplace $file_types $idx $idx] + } + + # Now remove each optional type. Obviously we can't require that + # these be present (they're optional), but removing them now + # allows us to perform the next check (see below). + foreach type_pattern $optional_types { + set idx [lsearch -regexp $file_types ${type_pattern}] + set file_types [lreplace $file_types $idx $idx] } - gdb_test_lines "info types" "" $output_lines + # With all of the required and optional types removed from + # FILE_TYPES, the FILE_TYPES list should now be empty. + gdb_assert { [llength $file_types] == 0 } } run_test $lang ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-22 21:52 ` Andrew Burgess @ 2021-06-23 11:35 ` Andrew Burgess 2021-06-23 12:04 ` Tom de Vries 1 sibling, 0 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 11:35 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Here's an updated version. Compared to the previous version: 1. we now enforce the ordering of the reported types (which are in alphabetical order), and 2. fixes a bug when the optional type is not present, 3. Includes ChangeLog and a commit message (ripped off from what Simon wrote). Thoughts? Thanks, Andrew --- commit f97f8ecbf28109d4294217df999c31be9176e56b Author: Andrew Burgess <andrew.burgess@embecosm.com> Date: Wed Jun 23 12:08:19 2021 +0100 gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info On Ubuntu 20.04, when the debug info package for libc is not installed, I get: FAIL: gdb.base/info-types-c++.exp: info types FAIL: gdb.base/info-types-c.exp: info types The reason is that the output of info types is exactly: (gdb) info types All defined types: File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: 52: typedef enum {...} anon_enum_t; 45: typedef struct {...} anon_struct_t; 68: typedef union {...} anon_union_t; 28: typedef struct baz_t baz; 31: typedef struct baz_t * baz_ptr; 21: struct baz_t; double 33: enum enum_t; float int 38: typedef enum enum_t my_enum_t; 17: typedef float my_float_t; 16: typedef int my_int_t; 54: typedef enum {...} nested_anon_enum_t; 47: typedef struct {...} nested_anon_struct_t; 70: typedef union {...} nested_anon_union_t; 30: typedef struct baz_t nested_baz; 29: typedef struct baz_t nested_baz_t; 39: typedef enum enum_t nested_enum_t; 19: typedef float nested_float_t; 18: typedef int nested_int_t; 62: typedef union union_t nested_union_t; 56: union union_t; unsigned int (gdb) The lines we expect in the test contain an empty line at the end: ... "18:\[\t \]+typedef int nested_int_t;" \ "62:\[\t \]+typedef union_t nested_union_t;" \ "--optional" "\[\t \]+unsigned int" \ ""] This is written with the supposition that other files will be listed, so an empty line will be included to separate the symbols from this file from the next one. This empty line is not included when info-types.c is the only file listed. This commit rewrites info-types.exp.tcl to avoid using gdb_test_lines. Instead, gdb_test_multiple is used to collect all of the types reported from info-types.c, we then manually check this list against a list of patterns that define the types we expect to see. gdb/testsuite/ChangeLog: * gdb.base/info-types.exp.tcl (run_test): Rewrite to avoid using gdb_test_lines, we now no longer care if we see one source file reported or many. diff --git a/gdb/testsuite/gdb.base/info-types.exp.tcl b/gdb/testsuite/gdb.base/info-types.exp.tcl index c820adc4ac1..aec1a6b954a 100644 --- a/gdb/testsuite/gdb.base/info-types.exp.tcl +++ b/gdb/testsuite/gdb.base/info-types.exp.tcl @@ -39,13 +39,57 @@ proc run_test { lang } { } set file_re "File .*[string_to_regexp $srcfile]:" + set file_types {} + set collect_types False + set seen_header False + # Run 'info types' and collect all of the types associated with + # SRCFILE into the list FILE_TYPES. + gdb_test_multiple "info types" "" { + -re "^info types\r\n" { + exp_continue + } + -re "^\r\n" { + set collect_types False + exp_continue + } + -re "^All defined types:\r\n" { + set seen_header True + exp_continue + } + -re "^(File \[^\r\n\]+:)\r\n" { + set line $expect_out(1,string) + if {[regexp ${file_re} ${line}]} { + set collect_types True + } else { + set collect_types False + } + exp_continue + } + -re "^(\\d+:\\s+\[^\r\n\]+)\r\n" { + if { $collect_types } { + lappend file_types $expect_out(1,string) + } + exp_continue + } + -re "^(\\s+\[^\r\n\]+)\r\n" { + if { $collect_types } { + lappend file_types $expect_out(1,string) + } + exp_continue + } + -re "^$::gdb_prompt $" { + gdb_assert { $seen_header } + } + } + + # Get the list of all the types we expect to find in SRCFILE, as + # well as any optional types that might appear in SRCFILE. + # Optional types will depend on the compiler, these are just the + # ones we've seen so far. if { $lang == "c++" } { - set output_lines \ + set required_types \ [list \ - "All defined types:" \ - "--any" \ - $file_re \ "98:\[\t \]+CL;" \ "42:\[\t \]+anon_struct_t;" \ "65:\[\t \]+anon_union_t;" \ @@ -74,15 +118,14 @@ proc run_test { lang } { "39:\[\t \]+typedef enum_t nested_enum_t;" \ "19:\[\t \]+typedef float nested_float_t;" \ "18:\[\t \]+typedef int nested_int_t;" \ - "62:\[\t \]+typedef union_t nested_union_t;" \ - "--optional" "\[\t \]+unsigned int" \ - ""] + "62:\[\t \]+typedef union_t nested_union_t;"] + + set optional_types \ + [list \ + "\[\t \]+unsigned int"] } else { - set output_lines \ + set required_types \ [list \ - "All defined types:" \ - "--any" \ - $file_re \ "52:\[\t \]+typedef enum {\\.\\.\\.} anon_enum_t;" \ "45:\[\t \]+typedef struct {\\.\\.\\.} anon_struct_t;" \ "68:\[\t \]+typedef union {\\.\\.\\.} anon_union_t;" \ @@ -105,12 +148,45 @@ proc run_test { lang } { "19:\[\t \]+typedef float nested_float_t;" \ "18:\[\t \]+typedef int nested_int_t;" \ "62:\[\t \]+typedef union union_t nested_union_t;" \ - "56:\[\t \]+union union_t;" \ - "--optional" "\[\t \]+unsigned int" \ - ""] + "56:\[\t \]+union union_t;"] + + set optional_types \ + [list \ + "\[\t \]+unsigned int"] + } + + # Check that every required type was present. As we find each + # type we remove it from FILE_TYPES. The REQUIRED_TYPES list is + # in alphabetical order (of the type names), and we expect GDB to + # report the types in this order, this is checked by ensuring that + # when we find the type it is always at index 0 in the FILE_TYPES + # list. + # + # NOTE: The each match being at index 0 only works because the + # optional types are, if present, at the end of the FILE_TYPES + # list. If we ever find a compiler that includes some optional + # types that are mixed in with the required types then we might + # need to re-engineer how this works. + foreach type_pattern $required_types { + set idx [lsearch -regexp $file_types ${type_pattern}] + gdb_assert { $idx == 0 } \ + "found type '${type_pattern}'" + set file_types [lreplace $file_types $idx $idx] + } + + # Now remove each optional type. Obviously we can't require that + # these be present (they're optional), but removing them now + # allows us to perform the next check (see below). + foreach type_pattern $optional_types { + set idx [lsearch -regexp $file_types ${type_pattern}] + if { $idx > -1 } { + set file_types [lreplace $file_types $idx $idx] + } } - gdb_test_lines "info types" "" $output_lines + # With all of the required and optional types removed from + # FILE_TYPES, the FILE_TYPES list should now be empty. + gdb_assert { [llength $file_types] == 0 } } run_test $lang ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-22 21:52 ` Andrew Burgess 2021-06-23 11:35 ` Andrew Burgess @ 2021-06-23 12:04 ` Tom de Vries 2021-06-23 12:21 ` Andrew Burgess 1 sibling, 1 reply; 14+ messages in thread From: Tom de Vries @ 2021-06-23 12:04 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi; +Cc: gdb-patches On 6/22/21 11:52 PM, Andrew Burgess wrote: > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-22 13:24:47 -0400]: > >> On Ubuntu 20.04, when the debug info package for libc is not installed, >> I get: >> >> FAIL: gdb.base/info-types-c++.exp: info types >> FAIL: gdb.base/info-types-c.exp: info types >> >> The reason is that the output of info types is exactly: >> >> (gdb) info types >> All defined types: >> >> File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: >> 52: typedef enum {...} anon_enum_t; >> 45: typedef struct {...} anon_struct_t; >> 68: typedef union {...} anon_union_t; >> 28: typedef struct baz_t baz; >> 31: typedef struct baz_t * baz_ptr; >> 21: struct baz_t; >> double >> 33: enum enum_t; >> float >> int >> 38: typedef enum enum_t my_enum_t; >> 17: typedef float my_float_t; >> 16: typedef int my_int_t; >> 54: typedef enum {...} nested_anon_enum_t; >> 47: typedef struct {...} nested_anon_struct_t; >> 70: typedef union {...} nested_anon_union_t; >> 30: typedef struct baz_t nested_baz; >> 29: typedef struct baz_t nested_baz_t; >> 39: typedef enum enum_t nested_enum_t; >> 19: typedef float nested_float_t; >> 18: typedef int nested_int_t; >> 62: typedef union union_t nested_union_t; >> 56: union union_t; >> unsigned int >> (gdb) >> >> The lines we expect in the test contain an empty line at the end: >> >> ... >> "18:\[\t \]+typedef int nested_int_t;" \ >> "62:\[\t \]+typedef union_t nested_union_t;" \ >> "--optional" "\[\t \]+unsigned int" \ >> ""] >> >> This is written with the supposition that other files will be listed, so >> an empty line will be included to separate the symbols from this file >> from the next one. This empty line is not included when info-types.c is >> the only file listed. >> >> This patch fixes the issue by not requiring an empty line. I don't >> think it's a very good solution, however: the empty line ensures that >> there is no additional unexpected items after "unsigned int". >> >> I don't think that making the empty line optional (with --optional) >> would achieve anything, since the lines would still match if there was >> some unexpected output. >> >> So, I'll think about it a bit more, but any suggestions are appreciated. > > What about the patch below? Instead of using the builtin > gdb_test_lines, I grab the type lines (but just for $srcfile) using > gdb_test_multiple, then manually check for each type in turn. > > We no longer care about the order of the types in the output, but I > think that is fine, the order was always pretty random anyway. > I'm curious, could you elaborate on this? In what situations did you see a different order? [ Not that I much mind not matching in order, it doesn't look terribly important to me. ] Anyway, I have a minor concern about this patch: that it is a local, specific solution. Ideally we specify some matching data that is specific to the test-case, and pass it to a lib/gdb.exp proc, which is generic enough to handle similar cases. I tested the patch with check-read1, and no issues found. Thanks, - Tom > I don't have a ChangeLog or commit message for this, but feel free to > take this patch if you like the approach, or let me know and I can add > the missing bits. > > Thanks, > Andrew > > --- > > diff --git a/gdb/testsuite/gdb.base/info-types.exp.tcl b/gdb/testsuite/gdb.base/info-types.exp.tcl > index c820adc4ac1..729ae23f367 100644 > --- a/gdb/testsuite/gdb.base/info-types.exp.tcl > +++ b/gdb/testsuite/gdb.base/info-types.exp.tcl > @@ -39,13 +39,57 @@ proc run_test { lang } { > } > > set file_re "File .*[string_to_regexp $srcfile]:" > + set file_types {} > + set collect_types False > + set seen_header False > > + # Run 'info types' and collect all of the types associated with > + # SRCFILE into the list FILE_TYPES. > + gdb_test_multiple "info types" "" { > + -re "^info types\r\n" { > + exp_continue > + } > + -re "^\r\n" { > + set collect_types False > + exp_continue > + } > + -re "^All defined types:\r\n" { > + set seen_header True > + exp_continue > + } > + -re "^(File \[^\r\n\]+:)\r\n" { > + set line $expect_out(1,string) > + if {[regexp ${file_re} ${line}]} { > + set collect_types True > + } else { > + set collect_types False > + } > + exp_continue > + } > + -re "^(\\d+:\\s+\[^\r\n\]+)\r\n" { > + if { $collect_types } { > + lappend file_types $expect_out(1,string) > + } > + exp_continue > + } > + -re "^(\\s+\[^\r\n\]+)\r\n" { > + if { $collect_types } { > + lappend file_types $expect_out(1,string) > + } > + exp_continue > + } > + -re "^$::gdb_prompt $" { > + gdb_assert { $seen_header } > + } > + } > + > + # Get the list of all the types we expect to find in SRCFILE, as > + # well as any optional types that might appear in SRCFILE. > + # Optional types will depend on the compiler, these are just the > + # ones we've seen so far. > if { $lang == "c++" } { > - set output_lines \ > + set required_types \ > [list \ > - "All defined types:" \ > - "--any" \ > - $file_re \ > "98:\[\t \]+CL;" \ > "42:\[\t \]+anon_struct_t;" \ > "65:\[\t \]+anon_union_t;" \ > @@ -74,15 +118,14 @@ proc run_test { lang } { > "39:\[\t \]+typedef enum_t nested_enum_t;" \ > "19:\[\t \]+typedef float nested_float_t;" \ > "18:\[\t \]+typedef int nested_int_t;" \ > - "62:\[\t \]+typedef union_t nested_union_t;" \ > - "--optional" "\[\t \]+unsigned int" \ > - ""] > + "62:\[\t \]+typedef union_t nested_union_t;"] > + > + set optional_types \ > + [list \ > + "\[\t \]+unsigned int"] > } else { > - set output_lines \ > + set required_types \ > [list \ > - "All defined types:" \ > - "--any" \ > - $file_re \ > "52:\[\t \]+typedef enum {\\.\\.\\.} anon_enum_t;" \ > "45:\[\t \]+typedef struct {\\.\\.\\.} anon_struct_t;" \ > "68:\[\t \]+typedef union {\\.\\.\\.} anon_union_t;" \ > @@ -105,12 +148,33 @@ proc run_test { lang } { > "19:\[\t \]+typedef float nested_float_t;" \ > "18:\[\t \]+typedef int nested_int_t;" \ > "62:\[\t \]+typedef union union_t nested_union_t;" \ > - "56:\[\t \]+union union_t;" \ > - "--optional" "\[\t \]+unsigned int" \ > - ""] > + "56:\[\t \]+union union_t;"] > + > + set optional_types \ > + [list \ > + "\[\t \]+unsigned int"] > + } > + > + # Check that every required type was present. As we find each > + # type we remove it from FILE_TYPES. > + foreach type_pattern $required_types { > + set idx [lsearch -regexp $file_types ${type_pattern}] > + gdb_assert { $idx != -1 } \ > + "found type '${type_pattern}'" > + set file_types [lreplace $file_types $idx $idx] > + } > + > + # Now remove each optional type. Obviously we can't require that > + # these be present (they're optional), but removing them now > + # allows us to perform the next check (see below). > + foreach type_pattern $optional_types { > + set idx [lsearch -regexp $file_types ${type_pattern}] > + set file_types [lreplace $file_types $idx $idx] > } > > - gdb_test_lines "info types" "" $output_lines > + # With all of the required and optional types removed from > + # FILE_TYPES, the FILE_TYPES list should now be empty. > + gdb_assert { [llength $file_types] == 0 } > } > > run_test $lang > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-23 12:04 ` Tom de Vries @ 2021-06-23 12:21 ` Andrew Burgess 2021-06-23 12:37 ` Tom de Vries 0 siblings, 1 reply; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 12:21 UTC (permalink / raw) To: Tom de Vries; +Cc: Simon Marchi, gdb-patches * Tom de Vries <tdevries@suse.de> [2021-06-23 14:04:54 +0200]: > On 6/22/21 11:52 PM, Andrew Burgess wrote: > > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-22 13:24:47 -0400]: > > > >> On Ubuntu 20.04, when the debug info package for libc is not installed, > >> I get: > >> > >> FAIL: gdb.base/info-types-c++.exp: info types > >> FAIL: gdb.base/info-types-c.exp: info types > >> > >> The reason is that the output of info types is exactly: > >> > >> (gdb) info types > >> All defined types: > >> > >> File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: > >> 52: typedef enum {...} anon_enum_t; > >> 45: typedef struct {...} anon_struct_t; > >> 68: typedef union {...} anon_union_t; > >> 28: typedef struct baz_t baz; > >> 31: typedef struct baz_t * baz_ptr; > >> 21: struct baz_t; > >> double > >> 33: enum enum_t; > >> float > >> int > >> 38: typedef enum enum_t my_enum_t; > >> 17: typedef float my_float_t; > >> 16: typedef int my_int_t; > >> 54: typedef enum {...} nested_anon_enum_t; > >> 47: typedef struct {...} nested_anon_struct_t; > >> 70: typedef union {...} nested_anon_union_t; > >> 30: typedef struct baz_t nested_baz; > >> 29: typedef struct baz_t nested_baz_t; > >> 39: typedef enum enum_t nested_enum_t; > >> 19: typedef float nested_float_t; > >> 18: typedef int nested_int_t; > >> 62: typedef union union_t nested_union_t; > >> 56: union union_t; > >> unsigned int > >> (gdb) > >> > >> The lines we expect in the test contain an empty line at the end: > >> > >> ... > >> "18:\[\t \]+typedef int nested_int_t;" \ > >> "62:\[\t \]+typedef union_t nested_union_t;" \ > >> "--optional" "\[\t \]+unsigned int" \ > >> ""] > >> > >> This is written with the supposition that other files will be listed, so > >> an empty line will be included to separate the symbols from this file > >> from the next one. This empty line is not included when info-types.c is > >> the only file listed. > >> > >> This patch fixes the issue by not requiring an empty line. I don't > >> think it's a very good solution, however: the empty line ensures that > >> there is no additional unexpected items after "unsigned int". > >> > >> I don't think that making the empty line optional (with --optional) > >> would achieve anything, since the lines would still match if there was > >> some unexpected output. > >> > >> So, I'll think about it a bit more, but any suggestions are appreciated. > > > > What about the patch below? Instead of using the builtin > > gdb_test_lines, I grab the type lines (but just for $srcfile) using > > gdb_test_multiple, then manually check for each type in turn. > > > > We no longer care about the order of the types in the output, but I > > think that is fine, the order was always pretty random anyway. > > > > I'm curious, could you elaborate on this? In what situations did you > see a different order? > > [ Not that I much mind not matching in order, it doesn't look terribly > important to me. ] I didn't, see my follow up email. > > Anyway, I have a minor concern about this patch: that it is a local, > specific solution. Ideally we specify some matching data that is > specific to the test-case, and pass it to a lib/gdb.exp proc, which is > generic enough to handle similar cases. I'm happy with your approach. I'd already posted my follow up before your email arrived my end. Thanks, Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-23 12:21 ` Andrew Burgess @ 2021-06-23 12:37 ` Tom de Vries 0 siblings, 0 replies; 14+ messages in thread From: Tom de Vries @ 2021-06-23 12:37 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, gdb-patches On 6/23/21 2:21 PM, Andrew Burgess wrote: > * Tom de Vries <tdevries@suse.de> [2021-06-23 14:04:54 +0200]: > >> On 6/22/21 11:52 PM, Andrew Burgess wrote: >>> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-22 13:24:47 -0400]: >>> >>>> On Ubuntu 20.04, when the debug info package for libc is not installed, >>>> I get: >>>> >>>> FAIL: gdb.base/info-types-c++.exp: info types >>>> FAIL: gdb.base/info-types-c.exp: info types >>>> >>>> The reason is that the output of info types is exactly: >>>> >>>> (gdb) info types >>>> All defined types: >>>> >>>> File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: >>>> 52: typedef enum {...} anon_enum_t; >>>> 45: typedef struct {...} anon_struct_t; >>>> 68: typedef union {...} anon_union_t; >>>> 28: typedef struct baz_t baz; >>>> 31: typedef struct baz_t * baz_ptr; >>>> 21: struct baz_t; >>>> double >>>> 33: enum enum_t; >>>> float >>>> int >>>> 38: typedef enum enum_t my_enum_t; >>>> 17: typedef float my_float_t; >>>> 16: typedef int my_int_t; >>>> 54: typedef enum {...} nested_anon_enum_t; >>>> 47: typedef struct {...} nested_anon_struct_t; >>>> 70: typedef union {...} nested_anon_union_t; >>>> 30: typedef struct baz_t nested_baz; >>>> 29: typedef struct baz_t nested_baz_t; >>>> 39: typedef enum enum_t nested_enum_t; >>>> 19: typedef float nested_float_t; >>>> 18: typedef int nested_int_t; >>>> 62: typedef union union_t nested_union_t; >>>> 56: union union_t; >>>> unsigned int >>>> (gdb) >>>> >>>> The lines we expect in the test contain an empty line at the end: >>>> >>>> ... >>>> "18:\[\t \]+typedef int nested_int_t;" \ >>>> "62:\[\t \]+typedef union_t nested_union_t;" \ >>>> "--optional" "\[\t \]+unsigned int" \ >>>> ""] >>>> >>>> This is written with the supposition that other files will be listed, so >>>> an empty line will be included to separate the symbols from this file >>>> from the next one. This empty line is not included when info-types.c is >>>> the only file listed. >>>> >>>> This patch fixes the issue by not requiring an empty line. I don't >>>> think it's a very good solution, however: the empty line ensures that >>>> there is no additional unexpected items after "unsigned int". >>>> >>>> I don't think that making the empty line optional (with --optional) >>>> would achieve anything, since the lines would still match if there was >>>> some unexpected output. >>>> >>>> So, I'll think about it a bit more, but any suggestions are appreciated. >>> >>> What about the patch below? Instead of using the builtin >>> gdb_test_lines, I grab the type lines (but just for $srcfile) using >>> gdb_test_multiple, then manually check for each type in turn. >>> >>> We no longer care about the order of the types in the output, but I >>> think that is fine, the order was always pretty random anyway. >>> >> >> I'm curious, could you elaborate on this? In what situations did you >> see a different order? >> >> [ Not that I much mind not matching in order, it doesn't look terribly >> important to me. ] > > I didn't, see my follow up email. > Ah, I see. >> >> Anyway, I have a minor concern about this patch: that it is a local, >> specific solution. Ideally we specify some matching data that is >> specific to the test-case, and pass it to a lib/gdb.exp proc, which is >> generic enough to handle similar cases. > > I'm happy with your approach. Ack, thanks for the review. > I'd already posted my follow up before > your email arrived my end. Yeah, I was not CC-ed in your replies, so I didn't notice them in time, sorry. Perhaps you replied-to-list, or the mailing list tried to be clever about dropping CCs to people already on the list (which I find annoying, but apparently that's feature, not a bug). Thanks, - Tom ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-22 17:24 [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info Simon Marchi 2021-06-22 21:52 ` Andrew Burgess @ 2021-06-23 11:33 ` Tom de Vries 2021-06-23 15:36 ` Simon Marchi 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 2 siblings, 1 reply; 14+ messages in thread From: Tom de Vries @ 2021-06-23 11:33 UTC (permalink / raw) To: Simon Marchi, gdb-patches, Andrew Burgess [-- Attachment #1: Type: text/plain, Size: 3158 bytes --] On 6/22/21 7:24 PM, Simon Marchi wrote: > On Ubuntu 20.04, when the debug info package for libc is not installed, > I get: > > FAIL: gdb.base/info-types-c++.exp: info types > FAIL: gdb.base/info-types-c.exp: info types > > The reason is that the output of info types is exactly: > > (gdb) info types > All defined types: > > File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: > 52: typedef enum {...} anon_enum_t; > 45: typedef struct {...} anon_struct_t; > 68: typedef union {...} anon_union_t; > 28: typedef struct baz_t baz; > 31: typedef struct baz_t * baz_ptr; > 21: struct baz_t; > double > 33: enum enum_t; > float > int > 38: typedef enum enum_t my_enum_t; > 17: typedef float my_float_t; > 16: typedef int my_int_t; > 54: typedef enum {...} nested_anon_enum_t; > 47: typedef struct {...} nested_anon_struct_t; > 70: typedef union {...} nested_anon_union_t; > 30: typedef struct baz_t nested_baz; > 29: typedef struct baz_t nested_baz_t; > 39: typedef enum enum_t nested_enum_t; > 19: typedef float nested_float_t; > 18: typedef int nested_int_t; > 62: typedef union union_t nested_union_t; > 56: union union_t; > unsigned int > (gdb) > > The lines we expect in the test contain an empty line at the end: > > ... > "18:\[\t \]+typedef int nested_int_t;" \ > "62:\[\t \]+typedef union_t nested_union_t;" \ > "--optional" "\[\t \]+unsigned int" \ > ""] > > This is written with the supposition that other files will be listed, so > an empty line will be included to separate the symbols from this file > from the next one. This empty line is not included when info-types.c is > the only file listed. > > This patch fixes the issue by not requiring an empty line. I don't > think it's a very good solution, however: the empty line ensures that > there is no additional unexpected items after "unsigned int". > > I don't think that making the empty line optional (with --optional) > would achieve anything, since the lines would still match if there was > some unexpected output. > > So, I'll think about it a bit more, but any suggestions are appreciated. > Thanks for looking into this. [ I suspect this is a regression due to one of my patches to make this test pass on openSUSE and fix check-read1 fails. ] Anyway, the solution I came up with is to change the setup of gdb_test_lines. The current setup is that both reading and matching is done on a line-by-line basis. The new approach is to: - still do the reading on a line-by-line basis (so, no check-read1 problems) - do the matching on a all-lines basis, which greatly simplifies the matching process The new approach allows me to write a tcl regexp: ... "56:\[\t \]+union union_t;(" \ "\[\t \]+unsigned int)?" \ "($|\r\n.*)"] ... that handles the optional new line appropriately, meaning either: - it's not there, and it's the end of the output, or - it's there WDYT? Thanks, - Tom [-- Attachment #2: 0001-gdb-testsuite-Rewrite-gdb_test_lines.patch --] [-- Type: text/x-patch, Size: 8559 bytes --] [gdb/testsuite] Rewrite gdb_test_lines On Ubuntu 20.04, when the debug info package for libc is not installed, I get: FAIL: gdb.base/info-types-c++.exp: info types FAIL: gdb.base/info-types-c.exp: info types The reason is that the output of info types is exactly: (gdb) info types All defined types: File /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/info-types.c: 52: typedef enum {...} anon_enum_t; 45: typedef struct {...} anon_struct_t; 68: typedef union {...} anon_union_t; 28: typedef struct baz_t baz; 31: typedef struct baz_t * baz_ptr; 21: struct baz_t; double 33: enum enum_t; float int 38: typedef enum enum_t my_enum_t; 17: typedef float my_float_t; 16: typedef int my_int_t; 54: typedef enum {...} nested_anon_enum_t; 47: typedef struct {...} nested_anon_struct_t; 70: typedef union {...} nested_anon_union_t; 30: typedef struct baz_t nested_baz; 29: typedef struct baz_t nested_baz_t; 39: typedef enum enum_t nested_enum_t; 19: typedef float nested_float_t; 18: typedef int nested_int_t; 62: typedef union union_t nested_union_t; 56: union union_t; unsigned int (gdb) The lines we expect in the test contain an empty line at the end: ... "62:\[\t \]+typedef union union_t nested_union_t;" \ "56:\[\t \]+union union_t;" \ "--optional" "\[\t \]+unsigned int" \ ""] This is written with the supposition that other files will be listed, so an empty line will be included to separate the symbols from this file from the next one. This empty line is not included when info-types.c is the only file listed. Fix this by rewriting gdb_test_lines to accept a single, plain tcl multiline regexp, such that we can write: ... "62:\[\t \]+typedef union union_t nested_union_t;" \ "56:\[\t \]+union union_t;(" \ "\[\t \]+unsigned int)?" \ "($|\r\n.*)"] ... Tested affected test-cases: - gdb.base/info-types-c.exp - gdb.base/info-types-c++.exp - gdb.base/info-macros.exp - gdb.cp/cplusfuncs.exp on x86_64-linux (openSUSE Leap 15.2), both with check and check-read1. Also tested the first two with gcc-4.8. Also tested on ubuntu 18.04. gdb/testsuite/ChangeLog: 2021-06-23 Tom de Vries <tdevries@suse.de> * lib/gdb.exp (gdb_test_lines): Rewrite to accept single multiline tcl regexp. * gdb.base/info-types.exp.tcl: Update. Make empty line at end of regexp optional. * gdb.base/info-macros.exp: Update. * gdb.cp/cplusfuncs.exp: Update. --- gdb/testsuite/gdb.base/info-macros.exp | 7 ++- gdb/testsuite/gdb.base/info-types.exp.tcl | 22 ++++---- gdb/testsuite/gdb.cp/cplusfuncs.exp | 2 +- gdb/testsuite/lib/gdb.exp | 90 +++++++++---------------------- 4 files changed, 41 insertions(+), 80 deletions(-) diff --git a/gdb/testsuite/gdb.base/info-macros.exp b/gdb/testsuite/gdb.base/info-macros.exp index 3d096a3db51..44b0b45988d 100644 --- a/gdb/testsuite/gdb.base/info-macros.exp +++ b/gdb/testsuite/gdb.base/info-macros.exp @@ -273,6 +273,9 @@ gdb_test_multiple_with_read1_timeout_factor 10 "$test" $testname { set test "info macros info-macros.c:42" -set r1 "#define DEF_MACROS" +set r1 "#define DEF_MACROS " set r2 "#define ONE" -gdb_test_lines "$test" "" [list $r1 "--any" $r2] +gdb_test_lines "$test" "" [multi_line \ + "" \ + "$r1" \ + "(.*\r\n)?$r2"] diff --git a/gdb/testsuite/gdb.base/info-types.exp.tcl b/gdb/testsuite/gdb.base/info-types.exp.tcl index c820adc4ac1..20b54dad299 100644 --- a/gdb/testsuite/gdb.base/info-types.exp.tcl +++ b/gdb/testsuite/gdb.base/info-types.exp.tcl @@ -43,8 +43,8 @@ proc run_test { lang } { if { $lang == "c++" } { set output_lines \ [list \ - "All defined types:" \ - "--any" \ + "^All defined types:" \ + ".*" \ $file_re \ "98:\[\t \]+CL;" \ "42:\[\t \]+anon_struct_t;" \ @@ -74,14 +74,14 @@ proc run_test { lang } { "39:\[\t \]+typedef enum_t nested_enum_t;" \ "19:\[\t \]+typedef float nested_float_t;" \ "18:\[\t \]+typedef int nested_int_t;" \ - "62:\[\t \]+typedef union_t nested_union_t;" \ - "--optional" "\[\t \]+unsigned int" \ - ""] + "62:\[\t \]+typedef union_t nested_union_t;(" \ + "\[\t \]+unsigned int)?" \ + "($|\r\n.*)"] } else { set output_lines \ [list \ - "All defined types:" \ - "--any" \ + "^All defined types:" \ + ".*" \ $file_re \ "52:\[\t \]+typedef enum {\\.\\.\\.} anon_enum_t;" \ "45:\[\t \]+typedef struct {\\.\\.\\.} anon_struct_t;" \ @@ -105,12 +105,12 @@ proc run_test { lang } { "19:\[\t \]+typedef float nested_float_t;" \ "18:\[\t \]+typedef int nested_int_t;" \ "62:\[\t \]+typedef union union_t nested_union_t;" \ - "56:\[\t \]+union union_t;" \ - "--optional" "\[\t \]+unsigned int" \ - ""] + "56:\[\t \]+union union_t;(" \ + "\[\t \]+unsigned int)?" \ + "($|\r\n.*)"] } - gdb_test_lines "info types" "" $output_lines + gdb_test_lines "info types" "" [multi_line {*}$output_lines] } run_test $lang diff --git a/gdb/testsuite/gdb.cp/cplusfuncs.exp b/gdb/testsuite/gdb.cp/cplusfuncs.exp index 19be8abc2ac..73740155305 100644 --- a/gdb/testsuite/gdb.cp/cplusfuncs.exp +++ b/gdb/testsuite/gdb.cp/cplusfuncs.exp @@ -294,7 +294,7 @@ proc info_func_regexp { name demangled } { set file_re "File .*[string_to_regexp $srcfile]:" gdb_test_lines "info function $name" "info function for \"$name\"" \ - [list \ + [multi_line \ "$file_re" \ "$decimal:\t(class|)${demangled}.*"] } diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 4bb2da31c1f..ee6e932f77d 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -1432,86 +1432,44 @@ proc gdb_test_sequence { args } { } \f -# Match LINE against regexp OUTPUT_LINES[IDX]. Helper function for -# gdb_test_lines. -proc match_line { line output_lines idx_name } { - upvar $idx_name idx - - while { 1 } { - if { $idx == [llength $output_lines] } { - # Ran out of regexps, bail out. - return -1 - } - - set re [lindex $output_lines $idx] - set opt 0 - set any 0 - if { $re == "--optional" } { - # Optional, get actual regexp. - set opt 1 - incr idx - set re [lindex $output_lines $idx] - } elseif { $re == "--any" } { - set any 1 - incr idx - set re [lindex $output_lines $idx] - } - - if { [regexp $re $line] } { - # Match. - incr idx - if { $idx == [llength $output_lines] } { - # Last match, we're done. - return 1 - } - # Match found, keep looking for next match. - return 0 - } else { - # No match. - if { $idx == 0 } { - # First match not found, just keep looking for first match. - return 0 - } elseif { $opt } { - # Try next regexp on same line. - incr idx - continue - } elseif { $any } { - # Try again with next line. - incr idx -1 - return 0 - } else { - # Mismatch, bail out. - return -1 - } - } - break - } - - # Keep going. - return 0 -} - -# Match output of COMMAND line-by-line, using PATTERNS. +# Match output of COMMAND using re. Read output line-by-line. # Report pass/fail with MESSAGE. - -proc gdb_test_lines { command message patterns } { +# For a command foo with output +# (gdb) foo^M +# <line1>^M +# <line2>^M +# (gdb) +# the portion matched using re is: +# <line1>^M +# <line2>^M + +proc gdb_test_lines { command message re } { set found 0 set idx 0 if { $message == ""} { set message $command } + set lines "" gdb_test_multiple $command $message { -re "\r\n(\[^\r\n\]*)(?=\r\n)" { - if { $found == 0 } { - set line $expect_out(1,string) - set found [match_line $line $patterns idx] + set line $expect_out(1,string) + if { $lines eq "" } { + append lines "$line" + } else { + append lines "\r\n$line" } exp_continue } -re -wrap "" { - gdb_assert { $found == 1 } $gdb_test_name + append lines "\r\n" } } + + if { [regexp $re $lines] } { + pass $message + } else { + fail $message + } } # Test that a command gives an error. For pass or fail, return ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-23 11:33 ` Tom de Vries @ 2021-06-23 15:36 ` Simon Marchi 2021-06-23 21:51 ` Tom de Vries 0 siblings, 1 reply; 14+ messages in thread From: Simon Marchi @ 2021-06-23 15:36 UTC (permalink / raw) To: Tom de Vries, gdb-patches, Andrew Burgess > -# Match output of COMMAND line-by-line, using PATTERNS. > +# Match output of COMMAND using re. Read output line-by-line. re -> RE > # Report pass/fail with MESSAGE. > - > -proc gdb_test_lines { command message patterns } { > +# For a command foo with output > +# (gdb) foo^M > +# <line1>^M > +# <line2>^M > +# (gdb) > +# the portion matched using re is: > +# <line1>^M > +# <line2>^M To summarize, the intent is that it works pretty much like gdb_test, but it consumes the output as it is produced to avoid timing out? > + if { [regexp $re $lines] } { > + pass $message > + } else { > + fail $message > + } That could probably use gdb_assert: gdb_assert { [regexp $re $lines] } $message But otherwise, LGTM. Simon ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info 2021-06-23 15:36 ` Simon Marchi @ 2021-06-23 21:51 ` Tom de Vries 0 siblings, 0 replies; 14+ messages in thread From: Tom de Vries @ 2021-06-23 21:51 UTC (permalink / raw) To: Simon Marchi, gdb-patches, Andrew Burgess On 6/23/21 5:36 PM, Simon Marchi wrote: >> -# Match output of COMMAND line-by-line, using PATTERNS. >> +# Match output of COMMAND using re. Read output line-by-line. > > re -> RE > >> # Report pass/fail with MESSAGE. >> - >> -proc gdb_test_lines { command message patterns } { >> +# For a command foo with output >> +# (gdb) foo^M >> +# <line1>^M >> +# <line2>^M >> +# (gdb) >> +# the portion matched using re is: >> +# <line1>^M >> +# <line2>^M > > To summarize, the intent is that it works pretty much like gdb_test, but > it consumes the output as it is produced to avoid timing out? > Indeed, the only difference is in scope of match compared to gdb_test: - matching starts at the line after the prompt, so it skips the command - there's no implicit .* before the gdb prompt, so we can use anchor '$' in a meaningful way. Committed. Thanks, - Tom. >> + if { [regexp $re $lines] } { >> + pass $message >> + } else { >> + fail $message >> + } > > That could probably use gdb_assert: > > gdb_assert { [regexp $re $lines] } $message > > But otherwise, LGTM. > > Simon > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases 2021-06-22 17:24 [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info Simon Marchi 2021-06-22 21:52 ` Andrew Burgess 2021-06-23 11:33 ` Tom de Vries @ 2021-06-23 11:39 ` Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 1/3] " Andrew Burgess ` (3 more replies) 2 siblings, 4 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 11:39 UTC (permalink / raw) To: gdb-patches Changes since v1: - Updated the commit message for patch #1 after Simon's query, hopefully this should make it clearer what I'm talking about. - Passing a NULL terminated array around sucks now we have array_view, patch #2 changes the API to use array_view, which exposed another bug in this area (which is fixed in this patch). - After this discussion in the bug (gdb/27994), patch #3 updates the API even further, we now make use of gdb::optional instead of passing a pointer to an array_view. --- Andrew Burgess (3): gdb: fix regression in evaluate_funcall for non C++ like cases gdb: replace NULL terminated array with array_view gdb: use gdb::optional instead of passing a pointer to gdb::array_view gdb/ChangeLog | 51 ++++++++++++++++++++ gdb/ada-lang.c | 6 +-- gdb/eval.c | 19 ++++++-- gdb/f-lang.c | 2 +- gdb/guile/scm-value.c | 2 +- gdb/m2-lang.c | 4 +- gdb/opencl-lang.c | 2 +- gdb/python/py-value.c | 2 +- gdb/rust-lang.c | 18 +++---- gdb/testsuite/ChangeLog | 14 ++++++ gdb/testsuite/gdb.cp/method-call-in-c.cc | 52 +++++++++++++++++++++ gdb/testsuite/gdb.cp/method-call-in-c.exp | 48 +++++++++++++++++++ gdb/valarith.c | 2 +- gdb/valops.c | 57 ++++++++++++----------- gdb/value.h | 2 +- 15 files changed, 229 insertions(+), 52 deletions(-) create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp -- 2.25.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv2 1/3] gdb: fix regression in evaluate_funcall for non C++ like cases 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess @ 2021-06-23 11:39 ` Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 2/3] gdb: replace NULL terminated array with array_view Andrew Burgess ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 11:39 UTC (permalink / raw) To: gdb-patches This regression, as it is exposed by the test added in this commit, first became noticable with this commit: commit d182f2797922a305fbd1ef6a483cc39a56b43e02 Date: Mon Mar 8 07:27:57 2021 -0700 Convert c-exp.y to use operations But, this commit only added converted the C expression parser to make use of code that was added in this commit: commit a00b7254fb614af557de7ae7cc0eb39a0ce0e408 Date: Mon Mar 8 07:27:57 2021 -0700 Implement function call operations And it was this second commit that actually introduced the bugs (there are two). In structop_base_operation::evaluate_funcall we build up an argument list in the vector vals. Later in this function the argument list might be passed to value_struct_elt. Prior to commit a00b7254fb614 the vals vector (or argvec as it used to be called) stored the value for the function callee in the argvec at index 0. This 'callee' value is what ends up being passed to evaluate_subexp_do_call, and represents the function to be called, the value contents are the address of the function, and the value type is the function signature. The remaining items held in the argvec were the values to pass to the function. For a non-static member function the `this' pointer would be at index 1 in the array. After commit a00b7254fb614 this callee value is now held in a separate variable, not the vals array. So, for non-static member functions, the `this' pointer is now at index 0, with any other arguments after that. What this means is that previous, when we called value_struct_elt we would pass the address of argvec[1] as this was the first argument. But now we should be passing the address of vals[0]. Unfortunately, we are still passing vals[1], effectively skipping the first argument. The second issue is that, prior to commit a00b7254fb614, the argvec array was NULL terminated. This is required as value_struct_elt calls search_struct_method, which calls typecmp, and typecmp requires that the array have a NULL at the end. After commit a00b7254fb614 this NULL has been lost, and we are therefore violating the API requirements of typecmp. This commit fixes both of these regressions. I also extended the header comments on search_struct_method and value_struct_elt to make it clearer that the array required a NULL marker at the end. gdb/ChangeLog: PR gdb/27994 * eval.c (structop_base_operation::evaluate_funcall): Add a nullptr to the end of the args array, which should not be included in the argument array_view. Pass all the arguments through to value_struct_elt. * valops.c (search_struct_method): Update header comment. (value_struct_elt): Likewise. gdb/testsuite/ChangeLog: PR gdb/27994 * gdb.cp/method-call-in-c.cc: New file. * gdb.cp/method-call-in-c.exp: New file. --- gdb/ChangeLog | 10 ++++++ gdb/eval.c | 15 ++++++-- gdb/testsuite/ChangeLog | 6 ++++ gdb/testsuite/gdb.cp/method-call-in-c.cc | 44 +++++++++++++++++++++++ gdb/testsuite/gdb.cp/method-call-in-c.exp | 44 +++++++++++++++++++++++ gdb/valops.c | 7 +++- 6 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp diff --git a/gdb/eval.c b/gdb/eval.c index 659493c8237..ab070a3d9f6 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -872,7 +872,9 @@ structop_base_operation::evaluate_funcall (struct type *expect_type, struct expression *exp, enum noside noside, const std::vector<operation_up> &args) { - std::vector<value *> vals (args.size () + 1); + /* Allocate space for the function call arguments. Include space for a + `this' pointer at the start, and a trailing nullptr. */ + std::vector<value *> vals (args.size () + 2); /* First, evaluate the structure into vals[0]. */ enum exp_opcode op = opcode (); if (op == STRUCTOP_STRUCT) @@ -918,9 +920,16 @@ structop_base_operation::evaluate_funcall } } + /* Evaluate the arguments, and add the trailing nullptr. The '+ 1' here + is to allow for the `this' pointer we placed into vals[0]. */ for (int i = 0; i < args.size (); ++i) vals[i + 1] = args[i]->evaluate_with_coercion (exp, noside); - gdb::array_view<value *> arg_view = vals; + vals[args.size () + 1] = nullptr; + + /* The array view includes the `this' pointer, but not the trailing + nullptr. */ + gdb::array_view<value *> arg_view + = gdb::make_array_view (&vals[0], args.size () + 1); int static_memfuncp; value *callee; @@ -941,7 +950,7 @@ structop_base_operation::evaluate_funcall { struct value *temp = vals[0]; - callee = value_struct_elt (&temp, &vals[1], tstr, + callee = value_struct_elt (&temp, &vals[0], tstr, &static_memfuncp, op == STRUCTOP_STRUCT ? "structure" : "structure pointer"); diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc new file mode 100644 index 00000000000..09e4285ed5d --- /dev/null +++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc @@ -0,0 +1,44 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2021 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +struct baz_type +{ + int a = 0; + int b = 1; + int c = 2; +}; + +struct foo_type +{ + int func (baz_type b, float f) + { + return var++; + } + + int var = 123; +}; + +int +main (void) +{ + baz_type b = {}; + float f = 1.0; + + foo_type foo; + + return foo.func (b, f); /* Break here. */ +} diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.exp b/gdb/testsuite/gdb.cp/method-call-in-c.exp new file mode 100644 index 00000000000..eb48979e465 --- /dev/null +++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp @@ -0,0 +1,44 @@ +# Copyright 2021 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Ensure that calling a member function works correctly even when the +# language is forced to 'C' (this should be fine, so long at no +# overload resolution is required), or when overload-resolution is +# off. + +standard_testfile .cc + +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { + return -1 +} + +if ![runto_main] then { + return 0 +} + +gdb_breakpoint [gdb_get_line_number "Break here"] +gdb_continue_to_breakpoint "Break here" + +set result 123 +foreach_with_prefix lang { c++ c } { + foreach_with_prefix overload_resolution { on off } { + gdb_test_no_output "set overload-resolution ${overload_resolution}" + gdb_test "set language ${lang}" ".*" + + gdb_test "print foo.func (b, f)" " = ${result}" \ + "call foo.func with language auto;c++, overload-resolution on" + incr result + } +} diff --git a/gdb/valops.c b/gdb/valops.c index 8694c124b52..2b579304204 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -2181,6 +2181,10 @@ search_struct_field (const char *name, struct value *arg1, ARG1 by OFFSET bytes, and search in it assuming it has (class) type TYPE. + The ARGS array is a list of argument values used to help finding NAME, + though ARGS can be nullptr. If ARGS is not nullptr then the list itself + must have a NULL at the end. + If found, return value, else if name matched and args not return (value) -1, else return NULL. */ @@ -2309,7 +2313,8 @@ search_struct_method (const char *name, struct value **arg1p, ERR is used in the error message if *ARGP's type is wrong. C++: ARGS is a list of argument types to aid in the selection of - an appropriate method. Also, handle derived types. + an appropriate method. Also, handle derived types. The array ARGS must + have a NULL at the end. STATIC_MEMFUNCP, if non-NULL, points to a caller-supplied location where the truthvalue of whether the function that was resolved was -- 2.25.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv2 2/3] gdb: replace NULL terminated array with array_view 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 1/3] " Andrew Burgess @ 2021-06-23 11:39 ` Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 3/3] gdb: use gdb::optional instead of passing a pointer to gdb::array_view Andrew Burgess 2021-06-23 12:22 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 3 siblings, 0 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 11:39 UTC (permalink / raw) To: gdb-patches After the previous commit, this commit updates the value_struct_elt function to take an array_view rather than a NULL terminated array of values. The requirement for a NULL terminated array of values actually stems from typecmp, so the change from an array to array_view needs to be propagated through to this function. While making this change I noticed that this fixes another bug, in value_x_binop and value_x_unop GDB creates an array of values which doesn't have a NULL at the end. An array_view of this array is passed to value_user_defined_op, which then unpacks the array_view and passed the raw array to value_struct_elt, but only if the language is not C++. As value_x_binop and value_x_unop can only request member functions with the names of C++ operators, then most of the time, assuming the inferior is not a C++ program, then GDB will not find a matching member function with the call to value_struct_elt, and so typecmp will never be called, and so, GDB will avoid undefined behaviour. However, it is worth remembering that, when GDB's language is set to "auto", the current language is selected based on the language of the current compilation unit. As C++ programs usually link against libc, which is written in C, then, if the inferior is stopped in libc GDB will set the language to C. And so, it is possible that we will end up using value_struct_elt to try and lookup, and match, a C++ operator. If this occurs then GDB will experience undefined behaviour. I have extended the test added in the previous commit to also cover this case. Finally, this commit changes the API from passing around a pointer to an array to passing around a pointer to an array_view. The reason for this is that we need to be able to distinguish between the cases where we call value_struct_elt with no arguments, i.e. we are looking up a struct member, but we either don't have the arguments we want to pass yet, or we don't expect there to be any need for GDB to use the argument types to resolve any overloading; and the second case where we call value_struct_elt looking for a function that takes no arguments, that is, the argument list is empty. NOTE: While writing this I realise that if we pass an array_view at all then it will always have at least one item in it, the `this' pointer for the object we are planning to call the method on. So we could, I guess, pass an empty array_view to indicate the case where we don't know anything about the arguments, and when the array_view is length 1 or more, it means we do have the arguments. However, though we could do this, I don't think this would be better, the length 0 vs length 1 difference seems a little too subtle, I think that there's a better solution... I think a better solution would be to wrap the array_view in a gdb::optional, this would make the whole, do we have an array view or not question explicit. I haven't done this as part of this commit as making that change is much more extensive, every user of value_struct_elt will need to be updated, and as this commit already contains a bug fix, I wanted to keep the large refactoring in a separate commit, so, check out the next commit for the use of gdb::optional. gdb/ChangeLog: PR gdb/27994 * eval.c (structop_base_operation::evaluate_funcall): Pass array_view instead of array to value_struct_elt. * valarith.c (value_user_defined_op): Likewise. * valops.c (typecmp): Change parameter type from array pointer to array_view. Update header comment, and update body accordingly. (search_struct_method): Likewise. (value_struct_elt): Likewise. * value.h (value_struct_elt): Update declaration. gdb/testsuite/ChangeLog: PR gdb/27994 * gdb.cp/method-call-in-c.cc (struct foo_type): Add operator+=, change initial value of var member variable. (main): Make use of foo_type's operator+=. * gdb.cp/method-call-in-c.exp: Test use of operator+=. --- gdb/ChangeLog | 12 +++++ gdb/eval.c | 2 +- gdb/testsuite/ChangeLog | 8 ++++ gdb/testsuite/gdb.cp/method-call-in-c.cc | 10 +++- gdb/testsuite/gdb.cp/method-call-in-c.exp | 4 ++ gdb/valarith.c | 2 +- gdb/valops.c | 58 +++++++++++------------ gdb/value.h | 2 +- 8 files changed, 63 insertions(+), 35 deletions(-) diff --git a/gdb/eval.c b/gdb/eval.c index ab070a3d9f6..6bc4132288b 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -950,7 +950,7 @@ structop_base_operation::evaluate_funcall { struct value *temp = vals[0]; - callee = value_struct_elt (&temp, &vals[0], tstr, + callee = value_struct_elt (&temp, &arg_view, tstr, &static_memfuncp, op == STRUCTOP_STRUCT ? "structure" : "structure pointer"); diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc index 09e4285ed5d..95f3f3c22de 100644 --- a/gdb/testsuite/gdb.cp/method-call-in-c.cc +++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc @@ -29,7 +29,13 @@ struct foo_type return var++; } - int var = 123; + foo_type &operator+= (const baz_type &rhs) + { + var += (rhs.a + rhs.b + rhs.c); + return *this; + } + + int var = 120; }; int @@ -40,5 +46,7 @@ main (void) foo_type foo; + foo += b; + return foo.func (b, f); /* Break here. */ } diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.exp b/gdb/testsuite/gdb.cp/method-call-in-c.exp index eb48979e465..066e3648382 100644 --- a/gdb/testsuite/gdb.cp/method-call-in-c.exp +++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp @@ -40,5 +40,9 @@ foreach_with_prefix lang { c++ c } { gdb_test "print foo.func (b, f)" " = ${result}" \ "call foo.func with language auto;c++, overload-resolution on" incr result + + set result [expr $result + 3] + gdb_test "print foo += b" \ + " = \\((?:struct )?foo_type &\\) @${hex}: \\\{var = ${result}\\\}" } } diff --git a/gdb/valarith.c b/gdb/valarith.c index 299a99f4703..d61ad9170f8 100644 --- a/gdb/valarith.c +++ b/gdb/valarith.c @@ -344,7 +344,7 @@ value_user_defined_op (struct value **argp, gdb::array_view<value *> args, noside); } else - result = value_struct_elt (argp, args.data (), name, static_memfuncp, + result = value_struct_elt (argp, &args, name, static_memfuncp, "structure"); return result; diff --git a/gdb/valops.c b/gdb/valops.c index 2b579304204..0af7a6c3f27 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -44,14 +44,14 @@ /* Local functions. */ -static int typecmp (int staticp, int varargs, int nargs, - struct field t1[], struct value *t2[]); +static int typecmp (bool staticp, bool varargs, int nargs, + struct field t1[], const gdb::array_view<value *> t2); static struct value *search_struct_field (const char *, struct value *, struct type *, int); static struct value *search_struct_method (const char *, struct value **, - struct value **, + gdb::array_view<value *> *, LONGEST, int *, struct type *); static int find_oload_champ_namespace (gdb::array_view<value *> args, @@ -1785,15 +1785,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type) } \f -/* See if we can pass arguments in T2 to a function which takes - arguments of types T1. T1 is a list of NARGS arguments, and T2 is - a NULL-terminated vector. If some arguments need coercion of some - sort, then the coerced values are written into T2. Return value is +/* See if we can pass arguments in T2 to a function which takes arguments + of types T1. T1 is a list of NARGS arguments, and T2 is an array_view + of the values we're trying to pass. If some arguments need coercion of + some sort, then the coerced values are written into T2. Return value is 0 if the arguments could be matched, or the position at which they differ if not. STATICP is nonzero if the T1 argument list came from a static - member function. T2 will still include the ``this'' pointer, but + member function. T2 must still include the ``this'' pointer, but it will be skipped. For non-static member functions, we ignore the first argument, @@ -1803,19 +1803,15 @@ value_string (const char *ptr, ssize_t len, struct type *char_type) requested operation is type secure, shouldn't we? FIXME. */ static int -typecmp (int staticp, int varargs, int nargs, - struct field t1[], struct value *t2[]) +typecmp (bool staticp, bool varargs, int nargs, + struct field t1[], gdb::array_view<value *> t2) { int i; - if (t2 == 0) - internal_error (__FILE__, __LINE__, - _("typecmp: no argument list")); - /* Skip ``this'' argument if applicable. T2 will always include THIS. */ if (staticp) - t2 ++; + t2 = t2.slice (1); for (i = 0; (i < nargs) && t1[i].type ()->code () != TYPE_CODE_VOID; @@ -1823,7 +1819,7 @@ typecmp (int staticp, int varargs, int nargs, { struct type *tt1, *tt2; - if (!t2[i]) + if (i == t2.size ()) return i + 1; tt1 = check_typedef (t1[i].type ()); @@ -1868,7 +1864,7 @@ typecmp (int staticp, int varargs, int nargs, if (t1[i].type ()->code () != value_type (t2[i])->code ()) return i + 1; } - if (varargs || t2[i] == NULL) + if (varargs || i == t2.size ()) return 0; return i + 1; } @@ -2181,16 +2177,16 @@ search_struct_field (const char *name, struct value *arg1, ARG1 by OFFSET bytes, and search in it assuming it has (class) type TYPE. - The ARGS array is a list of argument values used to help finding NAME, - though ARGS can be nullptr. If ARGS is not nullptr then the list itself - must have a NULL at the end. + The ARGS array pointer is to a list of argument values used to help + finding NAME, though ARGS can be nullptr. The contents of ARGS can be + adjusted if type coercion is required in order to find a matching NAME. If found, return value, else if name matched and args not return (value) -1, else return NULL. */ static struct value * search_struct_method (const char *name, struct value **arg1p, - struct value **args, LONGEST offset, + gdb::array_view<value *> *args, LONGEST offset, int *static_memfuncp, struct type *type) { int i; @@ -2209,10 +2205,10 @@ search_struct_method (const char *name, struct value **arg1p, name_matched = 1; check_stub_method_group (type, i); - if (j > 0 && args == 0) + if (j > 0 && args == nullptr) error (_("cannot resolve overloaded method " "`%s': no arguments supplied"), name); - else if (j == 0 && args == 0) + else if (j == 0 && args == nullptr) { v = value_fn_field (arg1p, f, j, type, offset); if (v != NULL) @@ -2221,10 +2217,11 @@ search_struct_method (const char *name, struct value **arg1p, else while (j >= 0) { + gdb_assert (args != nullptr); if (!typecmp (TYPE_FN_FIELD_STATIC_P (f, j), TYPE_FN_FIELD_TYPE (f, j)->has_varargs (), TYPE_FN_FIELD_TYPE (f, j)->num_fields (), - TYPE_FN_FIELD_ARGS (f, j), args)) + TYPE_FN_FIELD_ARGS (f, j), *args)) { if (TYPE_FN_FIELD_VIRTUAL_P (f, j)) return value_virtual_fn_field (arg1p, f, j, @@ -2313,8 +2310,7 @@ search_struct_method (const char *name, struct value **arg1p, ERR is used in the error message if *ARGP's type is wrong. C++: ARGS is a list of argument types to aid in the selection of - an appropriate method. Also, handle derived types. The array ARGS must - have a NULL at the end. + an appropriate method. Also, handle derived types. STATIC_MEMFUNCP, if non-NULL, points to a caller-supplied location where the truthvalue of whether the function that was resolved was @@ -2324,7 +2320,7 @@ search_struct_method (const char *name, struct value **arg1p, found. */ struct value * -value_struct_elt (struct value **argp, struct value **args, +value_struct_elt (struct value **argp, gdb::array_view<value *> *args, const char *name, int *static_memfuncp, const char *err) { struct type *t; @@ -2354,7 +2350,7 @@ value_struct_elt (struct value **argp, struct value **args, if (static_memfuncp) *static_memfuncp = 0; - if (!args) + if (args == nullptr) { /* if there are no arguments ...do this... */ @@ -2366,7 +2362,7 @@ value_struct_elt (struct value **argp, struct value **args, /* C++: If it was not found as a data field, then try to return it as a pointer to a method. */ - v = search_struct_method (name, argp, args, 0, + v = search_struct_method (name, argp, args, 0, static_memfuncp, t); if (v == (struct value *) - 1) @@ -2381,9 +2377,9 @@ value_struct_elt (struct value **argp, struct value **args, return v; } - v = search_struct_method (name, argp, args, 0, + v = search_struct_method (name, argp, args, 0, static_memfuncp, t); - + if (v == (struct value *) - 1) { error (_("One of the arguments you tried to pass to %s could not " diff --git a/gdb/value.h b/gdb/value.h index a691f3cf3ff..40ad28e3dbb 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -826,7 +826,7 @@ extern struct value *value_neg (struct value *arg1); extern struct value *value_complement (struct value *arg1); extern struct value *value_struct_elt (struct value **argp, - struct value **args, + gdb::array_view <value *> *args, const char *name, int *static_memfuncp, const char *err); -- 2.25.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv2 3/3] gdb: use gdb::optional instead of passing a pointer to gdb::array_view 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 1/3] " Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 2/3] gdb: replace NULL terminated array with array_view Andrew Burgess @ 2021-06-23 11:39 ` Andrew Burgess 2021-06-23 12:22 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 3 siblings, 0 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 11:39 UTC (permalink / raw) To: gdb-patches Following on from the previous commit, this commit changes the API of value_struct_elt to take gdb::optional<gdb::array_view<value *>> instead of a pointer to the gdb::array_view. This makes the optional nature of the array_view parameter explicit. This commit is purely a refactoring commit, there should be no user visible change after this commit. I have deliberately kept this refactor separate from the previous two commits as this is a more extensive change, and I'm not 100% sure that using gdb::optional for the parameter type, instead of a pointer, is going to be to everyone's taste. If there's push back on this patch then this one can be dropped from the series. gdb/ChangeLog: * ada-lang.c (desc_bounds): Use '{}' instead of NULL to indicate an empty gdb::optional when calling value_struct_elt. (desc_data): Likewise. (desc_one_bound): Likewise. * eval.c (structop_base_operation::evaluate_funcall): Pass gdb::array_view, not a gdb::array_view* to value_struct_elt. (eval_op_structop_struct): Use '{}' instead of NULL to indicate an empty gdb::optional when calling value_struct_elt. (eval_op_structop_ptr): Likewise. * f-lang.c (fortran_structop_operation::evaluate): Likewise. * guile/scm-value.c (gdbscm_value_field): Likewise. * m2-lang.c (eval_op_m2_high): Likewise. (eval_op_m2_subscript): Likewise. * opencl-lang.c (opencl_structop_operation::evaluate): Likewise. * python/py-value.c (valpy_getitem): Likewise. * rust-lang.c (rust_val_print_str): Likewise. (rust_range): Likewise. (rust_subscript): Likewise. (eval_op_rust_structop): Likewise. (rust_aggregate_operation::evaluate): Likewise. * valarith.c (value_user_defined_op): Likewise. * valops.c (search_struct_method): Change parameter type, update function body accordingly, and update header comment. (value_struct_elt): Change parameter type, update function body accordingly. * value.h (value_struct_elt): Update declaration. --- gdb/ChangeLog | 29 +++++++++++++++++++++++++++++ gdb/ada-lang.c | 6 +++--- gdb/eval.c | 6 +++--- gdb/f-lang.c | 2 +- gdb/guile/scm-value.c | 2 +- gdb/m2-lang.c | 4 ++-- gdb/opencl-lang.c | 2 +- gdb/python/py-value.c | 2 +- gdb/rust-lang.c | 18 +++++++++--------- gdb/valarith.c | 2 +- gdb/valops.c | 24 +++++++++++++----------- gdb/value.h | 2 +- 12 files changed, 65 insertions(+), 34 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index 6ed6b65e705..dd9e1354617 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -1484,7 +1484,7 @@ desc_bounds (struct value *arr) else if (is_thick_pntr (type)) { - struct value *p_bounds = value_struct_elt (&arr, NULL, "P_BOUNDS", NULL, + struct value *p_bounds = value_struct_elt (&arr, {}, "P_BOUNDS", NULL, _("Bad GNAT array descriptor")); struct type *p_bounds_type = value_type (p_bounds); @@ -1566,7 +1566,7 @@ desc_data (struct value *arr) if (is_thin_pntr (type)) return thin_data_pntr (arr); else if (is_thick_pntr (type)) - return value_struct_elt (&arr, NULL, "P_ARRAY", NULL, + return value_struct_elt (&arr, {}, "P_ARRAY", NULL, _("Bad GNAT array descriptor")); else return NULL; @@ -1606,7 +1606,7 @@ desc_one_bound (struct value *bounds, int i, int which) char bound_name[20]; xsnprintf (bound_name, sizeof (bound_name), "%cB%d", which ? 'U' : 'L', i - 1); - return value_struct_elt (&bounds, NULL, bound_name, NULL, + return value_struct_elt (&bounds, {}, bound_name, NULL, _("Bad GNAT array descriptor bounds")); } diff --git a/gdb/eval.c b/gdb/eval.c index 6bc4132288b..ca13e4cd188 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -950,7 +950,7 @@ structop_base_operation::evaluate_funcall { struct value *temp = vals[0]; - callee = value_struct_elt (&temp, &arg_view, tstr, + callee = value_struct_elt (&temp, arg_view, tstr, &static_memfuncp, op == STRUCTOP_STRUCT ? "structure" : "structure pointer"); @@ -1135,7 +1135,7 @@ eval_op_structop_struct (struct type *expect_type, struct expression *exp, enum noside noside, struct value *arg1, const char *string) { - struct value *arg3 = value_struct_elt (&arg1, NULL, string, + struct value *arg3 = value_struct_elt (&arg1, {}, string, NULL, "structure"); if (noside == EVAL_AVOID_SIDE_EFFECTS) arg3 = value_zero (value_type (arg3), VALUE_LVAL (arg3)); @@ -1191,7 +1191,7 @@ eval_op_structop_ptr (struct type *expect_type, struct expression *exp, } } - struct value *arg3 = value_struct_elt (&arg1, NULL, string, + struct value *arg3 = value_struct_elt (&arg1, {}, string, NULL, "structure pointer"); if (noside == EVAL_AVOID_SIDE_EFFECTS) arg3 = value_zero (value_type (arg3), VALUE_LVAL (arg3)); diff --git a/gdb/f-lang.c b/gdb/f-lang.c index 5731c400305..16ec9e04044 100644 --- a/gdb/f-lang.c +++ b/gdb/f-lang.c @@ -1511,7 +1511,7 @@ fortran_structop_operation::evaluate (struct type *expect_type, arg1 = std::get<0> (m_storage)->evaluate (nullptr, exp, EVAL_NORMAL); } - value *elt = value_struct_elt (&arg1, NULL, str, NULL, "structure"); + value *elt = value_struct_elt (&arg1, {}, str, NULL, "structure"); if (noside == EVAL_AVOID_SIDE_EFFECTS) { diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c index 24bb5547957..d4d76df0411 100644 --- a/gdb/guile/scm-value.c +++ b/gdb/guile/scm-value.c @@ -694,7 +694,7 @@ gdbscm_value_field (SCM self, SCM field_scm) struct value *tmp = v_smob->value; - struct value *res_val = value_struct_elt (&tmp, NULL, field.get (), NULL, + struct value *res_val = value_struct_elt (&tmp, {}, field.get (), NULL, "struct/class/union"); return vlscm_scm_from_value (res_val); diff --git a/gdb/m2-lang.c b/gdb/m2-lang.c index be1a8ed1437..911d67d8672 100644 --- a/gdb/m2-lang.c +++ b/gdb/m2-lang.c @@ -50,7 +50,7 @@ eval_op_m2_high (struct type *expect_type, struct expression *exp, type = type->field (1).type (); /* i18n: Do not translate the "_m2_high" part! */ - arg1 = value_struct_elt (&temp, NULL, "_m2_high", NULL, + arg1 = value_struct_elt (&temp, {}, "_m2_high", NULL, _("unbounded structure " "missing _m2_high field")); @@ -83,7 +83,7 @@ eval_op_m2_subscript (struct type *expect_type, struct expression *exp, error (_("internal error: unbounded " "array structure is unknown")); /* i18n: Do not translate the "_m2_contents" part! */ - arg1 = value_struct_elt (&temp, NULL, "_m2_contents", NULL, + arg1 = value_struct_elt (&temp, {}, "_m2_contents", NULL, _("unbounded structure " "missing _m2_contents field")); diff --git a/gdb/opencl-lang.c b/gdb/opencl-lang.c index 1d6117a9285..136969ead76 100644 --- a/gdb/opencl-lang.c +++ b/gdb/opencl-lang.c @@ -708,7 +708,7 @@ opencl_structop_operation::evaluate (struct type *expect_type, noside); else { - struct value *v = value_struct_elt (&arg1, NULL, + struct value *v = value_struct_elt (&arg1, {}, std::get<1> (m_storage).c_str (), NULL, "structure"); diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index 43da8f0fbc4..8df8a15f8d6 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -992,7 +992,7 @@ valpy_getitem (PyObject *self, PyObject *key) scoped_value_mark free_values; if (field) - res_val = value_struct_elt (&tmp, NULL, field.get (), NULL, + res_val = value_struct_elt (&tmp, {}, field.get (), NULL, "struct/class/union"); else if (bitpos >= 0) res_val = value_struct_elt_bitpos (&tmp, bitpos, field_type, diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c index 3b15bb22a27..60ea89b1394 100644 --- a/gdb/rust-lang.c +++ b/gdb/rust-lang.c @@ -302,9 +302,9 @@ static void rust_val_print_str (struct ui_file *stream, struct value *val, const struct value_print_options *options) { - struct value *base = value_struct_elt (&val, NULL, "data_ptr", NULL, + struct value *base = value_struct_elt (&val, {}, "data_ptr", NULL, "slice"); - struct value *len = value_struct_elt (&val, NULL, "length", NULL, "slice"); + struct value *len = value_struct_elt (&val, {}, "length", NULL, "slice"); val_print_string (TYPE_TARGET_TYPE (value_type (base)), "UTF-8", value_as_address (base), value_as_long (len), stream, @@ -1030,7 +1030,7 @@ rust_range (struct type *expect_type, struct expression *exp, if (low != NULL) { - struct value *start = value_struct_elt (&result, NULL, "start", NULL, + struct value *start = value_struct_elt (&result, {}, "start", NULL, "range"); value_assign (start, low); @@ -1038,7 +1038,7 @@ rust_range (struct type *expect_type, struct expression *exp, if (high != NULL) { - struct value *end = value_struct_elt (&result, NULL, "end", NULL, + struct value *end = value_struct_elt (&result, {}, "end", NULL, "range"); value_assign (end, high); @@ -1176,8 +1176,8 @@ rust_subscript (struct type *expect_type, struct expression *exp, { struct value *len; - base = value_struct_elt (&lhs, NULL, "data_ptr", NULL, "slice"); - len = value_struct_elt (&lhs, NULL, "length", NULL, "slice"); + base = value_struct_elt (&lhs, {}, "data_ptr", NULL, "slice"); + len = value_struct_elt (&lhs, {}, "length", NULL, "slice"); low_bound = 0; high_bound = value_as_long (len); } @@ -1400,7 +1400,7 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp, try { - result = value_struct_elt (&lhs, NULL, field_name, + result = value_struct_elt (&lhs, {}, field_name, NULL, "structure"); } catch (const gdb_exception_error &except) @@ -1411,7 +1411,7 @@ eval_op_rust_structop (struct type *expect_type, struct expression *exp, } } else - result = value_struct_elt (&lhs, NULL, field_name, NULL, "structure"); + result = value_struct_elt (&lhs, {}, field_name, NULL, "structure"); if (noside == EVAL_AVOID_SIDE_EFFECTS) result = value_zero (value_type (result), VALUE_LVAL (result)); return result; @@ -1457,7 +1457,7 @@ rust_aggregate_operation::evaluate (struct type *expect_type, if (noside == EVAL_NORMAL) { const char *fieldname = item.first.c_str (); - value *field = value_struct_elt (&result, nullptr, fieldname, + value *field = value_struct_elt (&result, {}, fieldname, nullptr, "structure"); value_assign (field, val); } diff --git a/gdb/valarith.c b/gdb/valarith.c index d61ad9170f8..9ebad648b27 100644 --- a/gdb/valarith.c +++ b/gdb/valarith.c @@ -344,7 +344,7 @@ value_user_defined_op (struct value **argp, gdb::array_view<value *> args, noside); } else - result = value_struct_elt (argp, &args, name, static_memfuncp, + result = value_struct_elt (argp, args, name, static_memfuncp, "structure"); return result; diff --git a/gdb/valops.c b/gdb/valops.c index 0af7a6c3f27..a1c2bbf23fa 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -51,7 +51,7 @@ static struct value *search_struct_field (const char *, struct value *, struct type *, int); static struct value *search_struct_method (const char *, struct value **, - gdb::array_view<value *> *, + gdb::optional<gdb::array_view<value *>>, LONGEST, int *, struct type *); static int find_oload_champ_namespace (gdb::array_view<value *> args, @@ -2177,17 +2177,18 @@ search_struct_field (const char *name, struct value *arg1, ARG1 by OFFSET bytes, and search in it assuming it has (class) type TYPE. - The ARGS array pointer is to a list of argument values used to help - finding NAME, though ARGS can be nullptr. The contents of ARGS can be - adjusted if type coercion is required in order to find a matching NAME. + The ARGS array is to an optional list of argument values used to help + finding NAME. The contents of ARGS can be adjusted if type coercion is + required in order to find a matching NAME. If found, return value, else if name matched and args not return (value) -1, else return NULL. */ static struct value * search_struct_method (const char *name, struct value **arg1p, - gdb::array_view<value *> *args, LONGEST offset, - int *static_memfuncp, struct type *type) + gdb::optional<gdb::array_view<value *>> args, + LONGEST offset, int *static_memfuncp, + struct type *type) { int i; struct value *v; @@ -2205,10 +2206,10 @@ search_struct_method (const char *name, struct value **arg1p, name_matched = 1; check_stub_method_group (type, i); - if (j > 0 && args == nullptr) + if (j > 0 && !args.has_value ()) error (_("cannot resolve overloaded method " "`%s': no arguments supplied"), name); - else if (j == 0 && args == nullptr) + else if (j == 0 && !args.has_value ()) { v = value_fn_field (arg1p, f, j, type, offset); if (v != NULL) @@ -2217,7 +2218,7 @@ search_struct_method (const char *name, struct value **arg1p, else while (j >= 0) { - gdb_assert (args != nullptr); + gdb_assert (args.has_value ()); if (!typecmp (TYPE_FN_FIELD_STATIC_P (f, j), TYPE_FN_FIELD_TYPE (f, j)->has_varargs (), TYPE_FN_FIELD_TYPE (f, j)->num_fields (), @@ -2320,7 +2321,8 @@ search_struct_method (const char *name, struct value **arg1p, found. */ struct value * -value_struct_elt (struct value **argp, gdb::array_view<value *> *args, +value_struct_elt (struct value **argp, + gdb::optional<gdb::array_view<value *>> args, const char *name, int *static_memfuncp, const char *err) { struct type *t; @@ -2350,7 +2352,7 @@ value_struct_elt (struct value **argp, gdb::array_view<value *> *args, if (static_memfuncp) *static_memfuncp = 0; - if (args == nullptr) + if (!args.has_value ()) { /* if there are no arguments ...do this... */ diff --git a/gdb/value.h b/gdb/value.h index 40ad28e3dbb..379cddafbe7 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -826,7 +826,7 @@ extern struct value *value_neg (struct value *arg1); extern struct value *value_complement (struct value *arg1); extern struct value *value_struct_elt (struct value **argp, - gdb::array_view <value *> *args, + gdb::optional<gdb::array_view <value *>> args, const char *name, int *static_memfuncp, const char *err); -- 2.25.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess ` (2 preceding siblings ...) 2021-06-23 11:39 ` [PATCHv2 3/3] gdb: use gdb::optional instead of passing a pointer to gdb::array_view Andrew Burgess @ 2021-06-23 12:22 ` Andrew Burgess 3 siblings, 0 replies; 14+ messages in thread From: Andrew Burgess @ 2021-06-23 12:22 UTC (permalink / raw) To: gdb-patches Doh! I posted these in-reply-to the wrong email. Please just ignore these, and I'll resend to the correct thread. Sorry for the noise, Andrew * Andrew Burgess <andrew.burgess@embecosm.com> [2021-06-23 12:39:12 +0100]: > Changes since v1: > > - Updated the commit message for patch #1 after Simon's query, > hopefully this should make it clearer what I'm talking about. > > - Passing a NULL terminated array around sucks now we have > array_view, patch #2 changes the API to use array_view, which > exposed another bug in this area (which is fixed in this patch). > > - After this discussion in the bug (gdb/27994), patch #3 updates the > API even further, we now make use of gdb::optional instead of > passing a pointer to an array_view. > > --- > > Andrew Burgess (3): > gdb: fix regression in evaluate_funcall for non C++ like cases > gdb: replace NULL terminated array with array_view > gdb: use gdb::optional instead of passing a pointer to gdb::array_view > > gdb/ChangeLog | 51 ++++++++++++++++++++ > gdb/ada-lang.c | 6 +-- > gdb/eval.c | 19 ++++++-- > gdb/f-lang.c | 2 +- > gdb/guile/scm-value.c | 2 +- > gdb/m2-lang.c | 4 +- > gdb/opencl-lang.c | 2 +- > gdb/python/py-value.c | 2 +- > gdb/rust-lang.c | 18 +++---- > gdb/testsuite/ChangeLog | 14 ++++++ > gdb/testsuite/gdb.cp/method-call-in-c.cc | 52 +++++++++++++++++++++ > gdb/testsuite/gdb.cp/method-call-in-c.exp | 48 +++++++++++++++++++ > gdb/valarith.c | 2 +- > gdb/valops.c | 57 ++++++++++++----------- > gdb/value.h | 2 +- > 15 files changed, 229 insertions(+), 52 deletions(-) > create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.cc > create mode 100644 gdb/testsuite/gdb.cp/method-call-in-c.exp > > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-23 21:51 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-22 17:24 [RFC PATCH] gdb/testsuite: fix gdb.base/info-types*.exp when no libc debug info Simon Marchi 2021-06-22 21:52 ` Andrew Burgess 2021-06-23 11:35 ` Andrew Burgess 2021-06-23 12:04 ` Tom de Vries 2021-06-23 12:21 ` Andrew Burgess 2021-06-23 12:37 ` Tom de Vries 2021-06-23 11:33 ` Tom de Vries 2021-06-23 15:36 ` Simon Marchi 2021-06-23 21:51 ` Tom de Vries 2021-06-23 11:39 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 1/3] " Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 2/3] gdb: replace NULL terminated array with array_view Andrew Burgess 2021-06-23 11:39 ` [PATCHv2 3/3] gdb: use gdb::optional instead of passing a pointer to gdb::array_view Andrew Burgess 2021-06-23 12:22 ` [PATCHv2 0/3] gdb: fix regression in evaluate_funcall for non C++ like cases Andrew Burgess
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).