* The future of dwarf2_physname
@ 2011-05-18 22:35 Keith Seitz
2011-05-19 19:23 ` Jan Kratochvil
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Keith Seitz @ 2011-05-18 22:35 UTC (permalink / raw)
To: gdb-patches
Hi,
Several of us Red Hat folk have been discussing this internally, and
it's time to take this discussion public.
Jan's recent patch proposal in response to my typedef'd method
parameters patchset lead to some further investigation, and I believe I
(we?) are all coming to the conclusion that Jan's patch is probably the
best way to proceed for the future, i.e., post 7.3 release.
[Jan's proposal is here:
http://sourceware.org/ml/gdb-patches/2011-05/msg00358.html . NOTE:
DMGL_VERBOSE should be added to the demangler options in that patch if
you want to play with it.]
In short, if gdb uses DW_AT[_MIPS]_linkage_name when available, we can
bypass constructing the physname *and* canonicalization, so gdb would
only have to do this when DW_AT[_MIPS]_linkage_name is missing. Today
this only happens AFAIK with gcc and ctors. I don't know about other
compilers.
This could be a sizable performance win (not yet tested or quantified).
While it does introduce some regressions, many are compiler problems and
some are template problems (the linkage name contains the return type,
and 1) gdb isn't equipped to deal with that; 2) it makes for a horrible
user experience having to specify it all the time). There might be
others, I have only delved into this a bit myself today.
So the first question to be asked here is, Should we adopt Jan's patch,
or some variation of it, to use DW_AT_linkage_name when available?
The only hesitation I have is the compiler issues. Just using
DW_AT_linkage_name instead of computing it has lead to many test
regressions with differing compiler versions. [Again, I have only tested
gcc.]
It appears that computing the physname offers gdb some buffer from
compiler changes/bugs. At least with gcc. [in case I haven't stressed
that enough]
What do others think?
------
The bigger issue is what to do for 7.3, which has been in limbo for
quite some time awaiting resolution of c++/12506 (for which I have
posted patches).
I guess there are basically two options:
1) Keep physname & apply the 12266/12506 patchset (when approved)
2) Adopt Jan's patch and fix the fallout now
My take on this today is that I think the least risky/most timely
solution is #1. Gdb has been used with the dwarf2_physname patchset for
over a year now, and it is not altogether broken. We are seeing some
regressions from previous releases (none of these are in the test
suite), and I have been trying to keep on top of them.
The 12266/12506 patchset is almost orthogonal to this decision. It fixes
several additional typedef-related problems. 12506 is fixed by using
DW_AT_linkage_name, but 12266 is not.
My big fear with #2 is "fixing" linespecs (yet again). Any changes to
this area take an unimaginably large amount of time and effort.
What do others think?
Keith
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: The future of dwarf2_physname 2011-05-18 22:35 The future of dwarf2_physname Keith Seitz @ 2011-05-19 19:23 ` Jan Kratochvil 2011-05-20 19:53 ` Keith Seitz 2011-05-19 21:00 ` Daniel Jacobowitz ` (2 subsequent siblings) 3 siblings, 1 reply; 24+ messages in thread From: Jan Kratochvil @ 2011-05-19 19:23 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches Hi Keith, On Thu, 19 May 2011 00:34:43 +0200, Keith Seitz wrote: [...] > It appears that computing the physname offers gdb some buffer from > compiler changes/bugs. At least with gcc. I find any difference from compiler's idea of a (demangled) linkage name to be a bug. When nm, objdump, other tools deal with some C++ name I believe GDB also should. If the linkage name is wrong for whatever reason this is an ABI issue for GCC, it is out of scope of GDB. Linkage name is defined by the ISO C++ standard, neither GCC nor GDB can change it. If GCC is not ISO C++ compliant then GCC should be fixed (and possibly deal with the ABI breakage) but such ISO C++ compliance issue is out of scope of GDB. Trying to introduce new "better" naming for C++ methods which is in 90% the same as GCC's and ISO C++'s idea seems a bit messy for me. That is the GCC and ISO C++ naming should be always available in GDB. I am not against allowing also other aliases for the same symbol. The availability of symbol aliases is discussed more at the end of this mail. As parts of code have DWARF (=full symbols) and part only ELF .symtab (=minimal symbols) the physname code (creating linkage names based on DWARF DIEs) just cannot apply to ELF symbols. Thefore if it creates different physname from DWARF than what can be figured out from the library. #include <sstream> std::stringstream v; int main () { v.unsetf (std::ios_base::hex); } x86_64-fedora15-linux-gnu with no /usr/lib/debug GDB with Re: [RFA] Typedef'd method parameters [0/4] http://sourceware.org/ml/gdb-patches/2011-05/msg00318.html without my patch [rfc] physname cross-check [Re: [RFA] Typedef'd method parameters [0/4]] http://sourceware.org/ml/gdb-patches/2011-05/msg00358.html (gdb) complete p 'std::ios_base::unsetf p 'std::ios_base::unsetf(std::_Ios_Fmtflags) p 'std::ios_base::unsetf(std::ios_base::fmtflags) (gdb) p 'std::ios_base::unsetf(std::_Ios_Fmtflags)' $2 = {<text variable, no debug info>} 0x400768 <std::ios_base::unsetf(std::ios_base::fmtflags)> (gdb) ptype 'std::ios_base::unsetf(std::_Ios_Fmtflags)' type = int (void) (gdb) p 'std::ios_base::unsetf(std::ios_base::fmtflags)' No symbol "std::ios_base::unsetf(std::ios_base::fmtflags)" in current context. (gdb) ptype 'std::ios_base::unsetf(std::ios_base::fmtflags)' No symbol "std::ios_base::unsetf(std::ios_base::fmtflags)" in current context. while pre-physname GDB: (gdb) complete p 'std::ios_base::unsetf p 'std::ios_base::unsetf(std::_Ios_Fmtflags) (gdb) p 'std::ios_base::unsetf(std::_Ios_Fmtflags)' $1 = {void (class std::ios_base * const, std::ios_base::fmtflags)} 0x400768 <std::ios_base::unsetf(std::_Ios_Fmtflags)> (gdb) ptype 'std::ios_base::unsetf(std::_Ios_Fmtflags)' type = void (class std::ios_base * const, std::ios_base::fmtflags) This is a regression. And it will always be a regression for any physname != DW_AT_linkage_name as with my cross-check patch it prints: Computed physname <std::ios_base::unsetf(enum std::_Ios_Fmtflags)> does not match demangled <std::ios_base::unsetf(std::_Ios_Fmtflags)> (from linkage <_ZNSt8ios_base6unsetfESt13_Ios_Fmtflags>) The idea is simple - the compiled application with DWARF will contain: <1><135>: Abbrev Number: 17 (DW_TAG_namespace) <136> DW_AT_name : std <2><44a>: Abbrev Number: 23 (DW_TAG_class_type) <44b> DW_AT_name : ios_base <3><454>: Abbrev Number: 24 (DW_TAG_subprogram) <455> DW_AT_external : 1 <456> DW_AT_name : unsetf <45a> DW_AT_decl_file : 1 <45b> DW_AT_decl_line : 612 <45d> DW_AT_MIPS_linkage_name: _ZNSt8ios_base6unsetfESt13_Ios_Fmtflags <461> DW_AT_accessibility: 1 (public) <462> DW_AT_declaration : 1 <463> DW_AT_object_pointer: <0x46b> <4><46b>: Abbrev Number: 25 (DW_TAG_formal_parameter) <46c> DW_AT_type : <0x18bb> <470> DW_AT_artificial : 1 <4><471>: Abbrev Number: 26 (DW_TAG_formal_parameter) <472> DW_AT_type : <0x476> <4><476>: Abbrev Number: 8 (DW_TAG_typedef) <477> DW_AT_name : fmtflags <47b> DW_AT_decl_file : 1 <47c> DW_AT_decl_line : 257 <47e> DW_AT_type : <0x32e> It does not (currently cannot - discussed at GCC PR debug/40040) contain DW_AT_low_pc+DW_AT_high_pc for std::io_base::unsetf as it is DW_AT_external. Therefore the linkage name (physname) which GDB thinks about must be always exactly DW_AT_linkage_name (*) (**). (*) As long as the GCC produced DW_AT_MIPS_linkage_name matches the .symtab ELF symbol name but there have never been bugs in this part in GCC AFAIK. (**) DW_AT_MIPS_linkage_name always equals DW_AT_linkage_name. So physname must be always the same as DW_AT_linkage_name. Even if GCC computes DW_AT_linkage_name wrongly then GDB has to be bug-to-bug compatible with that GCC. I found the only benefits of physname to reduce the debuginfo size by omitting DW_AT_linkage_name from DWARF as it is redundant. But in such case there should be a verification of GCC-output DW_AT_linkage_name names if they really match to what GDB thinkgs they should be. That should done during distro build of every package before stripping DW_AT_linkage_name. A rough guess is saving something between 3% to 10% of .debug files size which may be worth it with all the .debug size reduction efforts. (As long as other DWARF consumers do not need DW_AT_linkage_name but that is a bit out of scope of this discussion, currently I am not aware of any.) > The bigger issue is what to do for 7.3, which has been in limbo for > quite some time awaiting resolution of c++/12506 (for which I have > posted patches). > > I guess there are basically two options: > 1) Keep physname & apply the 12266/12506 patchset (when approved) > 2) Adopt Jan's patch and fix the fallout now > > My take on this today is that I think the least risky/most timely > solution is #1. As it introduces symbol names which are not compliant with ISO C++ I find it harmful to get people used for the new names in any new release. So far the non-compliant symbol names are present only in FSF gdb-7.2 which could be forgotten IMO but releasing another stable FSF GDB release with non-compliant symbol names seems to me making backward compatibility difficult. Still it is questionable whether the GDB-only symbol variants can create any compatibility issues - such as some .gdbinit scripts using such names. > Gdb has been used with the dwarf2_physname patchset > for over a year now, and it is not altogether broken. We are seeing > some regressions from previous releases (none of these are in the > test suite), and I have been trying to keep on top of them. There are some regressions when using DW_AT_linkage_name by that patch of mine but according to quick analysis IIUC they seem to be caused by coding the testcases based on the ISO C++ non-compliant naming introduced by physname. Therefore these should be fixed in the testcases. But these regressions need to be (attempted to be) fixed first to get definitive answer. > The 12266/12506 patchset is almost orthogonal to this decision. It > fixes several additional typedef-related problems. 12506 is fixed by > using DW_AT_linkage_name, but 12266 is not. 12266 is about `break func(typedefname)' which never worked before physname (before gdb-7.2). This is about providing canonicalization in linespec which you have implemented in the current pending patchset. decode_variable->cp_canonicalize_string_no_typedefs in: Re: [RFA] Typedef'd method parameters [3/4] http://sourceware.org/ml/gdb-patches/2011-05/msg00317.html typedef int t; void f(t p) {} int main () {} (gdb) b f(t) Breakpoint 1 at 0x4004db: file 4.C, line 1. This is a kind of "symbol alias" and I do not find it much critical if it does not exactly match in some cases - it is just user convenience and it does not break any symbol references used in the inferior code. > The only hesitation I have is the compiler issues. Just using > DW_AT_linkage_name instead of computing it has lead to many test > regressions with differing compiler versions. [Again, I have only > tested gcc.] + > My big fear with #2 is "fixing" linespecs (yet again). Any changes > to this area take an unimaginably large amount of time and effort. I do not see any real regressions except incorrect testcase assumptions. $ nm -C gdb.cp/cp-relocate.o 0000000000000000 W int func<1>(int) print func<1>(int) No symbol "func<1>" in current context. (gdb) FAIL: gdb.cp/cp-relocate.exp: get address of func<1>(int) but: (gdb) print 'int func<1>(int)' $1 = {int (int)} 0x28 <func<1>(int)> This is a regression against gdb-7.2 (physname) but in pre-physname it also did not work. The symbols canonicalization in decode_variable could also strip the leading return types. Thanks, Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-19 19:23 ` Jan Kratochvil @ 2011-05-20 19:53 ` Keith Seitz 2011-05-20 20:38 ` Jan Kratochvil 2011-05-20 20:39 ` Tom Tromey 0 siblings, 2 replies; 24+ messages in thread From: Keith Seitz @ 2011-05-20 19:53 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches Hi, Jan, On 05/19/2011 12:23 PM, Jan Kratochvil wrote: > If the linkage name is wrong for whatever reason this is an ABI issue for GCC, > it is out of scope of GDB. Linkage name is defined by the ISO C++ standard, > neither GCC nor GDB can change it. If GCC is not ISO C++ compliant then GCC > should be fixed (and possibly deal with the ABI breakage) but such ISO C++ > compliance issue is out of scope of GDB. We work around GCC ABI issues all the time GDB. dwarf2read.cc is riddled with workarounds for various flavors of GCC. > Trying to introduce new "better" naming for C++ methods which is in 90% the > same as GCC's and ISO C++'s idea seems a bit messy for me. If you mean "better" as in "more reliable", then I'm afraid that *was* the point. I don't like it, either, TBH, because there's a bunch of overhead involved. But it seemed like a reasonable thing to do at the time, and it still does, especially if you take GCC out of the discussion. I wouldn't say that DW_AT_MIPS_linkage_name is any more ISO C++ compliant than anything else. Just because the compiler outputs it does not necessarily make it sacrosanct. [gcc/33861] > That is the GCC and ISO C++ naming should be always available in GDB. I am > not against allowing also other aliases for the same symbol. The availability > of symbol aliases is discussed more at the end of this mail. I agree. > This is a regression. And it will always be a regression for any > physname != DW_AT_linkage_name as with my cross-check patch it prints: > Computed physname<std::ios_base::unsetf(enum std::_Ios_Fmtflags)> does not match demangled<std::ios_base::unsetf(std::_Ios_Fmtflags)> (from linkage<_ZNSt8ios_base6unsetfESt13_Ios_Fmtflags>) That's simply a bug. They get found, they get fixed. The sky is not falling. > I do not see any real regressions except incorrect testcase assumptions. Unless the assumption is that gdb can only set breakpoints on linkage names, I don't call the test cases broken or illegal. If your argument is that you don't like the way it was solved, that's an entirely different assertion, and one with much more merit. > $ nm -C gdb.cp/cp-relocate.o > 0000000000000000 W int func<1>(int) > print func<1>(int) > No symbol "func<1>" in current context. > (gdb) FAIL: gdb.cp/cp-relocate.exp: get address of func<1>(int) > > but: > (gdb) print 'int func<1>(int)' > $1 = {int (int)} 0x28<func<1>(int)> > > This is a regression against gdb-7.2 (physname) but in pre-physname it also > did not work. The symbols canonicalization in decode_variable could also > strip the leading return types. Another bug slipped in. It, too, can be fixed. [That looks like a psymtab-related bug, btw.] But as I keep saying, I am not a maintainer. This is a decision for maintainers. I was simply asking how maintainers wanted me to proceed. Two maintainers believe that making the switch now is the best approach. As far as I am concerned, the matter is closed. Keith ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-20 19:53 ` Keith Seitz @ 2011-05-20 20:38 ` Jan Kratochvil 2011-05-20 20:39 ` Tom Tromey 1 sibling, 0 replies; 24+ messages in thread From: Jan Kratochvil @ 2011-05-20 20:38 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches Hi Keith, On Fri, 20 May 2011 21:53:24 +0200, Keith Seitz wrote: > I wouldn't say that DW_AT_MIPS_linkage_name is any more ISO C++ > compliant than anything else. We still do not get to the agreement. The primary source of addresses are ELF symbols. DW_AT_low_pc+DW_AT_high_pc are ignored in many cases. (In other cases DW_AT_low_pc+DW_AT_high_pc is the only available address we have but that is offtopic here.) If you find different linkage name than DW_AT_MIPS_linkage_name then you will not find the ELF symbol and therefore GDB will fail to find the function, as I already reproduced on the sample code I provided. > Just because the compiler outputs it > does not necessarily make it sacrosanct. [gcc/33861] DW_AT_MIPS_linkage_name is fortunately or unfortunately always right because it is used for the ELF symbol. > >This is a regression. And it will always be a regression for any > >physname != DW_AT_linkage_name as with my cross-check patch it prints: > >Computed physname<std::ios_base::unsetf(enum std::_Ios_Fmtflags)> does not match demangled<std::ios_base::unsetf(std::_Ios_Fmtflags)> (from linkage<_ZNSt8ios_base6unsetfESt13_Ios_Fmtflags>) > > That's simply a bug. They get found, they get fixed. The sky is not falling. The sky has fallen as without that patch of mine GDB failed to find the function as I have shown before: Message-ID: <20110516154851.GA24555@host1.jankratochvil.net> During symbol reading, Computed physname <C::m(struct x *)> does not match demangled <C::m(x*)> (from linkage <_ZN1C1mEP1x>) - DIE at 0x3d [in module .../1]. (gdb) p c.m pre-phyname: $2 = {void (C *, x *)} 0x4004f0 <C::m(x*)> HEAD + your patch: Cannot take address of method m. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is a regression. Until we find agreement on this point the mails do not make sense. And if you fix the specific `struct' physname problem I can find many other problems as long as any "Computed physname ... does not match demangled" message gets printed. After you fix all of the problems you will reach the functionality of my patch preferring DW_AT_MIPS_linkage_name and we can continue the discussion. > >I do not see any real regressions except incorrect testcase assumptions. > > Unless the assumption is that gdb can only set breakpoints on > linkage names, Yes. You do not always have DWARF for very every part of the program and all of its libraries, you need to stay compatible with ELF .symtab naming. > If your argument is that you don't like the way it was solved, that's an > entirely different assertion, and one with much more merit. I really do not mind which way to solve it, I would never work on GDB if I mind anything like it. But it must be able to print and break very every function in at least libstdc++so.6 and something big like /usr/lib/debug/usr/lib64/libwebkitgtk-1.0.so.debug . There are about 10000 failed lookups on it now. > Another bug slipped in. It, too, can be fixed. [That looks like a > psymtab-related bug, btw.] Yes, you can be fixing it all the following months. Or we can use DW_AT_linkage_name till the time you fix it. We need to look verify your fixes against DW_AT_linkage_name anyway. > Two maintainers believe that making the switch now is the best > approach. As far as I am concerned, the matter is closed. The problem is physname introduced some new feature - like it introduced DMGL_RET_POSTFIX-like functionality. If we go the standard demangling way we regress a bit against gdb-7.2 (but not against gdb-7.1). So I am trying to find a way to regress neither against gdb-7.1 nor against gdb-7.2. Thanks, Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-20 19:53 ` Keith Seitz 2011-05-20 20:38 ` Jan Kratochvil @ 2011-05-20 20:39 ` Tom Tromey 2011-05-21 20:37 ` Daniel Jacobowitz 1 sibling, 1 reply; 24+ messages in thread From: Tom Tromey @ 2011-05-20 20:39 UTC (permalink / raw) To: Keith Seitz; +Cc: Jan Kratochvil, gdb-patches Keith> I wouldn't say that DW_AT_MIPS_linkage_name is any more ISO C++ Keith> compliant than anything else. Just because the compiler outputs it Keith> does not necessarily make it sacrosanct. [gcc/33861] I think that bug shows the opposite, actually. If you look at the DWARF: <1><25>: Abbrev Number: 2 (DW_TAG_structure_type) <26> DW_AT_name : (indirect string, offset: 0x77): Qux<((char*)(& name))> That is, the bug here is that G++ emits a bad and weird DW_AT_name. But, if you look at the member function: <2><31>: Abbrev Number: 3 (DW_TAG_subprogram) <32> DW_AT_external : 1 <33> DW_AT_name : foo <37> DW_AT_decl_file : 1 <38> DW_AT_decl_line : 7 <39> DW_AT_MIPS_linkage_name: (indirect string, offset: 0x5a): _ZN3QuxIXadL_Z4nameEEE3fooEv .. run through c++filt: <39> DW_AT_MIPS_linkage_name: (indirect string, offset: 0x5a): Qux<&name>::foo() Here, the linkage name and demangler are correct. Also even if you strip down the class' DW_AT_name, you probably have trouble reconstructing the template argument: <2><51>: Abbrev Number: 5 (DW_TAG_template_value_param) <52> DW_AT_name : V <54> DW_AT_type : <0xaf> <58> DW_AT_location : 6 byte block: 3 0 0 0 0 9f (DW_OP_addr: 0; DW_OP_stack_value) ... since that is just the address of 'name', which IIUC may appear elsewhere, perhaps without debuginfo. I guess it ought to always appear in minimal symbols in that case... maybe it isn't as bad as I thought. I get this from my patch branch: opsy. ../gdb -readnow -batch -ex 'set complaint 10000' -ex 'file /tmp/q.o' During symbol reading...Computed physname <Qux<(char*)(&name)>::foo()> does not match demangled <Qux<&name>::foo()> (from linkage <_ZN3QuxIXadL_Z4nameEEE3fooEv>) - DIE at 0x31 [in module /tmp/q.o]... Jan> That is the GCC and ISO C++ naming should be always available in Jan> GDB. I am not against allowing also other aliases for the same Jan> symbol. The availability of symbol aliases is discussed more at Jan> the end of this mail. Keith> I agree. Great, we have a starting point :) Jan> I do not see any real regressions except incorrect testcase assumptions. Keith> Unless the assumption is that gdb can only set breakpoints on linkage Keith> names, I don't call the test cases broken or illegal. If your argument Keith> is that you don't like the way it was solved, that's an entirely Keith> different assertion, and one with much more merit. Yes, I actually agree with both of you, in the sense that I think both sets of tests ought to pass. Tom ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-20 20:39 ` Tom Tromey @ 2011-05-21 20:37 ` Daniel Jacobowitz 0 siblings, 0 replies; 24+ messages in thread From: Daniel Jacobowitz @ 2011-05-21 20:37 UTC (permalink / raw) To: Tom Tromey; +Cc: Keith Seitz, Jan Kratochvil, gdb-patches On Fri, May 20, 2011 at 02:38:51PM -0600, Tom Tromey wrote: > Also even if you strip down the class' DW_AT_name, you probably have > trouble reconstructing the template argument: > > <2><51>: Abbrev Number: 5 (DW_TAG_template_value_param) > <52> DW_AT_name : V > <54> DW_AT_type : <0xaf> > <58> DW_AT_location : 6 byte block: 3 0 0 0 0 9f (DW_OP_addr: 0; DW_OP_stack_value) > > ... since that is just the address of 'name', which IIUC may appear > elsewhere, perhaps without debuginfo. I guess it ought to always appear > in minimal symbols in that case... maybe it isn't as bad as I thought. You can never make this conversion from address to name reliably; e.g. __attribute__((alias)), ICF [Identical Code Folding] in the linker, et cetera. > > > I get this from my patch branch: > > opsy. ../gdb -readnow -batch -ex 'set complaint 10000' -ex 'file /tmp/q.o' > During symbol reading...Computed physname <Qux<(char*)(&name)>::foo()> does not match demangled <Qux<&name>::foo()> (from linkage <_ZN3QuxIXadL_Z4nameEEE3fooEv>) - DIE at 0x31 [in module /tmp/q.o]... What I think would be extraordinarily useful would be to put this feature together with GCC's quality testsuite and actually get all the names right... but that really begs the question of which component we're testing here and where the test should live! -- Daniel Jacobowitz ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-18 22:35 The future of dwarf2_physname Keith Seitz 2011-05-19 19:23 ` Jan Kratochvil @ 2011-05-19 21:00 ` Daniel Jacobowitz 2011-05-20 19:26 ` Tom Tromey 2011-05-20 19:10 ` Tom Tromey 2011-05-23 13:17 ` Jan Kratochvil 3 siblings, 1 reply; 24+ messages in thread From: Daniel Jacobowitz @ 2011-05-19 21:00 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches On Wed, May 18, 2011 at 03:34:43PM -0700, Keith Seitz wrote: > In short, if gdb uses DW_AT[_MIPS]_linkage_name when available, we > can bypass constructing the physname *and* canonicalization, so gdb > would only have to do this when DW_AT[_MIPS]_linkage_name is missing. > Today this only happens AFAIK with gcc and ctors. I don't know about > other compilers. For all intents and purposes, no other current compiler emits DW_AT_MIPS_linkage_name. > This could be a sizable performance win (not yet tested or > quantified). While it does introduce some regressions, many are > compiler problems and some are template problems (the linkage name > contains the return type, and 1) gdb isn't equipped to deal with > that; 2) it makes for a horrible user experience having to specify it > all the time). There might be others, I have only delved into this a > bit myself today. > > So the first question to be asked here is, Should we adopt Jan's > patch, or some variation of it, to use DW_AT_linkage_name when > available? I've spent a lot of time on this question. It's similar to the approach I took in my previous implementation, but I was never happy with it. You get bizarre differences in behavior between compilers (although the same thing does admittedly happen just based on the DWARF output). GCC ideally ought to get away from outputting linkage names; it's a big space cost in the debug information. That offers a potential tie-breaker between the two strategies; keep using the linkage names for old versions of GCC, and transition away as GCC improves. In general, the sort of cross-check Jan has implemented could never reliably pass without extensions to DWARF. Once you get up into templates, especially, there are template arguments that are hard or impossible to represent in template parameter DIEs. -- Daniel Jacobowitz ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-19 21:00 ` Daniel Jacobowitz @ 2011-05-20 19:26 ` Tom Tromey 2011-05-21 20:34 ` Daniel Jacobowitz 0 siblings, 1 reply; 24+ messages in thread From: Tom Tromey @ 2011-05-20 19:26 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes: Daniel> In general, the sort of cross-check Jan has implemented could never Daniel> reliably pass without extensions to DWARF. Once you get up into Daniel> templates, especially, there are template arguments that are hard Daniel> or impossible to represent in template parameter DIEs. DWARF 4 added some stuff to help with this, but I think g++ hasn't yet caught up. What problems do you know about? I know of this: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41736 ... namely, that g++ doesn't emit a value for a pointer-to-member template parameter. But this is a g++ bug, not a DWARF omission. Also there is: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33861 I don't think I totally understand the issues with this one, but the DWARF just has an address, so presumably in some scenarios we can wind up with an odd canonical name (if we can't find the name corresponding to that address). It seems to me that if the DWARF spec is incomplete, then that is an argument in favor of either (1) fixing DWARF and g++ or (2) keeping DW_AT_linkage_name in g++ -- but not just dropping linkage-name, as that would cause user-visible regressions. Tom ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-20 19:26 ` Tom Tromey @ 2011-05-21 20:34 ` Daniel Jacobowitz 0 siblings, 0 replies; 24+ messages in thread From: Daniel Jacobowitz @ 2011-05-21 20:34 UTC (permalink / raw) To: Tom Tromey; +Cc: Keith Seitz, gdb-patches On Fri, May 20, 2011 at 01:25:49PM -0600, Tom Tromey wrote: > DWARF 4 added some stuff to help with this, but I think g++ hasn't yet > caught up. What problems do you know about? It's been five years since I was really working on this, but this... > Also there is: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33861 > > I don't think I totally understand the issues with this one, but the > DWARF just has an address, so presumably in some scenarios we can wind > up with an odd canonical name (if we can't find the name corresponding > to that address). ... is a pretty good example (I don't think it's the only kind, but I don't have anything more to hand). You can't reliably go from an address to a name and get the same thing the compiler got. I think there's a way (for function templates? parameters to templated functions?) to get floating point constants mangled, too. > It seems to me that if the DWARF spec is incomplete, then that is an > argument in favor of either (1) fixing DWARF and g++ or (2) keeping > DW_AT_linkage_name in g++ -- but not just dropping linkage-name, as that > would cause user-visible regressions. Completely agree. -- Daniel Jacobowitz ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-18 22:35 The future of dwarf2_physname Keith Seitz 2011-05-19 19:23 ` Jan Kratochvil 2011-05-19 21:00 ` Daniel Jacobowitz @ 2011-05-20 19:10 ` Tom Tromey 2011-05-23 13:17 ` Jan Kratochvil 3 siblings, 0 replies; 24+ messages in thread From: Tom Tromey @ 2011-05-20 19:10 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> So the first question to be asked here is, Should we adopt Jan's Keith> patch, or some variation of it, to use DW_AT_linkage_name when Keith> available? I think so. In fact, I think it should be preferred. My reasoning is that when DW_AT_linkage_name exists, it is definitive. Our exposure here is to compiler bugs (which would be very bad ones -- but not our problem) and to demangler bugs -- something I am not extremely concerned about. And, we have seen bugs where the DWARF is very incorrect but the linkage name is not (I think these have been fixed, but the point is that they confound physname, and even reasonably recent compilers, like the F13 gcc, still have this.) However, it is also now clear to me that we do need physname. First, there are cases where GCC does not emit DW_AT_linkage_name. I finally filed the one case we know about: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49047 Second, maybe DW_AT_linkage_name will be dropped in order to shrink debuginfo. Third, there are other compilers. Keith> The only hesitation I have is the compiler issues. Just using Keith> DW_AT_linkage_name instead of computing it has lead to many test Keith> regressions with differing compiler versions. [Again, I have only Keith> tested gcc.] Based on Jan's response, and discussions we've had recently, it seems that the regressions in question are really just differences between physname and the demangler, correct? And the problem specifically is that template functions have the return type in the demangled form, requiring users, for the time being, to add quotes? Keith> The bigger issue is what to do for 7.3, which has been in limbo for Keith> quite some time awaiting resolution of c++/12506 (for which I have Keith> posted patches). Keith> I guess there are basically two options: Keith> 1) Keep physname & apply the 12266/12506 patchset (when approved) Keith> 2) Adopt Jan's patch and fix the fallout now Or (3) Apply both and make linkage_name the preferred choice. At present this is what I think we should do. Keith> My big fear with #2 is "fixing" linespecs (yet again). Any changes to Keith> this area take an unimaginably large amount of time and effort. I think for 7.3 we would accept that there are some regressions, and then fix them for a future release. I am partial to Jan's view that we should be using the full name of the object, and then have symbol aliases or code in linespec to expand nice shortcuts to the full name. Tom ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-18 22:35 The future of dwarf2_physname Keith Seitz ` (2 preceding siblings ...) 2011-05-20 19:10 ` Tom Tromey @ 2011-05-23 13:17 ` Jan Kratochvil 2011-05-23 19:52 ` Tom Tromey ` (2 more replies) 3 siblings, 3 replies; 24+ messages in thread From: Jan Kratochvil @ 2011-05-23 13:17 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches On Thu, 19 May 2011 00:34:43 +0200, Keith Seitz wrote: > I guess there are basically two options: > 1) Keep physname & apply the 12266/12506 patchset (when approved) > 2) Adopt Jan's patch and fix the fallout now > > My take on this today is that I think the least risky/most timely > solution is #1. #1 is still IMO a lot of work (see "Computed physname" errors at the end of this mail) and #1 should always be backed by #2 anyway and as #2 with current GCCs has the same user experience as #1 I do not see a reason why to wait on #1 for gdb-7.3. I have concerns about regressions for gdb-7.3, I would like to fix regressions gdb-7.1 (pre-physname) -> gdb-7.2 (physname) but possibly not introduce regressions gdb-7.2 (physname) -> gdb-7.3 (?). Trying to propose a simple DW_AT_linkage_name patch of mine but I do not yet have it finished. problem #1 of post-physname GDB (gdb) p 'f<char>()' $1 = {long (void)} 0x4004e4 <f<char>()> (gdb) p 'long f<char>()' $2 = {<text variable, no debug info>} 0x4004e4 <f<char>()> (gdb) p 'f<short>()' No symbol "f<short>()" in current context. (gdb) p 'long f<short>()' $3 = {<text variable, no debug info>} 0x4004fb <long f<short>()> ==> dis.h <== template <typename T> long f () { return 1; } ==> dis1.C <== #include "dis.h" int main () { f<char> (); } ==> dis2.C <== #include "dis.h" void g () { f<short> (); } The two different aliases for 0x4004e4 where the $2 one does not have any type is a bug, at least both should have the same type. It can be seen also on the f<short>() case ($3) where the return type missing alias is not present because only ELF (and not DWARF) symbol is present. I haven't found how to reproduce the regression "Cannot take address of method m." for function templates (which use the return type in their linkage) as I haven't found how to reference function template externally from a class - this field is missing there in such case: class C { public: template <typename T> static long f (); }; And I get error: explicit specialization in non-namespace scope ‘class C’ for class C { public: template <typename T> static long f (); template <> static long f<char> (); }; And in other cases there is alread the template function definition with associated DWARF which gets processed by GDB-physname and the return type gets stripped. problem #2 of post-physname GDB (gdb) p 'int f<long>()' $1 = {<text variable, no debug info>} 0x40050f <f<long>()> (gdb) p 'short f<long>()' $2 = {<text variable, no debug info>} 0x400531 <f<long>()> (gdb) p 'f<long>()' $3 = {int (void)} 0x40050f <f<long>()> ==> manya.C <== template <typename T> int f () { return 4; } int a () { return f<long> () == 4; } extern int b (); int main () { return a () && b () ? 0 : 1; } ==> manyb.C <== template <typename T> short f () { return 2; } int b () { return f<long> () == 2; } As the return type is stripped the name can become ambiguous. Different template functions with same name, same parameter types but different return type can be linked in a single program. It is true one cannot declare both such template functions in a single CU (Compilation Unit) so I do not expect such case is found in real world. I was trying to propose DMGL_RET_POSTFIX but it has other drawbacks and it is in fact a new feature (and not a regression fixup) because the trailing type was not present in gdb-7.2 anyway. So it can be considered after some tidy up for gdb-7.4 but it makes no sense for gdb-7.3. Re: [rfc] physname cross-check #2 http://sourceware.org/ml/gdb-patches/2011-05/msg00466.html Therefore it would apply for both ELF and both DWARF symbols producing the same form. For ELF symbols implemented by (patched) DMGL_RET_POSTFIX and for DWARF symbols missing DW_AT_linkage_name implemented by GDB-physname code. That is the 'int f<long>()' and 'short f<long>()' would be uniquely searchable as `f<long>()int' and `f<long>()short'. Still `nm -C', `objdump -C' normally do not (and even cannot) request DMGL_RET_POSTFIX therefore to be able to `break symbol_that_I_see_in_objdump_C' these DMGL_RET_POSTFIX-generated symbols should be only as aliases to also available non-DMGL_RET_POSTFIX symbols. Then there is the code for NAME_KIND_PRINT (with typedefs etc. preserved, different from NAME_KIND_PHYS with typedefs etc. resolved). That may be a nice new feature for gdb-7.4 but as such I do not find it relevant for gdb-7.3. cp_canonicalize_string_no_typedefs in decode_variable is also a nice feature (`break func(typedef)') but it is a new feature not present anytime before, therefore for gdb-7.4. Also the current NAME_KIND_PRINT behavior does not seem clearly the right one: typedef int t; void f(t p) {} int main () {} (gdb) p f $1 = {void (t)} 0x4004d4 <f(t)> - correct, in this CU `f' has the NAME_KIND_PRINT `t'. (gdb) info sym 0x4004d4 f(int) in section .text - correct - `info symbol' uses only minimal=ELF symbols (gdb) complete p 'f( p 'f(int) p 'f(t) - I do not find it clear this is right. There should be definitely the `f(int)' - demangled linkage name - variant. But whether to show the "convenient" aliases as SYMBOL_SEARCH_NAME I do not find clear, it may be better but it also complicates the list of all the available overloads for a function name, one should ask practical C++ programmer for it IMO. After all there is still only a single function `f': (gdb) p f(t) $2 = {void (t)} 0x4004d4 <f(t)> (gdb) p f(int) $3 = {void (t)} 0x4004d4 <f(t)> Regarding the proposals to fix dwarf2_physname computation instead of preferring DW_AT_linkage_name. ISTM we have never found the agreement with Keith, when DW_AT_linkage_name is present it is guaranteed to be correct and I do not know why not to use it. It is a good verification way to fix all the existing dwarf2_physname computation issues so that in the future one may stop shipping the redundant size-costly (3-10% of .debug files) DW_AT_linkage_name strings. But still even in future I do not see why GDB should not prefer DW_AT_linkage_name over the dwarf2_physname computated name, when it is present. Preferring dwarf2_physname computation over DW_AT_linkage_name - when they differ - means a failure to match the ELF demangled symbols leading to the error "Cannot take address of method m." and/or missing function types etc. So that way I find as always wrong. Keith proposes to fix dwarf2_physname instead of using DW_AT_linkage_name. If it really always matches sure there is no difference although (a) it still may take some time for gdb-7.3 and (b) even in the future I do not see where is any advantage over preferring DW_AT_linkage_name when it is available. Bugs to fix for /usr/lib/debug/usr/lib64/libwebkitgtk-1.0.so.0.7.0.debug from webkitgtk-debuginfo-1.4.0-1.fc15.x86_64 - it is definitely not a complete bug list, raw messages list has 56490 lines: http://people.redhat.com/jkratoch/webkitgtk-debuginfo-1.4.0-1.fc15.x86_64-gdb.err.xz Computed physname <JSC::JSCell::markChildren(struct JSC::MarkStack &)> does not match demangled <JSC::JSCell::markChildren(JSC::MarkStack&)> (from linkage <_ZN3JSC6JSCell12markChildrenERNS_9MarkStackE>) - you reported the `struct' bug is already fixed in a patch being prepared Computed physname <WebCore::lineCapName(enum WebCore::LineCap)> does not match demangled <WebCore::lineCapName(WebCore::LineCap)> (from linkage <_ZN7WebCore11lineCapNameENS_7LineCapE>) - IIRC you reported the `enum' bug is also already fixed Computed physname <WTF::FixedArray<double, 8u>::data()> does not match demangled <WTF::FixedArray<double, 8ul>::data()> (from linkage <_ZN3WTF10FixedArrayIdLm8EE4dataEv>) - FYI its DW_TAG_class_type DW_AT_name is: FixedArray<double, 8u> Computed physname <WTF::PtrHash<void const*>::hash(void*)> does not match demangled <WTF::PtrHash<void const*>::hash(void const*)> (from linkage <_ZN3WTF7PtrHashIPKvE4hashES2_>) - const issue Computed physname <WebCore::RangeBoundaryPoint::offset()> does not match demangled <WebCore::RangeBoundaryPoint::offset() const> (from linkage <_ZNK7WebCore18RangeBoundaryPoint6offsetEv>) - maybe different const issue Computed physname <WebCore::IntPoint::operator GdkPoint() const> does not match demangled <WebCore::IntPoint::operator _GdkPoint() const> (from linkage <_ZNK7WebCore8IntPointcv9_GdkPointEv>) - some leading _ Computed physname <JSC::JSParser::parseBreakStatement<JSC::ASTBuilder>(JSC::ASTBuilder&)> does not match demangled <JSC::ASTBuilder::Statement JSC::JSParser::parseBreakStatement<JSC::ASTBuilder>(JSC::ASTBuilder&)> (from linkage <_ZN3JSC8JSParser19parseBreakStatementINS_10ASTBuilderEEENT_9StatementERS3_>) - missing return type etc. After all I still do not have a patch at hand but I gave some thoughts of mine on it. Thanks, Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-23 13:17 ` Jan Kratochvil @ 2011-05-23 19:52 ` Tom Tromey 2011-05-23 19:57 ` Keith Seitz 2011-05-24 21:12 ` [rfc 1/2] libiberty/ options code reshuffle [Re: The future of dwarf2_physname] Jan Kratochvil 2011-05-24 21:21 ` [rfc 2/2] Follow DW_AT_linkage_name for methods " Jan Kratochvil 2 siblings, 1 reply; 24+ messages in thread From: Tom Tromey @ 2011-05-23 19:52 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Keith Seitz, gdb-patches Jan> I have concerns about regressions for gdb-7.3, I would like to fix Jan> regressions gdb-7.1 (pre-physname) -> gdb-7.2 (physname) but Jan> possibly not introduce regressions gdb-7.2 (physname) -> gdb-7.3 Jan> (?). I don't see how that is really possible. I am willing to accept some minor regressions for 7.3 in exchange for doing better in the long run. Jan> I was trying to propose DMGL_RET_POSTFIX but it has other drawbacks Jan> and it is in fact a new feature (and not a regression fixup) Jan> because the trailing type was not present in gdb-7.2 anyway. I think we should not take this approach, for the simple reason that it will be weird to C++ users. I think it would be better to just require quotes in this case for 7.3, like "break 'int f<int> ()'", then see about fixing up linespec, or whatever, in 7.4+. Also for later would be making some kind of symbol alias for this, so that in the normal case users can omit the return type. Jan> (gdb) complete p 'f( Jan> p 'f(int) Jan> p 'f(t) Jan> - I do not find it clear this is right. There should be Jan> definitely the `f(int)' - demangled linkage name - variant. But Jan> whether to show the "convenient" aliases as SYMBOL_SEARCH_NAME I Jan> do not find clear, it may be better but it also complicates the Jan> list of all the available overloads for a function name, one Jan> should ask practical C++ programmer for it IMO. I think this one is ok, but I can see how that could go either way. I find it a side issue at present. Jan> Regarding the proposals to fix dwarf2_physname computation instead Jan> of preferring DW_AT_linkage_name. ISTM we have never found the Jan> agreement with Keith, when DW_AT_linkage_name is present it is Jan> guaranteed to be correct and I do not know why not to use it. I think we should move forward with it, on the basis that it is more correct and probably has better performance to boot. I suggest something like the appended, on top of Keith's patches and your patch. The idea here is to prefer the fast path, but have a debug setting so we can easily do physname checking. WDYT? I didn't run it through the test suite or anything yet. Jan> Keith proposes to fix dwarf2_physname instead of using Jan> DW_AT_linkage_name. We must do both in the medium term. Tom diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 7530e3e..fff8e3b 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -119,6 +119,9 @@ _STATEMENT_PROLOGUE; /* When non-zero, dump DIEs after they are read in. */ static int dwarf2_die_debug = 0; +/* When non-zero, cross-check physname against demangler. */ +static int check_physname = 0; + static int pagesize; /* When set, the file that we're processing is known to have debugging @@ -5142,71 +5145,71 @@ dwarf2_full_name (char *name, struct die_info *die, struct dwarf2_cu *cu) static const char * dwarf2_physname (char *name, struct die_info *die, struct dwarf2_cu *cu) { - const char *physname = dwarf2_compute_name (name, die, cu, NAME_KIND_PHYS); + const char *physname; struct attribute *attr; - const char *mangled, *retval; - char *demangled, *canon; - struct cleanup *back_to; - - /* Do not check PHYSNAME if it never will be needed by GDB. GDB does not - need to support something it has no use for. */ - if (!die_needs_namespace (die, cu)) - return physname; + const char *retval, *mangled = NULL; + char *canon = NULL; + struct cleanup *back_to = make_cleanup (null_cleanup, NULL); + int need_copy = 1; attr = dwarf2_attr (die, DW_AT_linkage_name, cu); if (!attr) attr = dwarf2_attr (die, DW_AT_MIPS_linkage_name, cu); - if (!attr || !DW_STRING (attr)) + /* DW_AT_linkage_name is missing in some cases - depend on what GDB + has computed. */ + if (attr && DW_STRING (attr)) { - /* DW_AT_linkage_name is missing in some cases - depend on what GDB has - computed. */ - - return physname; - } - mangled = DW_STRING (attr); - - /* As both DW_AT_linkage_name (MANGLED) and computed PHYSNAME are present - cross-check them. */ - - back_to = make_cleanup (null_cleanup, 0); + char *demangled; - demangled = cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI); - if (demangled) - make_cleanup (xfree, demangled); - else - demangled = (char *) mangled; + mangled = DW_STRING (attr); - if (cu->language == language_cplus) - { - canon = cp_canonicalize_string (demangled); - if (canon != NULL) - make_cleanup (xfree, canon); + demangled = cplus_demangle (mangled, (DMGL_PARAMS | DMGL_ANSI + | DMGL_VERBOSE)); + if (demangled) + { + make_cleanup (xfree, demangled); + canon = demangled; + } else - canon = demangled; + { + canon = (char *) mangled; + need_copy = 0; + } } - else - canon = demangled; - if (strcmp (physname, canon) != 0) + if (canon == NULL || check_physname) { - /* It may not mean a bug in GDB. The compiler could also compute - DW_AT_linkage_name incorrectly. But in such case GDB would need to be - bug-to-bug compatible. */ + physname = dwarf2_compute_name (name, die, cu, NAME_KIND_PHYS); - complaint (&symfile_complaints, - _("Computed physname <%s> does not match demangled <%s> " - "(from linkage <%s>) - DIE at 0x%x [in module %s]"), - physname, canon, mangled, die->offset, cu->objfile->name); + if (canon != NULL && strcmp (physname, canon) != 0) + { + /* It may not mean a bug in GDB. The compiler could also + compute DW_AT_linkage_name incorrectly. But in such case + GDB would need to be bug-to-bug compatible. */ + + complaint (&symfile_complaints, + _("Computed physname <%s> does not match demangled <%s> " + "(from linkage <%s>) - DIE at 0x%x [in module %s]"), + physname, canon, mangled, die->offset, cu->objfile->name); - /* Prefer DW_AT_linkage_name (in the CANON form) - when it is available - here - over computed PHYSNAME. It is safer against both buggy GDB and - buggy compilers. */ + /* Prefer DW_AT_linkage_name (in the CANON form) - when it + is available here - over computed PHYSNAME. It is safer + against both buggy GDB and buggy compilers. */ - retval = obsavestring (canon, strlen (canon), - &cu->objfile->objfile_obstack); + retval = canon; + } + else + { + retval = physname; + need_copy = 0; + } } else - retval = physname; + retval = canon; + + if (need_copy) + retval = obsavestring (retval, strlen (retval), + &cu->objfile->objfile_obstack); do_cleanups (back_to); return retval; @@ -16247,6 +16250,15 @@ show_dwarf2_always_disassemble (struct ui_file *file, int from_tty, value); } +static void +show_check_physname (struct ui_file *file, int from_tty, + struct cmd_list_element *c, const char *value) +{ + fprintf_filtered (file, + _("Whether to check \"physname\" is %s.\n"), + value); +} + void _initialize_dwarf2_read (void); void @@ -16302,6 +16314,14 @@ The value is the maximum depth to print."), NULL, &setdebuglist, &showdebuglist); + add_setshow_boolean_cmd ("check-physname", no_class, &check_physname, _("\ +Set cross-checking of \"physname\" code against demangler."), _("\ +Show cross-checking of \"physname\" code against demangler."), _("\ +When enabled, GDB's internal \"physname\" code is checked against\n\ +the demangler."), + NULL, show_check_physname, + &setdebuglist, &showdebuglist); + c = add_cmd ("gdb-index", class_files, save_gdb_index_command, _("\ Save a gdb-index file.\n\ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: The future of dwarf2_physname 2011-05-23 19:52 ` Tom Tromey @ 2011-05-23 19:57 ` Keith Seitz 0 siblings, 0 replies; 24+ messages in thread From: Keith Seitz @ 2011-05-23 19:57 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 05/23/2011 12:52 PM, Tom Tromey wrote: > I suggest something like the appended, on top of Keith's patches and > your patch. The idea here is to prefer the fast path, but have a debug > setting so we can easily do physname checking. > > WDYT? I didn't run it through the test suite or anything yet. I know this question was for Jan, but I just wanted to chime in here and say that I was going to propose/do exactly the same thing. I'm all for it. Keith ^ permalink raw reply [flat|nested] 24+ messages in thread
* [rfc 1/2] libiberty/ options code reshuffle [Re: The future of dwarf2_physname] 2011-05-23 13:17 ` Jan Kratochvil 2011-05-23 19:52 ` Tom Tromey @ 2011-05-24 21:12 ` Jan Kratochvil 2011-06-02 15:36 ` obsolete: " Jan Kratochvil 2011-05-24 21:21 ` [rfc 2/2] Follow DW_AT_linkage_name for methods " Jan Kratochvil 2 siblings, 1 reply; 24+ messages in thread From: Jan Kratochvil @ 2011-05-24 21:12 UTC (permalink / raw) To: gdb-patches; +Cc: Keith Seitz Hi, this patch is not interesting, no functionality change, just a prerequisite for [rfc 2/2]. Thanks, Jan libiberty/ 2011-05-24 Jan Kratochvil <jan.kratochvil@redhat.com> * cp-demangle.c (struct d_print_info): Remove field options. (d_print_init, d_print_comp, d_print_mod_list, d_print_mod) (d_print_function_type, d_print_array_type, d_print_expr_op) (d_print_cast, d_print_subexpr): Add parameter options, update all the callers. --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -278,8 +278,6 @@ struct d_growable_string enum { D_PRINT_BUFFER_LENGTH = 256 }; struct d_print_info { - /* The options passed to the demangler. */ - int options; /* Fixed-length allocated buffer for demangled data, flushed to the callback with a NUL termination once full. */ char buf[D_PRINT_BUFFER_LENGTH]; @@ -436,7 +434,7 @@ static void d_growable_string_callback_adapter (const char *, size_t, void *); static void -d_print_init (struct d_print_info *, int, demangle_callbackref, void *); +d_print_init (struct d_print_info *, demangle_callbackref, void *); static inline void d_print_error (struct d_print_info *); @@ -454,32 +452,32 @@ static inline void d_append_string (struct d_print_info *, const char *); static inline char d_last_char (struct d_print_info *); static void -d_print_comp (struct d_print_info *, const struct demangle_component *); +d_print_comp (struct d_print_info *, const struct demangle_component *, int); static void d_print_java_identifier (struct d_print_info *, const char *, int); static void -d_print_mod_list (struct d_print_info *, struct d_print_mod *, int); +d_print_mod_list (struct d_print_info *, struct d_print_mod *, int, int); static void -d_print_mod (struct d_print_info *, const struct demangle_component *); +d_print_mod (struct d_print_info *, const struct demangle_component *, int); static void d_print_function_type (struct d_print_info *, const struct demangle_component *, - struct d_print_mod *); + struct d_print_mod *, int); static void d_print_array_type (struct d_print_info *, const struct demangle_component *, - struct d_print_mod *); + struct d_print_mod *, int); static void -d_print_expr_op (struct d_print_info *, const struct demangle_component *); +d_print_expr_op (struct d_print_info *, const struct demangle_component *, int); static void -d_print_cast (struct d_print_info *, const struct demangle_component *); +d_print_cast (struct d_print_info *, const struct demangle_component *, int); static int d_demangle_callback (const char *, int, demangle_callbackref, void *); @@ -3293,10 +3291,9 @@ d_growable_string_callback_adapter (const char *s, size_t l, void *opaque) /* Initialize a print information structure. */ static void -d_print_init (struct d_print_info *dpi, int options, - demangle_callbackref callback, void *opaque) +d_print_init (struct d_print_info *dpi, demangle_callbackref callback, + void *opaque) { - dpi->options = options; dpi->len = 0; dpi->last_char = '\0'; dpi->templates = NULL; @@ -3392,9 +3389,9 @@ cplus_demangle_print_callback (int options, { struct d_print_info dpi; - d_print_init (&dpi, options, callback, opaque); + d_print_init (&dpi, callback, opaque); - d_print_comp (&dpi, dc); + d_print_comp (&dpi, dc, options); d_print_flush (&dpi); @@ -3537,8 +3534,8 @@ d_pack_length (const struct demangle_component *dc) if needed. */ static void -d_print_subexpr (struct d_print_info *dpi, - const struct demangle_component *dc) +d_print_subexpr (struct d_print_info *dpi, const struct demangle_component *dc, + int options) { int simple = 0; if (dc->type == DEMANGLE_COMPONENT_NAME @@ -3546,7 +3543,7 @@ d_print_subexpr (struct d_print_info *dpi, simple = 1; if (!simple) d_append_char (dpi, '('); - d_print_comp (dpi, dc); + d_print_comp (dpi, dc, options); if (!simple) d_append_char (dpi, ')'); } @@ -3554,8 +3551,8 @@ d_print_subexpr (struct d_print_info *dpi, /* Subroutine to handle components. */ static void -d_print_comp (struct d_print_info *dpi, - const struct demangle_component *dc) +d_print_comp (struct d_print_info *dpi, const struct demangle_component *dc, + int options) { if (dc == NULL) { @@ -3568,7 +3565,7 @@ d_print_comp (struct d_print_info *dpi, switch (dc->type) { case DEMANGLE_COMPONENT_NAME: - if ((dpi->options & DMGL_JAVA) == 0) + if ((options & DMGL_JAVA) == 0) d_append_buffer (dpi, dc->u.s_name.s, dc->u.s_name.len); else d_print_java_identifier (dpi, dc->u.s_name.s, dc->u.s_name.len); @@ -3576,12 +3573,12 @@ d_print_comp (struct d_print_info *dpi, case DEMANGLE_COMPONENT_QUAL_NAME: case DEMANGLE_COMPONENT_LOCAL_NAME: - d_print_comp (dpi, d_left (dc)); - if ((dpi->options & DMGL_JAVA) == 0) + d_print_comp (dpi, d_left (dc), options); + if ((options & DMGL_JAVA) == 0) d_append_string (dpi, "::"); else d_append_char (dpi, '.'); - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); return; case DEMANGLE_COMPONENT_TYPED_NAME: @@ -3671,7 +3668,7 @@ d_print_comp (struct d_print_info *dpi, } } - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); if (typed_name->type == DEMANGLE_COMPONENT_TEMPLATE) dpi->templates = dpt.next; @@ -3684,7 +3681,7 @@ d_print_comp (struct d_print_info *dpi, if (! adpm[i].printed) { d_append_char (dpi, ' '); - d_print_mod (dpi, adpm[i].mod); + d_print_mod (dpi, adpm[i].mod, options); } } @@ -3707,7 +3704,7 @@ d_print_comp (struct d_print_info *dpi, dcl = d_left (dc); - if ((dpi->options & DMGL_JAVA) != 0 + if ((options & DMGL_JAVA) != 0 && dcl->type == DEMANGLE_COMPONENT_NAME && dcl->u.s_name.len == 6 && strncmp (dcl->u.s_name.s, "JArray", 6) == 0) @@ -3715,16 +3712,16 @@ d_print_comp (struct d_print_info *dpi, /* Special-case Java arrays, so that JArray<TYPE> appears instead as TYPE[]. */ - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); d_append_string (dpi, "[]"); } else { - d_print_comp (dpi, dcl); + d_print_comp (dpi, dcl, options); if (d_last_char (dpi) == '<') d_append_char (dpi, ' '); d_append_char (dpi, '<'); - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); /* Avoid generating two consecutive '>' characters, to avoid the C++ syntactic ambiguity. */ if (d_last_char (dpi) == '>') @@ -3759,7 +3756,7 @@ d_print_comp (struct d_print_info *dpi, hold_dpt = dpi->templates; dpi->templates = hold_dpt->next; - d_print_comp (dpi, a); + d_print_comp (dpi, a, options); dpi->templates = hold_dpt; @@ -3767,79 +3764,79 @@ d_print_comp (struct d_print_info *dpi, } case DEMANGLE_COMPONENT_CTOR: - d_print_comp (dpi, dc->u.s_ctor.name); + d_print_comp (dpi, dc->u.s_ctor.name, options); return; case DEMANGLE_COMPONENT_DTOR: d_append_char (dpi, '~'); - d_print_comp (dpi, dc->u.s_dtor.name); + d_print_comp (dpi, dc->u.s_dtor.name, options); return; case DEMANGLE_COMPONENT_VTABLE: d_append_string (dpi, "vtable for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_VTT: d_append_string (dpi, "VTT for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_CONSTRUCTION_VTABLE: d_append_string (dpi, "construction vtable for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); d_append_string (dpi, "-in-"); - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); return; case DEMANGLE_COMPONENT_TYPEINFO: d_append_string (dpi, "typeinfo for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_TYPEINFO_NAME: d_append_string (dpi, "typeinfo name for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_TYPEINFO_FN: d_append_string (dpi, "typeinfo fn for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_THUNK: d_append_string (dpi, "non-virtual thunk to "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_VIRTUAL_THUNK: d_append_string (dpi, "virtual thunk to "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_COVARIANT_THUNK: d_append_string (dpi, "covariant return thunk to "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_JAVA_CLASS: d_append_string (dpi, "java Class for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_GUARD: d_append_string (dpi, "guard variable for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_REFTEMP: d_append_string (dpi, "reference temporary for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_HIDDEN_ALIAS: d_append_string (dpi, "hidden alias for "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_SUB_STD: @@ -3866,7 +3863,7 @@ d_print_comp (struct d_print_info *dpi, break; if (pdpm->mod->type == dc->type) { - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; } } @@ -3892,12 +3889,12 @@ d_print_comp (struct d_print_info *dpi, dpm.printed = 0; dpm.templates = dpi->templates; - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); /* If the modifier didn't get printed by the type, print it now. */ if (! dpm.printed) - d_print_mod (dpi, dc); + d_print_mod (dpi, dc, options); dpi->modifiers = dpm.next; @@ -3905,7 +3902,7 @@ d_print_comp (struct d_print_info *dpi, } case DEMANGLE_COMPONENT_BUILTIN_TYPE: - if ((dpi->options & DMGL_JAVA) == 0) + if ((options & DMGL_JAVA) == 0) d_append_buffer (dpi, dc->u.s_builtin.type->name, dc->u.s_builtin.type->len); else @@ -3914,13 +3911,13 @@ d_print_comp (struct d_print_info *dpi, return; case DEMANGLE_COMPONENT_VENDOR_TYPE: - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_FUNCTION_TYPE: { - if ((dpi->options & DMGL_RET_POSTFIX) != 0) - d_print_function_type (dpi, dc, dpi->modifiers); + if ((options & DMGL_RET_POSTFIX) != 0) + d_print_function_type (dpi, dc, dpi->modifiers, options); /* Print return type if present */ if (d_left (dc) != NULL) @@ -3935,7 +3932,7 @@ d_print_comp (struct d_print_info *dpi, dpm.printed = 0; dpm.templates = dpi->templates; - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); dpi->modifiers = dpm.next; @@ -3944,12 +3941,12 @@ d_print_comp (struct d_print_info *dpi, /* In standard prefix notation, there is a space between the return type and the function signature. */ - if ((dpi->options & DMGL_RET_POSTFIX) == 0) + if ((options & DMGL_RET_POSTFIX) == 0) d_append_char (dpi, ' '); } - if ((dpi->options & DMGL_RET_POSTFIX) == 0) - d_print_function_type (dpi, dc, dpi->modifiers); + if ((options & DMGL_RET_POSTFIX) == 0) + d_print_function_type (dpi, dc, dpi->modifiers, options); return; } @@ -4002,7 +3999,7 @@ d_print_comp (struct d_print_info *dpi, pdpm = pdpm->next; } - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); dpi->modifiers = hold_modifiers; @@ -4012,10 +4009,10 @@ d_print_comp (struct d_print_info *dpi, while (i > 1) { --i; - d_print_mod (dpi, adpm[i].mod); + d_print_mod (dpi, adpm[i].mod, options); } - d_print_array_type (dpi, dc, dpi->modifiers); + d_print_array_type (dpi, dc, dpi->modifiers, options); return; } @@ -4031,12 +4028,12 @@ d_print_comp (struct d_print_info *dpi, dpm.printed = 0; dpm.templates = dpi->templates; - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); /* If the modifier didn't get printed by the type, print it now. */ if (! dpm.printed) - d_print_mod (dpi, dc); + d_print_mod (dpi, dc, options); dpi->modifiers = dpm.next; @@ -4050,7 +4047,7 @@ d_print_comp (struct d_print_info *dpi, if (dc->u.s_fixed.length->u.s_builtin.type != &cplus_demangle_builtin_types['i'-'a']) { - d_print_comp (dpi, dc->u.s_fixed.length); + d_print_comp (dpi, dc->u.s_fixed.length, options); d_append_char (dpi, ' '); } if (dc->u.s_fixed.accum) @@ -4062,7 +4059,7 @@ d_print_comp (struct d_print_info *dpi, case DEMANGLE_COMPONENT_ARGLIST: case DEMANGLE_COMPONENT_TEMPLATE_ARGLIST: if (d_left (dc) != NULL) - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); if (d_right (dc) != NULL) { size_t len; @@ -4074,7 +4071,7 @@ d_print_comp (struct d_print_info *dpi, d_append_string (dpi, ", "); len = dpi->len; flush_count = dpi->flush_count; - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); /* If that didn't print anything (which can happen with empty template argument packs), remove the comma and space. */ if (dpi->flush_count == flush_count && dpi->len == len) @@ -4097,24 +4094,24 @@ d_print_comp (struct d_print_info *dpi, case DEMANGLE_COMPONENT_EXTENDED_OPERATOR: d_append_string (dpi, "operator "); - d_print_comp (dpi, dc->u.s_extended_operator.name); + d_print_comp (dpi, dc->u.s_extended_operator.name, options); return; case DEMANGLE_COMPONENT_CAST: d_append_string (dpi, "operator "); - d_print_cast (dpi, dc); + d_print_cast (dpi, dc, options); return; case DEMANGLE_COMPONENT_UNARY: if (d_left (dc)->type != DEMANGLE_COMPONENT_CAST) - d_print_expr_op (dpi, d_left (dc)); + d_print_expr_op (dpi, d_left (dc), options); else { d_append_char (dpi, '('); - d_print_cast (dpi, d_left (dc)); + d_print_cast (dpi, d_left (dc), options); d_append_char (dpi, ')'); } - d_print_subexpr (dpi, d_right (dc)); + d_print_subexpr (dpi, d_right (dc), options); return; case DEMANGLE_COMPONENT_BINARY: @@ -4132,18 +4129,18 @@ d_print_comp (struct d_print_info *dpi, && d_left (dc)->u.s_operator.op->name[0] == '>') d_append_char (dpi, '('); - d_print_subexpr (dpi, d_left (d_right (dc))); + d_print_subexpr (dpi, d_left (d_right (dc)), options); if (strcmp (d_left (dc)->u.s_operator.op->code, "ix") == 0) { d_append_char (dpi, '['); - d_print_comp (dpi, d_right (d_right (dc))); + d_print_comp (dpi, d_right (d_right (dc)), options); d_append_char (dpi, ']'); } else { if (strcmp (d_left (dc)->u.s_operator.op->code, "cl") != 0) - d_print_expr_op (dpi, d_left (dc)); - d_print_subexpr (dpi, d_right (d_right (dc))); + d_print_expr_op (dpi, d_left (dc), options); + d_print_subexpr (dpi, d_right (d_right (dc)), options); } if (d_left (dc)->type == DEMANGLE_COMPONENT_OPERATOR @@ -4165,11 +4162,11 @@ d_print_comp (struct d_print_info *dpi, d_print_error (dpi); return; } - d_print_subexpr (dpi, d_left (d_right (dc))); - d_print_expr_op (dpi, d_left (dc)); - d_print_subexpr (dpi, d_left (d_right (d_right (dc)))); + d_print_subexpr (dpi, d_left (d_right (dc)), options); + d_print_expr_op (dpi, d_left (dc), options); + d_print_subexpr (dpi, d_left (d_right (d_right (dc))), options); d_append_string (dpi, " : "); - d_print_subexpr (dpi, d_right (d_right (d_right (dc)))); + d_print_subexpr (dpi, d_right (d_right (d_right (dc))), options); return; case DEMANGLE_COMPONENT_TRINARY_ARG1: @@ -4200,7 +4197,7 @@ d_print_comp (struct d_print_info *dpi, { if (dc->type == DEMANGLE_COMPONENT_LITERAL_NEG) d_append_char (dpi, '-'); - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); switch (tp) { default: @@ -4250,13 +4247,13 @@ d_print_comp (struct d_print_info *dpi, } d_append_char (dpi, '('); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); d_append_char (dpi, ')'); if (dc->type == DEMANGLE_COMPONENT_LITERAL_NEG) d_append_char (dpi, '-'); if (tp == D_PRINT_FLOAT) d_append_char (dpi, '['); - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); if (tp == D_PRINT_FLOAT) d_append_char (dpi, ']'); } @@ -4268,12 +4265,12 @@ d_print_comp (struct d_print_info *dpi, case DEMANGLE_COMPONENT_JAVA_RESOURCE: d_append_string (dpi, "java resource "); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); return; case DEMANGLE_COMPONENT_COMPOUND_NAME: - d_print_comp (dpi, d_left (dc)); - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_left (dc), options); + d_print_comp (dpi, d_right (dc), options); return; case DEMANGLE_COMPONENT_CHARACTER: @@ -4282,7 +4279,7 @@ d_print_comp (struct d_print_info *dpi, case DEMANGLE_COMPONENT_DECLTYPE: d_append_string (dpi, "decltype ("); - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); d_append_char (dpi, ')'); return; @@ -4296,7 +4293,7 @@ d_print_comp (struct d_print_info *dpi, /* d_find_pack won't find anything if the only packs involved in this expansion are function parameter packs; in that case, just print the pattern and "...". */ - d_print_subexpr (dpi, d_left (dc)); + d_print_subexpr (dpi, d_left (dc), options); d_append_string (dpi, "..."); return; } @@ -4306,7 +4303,7 @@ d_print_comp (struct d_print_info *dpi, for (i = 0; i < len; ++i) { dpi->pack_index = i; - d_print_comp (dpi, dc); + d_print_comp (dpi, dc, options); if (i < len-1) d_append_string (dpi, ", "); } @@ -4321,17 +4318,17 @@ d_print_comp (struct d_print_info *dpi, case DEMANGLE_COMPONENT_GLOBAL_CONSTRUCTORS: d_append_string (dpi, "global constructors keyed to "); - d_print_comp (dpi, dc->u.s_binary.left); + d_print_comp (dpi, dc->u.s_binary.left, options); return; case DEMANGLE_COMPONENT_GLOBAL_DESTRUCTORS: d_append_string (dpi, "global destructors keyed to "); - d_print_comp (dpi, dc->u.s_binary.left); + d_print_comp (dpi, dc->u.s_binary.left, options); return; case DEMANGLE_COMPONENT_LAMBDA: d_append_string (dpi, "{lambda("); - d_print_comp (dpi, dc->u.s_unary_num.sub); + d_print_comp (dpi, dc->u.s_unary_num.sub, options); d_append_string (dpi, ")#"); d_append_num (dpi, dc->u.s_unary_num.num + 1); d_append_char (dpi, '}'); @@ -4405,8 +4402,8 @@ d_print_java_identifier (struct d_print_info *dpi, const char *name, int len) qualifiers on this after printing a function. */ static void -d_print_mod_list (struct d_print_info *dpi, - struct d_print_mod *mods, int suffix) +d_print_mod_list (struct d_print_info *dpi, struct d_print_mod *mods, + int suffix, int options) { struct d_print_template *hold_dpt; @@ -4419,7 +4416,7 @@ d_print_mod_list (struct d_print_info *dpi, || mods->mod->type == DEMANGLE_COMPONENT_VOLATILE_THIS || mods->mod->type == DEMANGLE_COMPONENT_CONST_THIS))) { - d_print_mod_list (dpi, mods->next, suffix); + d_print_mod_list (dpi, mods->next, suffix, options); return; } @@ -4430,13 +4427,13 @@ d_print_mod_list (struct d_print_info *dpi, if (mods->mod->type == DEMANGLE_COMPONENT_FUNCTION_TYPE) { - d_print_function_type (dpi, mods->mod, mods->next); + d_print_function_type (dpi, mods->mod, mods->next, options); dpi->templates = hold_dpt; return; } else if (mods->mod->type == DEMANGLE_COMPONENT_ARRAY_TYPE) { - d_print_array_type (dpi, mods->mod, mods->next); + d_print_array_type (dpi, mods->mod, mods->next, options); dpi->templates = hold_dpt; return; } @@ -4452,10 +4449,10 @@ d_print_mod_list (struct d_print_info *dpi, hold_modifiers = dpi->modifiers; dpi->modifiers = NULL; - d_print_comp (dpi, d_left (mods->mod)); + d_print_comp (dpi, d_left (mods->mod), options); dpi->modifiers = hold_modifiers; - if ((dpi->options & DMGL_JAVA) == 0) + if ((options & DMGL_JAVA) == 0) d_append_string (dpi, "::"); else d_append_char (dpi, '.'); @@ -4475,24 +4472,24 @@ d_print_mod_list (struct d_print_info *dpi, || dc->type == DEMANGLE_COMPONENT_CONST_THIS) dc = d_left (dc); - d_print_comp (dpi, dc); + d_print_comp (dpi, dc, options); dpi->templates = hold_dpt; return; } - d_print_mod (dpi, mods->mod); + d_print_mod (dpi, mods->mod, options); dpi->templates = hold_dpt; - d_print_mod_list (dpi, mods->next, suffix); + d_print_mod_list (dpi, mods->next, suffix, options); } /* Print a modifier. */ static void -d_print_mod (struct d_print_info *dpi, - const struct demangle_component *mod) +d_print_mod (struct d_print_info *dpi, const struct demangle_component *mod, + int options) { switch (mod->type) { @@ -4510,11 +4507,11 @@ d_print_mod (struct d_print_info *dpi, return; case DEMANGLE_COMPONENT_VENDOR_TYPE_QUAL: d_append_char (dpi, ' '); - d_print_comp (dpi, d_right (mod)); + d_print_comp (dpi, d_right (mod), options); return; case DEMANGLE_COMPONENT_POINTER: /* There is no pointer symbol in Java. */ - if ((dpi->options & DMGL_JAVA) == 0) + if ((options & DMGL_JAVA) == 0) d_append_char (dpi, '*'); return; case DEMANGLE_COMPONENT_REFERENCE: @@ -4532,22 +4529,22 @@ d_print_mod (struct d_print_info *dpi, case DEMANGLE_COMPONENT_PTRMEM_TYPE: if (d_last_char (dpi) != '(') d_append_char (dpi, ' '); - d_print_comp (dpi, d_left (mod)); + d_print_comp (dpi, d_left (mod), options); d_append_string (dpi, "::*"); return; case DEMANGLE_COMPONENT_TYPED_NAME: - d_print_comp (dpi, d_left (mod)); + d_print_comp (dpi, d_left (mod), options); return; case DEMANGLE_COMPONENT_VECTOR_TYPE: d_append_string (dpi, " __vector("); - d_print_comp (dpi, d_left (mod)); + d_print_comp (dpi, d_left (mod), options); d_append_char (dpi, ')'); return; default: /* Otherwise, we have something that won't go back on the modifier stack, so we can just print it. */ - d_print_comp (dpi, mod); + d_print_comp (dpi, mod, options); return; } } @@ -4557,7 +4554,7 @@ d_print_mod (struct d_print_info *dpi, static void d_print_function_type (struct d_print_info *dpi, const struct demangle_component *dc, - struct d_print_mod *mods) + struct d_print_mod *mods, int options) { int need_paren; int need_space; @@ -4615,7 +4612,7 @@ d_print_function_type (struct d_print_info *dpi, hold_modifiers = dpi->modifiers; dpi->modifiers = NULL; - d_print_mod_list (dpi, mods, 0); + d_print_mod_list (dpi, mods, 0, options); if (need_paren) d_append_char (dpi, ')'); @@ -4623,11 +4620,11 @@ d_print_function_type (struct d_print_info *dpi, d_append_char (dpi, '('); if (d_right (dc) != NULL) - d_print_comp (dpi, d_right (dc)); + d_print_comp (dpi, d_right (dc), options); d_append_char (dpi, ')'); - d_print_mod_list (dpi, mods, 1); + d_print_mod_list (dpi, mods, 1, options); dpi->modifiers = hold_modifiers; } @@ -4637,7 +4634,7 @@ d_print_function_type (struct d_print_info *dpi, static void d_print_array_type (struct d_print_info *dpi, const struct demangle_component *dc, - struct d_print_mod *mods) + struct d_print_mod *mods, int options) { int need_space; @@ -4669,7 +4666,7 @@ d_print_array_type (struct d_print_info *dpi, if (need_paren) d_append_string (dpi, " ("); - d_print_mod_list (dpi, mods, 0); + d_print_mod_list (dpi, mods, 0, options); if (need_paren) d_append_char (dpi, ')'); @@ -4681,7 +4678,7 @@ d_print_array_type (struct d_print_info *dpi, d_append_char (dpi, '['); if (d_left (dc) != NULL) - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); d_append_char (dpi, ']'); } @@ -4690,23 +4687,23 @@ d_print_array_type (struct d_print_info *dpi, static void d_print_expr_op (struct d_print_info *dpi, - const struct demangle_component *dc) + const struct demangle_component *dc, int options) { if (dc->type == DEMANGLE_COMPONENT_OPERATOR) d_append_buffer (dpi, dc->u.s_operator.op->name, dc->u.s_operator.op->len); else - d_print_comp (dpi, dc); + d_print_comp (dpi, dc, options); } /* Print a cast. */ static void -d_print_cast (struct d_print_info *dpi, - const struct demangle_component *dc) +d_print_cast (struct d_print_info *dpi, const struct demangle_component *dc, + int options) { if (d_left (dc)->type != DEMANGLE_COMPONENT_TEMPLATE) - d_print_comp (dpi, d_left (dc)); + d_print_comp (dpi, d_left (dc), options); else { struct d_print_mod *hold_dpm; @@ -4724,14 +4721,14 @@ d_print_cast (struct d_print_info *dpi, dpi->templates = &dpt; dpt.template_decl = d_left (dc); - d_print_comp (dpi, d_left (d_left (dc))); + d_print_comp (dpi, d_left (d_left (dc)), options); dpi->templates = dpt.next; if (d_last_char (dpi) == '<') d_append_char (dpi, ' '); d_append_char (dpi, '<'); - d_print_comp (dpi, d_right (d_left (dc))); + d_print_comp (dpi, d_right (d_left (dc)), options); /* Avoid generating two consecutive '>' characters, to avoid the C++ syntactic ambiguity. */ if (d_last_char (dpi) == '>') ^ permalink raw reply [flat|nested] 24+ messages in thread
* obsolete: [rfc 1/2] libiberty/ options code reshuffle [Re: The future of dwarf2_physname] 2011-05-24 21:12 ` [rfc 1/2] libiberty/ options code reshuffle [Re: The future of dwarf2_physname] Jan Kratochvil @ 2011-06-02 15:36 ` Jan Kratochvil 0 siblings, 0 replies; 24+ messages in thread From: Jan Kratochvil @ 2011-06-02 15:36 UTC (permalink / raw) To: gdb-patches obsoleted by: [gcc patch 0/3] libiberty: New DMGL_RET_DROP http://sourceware.org/ml/gdb-patches/2011-06/msg00019.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-05-23 13:17 ` Jan Kratochvil 2011-05-23 19:52 ` Tom Tromey 2011-05-24 21:12 ` [rfc 1/2] libiberty/ options code reshuffle [Re: The future of dwarf2_physname] Jan Kratochvil @ 2011-05-24 21:21 ` Jan Kratochvil 2011-05-25 19:40 ` Keith Seitz ` (2 more replies) 2 siblings, 3 replies; 24+ messages in thread From: Jan Kratochvil @ 2011-05-24 21:21 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches On Mon, 23 May 2011 15:16:59 +0200, Jan Kratochvil wrote: > problem #1 of post-physname GDB > > (gdb) p 'f<char>()' > $1 = {long (void)} 0x4004e4 <f<char>()> > (gdb) p 'long f<char>()' > $2 = {<text variable, no debug info>} 0x4004e4 <f<char>()> > (gdb) p 'f<short>()' > No symbol "f<short>()" in current context. > (gdb) p 'long f<short>()' > $3 = {<text variable, no debug info>} 0x4004fb <long f<short>()> > > ==> dis.h <== > template <typename T> > long f () { return 1; } > ==> dis1.C <== > #include "dis.h" > int main () { f<char> (); } > ==> dis2.C <== > #include "dis.h" > void g () { f<short> (); } > > The two different aliases for 0x4004e4 where the $2 one does not have any type > is a bug, at least both should have the same type. It can be seen also on the > f<short>() case ($3) where the return type missing alias is not present because > only ELF (and not DWARF) symbol is present. So far I have kept this bug of missing type as GDB currently cannot provide multiple search names for a single symbol. And IMO there should exist both `long f<char>()' as it is its demangled ELF (minimal symbol) name and also the `f<char>()' as otherwise user will never find such template function by <tab>-completion from CLI. > I haven't found how to reproduce the regression > "Cannot take address of method m." for function templates (which use the > return type in their linkage) as I haven't found how to reference function > template externally from a class - this field is missing there in such case: Therefore I do not find this double-entry for template function such a real problem, just the return type featured name is missing its real type. > problem #2 of post-physname GDB > > (gdb) p 'int f<long>()' > $1 = {<text variable, no debug info>} 0x40050f <f<long>()> > (gdb) p 'short f<long>()' > $2 = {<text variable, no debug info>} 0x400531 <f<long>()> > (gdb) p 'f<long>()' > $3 = {int (void)} 0x40050f <f<long>()> > > ==> manya.C <== > template <typename T> > int f () { return 4; } > int a () { return f<long> () == 4; } > extern int b (); > int main () { return a () && b () ? 0 : 1; } > ==> manyb.C <== > template <typename T> > short f () { return 2; } > int b () { return f<long> () == 2; } > > As the return type is stripped the name can become ambiguous. Different > template functions with same name, same parameter types but different return > type can be linked in a single program. It is true one cannot declare both > such template functions in a single CU (Compilation Unit) so I do not expect > such case is found in real world. I do not think such symbols exist in real world cases - when they are incompatible in a single CU anyway. Therefore the ambiguity is not a real world problem. > I was trying to propose DMGL_RET_POSTFIX but it has other drawbacks Also after a chat with Tom Tromey I no longer use DMGL_RET_POSTFIX. That is for ELF symbol `long m<char>()' GDB with proposed patch uses `m<char>()' (and not `m<char>()long'). This is compatible with gdb-7.2 (physname) template functions. But I have found GDB requires full DWARF symbols for methods anyway so some of my examples could be a bit unreal, I have to reconsider. Anyway DW_AT_linkage_name following patch with no regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu is attached. It would need GCC push of the libiberty/ change. Thanks, Jan gdb/ 2011-05-24 Jan Kratochvil <jan.kratochvil@redhat.com> Tom Tromey <tromey@redhat.com> * dwarf2read.c (check_physname): New variable. (dwarf2_physname): Prefer DW_AT_linkage_name over dwarf2_compute_name. (show_check_physname): New function. (_initialize_dwarf2_read): Add `check-physname' for check_physname. gdb/testsuite/ 2011-05-24 Jan Kratochvil <jan.kratochvil@redhat.com> * gdb.base/break-interp.exp (reach_1, test_ld): Allow also the prefix __GI_. * gdb.cp/psymtab-parameter.cc (func): Make it a template function. (f): New function. * gdb.cp/psymtab-parameter.exp (complete break 'func(): Rename to ... (complete p 'func<short>(): ... here. * gdb.dwarf2/dw2-linkage-name-trust-main.cc: New file. * gdb.dwarf2/dw2-linkage-name-trust.S: New file. * gdb.dwarf2/dw2-linkage-name-trust.exp: New file. include/ 2011-05-24 Jan Kratochvil <jan.kratochvil@redhat.com> * demangle.h (DMGL_RET_POSTFIX): Extend the comment. (DMGL_RET_DROP): New. * cp-demangle.c (d_print_comp) <DEMANGLE_COMPONENT_FUNCTION_TYPE>: Do not pass DMGL_RET_POSTFIX or DMGL_RET_DROP. Suppress d_print_mod for DMGL_RET_POSTFIX. Support DMGL_RET_DROP. * testsuite/demangle-expected: New testcases for --ret-postfix and --ret-drop. * testsuite/test-demangle.c: Document --ret-drop in a comment. (main): New variable ret_drop, fill it, call cplus_demangle with it. --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -119,6 +119,9 @@ _STATEMENT_PROLOGUE; /* When non-zero, dump DIEs after they are read in. */ static int dwarf2_die_debug = 0; +/* When non-zero, cross-check physname against demangler. */ +static int check_physname = 0; + static int pagesize; /* When set, the file that we're processing is known to have debugging @@ -5147,7 +5150,93 @@ dwarf2_full_name (char *name, struct die_info *die, struct dwarf2_cu *cu) static const char * dwarf2_physname (char *name, struct die_info *die, struct dwarf2_cu *cu) { - return dwarf2_compute_name (name, die, cu, 1); + struct attribute *attr; + const char *retval, *mangled = NULL; + char *canon = NULL; + struct cleanup *back_to; + int need_copy = 1; + + /* In this case dwarf2_compute_name is just a shortcut not building anything + on its own. */ + if (!die_needs_namespace (die, cu)) + return dwarf2_compute_name (name, die, cu, 1); + + back_to = make_cleanup (null_cleanup, NULL); + + attr = dwarf2_attr (die, DW_AT_linkage_name, cu); + if (!attr) + attr = dwarf2_attr (die, DW_AT_MIPS_linkage_name, cu); + + /* DW_AT_linkage_name is missing in some cases - depend on what GDB + has computed. */ + if (attr && DW_STRING (attr)) + { + char *demangled; + + mangled = DW_STRING (attr); + + /* Use DMGL_RET_DROP for C++ template functions to suppress their return + type. It is easier for GDB users to search for such functions as + `name(params)' than `long name(params)'. In such case the minimal + symbol names do not match the full symbol names but for template + functions there is never a need to look up their definition from their + declaration so the only disadvantage remains the minimal symbol + variant `long name(params)' does not have the proper inferior type. + */ + + demangled = cplus_demangle (mangled, (DMGL_PARAMS | DMGL_ANSI + | DMGL_VERBOSE + | (cu->language == language_java + ? DMGL_JAVA | DMGL_RET_POSTFIX + : DMGL_RET_DROP))); + if (demangled) + { + make_cleanup (xfree, demangled); + canon = demangled; + } + else + { + canon = (char *) mangled; + need_copy = 0; + } + } + + if (canon == NULL || check_physname) + { + const char *physname = dwarf2_compute_name (name, die, cu, 1); + + if (canon != NULL && strcmp (physname, canon) != 0) + { + /* It may not mean a bug in GDB. The compiler could also + compute DW_AT_linkage_name incorrectly. But in such case + GDB would need to be bug-to-bug compatible. */ + + complaint (&symfile_complaints, + _("Computed physname <%s> does not match demangled <%s> " + "(from linkage <%s>) - DIE at 0x%x [in module %s]"), + physname, canon, mangled, die->offset, cu->objfile->name); + + /* Prefer DW_AT_linkage_name (in the CANON form) - when it + is available here - over computed PHYSNAME. It is safer + against both buggy GDB and buggy compilers. */ + + retval = canon; + } + else + { + retval = physname; + need_copy = 0; + } + } + else + retval = canon; + + if (need_copy) + retval = obsavestring (retval, strlen (retval), + &cu->objfile->objfile_obstack); + + do_cleanups (back_to); + return retval; } /* Read the import statement specified by the given die and record it. */ @@ -16165,6 +16254,15 @@ show_dwarf2_always_disassemble (struct ui_file *file, int from_tty, value); } +static void +show_check_physname (struct ui_file *file, int from_tty, + struct cmd_list_element *c, const char *value) +{ + fprintf_filtered (file, + _("Whether to check \"physname\" is %s.\n"), + value); +} + void _initialize_dwarf2_read (void); void @@ -16220,6 +16318,14 @@ The value is the maximum depth to print."), NULL, &setdebuglist, &showdebuglist); + add_setshow_boolean_cmd ("check-physname", no_class, &check_physname, _("\ +Set cross-checking of \"physname\" code against demangler."), _("\ +Show cross-checking of \"physname\" code against demangler."), _("\ +When enabled, GDB's internal \"physname\" code is checked against\n\ +the demangler."), + NULL, show_check_physname, + &setdebuglist, &showdebuglist); + c = add_cmd ("gdb-index", class_files, save_gdb_index_command, _("\ Save a gdb-index file.\n\ --- a/gdb/testsuite/gdb.base/break-interp.exp +++ b/gdb/testsuite/gdb.base/break-interp.exp @@ -140,14 +140,14 @@ proc reach_1 {func command displacement} { } exp_continue } - -re "Breakpoint \[0-9\]+, \\.?$func \\(.*\\) at .*:\[0-9\]+\r\n.*$gdb_prompt $" { + -re "Breakpoint \[0-9\]+, \\.?(__GI_)?$func \\(.*\\) at .*:\[0-9\]+\r\n.*$gdb_prompt $" { if {$func == "_dl_debug_state"} { fail $test } else { pass $test } } - -re "Breakpoint \[0-9\]+, \[0-9xa-f\]+ in \\.?$func \\(\\).*\r\n$gdb_prompt $" { + -re "Breakpoint \[0-9\]+, \[0-9xa-f\]+ in \\.?(__GI_)?$func \\(\\).*\r\n$gdb_prompt $" { if {$func == "_dl_debug_state"} { fail $test } else { @@ -403,7 +403,7 @@ proc test_ld {file ifmain trynosym displacement} { reach "_dl_debug_state" "run" $displacement - gdb_test "bt" "#0 +\[^\r\n\]*\\m_dl_debug_state\\M.*" "dl bt" + gdb_test "bt" "#0 +\[^\r\n\]*\\m(__GI_)?_dl_debug_state\\M.*" "dl bt" if $ifmain { reach "main" continue "NONE" --- a/gdb/testsuite/gdb.cp/psymtab-parameter.cc +++ b/gdb/testsuite/gdb.cp/psymtab-parameter.cc @@ -16,7 +16,14 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. */ -typedef int typedefed; -void func (typedefed param) +template <typename T> +long +func () { } + +void +f () +{ + func<short> (); +} --- a/gdb/testsuite/gdb.cp/psymtab-parameter.exp +++ b/gdb/testsuite/gdb.cp/psymtab-parameter.exp @@ -35,5 +35,6 @@ gdb_test_no_output "set language c++" # XFAIL than FAIL here. For example -readnow breaks it. gdb_test_no_output "maintenance info symtabs" -# GDB has shown only the `func(int)' entry before. -gdb_test "complete break 'func(" "break 'func\\(int\\)\r\nbreak 'func\\(typedefed\\)" +# GDB has shown only the `long func<short>()' ELF symbol before, not the DWARF +# symbol +gdb_test "complete p 'func<short>(" "p 'func<short>\\(\\)" --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-linkage-name-trust-main.cc @@ -0,0 +1,41 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2011 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 C + { + public: + static int f (); + }; */ + +asm (".globl cu_text_start"); +asm ("cu_text_start:"); + +int +f (void) +{ + return 31173; +} + +int +main (void) +{ + f (); + return 0; +} + +asm (".globl cu_text_end"); +asm ("cu_text_end:"); --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-linkage-name-trust.S @@ -0,0 +1,134 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2011 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/>. */ + +/* Debug information */ + + .data + .globl c +c: .4byte 0 + + .section .debug_info +.Lcu1_begin: + /* CU header */ + .4byte .Lcu1_end - .Lcu1_start /* Length of Compilation Unit */ +.Lcu1_start: + .2byte 2 /* DWARF Version */ + .4byte .Labbrev1_begin /* Offset into abbrev section */ + .byte 4 /* Pointer size */ + + /* CU die */ + .uleb128 1 /* Abbrev: DW_TAG_compile_unit */ + .4byte cu_text_start /* DW_AT_low_pc */ + .4byte cu_text_end /* DW_AT_high_pc */ + .ascii "file1.txt\0" /* DW_AT_name */ + .ascii "GNU C 3.3.3\0" /* DW_AT_producer */ + .byte 4 /* DW_AT_language (DW_LANG_C_plus_plus) */ + +.Ltype_class: + .uleb128 3 /* Abbrev: DW_TAG_class_type */ + .ascii "C\0" /* DW_AT_name */ + + .uleb128 5 /* Abbrev: DW_TAG_subprogram */ + .byte 1 /* DW_AT_external */ + .ascii "membername\0" /* DW_AT_name */ + .ascii "f\0" /* DW_AT_MIPS_linkage_name */ + .4byte .Ltype_int-.Lcu1_begin /* DW_AT_type */ + + .byte 0 /* End of children of DW_TAG_class_type */ + + .uleb128 4 /* Abbrev: DW_TAG_variable */ + .ascii "c\0" /* DW_AT_name */ + .4byte .Ltype_class-.Lcu1_begin /* DW_AT_type */ + .byte 1 /* DW_AT_external */ + +.Ltype_int: + .uleb128 2 /* Abbrev: DW_TAG_base_type */ + .ascii "int\0" /* DW_AT_name */ + .byte 4 /* DW_AT_byte_size */ + .byte 5 /* DW_AT_encoding */ + + .byte 0 /* End of children of CU */ + +.Lcu1_end: + +/* Abbrev table */ + .section .debug_abbrev +.Labbrev1_begin: + .uleb128 1 /* Abbrev code */ + .uleb128 0x11 /* DW_TAG_compile_unit */ + .byte 1 /* has_children */ + .uleb128 0x11 /* DW_AT_low_pc */ + .uleb128 0x1 /* DW_FORM_addr */ + .uleb128 0x12 /* DW_AT_high_pc */ + .uleb128 0x1 /* DW_FORM_addr */ + .uleb128 0x3 /* DW_AT_name */ + .uleb128 0x8 /* DW_FORM_string */ + .uleb128 0x25 /* DW_AT_producer */ + .uleb128 0x8 /* DW_FORM_string */ + .uleb128 0x13 /* DW_AT_language */ + .uleb128 0xb /* DW_FORM_data1 */ + .byte 0x0 /* Terminator */ + .byte 0x0 /* Terminator */ + + .uleb128 2 /* Abbrev code */ + .uleb128 0x24 /* DW_TAG_base_type */ + .byte 0 /* has_children */ + .uleb128 0x3 /* DW_AT_name */ + .uleb128 0x8 /* DW_FORM_string */ + .uleb128 0xb /* DW_AT_byte_size */ + .uleb128 0xb /* DW_FORM_data1 */ + .uleb128 0x3e /* DW_AT_encoding */ + .uleb128 0xb /* DW_FORM_data1 */ + .byte 0x0 /* Terminator */ + .byte 0x0 /* Terminator */ + + .uleb128 3 /* Abbrev code */ + .uleb128 0x2 /* DW_TAG_class_type */ + .byte 1 /* has_children */ + .uleb128 0x3 /* DW_AT_name */ + .uleb128 0x8 /* DW_FORM_string */ + .byte 0x0 /* Terminator */ + .byte 0x0 /* Terminator */ + + .uleb128 4 /* Abbrev code */ + .uleb128 0x34 /* DW_TAG_variable */ + .byte 0 /* has_children */ + .uleb128 0x3 /* DW_AT_name */ + .uleb128 0x8 /* DW_FORM_string */ + .uleb128 0x49 /* DW_AT_type */ + .uleb128 0x13 /* DW_FORM_ref4 */ + .uleb128 0x3f /* DW_AT_external */ + .uleb128 0xc /* DW_FORM_flag */ + .byte 0x0 /* Terminator */ + .byte 0x0 /* Terminator */ + + .uleb128 5 /* Abbrev code */ + .uleb128 0x2e /* DW_TAG_subprogram */ + .byte 0 /* has_children */ + .uleb128 0x3f /* DW_AT_external */ + .uleb128 0xc /* DW_FORM_flag */ + .uleb128 0x3 /* DW_AT_name */ + .uleb128 0x8 /* DW_FORM_string */ + .uleb128 0x2007 /* DW_AT_MIPS_linkage_name */ + .uleb128 0x8 /* DW_FORM_string */ + .uleb128 0x49 /* DW_AT_type */ + .uleb128 0x13 /* DW_FORM_ref4 */ + .byte 0x0 /* Terminator */ + .byte 0x0 /* Terminator */ + + .byte 0x0 /* Terminator */ + .byte 0x0 /* Terminator */ --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-linkage-name-trust.exp @@ -0,0 +1,55 @@ +# Copyright 2011 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/>. + +# Check that GDB can call C++ functions whose parameters or return values have +# type containing a static member of the same type. + +# Still no C++ compiler is used. +if { [skip_cplus_tests] } { continue } + +load_lib dwarf.exp +# This test can only be run on targets which support DWARF-2 and use gas. +if {![dwarf2_support]} { + return 0 +} + +set testfile "dw2-linkage-name-trust" +set executable ${testfile} +set binfile ${objdir}/${subdir}/${executable} +if { [gdb_compile ${srcdir}/${subdir}/${testfile}-main.cc "${objdir}/${subdir}/${testfile}-main.o" object {c++ debug}] != "" + || [gdb_compile "${srcdir}/${subdir}/${testfile}.S" "${objdir}/${subdir}/${testfile}.o" object {}] != "" + || [gdb_compile "${objdir}/${subdir}/${testfile}-main.o ${objdir}/${subdir}/${testfile}.o" "${binfile}" executable {c++}] != "" } { + return -1 +} + +clean_restart $executable + +if ![runto_main] then { + return -1 +} + +# main is not provided by DWARF. +gdb_test_no_output "set language c++" + +# There are no mangled names in DWARF to suggest the v3 ABI. +gdb_test_no_output "set cp-abi gnu-v3" + +# GDB cannot resolve external member function for which only ELF (and not +# DWARF) symbol is available. Therefore the function `f' must have DWARF which +# confuses it a bit. + +gdb_test "p c.membername" " = {.*} 0x\[0-9a-f\]+ <f\\(\\)>" +gdb_breakpoint "C::membername" +gdb_test "p c.membername ()" "\r\nBreakpoint \[0-9\]+, .*" --- a/include/demangle.h +++ b/include/demangle.h @@ -45,7 +45,12 @@ extern "C" { #define DMGL_VERBOSE (1 << 3) /* Include implementation details. */ #define DMGL_TYPES (1 << 4) /* Also try to demangle type encodings. */ #define DMGL_RET_POSTFIX (1 << 5) /* Print function return types (when - present) after function signature */ + present) after function signature. + It applies only for the toplevel + function type. */ +#define DMGL_RET_DROP (1 << 6) /* Suppress function return types, even + if present. It applies only for the + toplevel function type. */ #define DMGL_AUTO (1 << 8) #define DMGL_GNU (1 << 9) --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -3917,10 +3917,14 @@ d_print_comp (struct d_print_info *dpi, const struct demangle_component *dc, case DEMANGLE_COMPONENT_FUNCTION_TYPE: { if ((options & DMGL_RET_POSTFIX) != 0) - d_print_function_type (dpi, dc, dpi->modifiers, options); + d_print_function_type (dpi, dc, dpi->modifiers, + options & ~(DMGL_RET_POSTFIX | DMGL_RET_DROP)); /* Print return type if present */ - if (d_left (dc) != NULL) + if (d_left (dc) != NULL && (options & DMGL_RET_POSTFIX) != 0) + d_print_comp (dpi, d_left (dc), + options & ~(DMGL_RET_POSTFIX | DMGL_RET_DROP)); + else if (d_left (dc) != NULL && (options & DMGL_RET_DROP) == 0) { struct d_print_mod dpm; @@ -3932,7 +3936,8 @@ d_print_comp (struct d_print_info *dpi, const struct demangle_component *dc, dpm.printed = 0; dpm.templates = dpi->templates; - d_print_comp (dpi, d_left (dc), options); + d_print_comp (dpi, d_left (dc), + options & ~(DMGL_RET_POSTFIX | DMGL_RET_DROP)); dpi->modifiers = dpm.next; @@ -3946,7 +3951,8 @@ d_print_comp (struct d_print_info *dpi, const struct demangle_component *dc, } if ((options & DMGL_RET_POSTFIX) == 0) - d_print_function_type (dpi, dc, dpi->modifiers, options); + d_print_function_type (dpi, dc, dpi->modifiers, + options & ~(DMGL_RET_POSTFIX | DMGL_RET_DROP)); return; } --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -3959,6 +3959,33 @@ f(decltype(nullptr)) --format=gnu-v3 _ZN5aaaaa6bbbbbb5cccccIN23ddddddddddddddddddddddd3eeeENS2_4ffff16ggggggggggggggggENS0_9hhhhhhhhhES6_S6_S6_S6_S6_S6_S6_EE aaaaa::bbbbbb::ccccc<ddddddddddddddddddddddd::eee, ddddddddddddddddddddddd::ffff::gggggggggggggggg, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh, aaaaa::bbbbbb::hhhhhhhhh> +--format=gnu-v3 +_Z5outerIsEcPFilE +char outer<short>(int (*)(long)) +--format=gnu-v3 +_Z5outerPFsiEl +outer(short (*)(int), long) +--format=gnu-v3 +_Z6outer2IsEPFilES1_ +int (*outer2<short>(int (*)(long)))(long) +--format=gnu-v3 --ret-postfix +_Z5outerIsEcPFilE +outer<short>(int (*)(long))char +--format=gnu-v3 --ret-postfix +_Z5outerPFsiEl +outer(short (*)(int), long) +--format=gnu-v3 --ret-postfix +_Z6outer2IsEPFilES1_ +outer2<short>(int (*)(long))int (*)(long) +--format=gnu-v3 --ret-drop +_Z5outerIsEcPFilE +outer<short>(int (*)(long)) +--format=gnu-v3 --ret-drop +_Z5outerPFsiEl +outer(short (*)(int), long) +--format=gnu-v3 --ret-drop +_Z6outer2IsEPFilES1_ +outer2<short>(int (*)(long)) # # Ada (GNAT) tests. # --- a/libiberty/testsuite/test-demangle.c +++ b/libiberty/testsuite/test-demangle.c @@ -159,6 +159,7 @@ exp: %s\n", output is an integer representing ctor_kind. --is-v3-dtor Likewise, but for dtors. --ret-postfix Passes the DMGL_RET_POSTFIX option + --ret-drop Passes the DMGL_RET_DROP option For compatibility, just in case it matters, the options line may be empty, to mean --format=auto. If it doesn't start with --, then it @@ -174,7 +175,7 @@ main(argc, argv) int no_params; int is_v3_ctor; int is_v3_dtor; - int ret_postfix; + int ret_postfix, ret_drop; struct line format; struct line input; struct line expect; @@ -209,6 +210,7 @@ main(argc, argv) no_params = 0; ret_postfix = 0; + ret_drop = 0; is_v3_ctor = 0; is_v3_dtor = 0; if (format.data[0] == '\0') @@ -265,6 +267,8 @@ main(argc, argv) is_v3_dtor = 1; else if (strcmp (opt, "--ret-postfix") == 0) ret_postfix = 1; + else if (strcmp (opt, "--ret-drop") == 0) + ret_drop = 1; else { printf ("FAIL at line %d: unrecognized option %s\n", @@ -307,9 +311,9 @@ main(argc, argv) cplus_demangle_set_style (style); - result = cplus_demangle (inp, - DMGL_PARAMS|DMGL_ANSI|DMGL_TYPES - |(ret_postfix ? DMGL_RET_POSTFIX : 0)); + result = cplus_demangle (inp, (DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES + | (ret_postfix ? DMGL_RET_POSTFIX : 0) + | (ret_drop ? DMGL_RET_DROP : 0))); if (result ? strcmp (result, expect.data) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-05-24 21:21 ` [rfc 2/2] Follow DW_AT_linkage_name for methods " Jan Kratochvil @ 2011-05-25 19:40 ` Keith Seitz 2011-05-25 20:42 ` Tom Tromey 2011-05-25 20:40 ` Tom Tromey 2011-06-02 19:03 ` obsolete: " Jan Kratochvil 2 siblings, 1 reply; 24+ messages in thread From: Keith Seitz @ 2011-05-25 19:40 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches I don't know if you are waiting on me for a reply on this or not, but I thought I would throw out my fractional pennies. On 05/24/2011 02:21 PM, Jan Kratochvil wrote: > Also after a chat with Tom Tromey I no longer use DMGL_RET_POSTFIX. That is > for ELF symbol `long m<char>()' GDB with proposed patch uses `m<char>()' > (and not `m<char>()long'). This is compatible with gdb-7.2 (physname) > template functions. I am relieved to hear this (mis?)feature is staying put. I'm a practical kind of person, and I know I'd really *hate* to have to type "break 'int blah<int>(blah)'" every time. Yuck. As for how to address the (remote?) possibility that we will need a return type, I'd like to offer the idea that we might introduce some sort of qualifier flag, for example, "break blah<int>/int" or "break/int blah<int>" or something along those lines. Of course, this is well beyond the scope of 7.3. Just thought I'd toss this out there, though, as a starting point should this actually become a problem. > Anyway DW_AT_linkage_name following patch with no regressions on > {x86_64,x86_64-m32,i686}-fedora15-linux-gnu is attached. It would need GCC > push of the libiberty/ change. I'll reiterate what I wrote to Tom yesterday, "I'm all for it." Keith ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-05-25 19:40 ` Keith Seitz @ 2011-05-25 20:42 ` Tom Tromey 0 siblings, 0 replies; 24+ messages in thread From: Tom Tromey @ 2011-05-25 20:42 UTC (permalink / raw) To: Keith Seitz; +Cc: Jan Kratochvil, gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> As for how to address the (remote?) possibility that we will need a Keith> return type, I'd like to offer the idea that we might introduce some Keith> sort of qualifier flag, for example, "break blah<int>/int" or Keith> "break/int blah<int>" or something along those lines. Of course, this Keith> is well beyond the scope of 7.3. Just thought I'd toss this out there, Keith> though, as a starting point should this actually become a problem. If we are adding linespec syntax, it seems to me we should just recognize the return type as well: 'break int templatefunction()'. This is immediately recognizable to the C++ programmer and preserve the "nm -C cut-and-paste" property as well. Only if this cannot be done for some reason do I think we should consider gdb-specific alternatives. Tom ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-05-24 21:21 ` [rfc 2/2] Follow DW_AT_linkage_name for methods " Jan Kratochvil 2011-05-25 19:40 ` Keith Seitz @ 2011-05-25 20:40 ` Tom Tromey 2011-06-01 18:35 ` Keith Seitz 2011-06-02 19:03 ` obsolete: " Jan Kratochvil 2 siblings, 1 reply; 24+ messages in thread From: Tom Tromey @ 2011-05-25 20:40 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Keith Seitz, gdb-patches >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes: Jan> Also after a chat with Tom Tromey I no longer use DMGL_RET_POSTFIX. Jan> That is for ELF symbol `long m<char>()' GDB with proposed patch Jan> uses `m<char>()' (and not `m<char>()long'). This is compatible Jan> with gdb-7.2 (physname) template functions. This patch looks pretty good to me. Keith, are there parts of your series which are still needed for 7.3 if this patch is committed there? The new command probably needs documentation (it is unclear to me whether we document, or even whether we should document, "set debug" commands). Jan> + canon = (char *) mangled; I think 'canon' can be made const and this cast reomved. Tom ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-05-25 20:40 ` Tom Tromey @ 2011-06-01 18:35 ` Keith Seitz 2011-06-02 16:26 ` Jan Kratochvil 0 siblings, 1 reply; 24+ messages in thread From: Keith Seitz @ 2011-06-01 18:35 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 05/25/2011 01:40 PM, Tom Tromey wrote: >>>>>> "Jan" == Jan Kratochvil<jan.kratochvil@redhat.com> writes: > > Jan> Also after a chat with Tom Tromey I no longer use DMGL_RET_POSTFIX. > Jan> That is for ELF symbol `long m<char>()' GDB with proposed patch > Jan> uses `m<char>()' (and not `m<char>()long'). This is compatible > Jan> with gdb-7.2 (physname) template functions. > > This patch looks pretty good to me. > > Keith, are there parts of your series which are still needed for 7.3 if > this patch is committed there? If we want to fix 12266, yes, we're still going to need much of the patchset: cp_canonicalize_no_typedefs, printname, and the cp_demangled_name_parse_free patches. Those patches fix 12266 and a bunch of related problems (which are not regressions): *** Using pre-physname: (gdb) b calltest(foo) Function "calltest(foo)" not defined. Make breakpoint pending on future shared library load? (y or [n]) n (gdb) b calltest(std::string) Breakpoint 1 at 0x40075c: file 12266.cc, line 5. (gdb) b calltest(std::basic_string<char, std::char_traits<char>, std::allocator<char> >) Function "calltest(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)" not defined. Make breakpoint pending on future shared library load? (y or [n]) n *** Using DW_AT_linkage_name (Jan's patches): (gdb) b calltest(foo) Function "calltest(foo)" not defined. Make breakpoint pending on future shared library load? (y or [n]) n (gdb) b calltest(std::string) Function "calltest(std::string)" not defined. Make breakpoint pending on future shared library load? (y or [n]) n (gdb) b calltest(std::basic_string<char, std::char_traits<char>, std::allocator<char> >) Breakpoint 1 at 0x40075c: file 12266.cc, line 5. *** Using 12266 patches (on top of Jan's DW_AT_linkage_name patches): (gdb) b calltest(foo) Breakpoint 1 at 0x40075c: file 12266.cc, line 5. (gdb) b calltest(std::string) Note: breakpoint 1 also set at pc 0x40075c. Breakpoint 2 at 0x40075c: file 12266.cc, line 5. (gdb) b calltest(std::basic_string<char, std::char_traits<char>, std::allocator<char> >) Note: breakpoints 1 and 2 also set at pc 0x40075c. Breakpoint 3 at 0x40075c: file 12266.cc, line 5. Mind you, I don't know if I would recommend taking in such a big patch this late in the game... Nonetheless, I hope to have this resubmitted today. Keith ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-06-01 18:35 ` Keith Seitz @ 2011-06-02 16:26 ` Jan Kratochvil 2011-06-02 18:20 ` Tom Tromey 0 siblings, 1 reply; 24+ messages in thread From: Jan Kratochvil @ 2011-06-02 16:26 UTC (permalink / raw) To: Keith Seitz; +Cc: Tom Tromey, gdb-patches On Wed, 01 Jun 2011 20:35:15 +0200, Keith Seitz wrote: > *** Using pre-physname: [...] > (gdb) b calltest(std::string) > Breakpoint 1 at 0x40075c: file 12266.cc, line 5. [...] > *** Using DW_AT_linkage_name (Jan's patches): [...] > (gdb) b calltest(std::string) > Function "calltest(std::string)" not defined. > Make breakpoint pending on future shared library load? (y or [n]) n [...] > *** Using 12266 patches (on top of Jan's DW_AT_linkage_name patches): [...] > (gdb) b calltest(std::string) > Note: breakpoint 1 also set at pc 0x40075c. > Breakpoint 2 at 0x40075c: file 12266.cc, line 5. [...] This is a regression by the physname patch which introduced DMGL_VERBOSE in symbol_find_demangled_name, some other patches of mine being posted are going to remove DMGL_VERBOSE there. Thanks, Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-06-02 16:26 ` Jan Kratochvil @ 2011-06-02 18:20 ` Tom Tromey 2011-06-02 18:28 ` Jan Kratochvil 0 siblings, 1 reply; 24+ messages in thread From: Tom Tromey @ 2011-06-02 18:20 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Keith Seitz, gdb-patches >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes: Jan> This is a regression by the physname patch which introduced Jan> DMGL_VERBOSE in symbol_find_demangled_name, some other patches of Jan> mine being posted are going to remove DMGL_VERBOSE there. I infer this to mean that, with your DW_AT_linkage_name patch, we do not need Keith's series for 7.3. Is that correct? Tom ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-06-02 18:20 ` Tom Tromey @ 2011-06-02 18:28 ` Jan Kratochvil 0 siblings, 0 replies; 24+ messages in thread From: Jan Kratochvil @ 2011-06-02 18:28 UTC (permalink / raw) To: Tom Tromey; +Cc: Keith Seitz, gdb-patches On Thu, 02 Jun 2011 20:20:01 +0200, Tom Tromey wrote: > >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes: > > Jan> This is a regression by the physname patch which introduced > Jan> DMGL_VERBOSE in symbol_find_demangled_name, some other patches of > Jan> mine being posted are going to remove DMGL_VERBOSE there. > > I infer this to mean that, with your DW_AT_linkage_name patch, we do not > need Keith's series for 7.3. Is that correct? For ctors/dtors (and for non-GCC compilers) DW_AT_linkage_name does not exist and it falls back to the physname code. Some other former code dealing with ctors/dtors in symtabs before physname code was introduced has been already removed as obsoleted by physname. Therefore I believe any physname improvements still have effect for ctors/dtors handling. Thanks, Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* obsolete: [rfc 2/2] Follow DW_AT_linkage_name for methods [Re: The future of dwarf2_physname] 2011-05-24 21:21 ` [rfc 2/2] Follow DW_AT_linkage_name for methods " Jan Kratochvil 2011-05-25 19:40 ` Keith Seitz 2011-05-25 20:40 ` Tom Tromey @ 2011-06-02 19:03 ` Jan Kratochvil 2 siblings, 0 replies; 24+ messages in thread From: Jan Kratochvil @ 2011-06-02 19:03 UTC (permalink / raw) To: gdb-patches obsoleted/reposted by: [patch] Follow DW_AT_linkage_name for methods #2 http://sourceware.org/ml/gdb-patches/2011-06/msg00034.html ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2011-06-02 19:03 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-05-18 22:35 The future of dwarf2_physname Keith Seitz 2011-05-19 19:23 ` Jan Kratochvil 2011-05-20 19:53 ` Keith Seitz 2011-05-20 20:38 ` Jan Kratochvil 2011-05-20 20:39 ` Tom Tromey 2011-05-21 20:37 ` Daniel Jacobowitz 2011-05-19 21:00 ` Daniel Jacobowitz 2011-05-20 19:26 ` Tom Tromey 2011-05-21 20:34 ` Daniel Jacobowitz 2011-05-20 19:10 ` Tom Tromey 2011-05-23 13:17 ` Jan Kratochvil 2011-05-23 19:52 ` Tom Tromey 2011-05-23 19:57 ` Keith Seitz 2011-05-24 21:12 ` [rfc 1/2] libiberty/ options code reshuffle [Re: The future of dwarf2_physname] Jan Kratochvil 2011-06-02 15:36 ` obsolete: " Jan Kratochvil 2011-05-24 21:21 ` [rfc 2/2] Follow DW_AT_linkage_name for methods " Jan Kratochvil 2011-05-25 19:40 ` Keith Seitz 2011-05-25 20:42 ` Tom Tromey 2011-05-25 20:40 ` Tom Tromey 2011-06-01 18:35 ` Keith Seitz 2011-06-02 16:26 ` Jan Kratochvil 2011-06-02 18:20 ` Tom Tromey 2011-06-02 18:28 ` Jan Kratochvil 2011-06-02 19:03 ` obsolete: " Jan Kratochvil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox