public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with
@ 2023-01-17 13:00 Bruno Larsen
  2023-01-17 13:00 ` [PATCH 1/2] gdb/testsuite: add test with regex for multiple completion patterns Bruno Larsen
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Bruno Larsen @ 2023-01-17 13:00 UTC (permalink / raw)
  To: gdb-patches; +Cc: Bruno Larsen

When running the test mentioned above with clang, I ran into the failure
explained in patch 2. To completely fix the testcase, however, I needed
to add a way to use regexes when expecting multiple complete
suggestions. That is what patch 1 does.

Bruno Larsen (2):
  gdb/testsuite: add test with regex for multiple completion patterns
  gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with
    clang

 .../gdb.linespec/cp-completion-aliases.exp    | 14 ++--
 gdb/testsuite/lib/completion-support.exp      | 73 +++++++++++++++----
 2 files changed, 67 insertions(+), 20 deletions(-)

-- 
2.39.0


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

* [PATCH 1/2] gdb/testsuite: add test with regex for multiple completion patterns
  2023-01-17 13:00 [PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen
@ 2023-01-17 13:00 ` Bruno Larsen
  2023-01-17 13:00 ` [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang Bruno Larsen
  2023-02-03  8:04 ` [PING][PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen
  2 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2023-01-17 13:00 UTC (permalink / raw)
  To: gdb-patches; +Cc: Bruno Larsen

Currently there is no way to test tab completion on GDB using regexes if
you expect multiple suggestions. This commit adds a proc for that,
test_gdb_complete_multiple_re, which does just that.

To achieve that, test_gdb_complete_cmd_multiple_re and
test_gdb_complete_tab_multiple_re are introduced, following a similar
logic to the unique tests, which already have an _re version.
---
 gdb/testsuite/lib/completion-support.exp | 73 +++++++++++++++++++-----
 1 file changed, 60 insertions(+), 13 deletions(-)

diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index bf9c5ad352c..babe802e9a1 100644
--- a/gdb/testsuite/lib/completion-support.exp
+++ b/gdb/testsuite/lib/completion-support.exp
@@ -47,7 +47,7 @@ proc make_tab_completion_list_re { completion_list } {
 
     set completion_list_re ""
     foreach c $completion_list {
-	append completion_list_re [string_to_regexp $c]
+	append completion_list_re $c
 	append completion_list_re $ws
     }
     append completion_list_re $ws
@@ -58,13 +58,18 @@ proc make_tab_completion_list_re { completion_list } {
 # Make a regular expression that matches a "complete" command
 # completion list.  CMD_PREFIX is the command prefix added to each
 # completion match.
+# COMPLETION_LIST is expected to already be regexp.  This is done so
+# we can have regex options in the completion list, to account for
+# differences in compiler results.
 
 proc make_cmd_completion_list_re { cmd_prefix completion_list start_quote_char end_quote_char } {
 
     set completion_list_re ""
     foreach c $completion_list {
 	# The command prefix is included in all completion matches.
-	append completion_list_re [string_to_regexp $cmd_prefix$start_quote_char$c$end_quote_char]
+	append completion_list_re [string_to_regexp $cmd_prefix$start_quote_char]
+	append completion_list_re $c
+	append completion_list_re [string_to_regexp $end_quote_char]
 	append completion_list_re "\r\n"
     }
 
@@ -124,18 +129,18 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
 }
 
 # Test that completing INPUT_LINE with TAB completes to "INPUT_LINE +
-# ADD_COMPLETED_LINE" and that it displays the completion matches in
+# ADD_COMPLETED_LINE_RE" and that it displays the completion matches in
 # COMPLETION_LIST.  If MAX_COMPLETIONS then we expect the completion
 # to hit the max-completions limit.
+# ADD_COMPLETED_LINE_RE and EXPECTED_RE must be regular expressions,
+# while INPUT_LINE is does is not.
 
-proc test_gdb_complete_tab_multiple { input_line add_completed_line \
-					  completion_list {max_completions 0}} {
+proc test_gdb_complete_tab_multiple_re { input_line add_completed_line_re \
+					  completion_list_re {max_completions 0}} {
     global gdb_prompt
 
     set input_line_re [string_to_regexp $input_line]
-    set add_completed_line_re [string_to_regexp $add_completed_line]
-
-    set expected_re [make_tab_completion_list_re $completion_list]
+    set expected_re [make_tab_completion_list_re $completion_list_re]
 
     if {$max_completions} {
 	append expected_re "\r\n"
@@ -150,14 +155,14 @@ proc test_gdb_complete_tab_multiple { input_line add_completed_line \
 	    send_gdb "\t"
 	    # If we auto-completed to an ambiguous prefix, we need an
 	    # extra tab to show the matches list.
-	    if {$add_completed_line != ""} {
+	    if {$add_completed_line_re != ""} {
 		send_gdb "\t"
 		set maybe_bell ${completion::bell_re}
 	    } else {
 		set maybe_bell ""
 	    }
 	    gdb_test_multiple "" "$test (second tab)" {
-		-re "^${maybe_bell}\r\n$expected_re\r\n$gdb_prompt " {
+		-re "^$maybe_bell\r\n$expected_re\r\n$gdb_prompt " {
 		    gdb_test_multiple "" "$test (second tab)" {
 			-re "^$input_line_re$add_completed_line_re$" {
 			    pass "$test"
@@ -171,6 +176,21 @@ proc test_gdb_complete_tab_multiple { input_line add_completed_line \
     clear_input_line $test
 }
 
+# Simplified call test completeing an INPUT_LINE using TAB.  This just
+# turns the given COMPLETION_LIST into regexps and calls the proc
+# test_gdb_complete_tab_multiple_re
+
+proc test_gdb_complete_tab_multiple { input_line add_completed_line \
+					  completion_list {max_completions 0}} {
+    set add_completed_line_re [string_to_regexp $add_completed_line]
+    set completion_list_re ""
+    foreach c $completion_list {
+	lappend completion_list_re [string_to_regexp $c]
+    }
+
+    test_gdb_complete_tab_multiple_re $input_line $add_completed_line_re \
+	$completion_list_re $max_completions
+}
 # Test that completing LINE with the complete command completes to
 # nothing.
 
@@ -195,15 +215,16 @@ proc test_gdb_complete_cmd_unique { input_line complete_line_re } {
 }
 
 # Test that completing "CMD_PREFIX + COMPLETION_WORD" with the
-# complete command displays the COMPLETION_LIST completion list.  Each
+# complete command displays the COMPLETION_LIST_RE completion list.  Each
 # entry in the list should be prefixed by CMD_PREFIX.  If
 # MAX_COMPLETIONS then we expect the completion to hit the
 # max-completions limit.
+# COMPLETION_LIST_RE must already be valid regexes.
 
-proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list {start_quote_char ""} {end_quote_char ""} {max_completions 0}} {
+proc test_gdb_complete_cmd_multiple_re { cmd_prefix completion_word completion_list_re {start_quote_char ""} {end_quote_char ""} {max_completions 0}} {
     global gdb_prompt
 
-    set expected_re [make_cmd_completion_list_re $cmd_prefix $completion_list $start_quote_char $end_quote_char]
+    set expected_re [make_cmd_completion_list_re $cmd_prefix $completion_list_re $start_quote_char $end_quote_char]
 
     if {$max_completions} {
 	set cmd_prefix_re [string_to_regexp $cmd_prefix]
@@ -220,6 +241,19 @@ proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list
     }
 }
 
+# Simplified call to test_gdb_complete_cmd_multiple_re, allowing for
+# passing completions that are not regular expressions.
+
+proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list {start_quote_char ""} {end_quote_char ""} {max_completions 0}} {
+    set completion_list_re ""
+    foreach c $completion_list {
+	lappend completion_list_re [string_to_regexp $c]
+    }
+
+    test_gdb_complete_cmd_multiple_re $cmd_prefix $completion_word \
+	$completion_list_re $start_quote_char $end_quote_char $max_completions
+}
+
 # Test that completing LINE completes to nothing.
 
 proc test_gdb_complete_none { input_line } {
@@ -300,6 +334,19 @@ proc test_gdb_complete_multiple {
     test_gdb_complete_cmd_multiple $cmd_prefix $completion_word $completion_list $start_quote_char $end_quote_char $max_completions
 }
 
+# Similar to test_gdb_complete_multiple, but requires regular expressions
+# in COMPLETION_LIST_RE.
+
+proc test_gdb_complete_multiple_re {
+  cmd_prefix completion_word add_completed_line completion_list_re
+  {start_quote_char ""} {end_quote_char ""} {max_completions 0}
+} {
+    if { [readline_is_used] } {
+      test_gdb_complete_tab_multiple_re "$cmd_prefix$completion_word" $add_completed_line $completion_list_re $max_completions
+    }
+    test_gdb_complete_cmd_multiple_re $cmd_prefix $completion_word $completion_list_re $start_quote_char $end_quote_char $max_completions
+}
+
 # Test that all the substring prefixes of INPUT from [0..START) to
 # [0..END) complete to COMPLETION_RE (a regular expression).  If END
 # is ommitted, default to the length of INPUT.
-- 
2.39.0


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

* [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang
  2023-01-17 13:00 [PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen
  2023-01-17 13:00 ` [PATCH 1/2] gdb/testsuite: add test with regex for multiple completion patterns Bruno Larsen
@ 2023-01-17 13:00 ` Bruno Larsen
  2023-02-03 13:44   ` Andrew Burgess
  2023-02-03  8:04 ` [PING][PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen
  2 siblings, 1 reply; 8+ messages in thread
From: Bruno Larsen @ 2023-01-17 13:00 UTC (permalink / raw)
  To: gdb-patches; +Cc: Bruno Larsen

Currently, all important tests in gdb.linespec/cp-completion-aliases.exp
are failing when using clang because the wrong type is being suggested
for the completion.  For example, running with gcc and clang we get the
following output respectively:

(gdb) break get_value(object_p) <- gcc version
(gdb) break get_value(object*)  <- clang version

Since both suggestions are acceptable on their own, and the original bug
was if GDB suggested both at the same time, this commit updates the test
to accept both, gcc's and clang's outputs.
---
 .../gdb.linespec/cp-completion-aliases.exp         | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
index 33ad72e6f05..01ec61f4591 100644
--- a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
@@ -27,15 +27,15 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
 # Disable the completion limit for the whole testcase.
 gdb_test_no_output "set max-completions unlimited"
 
-test_gdb_complete_unique \
+test_gdb_complete_unique_re \
     "break get_v" \
-    "break get_value(object_p)"
+    "break get_value\\(object(_p|\\\*)\\)"
 
-test_gdb_complete_unique \
+test_gdb_complete_unique_re \
     "break gr" \
-    "break grab_it(int_magic_t*)"
+    "break grab_it\\((int_magic_t|magic<int>)\\\*\\)"
 
-test_gdb_complete_multiple "break " "get_som" "ething(" {
-    "get_something(my_string_t)"
-    "get_something(object_p)"
+test_gdb_complete_multiple_re "break " "get_som" "ething\\\(" {
+    "get_something\\((my_string_t|char\ const\\*)\\)"
+    "get_something\\(object(_p|\\*)\\)"
 }
-- 
2.39.0


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

* [PING][PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with
  2023-01-17 13:00 [PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen
  2023-01-17 13:00 ` [PATCH 1/2] gdb/testsuite: add test with regex for multiple completion patterns Bruno Larsen
  2023-01-17 13:00 ` [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang Bruno Larsen
@ 2023-02-03  8:04 ` Bruno Larsen
  2 siblings, 0 replies; 8+ messages in thread
From: Bruno Larsen @ 2023-02-03  8:04 UTC (permalink / raw)
  To: Bruno Larsen, Bruno Larsen via Gdb-patches

On 17/01/2023 14:00, Bruno Larsen wrote:
> When running the test mentioned above with clang, I ran into the failure
> explained in patch 2. To completely fix the testcase, however, I needed
> to add a way to use regexes when expecting multiple complete
> suggestions. That is what patch 1 does.
>
> Bruno Larsen (2):
>    gdb/testsuite: add test with regex for multiple completion patterns
>    gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with
>      clang
>
>   .../gdb.linespec/cp-completion-aliases.exp    | 14 ++--
>   gdb/testsuite/lib/completion-support.exp      | 73 +++++++++++++++----
>   2 files changed, 67 insertions(+), 20 deletions(-)
>
Ping!

-- 
Cheers,
Bruno


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

* Re: [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang
  2023-01-17 13:00 ` [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang Bruno Larsen
@ 2023-02-03 13:44   ` Andrew Burgess
  2023-02-03 15:49     ` Andrew Burgess
  2023-02-06 14:10     ` Bruno Larsen
  0 siblings, 2 replies; 8+ messages in thread
From: Andrew Burgess @ 2023-02-03 13:44 UTC (permalink / raw)
  To: Bruno Larsen via Gdb-patches, gdb-patches; +Cc: Bruno Larsen

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> Currently, all important tests in gdb.linespec/cp-completion-aliases.exp
> are failing when using clang because the wrong type is being suggested
> for the completion.  For example, running with gcc and clang we get the
> following output respectively:
>
> (gdb) break get_value(object_p) <- gcc version
> (gdb) break get_value(object*)  <- clang version

You are correct that what we print for Clang is not wrong.  But I don't
think it's as correct as what we print for GCC.  The user wrote
object_p, and the DWARF does encode the object_p for both versions.
It's just in the Clang case there's an extra DW_AT_linkage_name which
seems to send GDB off doing the "wrong" thing.

I wonder how hard it would actually be to "fix" this so that we print
object_p for Clang?  I think it would be helpful to really understand at
which point we diverge when comparing Clang and GCC binaries.

>
> Since both suggestions are acceptable on their own, and the original bug
> was if GDB suggested both at the same time, this commit updates the test
> to accept both, gcc's and clang's outputs.

On the content of this patch though...

I think we can achieve the same results without adding anything from
patch #1 in this series.  My idea would be to just ask GDB what it's
going to print using 'info functions', then we can expect exactly the
right strings.  The patch below implements this idea.

What do you think?

Thanks,
Andrew

---

diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
index 33ad72e6f05..342359ea6e1 100644
--- a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
@@ -24,18 +24,67 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
     return -1
 }
 
+# Clang adds a DW_AT_linkage_name to the DW_TAG_subprogram entry,
+# which results in GDB showing the underlying type names for the
+# arguments, rather than the typedef names.
+#
+# In contrast, GCC doesn't include the DW_AT_linkage_name, and so GDB
+# shows the type name aliases instead.
+#
+# To figure out which we expect to see in the tab-completion output,
+# use 'info functions' output.  Look for the functions we care about,
+# and build a dictionary mapping line number to the argument type.
+set types_dict [dict create]
+foreach_with_prefix name {get_something grab_it get_value} {
+    gdb_test_multiple "info function $name" "" {
+	-re "^($::decimal):\\s+static int $name\\((\[^)\]+)\\);\r\n" {
+	    set linum $expect_out(1,string)
+	    set type $expect_out(2,string)
+	    dict set types_dict $linum $type
+	    exp_continue
+	}
+
+	-re "^\[^0-9\]\[^\r\n\]+\r\n" {
+	    exp_continue
+	}
+
+	-re "^\\s*\r\n" {
+	    exp_continue
+	}
+
+	-re "^$::gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
+# Now look in the dictionary we built above to figure out how GDB will
+# display the types for different arguments.
+set linum [gdb_get_line_number "get_value (object_p obj)"]
+set object_pattern [dict get $types_dict $linum]
+
+set linum [gdb_get_line_number "grab_it (int_magic_t *var)"]
+set magic_pattern [dict get $types_dict $linum]
+
+set linum [gdb_get_line_number "get_something (my_string_t msg)"]
+set string_pattern [dict get $types_dict $linum]
+
+# Restart GDB, just in case the function lookup above in someway
+# impacted what tab-completion results we might return.
+clean_restart $binfile
+
 # Disable the completion limit for the whole testcase.
 gdb_test_no_output "set max-completions unlimited"
 
 test_gdb_complete_unique \
     "break get_v" \
-    "break get_value(object_p)"
+    "break get_value($object_pattern)"
 
 test_gdb_complete_unique \
     "break gr" \
-    "break grab_it(int_magic_t*)"
+    "break grab_it($magic_pattern)"
 
-test_gdb_complete_multiple "break " "get_som" "ething(" {
-    "get_something(my_string_t)"
-    "get_something(object_p)"
-}
+test_gdb_complete_multiple "break " "get_som" "ething(" [list \
+    "get_something($string_pattern)" \
+    "get_something($object_pattern)" \
+]


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

* Re: [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang
  2023-02-03 13:44   ` Andrew Burgess
@ 2023-02-03 15:49     ` Andrew Burgess
  2023-02-06 14:10     ` Bruno Larsen
  1 sibling, 0 replies; 8+ messages in thread
From: Andrew Burgess @ 2023-02-03 15:49 UTC (permalink / raw)
  To: Bruno Larsen via Gdb-patches, gdb-patches; +Cc: Bruno Larsen

Andrew Burgess <aburgess@redhat.com> writes:

> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> Currently, all important tests in gdb.linespec/cp-completion-aliases.exp
>> are failing when using clang because the wrong type is being suggested
>> for the completion.  For example, running with gcc and clang we get the
>> following output respectively:
>>
>> (gdb) break get_value(object_p) <- gcc version
>> (gdb) break get_value(object*)  <- clang version
>
> You are correct that what we print for Clang is not wrong.  But I don't
> think it's as correct as what we print for GCC.  The user wrote
> object_p, and the DWARF does encode the object_p for both versions.
> It's just in the Clang case there's an extra DW_AT_linkage_name which
> seems to send GDB off doing the "wrong" thing.
>
> I wonder how hard it would actually be to "fix" this so that we print
> object_p for Clang?  I think it would be helpful to really understand at
> which point we diverge when comparing Clang and GCC binaries.

The diff below fixes this issue for the specific test that you were
editing.  I've not tested this at all beyond that one test.  But I
wonder if something like this makes sense?

What's happening is that when we read the DWARF we end up calling
dwarf2_physname to figure out the name of the symbol.  If the DIE has a
linkage name then we just demangle that, and use that as the symbol
name.

But, at least for C++ I don't think that makes sense.  We know that the
demangled name will have stripped any typedefs.  While the computed name
will better reflect what's actually written in the code.

We already have something similar in place for Rust (commit
906bb4c58faa).

Anyway, just and idea...

Thanks,
Andrew


---

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ee4e7c1530a..00c46197e3c 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9133,7 +9133,7 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu)
   if (!die_needs_namespace (die, cu))
     return dwarf2_compute_name (name, die, cu, 1);
 
-  if (cu->lang () != language_rust)
+  if (cu->lang () != language_rust && cu->lang () != language_cplus)
     mangled = dw2_linkage_name (die, cu);
 
   /* DW_AT_linkage_name is missing in some cases - depend on what GDB


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

* Re: [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang
  2023-02-03 13:44   ` Andrew Burgess
  2023-02-03 15:49     ` Andrew Burgess
@ 2023-02-06 14:10     ` Bruno Larsen
  2023-02-06 15:48       ` Andrew Burgess
  1 sibling, 1 reply; 8+ messages in thread
From: Bruno Larsen @ 2023-02-06 14:10 UTC (permalink / raw)
  To: Andrew Burgess, Bruno Larsen via Gdb-patches

On 03/02/2023 14:44, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> Currently, all important tests in gdb.linespec/cp-completion-aliases.exp
>> are failing when using clang because the wrong type is being suggested
>> for the completion.  For example, running with gcc and clang we get the
>> following output respectively:
>>
>> (gdb) break get_value(object_p) <- gcc version
>> (gdb) break get_value(object*)  <- clang version
> You are correct that what we print for Clang is not wrong.  But I don't
> think it's as correct as what we print for GCC.  The user wrote
> object_p, and the DWARF does encode the object_p for both versions.
> It's just in the Clang case there's an extra DW_AT_linkage_name which
> seems to send GDB off doing the "wrong" thing.
>
> I wonder how hard it would actually be to "fix" this so that we print
> object_p for Clang?  I think it would be helpful to really understand at
> which point we diverge when comparing Clang and GCC binaries.
I took a look at the next patch you sent, but it introduced over 200 
regressions, I will take a look at solving this later.
>
>> Since both suggestions are acceptable on their own, and the original bug
>> was if GDB suggested both at the same time, this commit updates the test
>> to accept both, gcc's and clang's outputs.
> On the content of this patch though...
>
> I think we can achieve the same results without adding anything from
> patch #1 in this series.  My idea would be to just ask GDB what it's
> going to print using 'info functions', then we can expect exactly the
> right strings.  The patch below implements this idea.
>
> What do you think?

This idea sounds good. Would you like this changed even if the clang 
output is fixed? I can send a patch for it before looking further into 
how to change GDB's output.

Also, do you think the first patch should be abandoned? I feel like it 
would be a good addition even if we eventually drop my change, but I can 
drop it if you disagree.

>
> Thanks,
> Andrew
>
> ---
>
> diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
> index 33ad72e6f05..342359ea6e1 100644
> --- a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
> +++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
> @@ -24,18 +24,67 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
>       return -1
>   }
>   
> +# Clang adds a DW_AT_linkage_name to the DW_TAG_subprogram entry,
> +# which results in GDB showing the underlying type names for the
> +# arguments, rather than the typedef names.
> +#
> +# In contrast, GCC doesn't include the DW_AT_linkage_name, and so GDB
> +# shows the type name aliases instead.
> +#
> +# To figure out which we expect to see in the tab-completion output,
> +# use 'info functions' output.  Look for the functions we care about,
> +# and build a dictionary mapping line number to the argument type.
> +set types_dict [dict create]
> +foreach_with_prefix name {get_something grab_it get_value} {
> +    gdb_test_multiple "info function $name" "" {
> +	-re "^($::decimal):\\s+static int $name\\((\[^)\]+)\\);\r\n" {
> +	    set linum $expect_out(1,string)
> +	    set type $expect_out(2,string)
> +	    dict set types_dict $linum $type
> +	    exp_continue
> +	}
> +
> +	-re "^\[^0-9\]\[^\r\n\]+\r\n" {
> +	    exp_continue
> +	}
> +
> +	-re "^\\s*\r\n" {
> +	    exp_continue
> +	}
> +
> +	-re "^$::gdb_prompt $" {
> +	    pass $gdb_test_name
> +	}
> +    }
> +}
> +
> +# Now look in the dictionary we built above to figure out how GDB will
> +# display the types for different arguments.
> +set linum [gdb_get_line_number "get_value (object_p obj)"]
> +set object_pattern [dict get $types_dict $linum]
> +
> +set linum [gdb_get_line_number "grab_it (int_magic_t *var)"]
> +set magic_pattern [dict get $types_dict $linum]
> +
> +set linum [gdb_get_line_number "get_something (my_string_t msg)"]
> +set string_pattern [dict get $types_dict $linum]
> +
> +# Restart GDB, just in case the function lookup above in someway
> +# impacted what tab-completion results we might return.
> +clean_restart $binfile
> +
>   # Disable the completion limit for the whole testcase.
>   gdb_test_no_output "set max-completions unlimited"
>   
>   test_gdb_complete_unique \
>       "break get_v" \
> -    "break get_value(object_p)"
> +    "break get_value($object_pattern)"
>   
>   test_gdb_complete_unique \
>       "break gr" \
> -    "break grab_it(int_magic_t*)"
> +    "break grab_it($magic_pattern)"
>   
> -test_gdb_complete_multiple "break " "get_som" "ething(" {
> -    "get_something(my_string_t)"
> -    "get_something(object_p)"
> -}
> +test_gdb_complete_multiple "break " "get_som" "ething(" [list \
> +    "get_something($string_pattern)" \
> +    "get_something($object_pattern)" \
> +]
>

-- 
Cheers,
Bruno


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

* Re: [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang
  2023-02-06 14:10     ` Bruno Larsen
@ 2023-02-06 15:48       ` Andrew Burgess
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Burgess @ 2023-02-06 15:48 UTC (permalink / raw)
  To: Bruno Larsen, Bruno Larsen via Gdb-patches

Bruno Larsen <blarsen@redhat.com> writes:

> On 03/02/2023 14:44, Andrew Burgess wrote:
>> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>>> Currently, all important tests in gdb.linespec/cp-completion-aliases.exp
>>> are failing when using clang because the wrong type is being suggested
>>> for the completion.  For example, running with gcc and clang we get the
>>> following output respectively:
>>>
>>> (gdb) break get_value(object_p) <- gcc version
>>> (gdb) break get_value(object*)  <- clang version
>> You are correct that what we print for Clang is not wrong.  But I don't
>> think it's as correct as what we print for GCC.  The user wrote
>> object_p, and the DWARF does encode the object_p for both versions.
>> It's just in the Clang case there's an extra DW_AT_linkage_name which
>> seems to send GDB off doing the "wrong" thing.
>>
>> I wonder how hard it would actually be to "fix" this so that we print
>> object_p for Clang?  I think it would be helpful to really understand at
>> which point we diverge when comparing Clang and GCC binaries.
> I took a look at the next patch you sent, but it introduced over 200 
> regressions, I will take a look at solving this later.
>>
>>> Since both suggestions are acceptable on their own, and the original bug
>>> was if GDB suggested both at the same time, this commit updates the test
>>> to accept both, gcc's and clang's outputs.
>> On the content of this patch though...
>>
>> I think we can achieve the same results without adding anything from
>> patch #1 in this series.  My idea would be to just ask GDB what it's
>> going to print using 'info functions', then we can expect exactly the
>> right strings.  The patch below implements this idea.
>>
>> What do you think?
>
> This idea sounds good. Would you like this changed even if the clang 
> output is fixed? I can send a patch for it before looking further into 
> how to change GDB's output.

Having thought about this more since I posted, I feel that we should try
to fix how GDB displays the functions for the Clang compiled binary.  My
feeling now is that GDB is just getting it wrong in the Clang case.

I guess we would need to dig into those 200 regressions and see what's
actually going on there.

> Also, do you think the first patch should be abandoned? I feel like it 
> would be a good addition even if we eventually drop my change, but I can 
> drop it if you disagree.

I'd hold off until we have a use case for it.  I think if we fix up how
GDB handle Clang then we'll have no use case for now.

Thanks,
Andrew


>
>>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
>> index 33ad72e6f05..342359ea6e1 100644
>> --- a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
>> +++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
>> @@ -24,18 +24,67 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
>>       return -1
>>   }
>>   
>> +# Clang adds a DW_AT_linkage_name to the DW_TAG_subprogram entry,
>> +# which results in GDB showing the underlying type names for the
>> +# arguments, rather than the typedef names.
>> +#
>> +# In contrast, GCC doesn't include the DW_AT_linkage_name, and so GDB
>> +# shows the type name aliases instead.
>> +#
>> +# To figure out which we expect to see in the tab-completion output,
>> +# use 'info functions' output.  Look for the functions we care about,
>> +# and build a dictionary mapping line number to the argument type.
>> +set types_dict [dict create]
>> +foreach_with_prefix name {get_something grab_it get_value} {
>> +    gdb_test_multiple "info function $name" "" {
>> +	-re "^($::decimal):\\s+static int $name\\((\[^)\]+)\\);\r\n" {
>> +	    set linum $expect_out(1,string)
>> +	    set type $expect_out(2,string)
>> +	    dict set types_dict $linum $type
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^\[^0-9\]\[^\r\n\]+\r\n" {
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^\\s*\r\n" {
>> +	    exp_continue
>> +	}
>> +
>> +	-re "^$::gdb_prompt $" {
>> +	    pass $gdb_test_name
>> +	}
>> +    }
>> +}
>> +
>> +# Now look in the dictionary we built above to figure out how GDB will
>> +# display the types for different arguments.
>> +set linum [gdb_get_line_number "get_value (object_p obj)"]
>> +set object_pattern [dict get $types_dict $linum]
>> +
>> +set linum [gdb_get_line_number "grab_it (int_magic_t *var)"]
>> +set magic_pattern [dict get $types_dict $linum]
>> +
>> +set linum [gdb_get_line_number "get_something (my_string_t msg)"]
>> +set string_pattern [dict get $types_dict $linum]
>> +
>> +# Restart GDB, just in case the function lookup above in someway
>> +# impacted what tab-completion results we might return.
>> +clean_restart $binfile
>> +
>>   # Disable the completion limit for the whole testcase.
>>   gdb_test_no_output "set max-completions unlimited"
>>   
>>   test_gdb_complete_unique \
>>       "break get_v" \
>> -    "break get_value(object_p)"
>> +    "break get_value($object_pattern)"
>>   
>>   test_gdb_complete_unique \
>>       "break gr" \
>> -    "break grab_it(int_magic_t*)"
>> +    "break grab_it($magic_pattern)"
>>   
>> -test_gdb_complete_multiple "break " "get_som" "ething(" {
>> -    "get_something(my_string_t)"
>> -    "get_something(object_p)"
>> -}
>> +test_gdb_complete_multiple "break " "get_som" "ething(" [list \
>> +    "get_something($string_pattern)" \
>> +    "get_something($object_pattern)" \
>> +]
>>
>
> -- 
> Cheers,
> Bruno


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

end of thread, other threads:[~2023-02-06 15:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-17 13:00 [PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen
2023-01-17 13:00 ` [PATCH 1/2] gdb/testsuite: add test with regex for multiple completion patterns Bruno Larsen
2023-01-17 13:00 ` [PATCH 2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang Bruno Larsen
2023-02-03 13:44   ` Andrew Burgess
2023-02-03 15:49     ` Andrew Burgess
2023-02-06 14:10     ` Bruno Larsen
2023-02-06 15:48       ` Andrew Burgess
2023-02-03  8:04 ` [PING][PATCH 0/2] Fix testing gdb.linespec/cp-completion-aliases with Bruno Larsen

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).