[PATCH 5/5] Fix for D demangling in GDB

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
This both fixes D demangling in GDB and updates it to the current D
mangling spec.

2014-01-07  Iain Buclaw  <[hidden email]>

    gdb/

        * d-lang.h (d_parse_symbol): Add declaration.
        * d-lang.c (extract_identifiers)
        (extract_type_info): Remove functions.
        (d_demangle): Use d_parse_symbol implemented in d-support.c to
        demangle D symbols.
        * d-support.c: New file.

    gdb/testsuite/

        * gdb.dlang/demangle.exp: New file.

---

dlang-p5.patch (51K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Tom Tromey
>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:

Iain>         * d-lang.h (d_parse_symbol): Add declaration.
Iain>         * d-lang.c (extract_identifiers)
Iain>         (extract_type_info): Remove functions.
Iain>         (d_demangle): Use d_parse_symbol implemented in d-support.c to
Iain>         demangle D symbols.
Iain>         * d-support.c: New file.

The usual approach in cases like this is to do a "pure move" patch to
move the functions to another file, followed by a second patch to
implement the fixes.

It's also worth noting that with a bit more work you could push the D
demangler into libiberty (see ada_demangle there) and then get
demangling from "nm" and the other binutils.

Iain> +proc catch_demangling_errors {command} {
Iain> +    if {[catch $command result]} {
Iain> + puts "ERROR: demangle.exp: while running $command: $result"
Iain> +    }
Iain> +}

Iain> +    # Using catch_demangling_errors this way ensures that, if one of
Iain> +    # the functions raises a Tcl error, then it'll get reported, and
Iain> +    # the rest of the functions will still run.
Iain> +    catch_demangling_errors test_d_demangling

I don't think this stuff is needed.  Usually we just let Tcl errors keep
going, since ordinarily they represent bugs in the test case.  Is there
a particular failure you were seeing?

Iain> +
Iain> +} else {
Iain> +    warning "D demangling tests suppressed."

I think "unsupported" instead of "warning" here.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
On 9 January 2014 21:54, Tom Tromey <[hidden email]> wrote:

>>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:
>
> Iain>         * d-lang.h (d_parse_symbol): Add declaration.
> Iain>         * d-lang.c (extract_identifiers)
> Iain>         (extract_type_info): Remove functions.
> Iain>         (d_demangle): Use d_parse_symbol implemented in d-support.c to
> Iain>         demangle D symbols.
> Iain>         * d-support.c: New file.
>
> The usual approach in cases like this is to do a "pure move" patch to
> move the functions to another file, followed by a second patch to
> implement the fixes.
>

OK, I'll split it into two separate patches.

> It's also worth noting that with a bit more work you could push the D
> demangler into libiberty (see ada_demangle there) and then get
> demangling from "nm" and the other binutils.
>

That sounds like a good plan.  I'll keep a note to get round to do that.


> Iain> +proc catch_demangling_errors {command} {
> Iain> +    if {[catch $command result]} {
> Iain> + puts "ERROR: demangle.exp: while running $command: $result"
> Iain> +    }
> Iain> +}
>
> Iain> +    # Using catch_demangling_errors this way ensures that, if one of
> Iain> +    # the functions raises a Tcl error, then it'll get reported, and
> Iain> +    # the rest of the functions will still run.
> Iain> +    catch_demangling_errors test_d_demangling
>
> I don't think this stuff is needed.  Usually we just let Tcl errors keep
> going, since ordinarily they represent bugs in the test case.  Is there
> a particular failure you were seeing?
>

This was copied from cp-demangle.exp.  I believe it is written that
way so that all demangle tests are ran, rather than stopping at the
first error?
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
On 10 January 2014 13:24, Iain Buclaw <[hidden email]> wrote:
> On 9 January 2014 21:54, Tom Tromey <[hidden email]> wrote:
>> The usual approach in cases like this is to do a "pure move" patch to
>> move the functions to another file, followed by a second patch to
>> implement the fixes.
>>
>
> OK, I'll split it into two separate patches.
>

Couldn't find an easy way to move, then fix.  So I did fix, then move.

First change, fixes D demangling in GDB as per first post.

2014-01-10  Iain Buclaw  <[hidden email]>

    gdb/

        * d-lang.h (d_parse_symbol): Add declaration.
        * d-lang.c (extract_identifiers)
        (extract_type_info): Remove functions.
        (parse_call_convention, parse_attributes)
        (parse_function_types, parse_function_args)
        (parse_type, parse_identifier, call_convention_p)
        (d_parse_symbol): New functions.
        (d_demangle): Use d_parse_symbol to demangle D symbols.

    gdb/testsuite/

        * gdb.dlang/demangle.exp: New file.

---

dlang-p5a.patch (48K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
In reply to this post by Iain Buclaw
On 10 January 2014 13:24, Iain Buclaw <[hidden email]> wrote:
> On 9 January 2014 21:54, Tom Tromey <[hidden email]> wrote:
>> The usual approach in cases like this is to do a "pure move" patch to
>> move the functions to another file, followed by a second patch to
>> implement the fixes.
>>
>
> OK, I'll split it into two separate patches.
>

Second change, move D demangling routines out of d-lang.c into
d-support.c, which is intended to house other language support
functions that don't really have a home elsewhere.


2014-01-10  Iain Buclaw  <[hidden email]>

    gdb/

        * d-lang.c (parse_call_convention)
        (parse_attributes, parse_function_types)
        (parse_function_args, parse_type, parse_identifier)
        (call_convention_p, d_parse_symbol): Move functions to ...
        * d-support.c: ... New file.

---

dlang-p5b.patch (44K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Tom Tromey
In reply to this post by Iain Buclaw
>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:

Tom> It's also worth noting that with a bit more work you could push the D
Tom> demangler into libiberty (see ada_demangle there) and then get
Tom> demangling from "nm" and the other binutils.

Iain> That sounds like a good plan.  I'll keep a note to get round to do that.

Just FYI - I'm not sure if you know this or not, but libiberty is
canonically maintained in the GCC tree, so if you do this, it has to be
submitted there first.  Then it will be merged (either by me, or by
whoever else seems to be doing (semi-)automated merges) into
binutils-gdb.git.  So, it's a little bit of a pain.

Iain> This was copied from cp-demangle.exp.  I believe it is written that
Iain> way so that all demangle tests are ran, rather than stopping at the
Iain> first error?

The C++ one only works proc-by-proc.  If a test fails with a Tcl error
-- which btw isn't the same as just an ordinary failure, those don't
cause particular problems -- then it will run the subsequent procs.
Your test file only has a single proc; and anyway I'm guessing that code
in the C++ test is not useful anyway.  I think dropping it from your
patch is safe.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Tom Tromey
In reply to this post by Iain Buclaw
>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:

Iain> Couldn't find an easy way to move, then fix.  So I did fix, then move.

Thank you.

Iain> +/* Demangle the calling convention from MANGLE and append it to TEMPBUF.
Iain> +   Return the remaining string on success or NULL on failure.  */
Iain> +static const char *
Iain> +parse_call_convention (struct obstack *tempbuf, const char *mangle)

Blank line between comment and start of function; here and elsewhere.

Iain> +}
Iain> +
Iain>  static int
Iain> -extract_type_info (const char *mangled_str, struct obstack *tempbuf)
Iain> +call_convention_p (const char *mangle)
Iain>  {

Needs an intro comment.

Iain> +      if (mangle && call_convention_p (mangle))

We're trying to use the explicit checking form these days, so
"if (mangle != NULL && ..."

I didn't check the rest of the patch for this nit -- could you take a
look?


Other than the nits and the remaining test suite thing, this looks good.
Thanks again for splitting the patch.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Tom Tromey
In reply to this post by Iain Buclaw
>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:

Iain> Second change, move D demangling routines out of d-lang.c into
Iain> d-support.c, which is intended to house other language support
Iain> functions that don't really have a home elsewhere.

Thanks.

Iain>         * d-lang.c (parse_call_convention)
Iain>         (parse_attributes, parse_function_types)
Iain>         (parse_function_args, parse_type, parse_identifier)
Iain>         (call_convention_p, d_parse_symbol): Move functions to ...
Iain>         * d-support.c: ... New file.

This should mention the Makefile.in change, like:

        * Makefile.in (SFILES): Add d-support.c.
        (COMMON_OBS): Add d-support.o.

... and the d-lang.h change, even though it is trivial.

Otherwise the patch is fine.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
In reply to this post by Tom Tromey
On 10 January 2014 21:22, Tom Tromey <[hidden email]> wrote:

>>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:
>
> Tom> It's also worth noting that with a bit more work you could push the D
> Tom> demangler into libiberty (see ada_demangle there) and then get
> Tom> demangling from "nm" and the other binutils.
>
> Iain> That sounds like a good plan.  I'll keep a note to get round to do that.
>
> Just FYI - I'm not sure if you know this or not, but libiberty is
> canonically maintained in the GCC tree, so if you do this, it has to be
> submitted there first.  Then it will be merged (either by me, or by
> whoever else seems to be doing (semi-)automated merges) into
> binutils-gdb.git.  So, it's a little bit of a pain.
>

Incidentally, I have a assignment form for GCC that has been sitting
in my inbox waiting to be signed and sent off.  This will probably
give me an excuse to get round to do that.

I've ported this demangler over to libiberty, changing obstack ->
string where appropriate.  Whilst happy at the result, I think I'll
wait until I've finished off demangling D templates. Which is the only
part of demangling where things get a bit hairy (and also why I chose
to move demangling in a separate file).


> Iain> This was copied from cp-demangle.exp.  I believe it is written that
> Iain> way so that all demangle tests are ran, rather than stopping at the
> Iain> first error?
>
> The C++ one only works proc-by-proc.  If a test fails with a Tcl error
> -- which btw isn't the same as just an ordinary failure, those don't
> cause particular problems -- then it will run the subsequent procs.
> Your test file only has a single proc; and anyway I'm guessing that code
> in the C++ test is not useful anyway.  I think dropping it from your
> patch is safe.
>


OK, thanks for the explaination.

Regards
Iain.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
In reply to this post by Tom Tromey
On 10 January 2014 21:43, Tom Tromey <[hidden email]> wrote:
>
> Other than the nits and the remaining test suite thing, this looks good.
> Thanks again for splitting the patch.
>
> Tom

Updated with nits, testsuite, and explicit null pointer checks.

Changelog entry same as before:

    gdb/

        * d-lang.h (d_parse_symbol): Add declaration.
        * d-lang.c (extract_identifiers)
        (extract_type_info): Remove functions.
        (parse_call_convention, parse_attributes)
        (parse_function_types, parse_function_args)
        (parse_type, parse_identifier, call_convention_p)
        (d_parse_symbol): New functions.
        (d_demangle): Use d_parse_symbol to demangle D symbols.

    gdb/testsuite/

        * gdb.dlang/demangle.exp: New file.

---

dlang-p5a.patch (48K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
In reply to this post by Tom Tromey
On 10 January 2014 21:45, Tom Tromey <[hidden email]> wrote:

>
> This should mention the Makefile.in change, like:
>
>         * Makefile.in (SFILES): Add d-support.c.
>         (COMMON_OBS): Add d-support.o.
>
> ... and the d-lang.h change, even though it is trivial.
>
> Otherwise the patch is fine.
>
> Tom
And patch to move functions to d-support.c. Updated from previous
patch changes, and ChangeLog entries added.


2014-01-11  Iain Buclaw  <[hidden email]>

        * Makefile.in (SFILES): Add d-support.c.
        (COMMON_OBS): Add d-support.o.
        * d-lang.h (d_parse_symbol): Add comment, now defined in
        d-support.c.
        * d-lang.c (parse_call_convention)
        (parse_attributes, parse_function_types)
        (parse_function_args, parse_type, parse_identifier)
        (call_convention_p, d_parse_symbol): Move functions to ...
        * d-support.c: ... New file.

---

dlang-p5b.patch (45K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Tom Tromey
>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:

Iain> 2014-01-11  Iain Buclaw  <[hidden email]>
Iain>         * Makefile.in (SFILES): Add d-support.c.
Iain>         (COMMON_OBS): Add d-support.o.
Iain>         * d-lang.h (d_parse_symbol): Add comment, now defined in
Iain>         d-support.c.
Iain>         * d-lang.c (parse_call_convention)
Iain>         (parse_attributes, parse_function_types)
Iain>         (parse_function_args, parse_type, parse_identifier)
Iain>         (call_convention_p, d_parse_symbol): Move functions to ...
Iain>         * d-support.c: ... New file.

Thanks, this is ok.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/5] Fix for D demangling in GDB

Iain Buclaw
On 13 January 2014 20:04, Tom Tromey <[hidden email]> wrote:

>>>>>> "Iain" == Iain Buclaw <[hidden email]> writes:
>
> Iain> 2014-01-11  Iain Buclaw  <[hidden email]>
> Iain>         * Makefile.in (SFILES): Add d-support.c.
> Iain>         (COMMON_OBS): Add d-support.o.
> Iain>         * d-lang.h (d_parse_symbol): Add comment, now defined in
> Iain>         d-support.c.
> Iain>         * d-lang.c (parse_call_convention)
> Iain>         (parse_attributes, parse_function_types)
> Iain>         (parse_function_args, parse_type, parse_identifier)
> Iain>         (call_convention_p, d_parse_symbol): Move functions to ...
> Iain>         * d-support.c: ... New file.
>
> Thanks, this is ok.
>
> Tom

Thanks, now all done!

Regards
Iain