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 6B669398AC2E for ; Thu, 17 Nov 2022 09:58:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6B669398AC2E 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=1668679101; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xCySBrDi1SLeTyHVgpabTiTLC4dE+wBaIGJhDRi6Q7I=; b=IphwxHP9yiL9izqLYzJxoB3VAplJNoZDxrJO9falO3H2qwr9ukIJwkDdv6Ya7LScn57WDO ek6vuKdrSO3Dstc48GQTHaPmaSzvUO2KjjceQ6NOfP48OaXrRB/++5872bwKPUevQInTnM 3pdBHDXztoqCYk5AkeReRMA4dkmuLMA= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-474-bV3xZibDPhun3xCYArjYHw-1; Thu, 17 Nov 2022 04:58:06 -0500 X-MC-Unique: bV3xZibDPhun3xCYArjYHw-1 Received: by mail-wm1-f72.google.com with SMTP id m17-20020a05600c3b1100b003cf9cc47da5so403104wms.9 for ; Thu, 17 Nov 2022 01:58:06 -0800 (PST) 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:cc: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=xCySBrDi1SLeTyHVgpabTiTLC4dE+wBaIGJhDRi6Q7I=; b=grfn7jsgTe8+HKfJ0m4WRLvs2oa6tbcsXT38oGlrVurtZcOOP0yBmYMfPbJJ8aZrWo RpqrWMMP1plhW0Pkr/oFitkSWIyEKS1AScIxLaSDMgvc1J6Dl2O7M2Wf84qP/Qc6ty+h 6EcLlOBA+nCnEUc5ERlxCyv+NXso3Q8SxYRGRKY9TEMaGreE5kNjlEBJl7VAUBA7IrT6 JODKmvojMw2jwqSlKa1I0OFT0gCC7E6P+tuRC2UYac6cRH0O5tDiBzoQK/vHTeXM+18d YYm3sF3eUuNWSZnjZ0uNs09EX5g6GyTYhJ+x6BANdD41C769dXuJJXuIqKRN5H8Hj8Q2 yqCQ== X-Gm-Message-State: ANoB5pnpQVJdEAcwVsSdERlktr220XedbTeRb+d12ZH0n4LsZsZz9Wpa qnyynrGvQA2fDZKvsVk7YtoBd4J3jh7vv/Rb/90IiF3nW9DvpVy1NeunqxXwnyquvZIQtk0FL/v g0/d3nBjbs+WVyjsLbGkj/w== X-Received: by 2002:a5d:6602:0:b0:236:651d:60cb with SMTP id n2-20020a5d6602000000b00236651d60cbmr967878wru.474.1668679085040; Thu, 17 Nov 2022 01:58:05 -0800 (PST) X-Google-Smtp-Source: AA0mqf7x9EImitnyTcpoF8gO4ivOdzM4GijU0ZR5hpuB3/eQ20hIbO6Lag/wHjZB7e70kQe75Q5JiA== X-Received: by 2002:a5d:6602:0:b0:236:651d:60cb with SMTP id n2-20020a5d6602000000b00236651d60cbmr967862wru.474.1668679084773; Thu, 17 Nov 2022 01:58:04 -0800 (PST) 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 f19-20020a05600c4e9300b003cfd4a50d5asm5513875wmq.34.2022.11.17.01.58.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Nov 2022 01:58:04 -0800 (PST) Message-ID: Date: Thu, 17 Nov 2022 10:58:03 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH v2 2/2] gdb/c++: Detect ambiguous variables in imported namespaces To: Lancelot SIX Cc: gdb-patches@sourceware.org, tom@tromey.com References: <20221116141336.1160869-1-blarsen@redhat.com> <20221116141336.1160869-3-blarsen@redhat.com> <20221116174913.hd3l6yyh5z6w7n7y@ubuntu.lan> From: Bruno Larsen In-Reply-To: <20221116174913.hd3l6yyh5z6w7n7y@ubuntu.lan> 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: 8bit X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: On 16/11/2022 18:49, Lancelot SIX wrote: > Hi Bruno, > > Thanks for working on this. I have included remarks inlined in the path. > > On Wed, Nov 16, 2022 at 03:13:37PM +0100, Bruno Larsen via Gdb-patches wrote: >> When running gdb.cp/nsusing.cc and stopping at line 17, we can ask GDB >> to print x and get a compiler-dependent answer. Using gcc 12.2.1, GDB >> will print M::x, and using clang 16.0.0 prints N::x. Not only is this >> behavior confusing to users, it is also not consistent with compiler >> behaviors, which would warn that using x is ambiguous at this point. >> >> This commit makes GDB behavior consistent with compilers. it achieves >> this by making it so instead of exiting early when finding any symbol >> with the correct name, GDB continues searching through all include >> directives, storing all matching symbols in a relational map betwen the >> mangled name and the found symbols. >> >> If the resulting map has more than one entry, GDB says that the >> reference is ambiguous and lists all possibilities. Otherwise it returns >> the map containing zero or one entries. >> >> The commit also changes gdb.cp/nsusing.exp to test the ambiguous >> detection. >> --- >> gdb/cp-namespace.c | 69 +++++++++++++++++++++++--------- >> gdb/testsuite/gdb.cp/nsusing.exp | 13 +++++- >> 2 files changed, 61 insertions(+), 21 deletions(-) >> >> diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c >> index 6ecb29fb1ac..df177b20d92 100644 >> --- a/gdb/cp-namespace.c >> +++ b/gdb/cp-namespace.c >> @@ -32,6 +32,7 @@ >> #include "buildsym.h" >> #include "language.h" >> #include "namespace.h" >> +#include >> #include >> >> static struct block_symbol >> @@ -372,7 +373,7 @@ cp_lookup_symbol_in_namespace (const char *the_namespace, const char *name, >> SEARCH_SCOPE_FIRST is an internal implementation detail: Callers must >> pass 0 for it. Internally we pass 1 when recursing. */ > Should the function's description be updated to reflect the new return > type? i.e. describe a case where a map of more than 1 element is > returned. Yes, thanks! > >> >> -static struct block_symbol >> +static std::map >> cp_lookup_symbol_via_imports (const char *scope, >> const char *name, >> const struct block *block, >> @@ -386,13 +387,20 @@ cp_lookup_symbol_via_imports (const char *scope, >> int len; >> int directive_match; >> >> + /* All the symbols we found will be kept in this relational map between >> + the mangled name and the block_symbol found. We do this so that GDB >> + won't incorrectly report an ambiguous symbol for finding the same >> + thing twice. */ >> + std::map found_symbols; >> + >> /* First, try to find the symbol in the given namespace if requested. */ >> if (search_scope_first) >> - sym = cp_lookup_symbol_in_namespace (scope, name, >> - block, domain, 1); >> - >> - if (sym.symbol != NULL) >> - return sym; >> + { >> + sym = cp_lookup_symbol_in_namespace (scope, name, >> + block, domain, 1); >> + if (sym.symbol != nullptr) >> + found_symbols[sym.symbol->m_name] = sym; >> + } >> >> /* Due to a GCC bug, we need to know the boundaries of the current block >> to know if a certain using directive is valid. */ >> @@ -446,7 +454,7 @@ cp_lookup_symbol_via_imports (const char *scope, >> if (declaration_only || sym.symbol != NULL || current->declaration) >> { >> if (sym.symbol != NULL) >> - return sym; >> + found_symbols[sym.symbol->m_name] = sym; > Looks like you have changed the indentation here with two more spaces. > >> >> continue; >> } >> @@ -467,23 +475,43 @@ cp_lookup_symbol_via_imports (const char *scope, >> sym = cp_lookup_symbol_in_namespace (scope, >> current->import_src, >> block, domain, 1); >> + found_symbols[sym.symbol->m_name] = sym; >> } >> else if (current->alias == NULL) >> { >> /* If this import statement creates no alias, pass >> current->inner as NAMESPACE to direct the search >> towards the imported namespace. */ >> - sym = cp_lookup_symbol_via_imports (current->import_src, >> + std::map sym_map = >> + cp_lookup_symbol_via_imports (current->import_src, >> name, block, >> domain, 1, 0, 0); > I think the usual coding style is to have the "=" after the line break. > This would become > > std::map sym_map > = cp_lookup_symbol_via_imports (…); > >> + found_symbols.merge(sym_map); >> } >> >> - if (sym.symbol != NULL) >> - return sym; >> } >> } >> >> - return {}; >> + if (found_symbols.size () > 1) >> + { >> + auto itr = found_symbols.begin(); > Could it be `cbegin ()` ? (and you forgot a space before the parens). > >> + std::string error_str = "Reference to \""; >> + error_str += name; >> + error_str += "\" is ambiguous, possibilities are: "; >> + error_str += itr->second.symbol->print_name (); >> + for (itr++; itr != found_symbols.end (); itr++) >> + { >> + error_str += " and "; >> + error_str += itr->second.symbol->print_name (); >> + } >> + error (_("%s"), error_str.c_str ()); > I do not know if this something we care about, but your error message > will list the options in an unspecified order. Iterating over the map > will follow the order of the keys, which are the addresses of the > mangled names of the symbols. > > My instinct would prefer to list the options in lexicographical order > (of the demangled name). This is probably not really important but it > could be annoying when writing test checking for the output message. Would you be OK with lexicological ordering of mangled names? That could be easily done by changing to std::map and we keep all of the rest of the code. > >> + } >> + else >> + return found_symbols; >> + >> + /* This is needed to silence a -Werror=return-type warning, because >> + the above if case doesn't have a return statement. */ >> + gdb_assert_not_reached (); >> } >> >> /* Helper function that searches an array of symbols for one named NAME. */ >> @@ -514,7 +542,6 @@ cp_lookup_symbol_imports_or_template (const char *scope, >> const domain_enum domain) >> { >> struct symbol *function = block->function (); >> - struct block_symbol result; >> >> if (symbol_lookup_debug) >> { >> @@ -596,15 +623,19 @@ cp_lookup_symbol_imports_or_template (const char *scope, >> } >> } >> >> - result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1); >> + std::map result = >> + cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1); > Same remark for the "=" before the line break. > >> if (symbol_lookup_debug) >> { >> gdb_printf (gdb_stdlog, >> "cp_lookup_symbol_imports_or_template (...) = %s\n", >> - result.symbol != NULL >> - ? host_address_to_string (result.symbol) : "NULL"); >> + result.size() == 1 >> + ? host_address_to_string (result[0].symbol) : "NULL"); > If result is a std::map, result[0] will look for the > key (const char *) 0, which is most probably in the map. It therefore > creates a new instance of T using the default ctor, insert it in the map > for key 0 and returns a reference to it. > > I think yoy want something like result.begin ().second.symbol or > something along those lines like you do… Right, yes, I encountered this testing locally, but forgot to change this part. oops > >> } >> - return result; >> + if (result.empty ()) >> + return {}; >> + else >> + return result.begin ()->second; > … here! > > This however raises the question: if cp_lookup_symbol_via_imports can > return multiple block_symbol, shouldn't > cp_lookup_symbol_imports_or_template do the same when it basically > forwards what cp_lookup_symbol_via_imports returns? Well, you just found a different flaw in my logic... My plan is that the function will only return multiple values when being called recursively, and the top-level call would error out in case it found multiple symbols. This makes it so asserting that the size is 1 or reacting to different sizes redundant outside cp_lookup_symbol_via_imports. However, I forgot to add the check for the function being top level (seeing if search_scope_first is 1), which could lead us to incomplete lists. I'll fix this and document this behavior better for v3 > > I am not really familiar with this part of the code, so I do not know if > this can happen (but I guess it could). > > I would say that: > - If the map can only contain one element and you only introduce it to > hold the result of cp_lookup_symbol_via_imports, then you should > assert that its size is 1 here (and maybe have a comment explaining > why it must be a map of size 1). As I mentioned above, before returning from the top-level call, cp_lookup_symbol_via_imports will already have errored out if there is more than one element. If you think it is important, I can document it. It felt self-explanatory to me, but I might just be in too deep at this point > - If it can be anything else, I guess you would need to return the > entire map, or have a comment explaining why any member could do. > > As the function's comment says "Like cp_lookup_symbol_via_imports, but > […]" I am tempted to think that it should also return a map. I am thinking that maybe that comment should be rewritten. The reason for the call may be similar, but the internal workings look pretty different (mainly because it doesn't implement anything from cp_lookup_symbol_via_imports, just calls that function). I'm thinking something along the lines of: search for symbols whose names match NAME. if BLOCK is a function, search through the template parameters and function type. Then (or if it is not a function) call cp_lookup_symbol_via imports to search for other symbols in the given SCOPE. Or something of the sorts. What do you think? -- Cheers, Bruno > > I hope those help. > All the best, > Lancelot. > >> } >> >> /* Search for NAME by applying relevant import statements belonging to BLOCK >> @@ -616,13 +647,13 @@ cp_lookup_symbol_via_all_imports (const char *scope, const char *name, >> const struct block *block, >> const domain_enum domain) >> { >> - struct block_symbol sym; >> + std::map sym; >> >> while (block != NULL) >> { >> sym = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 0, 1); >> - if (sym.symbol) >> - return sym; >> + if (sym.size() == 1) >> + return sym.begin ()->second; >> >> block = block->superblock (); >> } >> diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp >> index b79f3d26084..4ef0f48c507 100644 >> --- a/gdb/testsuite/gdb.cp/nsusing.exp >> +++ b/gdb/testsuite/gdb.cp/nsusing.exp >> @@ -127,11 +127,20 @@ gdb_test_multiple "print x" "print x, before using statement" { >> -re -wrap "No symbol .x. in current context.*" { >> pass $gdb_test_name >> } >> - -re -wrap "= 911.*" { >> + -re -wrap "Reference to .x. is ambiguous.*" { >> # GCC doesn't properly set the decl_line for namespaces, so GDB believes >> # that the "using namespace M" line has already passed at this point. >> xfail $gdb_test_name >> } >> } >> gdb_test "next" ".*" "using namespace M" >> -gdb_test "print x" "= 911" "print x, only using M" >> +gdb_test_multiple "print x" "print x, only using M" { >> + -re -wrap "= 911.*" { >> + pass $gdb_test_name >> + } >> + -re -wrap "Reference to .x. is ambiguous.*" { >> + xfail $gdb_test_name >> + } >> +} >> +gdb_test "next" ".*" "using namespace N" >> +gdb_test "print x" "Reference to .x. is ambiguous.*" "print x, with M and N" >> -- >> 2.38.1 >>