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.129.124]) by sourceware.org (Postfix) with ESMTPS id EE1D4385841C for ; Fri, 4 Nov 2022 13:55:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EE1D4385841C 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=1667570143; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=G9yy3iXpZlVgt1ZMfqu8v4W/7KLT0f1WDmYKR67NUE0=; b=a0QdgWNaH7mkvam4Z5vNoZcMSMWsnhLBwm9EAdbPUcDMQAYQj5l3xFaHuKAvS6zgnaf5qv Nr0IPzDjAg2w1eZ0im8FJnGCYXVfdaa5SQvTvdpYoWq+It1212ff2fIqiAHm0LTEveZtkI gEOMebzhAJnSpbfMkPaAfjCvoxI+So8= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-321-ekd1LCYGMLixfFDDOhJklA-1; Fri, 04 Nov 2022 09:55:42 -0400 X-MC-Unique: ekd1LCYGMLixfFDDOhJklA-1 Received: by mail-ed1-f70.google.com with SMTP id v18-20020a056402349200b004622e273bbbso3583223edc.14 for ; Fri, 04 Nov 2022 06:55:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=G9yy3iXpZlVgt1ZMfqu8v4W/7KLT0f1WDmYKR67NUE0=; b=SOUtSrenPYtp8t8+rv+EdT+q3yG9T2PJq1juW99IxMJa7yb/JHA+uPSDp7+5849J1w R3PX7oSr9h8sev+3opNYY57JEpdqNeHH0QHE2df2mznJzsTTK+t6z3A5lEA5whmhT5AO /Y25atrbVl7doo/nYxAeGgjkSBUs29QJJ3YynWgyHedl1KIo+N0cttVjAlWmNDCwIVqq DVdrSaSTNJ0+tdxcyyfEXl/KKpRXVF0TY7VbghO+kcTfOSsnRMj8IZDDQb2IcUvNJ0kq OgKAZVTjg1oLZr2dlvcPKLXnKkAK99YCJCvonGSXj5Kot8QKRXN4Vgb7qEH1aoDZ+IpO R/QQ== X-Gm-Message-State: ACrzQf1eCqEmadAwQNCwZd/Z1V9ASGQhp1myw+UnKLgtAM6fDqh8toLA KqqOvl+rb8lVmIqQgSU6YMm5Z0F6lLAF+PB6oGu2wlLf8Tfdv8AHPqoLcEtb9bqO5umqNukdrSP nNLUPeTuBo5oBKWUeRTURoA== X-Received: by 2002:a17:906:5ac2:b0:78d:3358:7694 with SMTP id x2-20020a1709065ac200b0078d33587694mr34689185ejs.276.1667570140657; Fri, 04 Nov 2022 06:55:40 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6WFfnNvBKm9mlkP1bokxz//kuZCV0cKZwvCLZR+7lOsmZCq+qOQXVJeiOsTIydY1ScLkd+EQ== X-Received: by 2002:a17:906:5ac2:b0:78d:3358:7694 with SMTP id x2-20020a1709065ac200b0078d33587694mr34689162ejs.276.1667570140275; Fri, 04 Nov 2022 06:55:40 -0700 (PDT) Received: from [192.168.0.45] (ip-62-245-66-121.bb.vodafone.cz. [62.245.66.121]) by smtp.gmail.com with ESMTPSA id ku10-20020a170907788a00b00782e3cf7277sm867279ejc.120.2022.11.04.06.55.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 04 Nov 2022 06:55:39 -0700 (PDT) Message-ID: Date: Fri, 4 Nov 2022 14:55:38 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH 2/2] gdb/c++: Improve error messages in overload resolution To: Andrew Burgess , gdb-patches@sourceware.org References: <20221013160114.4143323-1-blarsen@redhat.com> <20221013160114.4143323-3-blarsen@redhat.com> <871qqoc9vw.fsf@redhat.com> From: Bruno Larsen In-Reply-To: <871qqoc9vw.fsf@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,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: Cheers, Bruno On 31/10/2022 16:04, Andrew Burgess wrote: > 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. Ok, I'll do that > >> + >> +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. 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 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 > 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 > } >