Re: [PATCH 2/2] gdb/c++: Improve error messages in overload resolution

[Bruno Larsen <blarsen@redhat.com>]

Cheers,
Bruno

On 31/10/2022 16:04, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> when resolving overloaded functions, GDB relies on knowing relationships
>> between types, i.e. if a type inherits from another. However, some
>> compilers may not add complete information for given types as a way to
>> reduce unnecessary debug information. In these cases, GDB would just say
>> that it couldn't resolve the method or function, with no extra
>> information.
>>
>> The problem is that sometimes the user may not know that the type
>> information is incomplete, and may just assume that there is a bug in
>> GDB. To improve the user experience, we attempt to detect if the
>> overload match failed because of an incomplete type, and warn the user
>> of this.
>>
>> This commit also adds a testcase confirming that the message is only
>> triggered in the correct scenario. This test was not developed as an
>> expansion of gdb.cp/overload.cc because it needed the dwarf assembler,
>> and porting all of overload.cc seemed unnecessary.
>> ---
>>   .../gdb.cp/incomplete-type-overload.cc        |  45 +++++
>>   .../gdb.cp/incomplete-type-overload.exp       | 183 ++++++++++++++++++
>>   gdb/valops.c                                  |  49 ++++-
>>   3 files changed, 273 insertions(+), 4 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.cp/incomplete-type-overload.cc
>>   create mode 100644 gdb/testsuite/gdb.cp/incomplete-type-overload.exp
>>
>> diff --git a/gdb/testsuite/gdb.cp/incomplete-type-overload.cc b/gdb/testsuite/gdb.cp/incomplete-type-overload.cc
>> new file mode 100644
>> index 00000000000..a7b85c92bbe
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/incomplete-type-overload.cc
>> @@ -0,0 +1,45 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2022 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/>.  */
>> +class
>> +base {
>> +public:
>> +  int member;
>> +};
> I think this should be written as:
>
>    struct base
>    {
>      int member;
>    };
>
> Having 'class' immediately followed by 'public:' is, if I understand
> correctly, what 'struct' does, so we could just use that.  I might be
> tempted to switch the 'class'-es below to struct too, but that's up to
> you.
Ok, I'll do that
>
>> +
>> +class complete: public base { };
>> +
>> +class incomplete: public base { };
>> +
>> +complete comp;
>> +complete *cp = &comp;
>> +incomplete *inc;
>> +int *ip;
>> +
>> +int
>> +foo (base* b)
>> +{
>> +    asm ("foo_label: .globl foo_label");
>> +    return 1;
> Indent by 2 spaces.
>
>> +}
>> +
>> +int
>> +main (void)
> No need for 'void' here in C++ code.
>
>> +{
>> +    asm("main_label: .globl main_label");
> Missing space before '('.
>
>> +    comp.member = 0;
>> +    return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.cp/incomplete-type-overload.exp b/gdb/testsuite/gdb.cp/incomplete-type-overload.exp
>> new file mode 100644
>> index 00000000000..96ed25dd5d1
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/incomplete-type-overload.exp
>> @@ -0,0 +1,183 @@
>> +# Copyright 2022 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/>.
>> +
>> +# This file is part of GDB's testsuite.
>> +
>> +# This test intends to check the error message that GDB emits when unable
>> +# to determine the correct overloaded function due to incomplete types.
>> +
>> +load_lib dwarf.exp
>> +
>> +if { [skip_cplus_tests] } { return }
>> +
>> +if { ![dwarf2_support] } { return }
>> +
>> +standard_testfile .cc .S
>> +set asm_file [standard_output_file ${srcfile2}]
>> +
>> +if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}] {
>> +    return
>> +}
>> +
>> +if {[test_compiler_info clang-*-*]} {
> Missing 'c++' language for test_compiler_info call.
>
>> +    untested "gcc is required for dwarf assembler tests"
>> +    return
>> +}
>> +
>> +if ![runto_main] {
>> +    return
>> +}
>> +
>> +# Get important sizes to create fake dwarf for the test
>> +set int_size [get_sizeof "int" -1]
>> +set addr_size [get_sizeof "void *" -1]
>> +set struct_base_size [get_sizeof "base" 4]
>> +set struct_complete_size [get_sizeof "complete" 4]
>> +get_func_info foo
> Not that it makes much difference, but get_func_info has the ability to
> take compiler options, so I think this should really be:
>
>    get_func_info foo {debug c++}
>
>> +
>> +# Create fake DWARF for the .cc file.
>> +# This is the best way to ensure we have an incomplete type.
>> +Dwarf::assemble ${asm_file} {
>> +    global srcdir subdir srcfile srcfile2 foo_start foo_end
>> +    global int_size addr_size struct_base_size struct_complete_size
>> +    declare_labels L
>> +
>> +    cu {} {
>> +	DW_TAG_compile_unit {
>> +	    {DW_AT_language @DW_LANG_C_plus_plus}
>> +	    {name $srcfile}
>> +	    {stmt_list $L DW_FORM_sec_offset}
>> +	} {
>> +	    declare_labels int_label base_label complete_label incomplete_label
>> +	    declare_labels ptr_base_label ptr_inc_label ptr_comp_label ptr_int_label
>> +
>> +	    int_label: DW_TAG_base_type {
>> +		{DW_AT_byte_size $int_size DW_FORM_sdata}
>> +		{DW_AT_encoding @DW_ATE_signed}
>> +		{DW_AT_name "int"}
>> +	    }
>> +
>> +	    base_label: DW_TAG_class_type {
>> +		{DW_AT_byte_size $struct_base_size DW_FORM_sdata}
>> +		{DW_AT_name "base"}
>> +	    } {
>> +		DW_TAG_member {
>> +		    {DW_AT_name "member"}
>> +		    {DW_AT_type :$int_label}
>> +		    {DW_AT_data_member_location 0 DW_FORM_sdata}
>> +		}
>> +	    }
>> +
>> +	    complete_label: DW_TAG_class_type {
>> +		{DW_AT_byte_size $struct_complete_size DW_FORM_sdata}
>> +		{DW_AT_name "complete"}
>> +	    } {
>> +		DW_TAG_inheritance {
>> +		    {DW_AT_type :$base_label}
>> +		    {DW_AT_data_member_location 0 DW_FORM_sdata}
>> +		    {DW_AT_accessibility 1 DW_FORM_data1}
>> +		}
>> +	    }
>> +
>> +	    incomplete_label: DW_TAG_class_type {
>> +		{DW_AT_name "incomplete"}
>> +		{DW_AT_declaration 1 DW_FORM_flag_present}
>> +	    }
>> +
>> +	    ptr_base_label: DW_TAG_pointer_type {
>> +		{DW_AT_byte_size $addr_size DW_FORM_udata}
>> +		{DW_AT_type :$base_label}
>> +	    }
>> +
>> +	    ptr_inc_label: DW_TAG_pointer_type {
>> +		{DW_AT_byte_size $addr_size DW_FORM_udata}
>> +		{DW_AT_type :$incomplete_label}
>> +	    }
>> +
>> +	    ptr_comp_label: DW_TAG_pointer_type {
>> +		{DW_AT_byte_size $addr_size DW_FORM_udata}
>> +		{DW_AT_type :$complete_label}
>> +	    }
>> +
>> +	    ptr_int_label: DW_TAG_pointer_type {
>> +		{DW_AT_byte_size $addr_size DW_FORM_udata}
>> +		{DW_AT_type :$int_label}
>> +	    }
>> +
>> +	DW_TAG_variable {
>> +	    {DW_AT_name "comp"}
>> +	    {DW_AT_type :$complete_label}
>> +	    {DW_AT_location {DW_OP_addr [gdb_target_symbol "comp"]} SPECIAL_expr}
>> +	    {DW_AT_external 1 DW_FORM_flag}
>> +	}
>> +
>> +	DW_TAG_variable {
>> +	    {DW_AT_name "cp"}
>> +	    {DW_AT_type :$ptr_comp_label}
>> +	    {DW_AT_location {DW_OP_addr [gdb_target_symbol "cp"]} SPECIAL_expr}
>> +	    {DW_AT_external 1 DW_FORM_flag}
>> +	}
>> +
>> +	DW_TAG_variable {
>> +	    {DW_AT_name "inc"}
>> +	    {DW_AT_type :$ptr_inc_label}
>> +	    {DW_AT_location {DW_OP_addr [gdb_target_symbol "inc"]} SPECIAL_expr}
>> +	    {DW_AT_external 1 DW_FORM_flag}
>> +	}
>> +
>> +	DW_TAG_variable {
>> +	    {DW_AT_name "ip"}
>> +	    {DW_AT_type :$ptr_int_label}
>> +	    {DW_AT_location {DW_OP_addr [gdb_target_symbol "ip"]} SPECIAL_expr}
>> +	    {DW_AT_external 1 DW_FORM_flag}
>> +	}
>> +
>> +	    DW_TAG_subprogram {
>> +		{MACRO_AT_func {"main"}}
>> +		{DW_AT_external 1 flag}
>> +	    }
>> +	    DW_TAG_subprogram {
>> +		{MACRO_AT_func {"foo"}}
> While looking at this patch I wondered about these two MACRO_AT_ calls.
> These surely should be passing the language through too, just like the
> get_func_info call earlier?
>
> I've attached a patch at the end of this email that I wrote that allows
> these to change to:
>
>    {MACRO_AT_func {"main" {debug c++}}}
>    {MACRO_AT_func {"foo" {debug c++}}}
>
> I think this change would be better, but doesn't actually change the
> results of the tests or not, so feel free to use this or not, up to
> you.
This makes sense, but since it doesn't change anything I think you 
should post the patch separately and maybe have a second patch changing 
the relevant functions. Just so the patch series doesn't balloons.
>
>> +		{DW_AT_type :$int_label}
>> +		{DW_AT_external 1 flag}
>> +	    } { formal_parameter {
>> +		    {DW_AT_name "b"}
>> +		    {DW_AT_type :$ptr_base_label}
>> +		}
>> +	    }
>> +	}
>> +    }
>> +
>> +    lines {version 2} L {
>> +	include_dir "$srcdir/$subdir"
>> +	file_name $srcfile 1
>> +    }
>> +}
>> +
>> +if [prepare_for_testing "failed to prepare" $testfile [list $asm_file $srcfile] {}] {
>> +    return
>> +}
>> +
>> +if ![runto_main] {
>> +    return
>> +}
>> +
>> +gdb_test "print foo(cp)" "= 1" "successful invocation"
>> +gdb_test "print foo(inc)"\
>> +	 "The type. 'incomplete .' isn't fully known to GDB.*"\
>> +	 "unsuccessful because declaration"
>> +gdb_test "print foo(ip)"\
>> +	 "Cannot resolve function foo to any overloaded instance."\
>> +	 "unsuccessful because incorrect"
>> diff --git a/gdb/valops.c b/gdb/valops.c
>> index 748f154252a..2054e9ec303 100644
>> --- a/gdb/valops.c
>> +++ b/gdb/valops.c
>> @@ -41,6 +41,7 @@
>>   #include "extension.h"
>>   #include "gdbtypes.h"
>>   #include "gdbsupport/byte-vector.h"
>> +#include "typeprint.h"
>>   
>>   /* Local functions.  */
>>   
>> @@ -2933,14 +2934,54 @@ find_overload_match (gdb::array_view<value *> args,
>>   
>>     if (match_quality == INCOMPATIBLE)
>>       {
>> +      /* If we couldn't find a decent overload, it may be because we don't
>> +	 have enough information about the requested types.  Warn user if
>> +	 this is the case, so they can manually cast the types.  */
>> +      int incomplete_types = 0;
>> +      std::string incomplete_arg_names = "";
> I think the ' = ""' can be removed here.
>
>> +      for (struct value *arg: args)
> I think GDB style is to have a space before and after ':'.  Also I think
> you can make ARG const.
>
>> +	{
>> +	  struct type *t = value_type (arg);
>> +	  while (t->code () == TYPE_CODE_PTR)
>> +	    {
>> +	      t = t->target_type();
>> +	    }
> Single statement while body should have the '{' and '}' removed.
>
>> +	  if (t->is_stub ())
>> +	    {
>> +	      string_file buffer;
>> +	      if(incomplete_types > 0)
> Missing space before '(' here.
>
>> +		incomplete_arg_names += ", ";
>> +
>> +	      current_language->print_type(value_type(arg), "", &buffer,
>> +					   -1, 0, &type_print_raw_options);
> ... and here twice.
>
>> +
>> +	      incomplete_types++;
>> +	      incomplete_arg_names += buffer.string();
> ... and here again.
>
>> +	    }
>> +	}
>> +      std::string hint = "";
> I don't think the ' = ""' is needed here.
>
>> +      if (incomplete_types > 1)
>> +	{
>> +	  hint = string_printf (_("\nThe types: '%s' aren't fully known to GDB."
>> +				  " Please cast them directly to the desired"
>> +				  " typed in the function call."),
>> +				  incomplete_arg_names.c_str ());
>> +	}
> This, and the following single statement if blocks should have the '{'
> and '}' removed.
Ah, I thought the bit about multiple lines in the coding standard also 
applied to wrapping lines (at the end 
ofhttps://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Whitespaces)
>
>> +      else if (incomplete_types == 1)
>> +	{
>> +	  hint = string_printf (_("\nThe type: '%s' isn't fully known to GDB."
>> +				  " Please cast it directly to the desired"
>> +				  " typed in the function call."),
>> +				  incomplete_arg_names.c_str ());
>> +	}
> I think I would move this entire block of code into a new static
> function.  It only requires the ARGS gdb::array_view parameter, and
> returns a std::string.
>
>>         if (method == METHOD)
>> -	error (_("Cannot resolve method %s%s%s to any overloaded instance"),
>> +	error (_("Cannot resolve method %s%s%s to any overloaded instance.%s"),
>>   	       obj_type_name,
>>   	       (obj_type_name && *obj_type_name) ? "::" : "",
>> -	       name);
>> +	       name, hint.c_str());
>>         else
>> -	error (_("Cannot resolve function %s to any overloaded instance"),
>> -	       func_name);
>> +	error (_("Cannot resolve function %s to any overloaded instance.%s"),
>> +	       func_name, hint.c_str());
>>       }
>>     else if (match_quality == NON_STANDARD)
>>       {
>> -- 
>> 2.37.3
> ---
>
> commit d98626400f0e0a5721e8ea9ebdc0ea0edc480af3
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Mon Oct 31 12:57:51 2022 +0000
>
>      gdb/testsuite: allow options for MACRO_AT_range and MACRO_AT_func
>      
>      This commit allows the MACRO_AT_range and MACRO_AT_func, as used by
>      the DWARF assembler, to accept an options list.  These options will
>      then be forwarded to the function_range call that is contained within
>      the implementation of MACRO_AT_range.
>      
>      This will allow tests to pass through the correct compilation flags if
>      needed when these macros are used, though no such uses are added in
>      this commit.
>
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index b843b1acf75..593f4ea72f1 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -925,14 +925,20 @@ namespace eval Dwarf {
>       proc _handle_macro_at_range { attr_value } {
>   	variable _cu_is_fission
>   
> -	if {[llength $attr_value] != 1} {
> -	    error "usage: MACRO_AT_range { func }"
> +	if {[llength $attr_value] < 1 || [llength $attr_value] > 2} {
> +	    error "usage: MACRO_AT_range { func {options {debug}} }"
> +	}
> +
> +	if {[llength $attr_value] == 2} {
> +	    set options [lindex $attr_value 1]
> +	} else {
> +	    set options {debug}
>   	}
>   
>   	set func [lindex $attr_value 0]
>   	global srcdir subdir srcfile
>   	set src ${srcdir}/${subdir}/${srcfile}
> -	set result [function_range $func $src]
> +	set result [function_range $func $src $options]
>   
>   	set form DW_FORM_addr
>   	if { $_cu_is_fission } {
> @@ -947,9 +953,10 @@ namespace eval Dwarf {
>       # Handle macro attribute MACRO_AT_func.
>   
>       proc _handle_macro_at_func { attr_value } {
> -	if {[llength $attr_value] != 1} {
> -	    error "usage: MACRO_AT_func { func file }"
> +	if {[llength $attr_value] < 1 || [llength $attr_value] > 2} {
> +	    error "usage: MACRO_AT_func { func {options {debug}} }"
>   	}
> +
>   	_handle_attribute DW_AT_name [lindex $attr_value 0] DW_FORM_string
>   	_handle_macro_at_range $attr_value
>       }
>