[2/2] gdb/testsuite: fix running gdb.linespec/cp-completion-aliases.exp with clang

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

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)" \
+]
  

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
  

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)" \
> +]
>
  

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