From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 40EF13853808 for ; Mon, 31 Oct 2022 15:04:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 40EF13853808 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667228663; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=dDZY/OSUSOQLqZDpWEXA2CHWqz771jIRjznP7g/cXZ4=; b=cEAbXYVFehCpoXwvvmOHxn68yGoWlCAbBl7F/7SUMLaUCTTJZeXY0HCrhgKtQlJnf1elkL HIRAeW7tqR657lMtgHyN1Mvq0ZfHfmCoaK022D5k6ghJ+WKogunFDCpIGZSSTWNUkg18Xn CAsFSC+pUMOOfgmXiupG7RJY1EVD6R8= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-386-I19n1p9lMtOHi_uj5rJSxw-1; Mon, 31 Oct 2022 11:04:22 -0400 X-MC-Unique: I19n1p9lMtOHi_uj5rJSxw-1 Received: by mail-qt1-f197.google.com with SMTP id ew11-20020a05622a514b00b003a524196d31so1978414qtb.2 for ; Mon, 31 Oct 2022 08:04:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dDZY/OSUSOQLqZDpWEXA2CHWqz771jIRjznP7g/cXZ4=; b=A7d1XwCl/CNE/O72CydfPvrkVGnvPY3hqbEwYxqB9SuWf2TiS8kG3aCN/o0/GoRXbO TMTAjQVfJndFWP4iV0CMY5wmICj8uv1FJv7Om162BXEtyM3AGTHP1HCqfIA/pnJ0a8fJ KKw529rfw3rpL+s0lKBnwZ4WqzZYXn9NzyDIPMRhTUbVfWngaIOGan/aR0yped/vmgWh ahGRXqPOoPYq3OuDnW9o5H3/iGWWbvGfjGCznJRAOUmkLFqhkjFrx+TepbrTfQ8Lfur2 4estjZkaKIdCygtMFSxCEiA2pur4isK+rJd9GRJK94ml6qzh2N5IyhMzkTxNU5zfsUKI Q64Q== X-Gm-Message-State: ACrzQf0hmOClO0nL82d/jLqBcOH1/HUc2ISh5+tk7ZwTtsiHoBXycSiq ufLzZbuKSD8+YyeGyPrTxuncpDuKZMCuVRb4rfSQXXtjUyk9JECziznUP+CLE4FpVboz5EqMLtr h42Y1vvAhpTwQyh7S7A0DTA== X-Received: by 2002:ad4:5de9:0:b0:4bb:83a6:10bf with SMTP id jn9-20020ad45de9000000b004bb83a610bfmr11364549qvb.49.1667228661868; Mon, 31 Oct 2022 08:04:21 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4COu4K9vkUNoYAbKg4fO2unP+VSG3qUAbiw35Sc7QHlg1DxDrL/H12BhrcW20j0/KI9xf9Cg== X-Received: by 2002:ad4:5de9:0:b0:4bb:83a6:10bf with SMTP id jn9-20020ad45de9000000b004bb83a610bfmr11364479qvb.49.1667228661246; Mon, 31 Oct 2022 08:04:21 -0700 (PDT) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id bt4-20020ac86904000000b0039bde72b14asm3677071qtb.92.2022.10.31.08.04.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 31 Oct 2022 08:04:20 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] gdb/c++: Improve error messages in overload resolution In-Reply-To: <20221013160114.4143323-3-blarsen@redhat.com> References: <20221013160114.4143323-1-blarsen@redhat.com> <20221013160114.4143323-3-blarsen@redhat.com> Date: Mon, 31 Oct 2022 15:04:19 +0000 Message-ID: <871qqoc9vw.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Bruno Larsen via Gdb-patches 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 . */ > +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 = ∁ > +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 . > + > +# 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 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 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 }