Re: [PATCH] gdb: Remove support for old mangling schemes

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2019-01-08 19:24, Keith Seitz wrote:
> On 1/8/19 3:22 PM, Simon Marchi wrote:
>> An upcoming sync with gcc's libiberty [1] will remove support for old
>> mangling schemes (GNU v2, Lucid, ARM, HP and EDG).  It will remove the
>> cplus_demangle_opname function, so we need to get rid of its usages in
>> GDB (it's a GNU v2 specific function).
> 
> Thank you so much for doing this. I was working on my own version of 
> this
> before other work got in the way.

Thanks for looking at mine then!

> Your patch looks just about identical to mine, and I had a few 
> additional
> questions about my own patch that I needed to answer.
> 
> I would think this should have/deserve doc and NEWS changes.

I agree.

> The manual talks about several demangling-related features which
> raised questions for me about the general support for gnu-v2 code. If
> libiberty is removing support for v2 demangling, how useful will GDB be
> without the ability to demangle v2 symbols? Should we deprecate or 
> remove
> that, too? /me opens can of worms

Yes, probably.  The goal of this patch is to unblock the sync'ing with 
gcc's libiberty (remove usages of cplus_demangle_opname), but we can 
certainly do some cleanup later.

> WRT docs specifically, "set cp-abi" and "set demangling-style" still 
> carry
> either direct or indirect mentions of the different ABIs/mangling 
> schemes in
> the documentation that are now no longer valid, e.g., "lucid" and 
> "arm".
> Some comments in demangle.c do, too.

Well, this patch has the effect of removing those demangling styles, so 
it should certainly update the doc for "set demangle-style".  But it 
doesn't touch the cp-abi, so I won't change that part in this patch 
(although as you noted, gnu-v2 will probably be useless if binaries with 
this ABI also use the "gnu" demangling style).

> The help system references that exist today/right now, I presume, will 
> disappear
> when the libiberty patch goes back in (when libiberty_demanglers is 
> updated), so
> I *think* there's nothing to do there.

Indeed, "set demangle-style" and "set demande-style<TAB>" are both 
automatically updated.

> Now the bonus. :-)
> 
>> diff --git a/gdb/testsuite/gdb.cp/demangle.exp 
>> b/gdb/testsuite/gdb.cp/demangle.exp
>> index 698231b82b4..9231b54f787 100644
>> --- a/gdb/testsuite/gdb.cp/demangle.exp
>> +++ b/gdb/testsuite/gdb.cp/demangle.exp
>> -
>> -
>>  proc catch_demangling_errors {command} {
>>      if {[catch $command result]} {
>>  	puts "ERROR: demangle.exp: while running $command: $result"
>> @@ -1593,10 +183,7 @@ proc do_tests {} {
>>      # Using catch_demangling_errors this way ensures that, if one of
>>      # the functions raises a Tcl error, then it'll get reported, and
>>      # the rest of the functions will still run.
>> -    catch_demangling_errors test_lucid_style_demangling
>> -    catch_demangling_errors test_gnu_style_demangling
>> -    catch_demangling_errors test_arm_style_demangling
>> -    catch_demangling_errors test_hp_style_demangling
>> +    catch_demangling_errors test_gnuv3_style_demangling
>> 
> 
> Please consider getting rid of catch_demangling_errors and removing the
> "error" calls in set_demangling_style. They are not needed, and in 
> fact,
> they cause the test suite to hang if that error is ever called. I ran
> across this while prepping my patch, and planned to remove it.
> 
> Of course, this (nit) is sorta orthogonal, so please feel free to ask
> me to do it!
> ;-)

Right, I noticed the hang too.  I was thinking of doing a pass of 
cleanup/modernization on that file after, as I spotted some other things 
(such as unused "global").  I won't ask you to do it nor prevent you :P.

Simon