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

[Andrew Burgess <aburgess@redhat.com>]

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.

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

> +		{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.

> +      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
     }