From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by sourceware.org (Postfix) with ESMTPS id 98245388E801 for ; Fri, 24 Jul 2020 09:08:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 98245388E801 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x442.google.com with SMTP id f2so7605619wrp.7 for ; Fri, 24 Jul 2020 02:08:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=ydiXJV7sTnmqZ0+d3ApreV4ntZLKNm0Drfslq3K9ue8=; b=aW+3QE7K+mdUhODzJZZOtaHkQBE9E6G6Ht2/mum5ErmMi4BXpHdJzbkvdoG0DQm2bc x6HJ38lBJFaHtDoM4Wz852imFn+3wkp5Zau3VOkzgAdGJveOr6CYogZg/YeDrWSDRl6z iJIo9sF2H2T/21p5J9yBZWiHTfmADuug+wM7SLFMmuRc2+U7WjP13qrZ0LVh4KDdAubp OoBsIrY/5wnwZz/MU4Oin7B8VFnBu/HkTG942LH4mcLrUvEKNkY/83aI9K3FT2vrW81v XBPHRG/9SF8wNBC+oNHh+54nIZADlKu+T6i/6lkGd10rWsXSiuCtXNCE0kj66cwq5riO 7eag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ydiXJV7sTnmqZ0+d3ApreV4ntZLKNm0Drfslq3K9ue8=; b=jVBo+jSuYfDPpEGQlfqGooaBW99FSetSzRmzA3uveGjlKAvPD/0ixk0HYA5a1En9ml bcDQduLROPbBnQ7QyOnfA6nzQi2UG5CNfwEnOtjSPykEBtDvfbnIU3yPwDAxpix/LbdM fsKXLiT1TtVXA40ibrBqudl4ho5A621i1MH7FnLX661yllz0Qnr3eL4Y8M7fNRLqAijv If01KwgbMeGE6LohkntgiFgCrqZW32cmdcpHHgxY/Ghefgfuyqr5hPW7VCONoZd2X+wJ xp0XhNMOb5vFOyg4f54CetWIWUzxyD1EBmr85zGF3/PGGvxxqDgY7aBCyU0RpOiXeNjd BtJQ== X-Gm-Message-State: AOAM532j1174hxn7PHmguLQ9Nkb9txwZcyEqeogHXPvjtQYBrBM3QeAT zgSw3HdbMVL1ytSOvyItpWY/qWoHDgs= X-Google-Smtp-Source: ABdhPJyQLaMem+n82ghLW5GZ5KKkmQyhhxhgJ5WWe6HJiWPUV4mIy9wkAT/6wGeDdfcPMbaFCNMovw== X-Received: by 2002:adf:94e5:: with SMTP id 92mr7466125wrr.316.1595581701119; Fri, 24 Jul 2020 02:08:21 -0700 (PDT) Received: from localhost (host86-134-151-238.range86-134.btcentralplus.com. [86.134.151.238]) by smtp.gmail.com with ESMTPSA id p14sm576798wrg.96.2020.07.24.02.08.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Jul 2020 02:08:20 -0700 (PDT) Date: Fri, 24 Jul 2020 10:08:19 +0100 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Demangle function names when disassembling Message-ID: <20200724090819.GN853475@embecosm.com> References: <20200723161043.2191184-1-tromey@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200723161043.2191184-1-tromey@adacore.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 10:02:02 up 5 days, 18:16, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-9.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jul 2020 09:08:25 -0000 * Tom Tromey [2020-07-23 10:10:43 -0600]: > From: Andrew Burgess > > Andrew Burgess pointed out a regression, which he described in > PR symtab/26270: > > ================ > After commit: > > commit bcfe6157ca288efed127c5efe21ad7924e0d98cf (refs/bisect/bad) > Date: Fri Apr 24 15:35:01 2020 -0600 > > Use the linkage name if it exists > > The disassembler no longer demangles function names in its output. So > we see things like this: > > (gdb) disassemble tree_insert > Dump of assembler code for function _Z11tree_insertP4nodei: > .... > > Instead of this: > > (gdb) disassemble tree_insert > Dump of assembler code for function tree_insert(node*, int): > .... > > This is because find_pc_partial_function now returns the linkage name > rather than the demangled name. > ================ > > This patch fixes the problem by introducing a new "overload" of > find_pc_partial_function, which returns the general_symbol_info rather > than simply the name. This lets the disassemble command choose which > name to show. > > Regression tested on x86-64 Fedora 32. > > gdb/ChangeLog > 2020-07-23 Tom Tromey > > PR symtab/26270: > * symtab.h (find_pc_partial_function_sym): Declare. > * cli/cli-cmds.c (disassemble_command): Use > find_pc_partial_function_sym. Check asm_demangle. > * blockframe.c (cache_pc_function_sym): New global. > (cache_pc_function_name): Remove. > (clear_pc_function_cache): Update. > (find_pc_partial_function_sym): New function, from > find_pc_partial_function. > (find_pc_partial_function): Rewrite using > find_pc_partial_function_sym. > > gdb/testsuite/ChangeLog > 2020-07-23 Andrew Burgess > > PR symtab/26270: > * gdb.cp/disasm-func-name.cc: New file. > * gdb.cp/disasm-func-name.exp: New file. We discussed this patch on IRC and I made the observation that (for C++) even after this patch the behaviour of the disassemble command has changed from the 9.x releases of GDB. In 9.x a disassemble would show the demangled function name in the header line, while, even after this patch, the default is to display the mangled name. I wondered about changing the default for 'set print asm-demangle' to be on, then we'd maintain the same behaviour from 9.x through to 10.x. It turns out that the bug that originally caused GDB to display demangled names only applied for some languages, Ada, didn't have this issue, and so has always displayed the mangled names. As a result if I tried to "fix" the issue I reported in PR 26270 by changing the default for 'set print asm-demangle' then I'll "fix" C++, but change the default for Ada, which hardly seems fair. So, I'm posting this patch here as it might be of interest ... at least it feels like my notes should be available in case other people have the same questions I had. But I don't see how it can be merged in its current state. Thanks, Andrew --- commit c5935e59dc4e775615df21f92c97844886b0a427 Author: Andrew Burgess Date: Fri Jul 24 09:41:43 2020 +0100 gdb: Change 'set print asm-demangle' to an auto boolean, default on This commit relates to bug PR gdb/26270. When I raised this bug it was because I noticed, that when debugging a C++ application that the 'disassemble' command started displaying mangled names in its header line. This was a change from what I was used to. I tested GDB 7.6, and confirmed that for at least the last 6 years disassemble would display demangled names in the header line when disassembling C++ code. On further investigation it also turns out that 'info line' output also displays demangled names, and the symbols referenced in disassembly output are also demangled for the same time period. The reason this changed recently is that a long standing bug was fixed in GDB where (if I understand correctly) GDB was storing the demangled names where it should have been storing the mangled names, this made commands line 'set print demangle' and 'set print asm-demangle' not work, and gave us the behaviour I was seeing on the older GDBs. With this bug fix GDB now does what it "should" have all along. My problem here is that it feels like, if we've lived with things a particular way for 6 years, then when we change that behaviour it can seem confusing - it certainly confused me. So, one reason that we see things being printed as mangled is that though 'set print demangle' is on by default, 'set print asm-demangle' is off by default. So my initial thought was, lets make 'set print asm-demangle' be on by default. Then we've still fixed the bug in GDB (about storing the mangled vs demangled symbols), but the behaviour of GDB doesn't actually change (vs 9.x release). After a discussion on IRC the suggestion was made to have 'set print asm-demangle' be an auto-bool, then the default can be auto, which would defer to 'set print demangle', but the user can still take separate control if they want. So, that's what this patch does. Unfortunately there's some problems with this approach. First, and maybe the smallest problem, boolean variables have the "feature" that a user can just type 'set print asm-demangle', which is equivalent to 'set print asm-demangle on'. However, this doesn't work for auto-booleans, in that case you must type 'set print asm-demangle on'. This could possibly be worked around by overriding handling of the 'set print asm-demangle' command, but I haven't done that here. So this currently is a breaking change, though a pretty minor one I think. The biggest problem, and the blocker I think, is that under testing I found that the issues I listed above are, apparently, language specific. So for 6+ years GDB _for C++_ has been showing demangled names when it should have been displaying mangled ones, but for Ada, GDB has been doing the "right" thing. If I flip the default for 'asm-demangle' on the grounds that I don't think we should suddenly change GDB's behaviour then this would only be for C++, for Ada I would be suddenly changing the default. As a result I don't see any obvious way we could include this patch without changing GDB's behaviour for one language or another. What I did discover in my journey into GDB's demangling, is that these two options are a mess. The manual entry for 'set print demangle' and 'set print asm-demangle' are super unclear about which parts of GDB each are supposed to control. The comment on the controlling variable (asm_demangle) in the source code (gdb-demangle.h) suggest this variable should be overridden by 'demangle' if 'demangle' is set false, however, in some places 'asm_demangle' is completely independent, and in other places 'asm_demangle' actually overrides 'demangle' rather than deferring to it. gdb/ChangeLog: PR gdb/26270 * NEWS: Mention change in default for asm-demangle. * cli/cli-cmds.c (disassemble_command): Switch to use asm_demangle_p. * gdb-demangle.c (asm_demangle): Change type to auto bool, default is auto. (show_asm_demangle): Update to deal with new asm_demangle type. (asm_demangle_p): New function. (_initialize_gdb_demangle): Register 'set print asm-demangle' as an auto-boolean. * gdb-demangle.h (asm_demangle): Delete declaration. (asm_demangle_p): Declare. * printcmd.c (build_address_symbolic): Call asm gdb/doc/ChangeLog: PR gdb/26270 * gdb.texinfo (Print Settings): Note change in default for asm-demangle. Rewrite the @item line for this option. gdb/testsuite/ChangeLog: PR gdb/26270 * gdb.ada/win_fu_syms.exp: Update expected output. * gdb.base/default.exp: Update expected output. * gdb.cp/disasm-func-name.exp: Update for change in asm-demangle, add new tests. diff --git a/gdb/NEWS b/gdb/NEWS index ff0624ce6eb..f9e8b9c323a 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -25,6 +25,16 @@ You can get the latest version from https://sourceware.org/elfutils. +* 'set print asm-demangle' is now an auto boolean, meaning it can be + set to on, off, or auto. It's default is now auto, which means it + defers to the value of 'set print demangle'. For the last 6 years + GDB has contained a bug that, for some languages, caused 'set print + asm-demangle' to not function correctly, and effectively always be + on. This bug in GDB has now been fixed, but this means that for the + effected languages GDB's behaviour has now changed. In order to + avoid confusing users, by changing the default for this flag we + maintain the behaviour GDB has had for the last 6 years. + * New features in the GDB remote stub, GDBserver ** GDBserver is now supported on RISC-V GNU/Linux. diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 07b119038d2..05feaa79bf7 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -1541,7 +1541,7 @@ disassemble_command (const char *arg, int from_tty) if (!find_pc_partial_function_sym (pc, &symbol, &low, &high, &block)) error (_("No function contains specified address.")); - if (asm_demangle) + if (asm_demangle_p ()) name = symbol->print_name (); else name = symbol->linkage_name (); diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 1b9f76573b9..b9c810d2299 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -11556,11 +11556,16 @@ @item show print demangle Show whether C@t{++} names are printed in mangled or demangled form. -@item set print asm-demangle -@itemx set print asm-demangle on -Print C@t{++} names in their source form rather than their mangled form, even -in assembler code printouts such as instruction disassemblies. -The default is off. +@item set print asm-demangle [auto|on|off] +Print C@t{++} names in their source form rather than their mangled +form, even in assembler code printouts such as instruction +disassemblies. This flag interacts differently to @code{set print +demangle} in different parts of @value{GDBN}, in some places this flag +is independent to @code{set print demangle}, and in other places this +flag will override the setting in @code{set print demangle}. + +The default is auto, which means this flag defers to @code{set print +demangle}. @item show print asm-demangle Show whether C@t{++} names in assembly listings are printed in mangled diff --git a/gdb/gdb-demangle.c b/gdb/gdb-demangle.c index 6a664e99bfc..57fbe4af8f4 100644 --- a/gdb/gdb-demangle.c +++ b/gdb/gdb-demangle.c @@ -44,6 +44,7 @@ #endif /* See documentation in gdb-demangle.h. */ + bool demangle = true; static void @@ -57,16 +58,35 @@ show_demangle (struct ui_file *file, int from_tty, } /* See documentation in gdb-demangle.h. */ -bool asm_demangle = false; +static enum auto_boolean asm_demangle = AUTO_BOOLEAN_AUTO; static void show_asm_demangle (struct ui_file *file, int from_tty, struct cmd_list_element *c, const char *value) { - fprintf_filtered (file, - _("Demangling of C++/ObjC names in " - "disassembly listings is %s.\n"), - value); + if (asm_demangle == AUTO_BOOLEAN_AUTO) + fprintf_filtered (file, + _("Demangling of C++/ObjC names in disassembly " + "listings is \"%s\" (currently \"%s\").\n"), + value, (demangle ? "on" : "off")); + else if (asm_demangle == AUTO_BOOLEAN_TRUE && !demangle) + fprintf_filtered (file, + _("Demangling of C++/ObjC names in disassembly " + "listings is \"%s\", but is overridden by " + "'set print demangle' which is \"off\".\n"), + value); + else + fprintf_filtered (file, + _("Demangling of C++/ObjC names in disassembly " + "listings is \"%s\"."), value); +} + +/* See gdb-demangle.h. */ + +bool +asm_demangle_p () +{ + return demangle && (asm_demangle != AUTO_BOOLEAN_FALSE); } /* String name for the current demangling style. Set by the @@ -244,7 +264,7 @@ Show demangling of encoded C++/ObjC names when displaying symbols."), NULL, show_demangle, &setprintlist, &showprintlist); - add_setshow_boolean_cmd ("asm-demangle", class_support, &asm_demangle, _("\ + add_setshow_auto_boolean_cmd ("asm-demangle", class_support, &asm_demangle, _("\ Set demangling of C++/ObjC names in disassembly listings."), _("\ Show demangling of C++/ObjC names in disassembly listings."), NULL, NULL, diff --git a/gdb/gdb-demangle.h b/gdb/gdb-demangle.h index f159cb130a2..a454a176445 100644 --- a/gdb/gdb-demangle.h +++ b/gdb/gdb-demangle.h @@ -23,10 +23,13 @@ C++/ObjC form rather than raw. */ extern bool demangle; -/* True means that encoded C++/ObjC names should be printed out in their - C++/ObjC form even in assembler language displays. If this is set, but - DEMANGLE is false, names are printed raw, i.e. DEMANGLE controls. */ -extern bool asm_demangle; +/* Return true if symbols in assembler listing should be demangled. The + return value depends on the value of both 'set print asm-demangle' and + 'set print demangle'. If 'set print demangle' is off then this will + always return true. If 'set print asm-demangle' is set to auto then + this will mirror the value in 'set print demangle'. */ + +extern bool asm_demangle_p (); /* Check if a character is one of the commonly used C++ marker characters. */ extern bool is_cplus_marker (int); diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 309d2cabfff..87020e465ba 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -623,7 +623,7 @@ build_address_symbolic (struct gdbarch *gdbarch, addr = gdbarch_addr_bits_remove (gdbarch, addr); name_location = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (symbol)); - if (do_demangle || asm_demangle) + if (do_demangle || asm_demangle_p ()) name_temp = symbol->print_name (); else name_temp = symbol->linkage_name (); @@ -666,7 +666,7 @@ build_address_symbolic (struct gdbarch *gdbarch, symbol = 0; name_location = BMSYMBOL_VALUE_ADDRESS (msymbol); - if (do_demangle || asm_demangle) + if (do_demangle || asm_demangle_p ()) name_temp = msymbol.minsym->print_name (); else name_temp = msymbol.minsym->linkage_name (); @@ -715,7 +715,7 @@ print_address (struct gdbarch *gdbarch, CORE_ADDR addr, struct ui_file *stream) { fputs_styled (paddress (gdbarch, addr), address_style.style (), stream); - print_address_symbolic (gdbarch, addr, stream, asm_demangle, " "); + print_address_symbolic (gdbarch, addr, stream, asm_demangle_p (), " "); } /* Return a prefix for instruction address: diff --git a/gdb/testsuite/gdb.ada/win_fu_syms.exp b/gdb/testsuite/gdb.ada/win_fu_syms.exp index f27b0a82ab6..8447b28a932 100644 --- a/gdb/testsuite/gdb.ada/win_fu_syms.exp +++ b/gdb/testsuite/gdb.ada/win_fu_syms.exp @@ -25,11 +25,11 @@ clean_restart ${testfile} set loc [gdb_get_line_number "Integer" ${testdir}/foo.adb] gdb_test "info line foo.adb:$loc" \ - "Line $decimal of \".*foo\\.adb\" starts at address $hex <_ada_foo\\+$decimal> and ends at $hex <_ada_foo\\+$decimal>\\." \ + "Line $decimal of \".*foo\\.adb\" starts at address $hex and ends at $hex \\." \ "info line on variable declaration" set loc [gdb_get_line_number "Do_Nothing" ${testdir}/foo.adb] gdb_test "info line foo.adb:$loc" \ - "Line $decimal of \".*foo\\.adb\" starts at address $hex <_ada_foo\\+$decimal> and ends at $hex <_ada_foo\\+$decimal>\\." \ + "Line $decimal of \".*foo\\.adb\" starts at address $hex and ends at $hex \\." \ "info line on Do_Nothing call" diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp index ac1a0f5b6e6..ab4b5702a54 100644 --- a/gdb/testsuite/gdb.base/default.exp +++ b/gdb/testsuite/gdb.base/default.exp @@ -521,7 +521,9 @@ gdb_test_no_output "set print address" "set print address" #test set print array gdb_test_no_output "set print array" "set print array" #test set print asm-demangle -gdb_test_no_output "set print asm-demangle" "set print asm-demangle" +gdb_test "set print asm-demangle" \ + "\"on\", \"off\" or \"auto\" expected\\." \ + "set print asm-demangle" #test set print demangle gdb_test_no_output "set print demangle" "set print demangle" #test set print elements @@ -663,7 +665,7 @@ gdb_test "show print address" "Printing of addresses is on." #test show print array gdb_test "show print array" "Pretty formatting of arrays is on." #test show print asm-demangle -gdb_test "show print asm-demangle" "Demangling of C\[+\]+/ObjC names in disassembly listings is on." +gdb_test "show print asm-demangle" "Demangling of C\[+\]+/ObjC names in disassembly listings is \"auto\" \\(currently \"on\"\\)\\." #test show print demangle gdb_test "show print demangle" "Demangling of encoded C\[+\]+/ObjC names when displaying symbols is on." #test show print elements diff --git a/gdb/testsuite/gdb.cp/disasm-func-name.exp b/gdb/testsuite/gdb.cp/disasm-func-name.exp index 3fb63c89a28..2d4cdeb8768 100644 --- a/gdb/testsuite/gdb.cp/disasm-func-name.exp +++ b/gdb/testsuite/gdb.cp/disasm-func-name.exp @@ -41,10 +41,20 @@ proc check_disassembly_header { request expected } { "Dump of assembler code for function ${expected}:\r\n.*" } -gdb_test_no_output "set print asm-demangle on" +with_test_prefix "asm-demangle=on" { + check_disassembly_header "main" "main\\(\\)" + check_disassembly_header "process" "process\\(A\\*, int\\)" + check_disassembly_header "A::A" "A::A\\(int\\)" + check_disassembly_header "A::get_i" "A::get_i\\(\\) const" + check_disassembly_header "A::set_i" "A::set_i\\(int\\)" +} + +gdb_test_no_output "set print asm-demangle off" -check_disassembly_header "main" "main\\(\\)" -check_disassembly_header "process" "process\\(A\\*, int\\)" -check_disassembly_header "A::A" "A::A\\(int\\)" -check_disassembly_header "A::get_i" "A::get_i\\(\\) const" -check_disassembly_header "A::set_i" "A::set_i\\(int\\)" +with_test_prefix "asm-demangle=off" { + check_disassembly_header "main" "main\\(\\)" + check_disassembly_header "process" "\[^()\]+" + check_disassembly_header "A::A" "\[^()\]+" + check_disassembly_header "A::get_i" "\[^()\]+" + check_disassembly_header "A::set_i" "\[^()\]+" +}