Page MenuHomePhabricator

Support demangling for D symbols via dlopen
Needs ReviewPublic

Authored by timotheecour on Mar 9 2018, 12:28 PM.

Details

Summary

moved from: https://github.com/llvm-mirror/lldb/pull/3

addresses issue https://issues.dlang.org/show_bug.cgi?id=817 OSX

symbols mangled on gdb,ggdb,cgdb,lldb but not on ubuntu; no line numbers on stacktraces
also, a follow-up to https://reviews.llvm.org/D24794 (Use Clang for D language support until there is a proper language plugin for it.)
related discussion prior to this: http://lists.llvm.org/pipermail/lldb-dev/2016-September/011351.html [lldb-dev] Adding D language demangling support

to try it out, you need lldbdplugin

the shared library that is dlopen'd is defined separately in https://github.com/timotheecour/dtools/blob/master/dtools/lldbdplugin.d (but could be any other implementation with same entry point API)
add this to .lldbinit:
settings set plugin.language.D.pluginfile "path_to/liblldbdplugin.dylib"

before:

(lldb) bt
error: need to add support for DW_TAG_base_type 'immutable(char)' encoded with DW_ATE = 0x10, bit_size = 8
error: need to add support for DW_TAG_base_type 'char' encoded with DW_ATE = 0x10, bit_size = 8
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100000cd0 test TIMBREAK at foo/test2.d:3
    frame #1: 0x0000000100000e60 test _D4test3funFiZv(a=11) + 128 at test.d:11
    frame #2: 0x0000000100000ec7 test _D4test__T4fun1TiZQiFiZv(a=11) + 23 at test.d:16
    frame #3: 0x0000000100000e8c test _Dmain(args=string[] @ 0x00007ffeefbfcec8) + 28 at test.d:20
    frame #4: 0x000000010014d80f test _D2rt6dmain211_d_run_mainUiPPaPUAAaZiZ6runAllMFZ9__lambda1MFZv + 15
    frame #5: 0x000000010014d74e test _D2rt6dmain211_d_run_mainUiPPaPUAAaZiZ7tryExecMFMDFZvZv + 14
    frame #6: 0x000000010014d652 test _d_run_main + 466
    frame #7: 0x00000001000019b5 test main(argc=1, argv=0x00007ffeefbfd028) + 37 at __entrypoint.d:8
    frame #8: 0x00007fff7279f115 libdyld.dylib start + 1
    frame #9: 0x00007fff7279f115 libdyld.dylib start + 1
(lldb)

after:

(lldb) bt
error: need to add support for DW_TAG_base_type 'immutable(char)' encoded with DW_ATE = 0x10, bit_size = 8
error: need to add support for DW_TAG_base_type 'char' encoded with DW_ATE = 0x10, bit_size = 8
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100000cd0 test TIMBREAK at foo/test2.d:3
    frame #1: 0x0000000100000e60 test void test.fun(a=11) + 128 at test.d:11
    frame #2: 0x0000000100000ec7 test void test.fun1!(a=11).fun1(int) + 23 at test.d:16
    frame #3: 0x0000000100000e8c test _Dmain(args=string[] @ 0x00007ffeefbfce68) + 28 at test.d:20
    frame #4: 0x000000010014d80f test void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll().__lambda1() + 15
    frame #5: 0x000000010014d74e test void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) + 14
    frame #6: 0x000000010014d652 test _d_run_main + 466
    frame #7: 0x00000001000019b5 test main(argc=1, argv=0x00007ffeefbfcfc8) + 37 at __entrypoint.d:8
    frame #8: 0x00007fff7279f115 libdyld.dylib start + 1
    frame #9: 0x00007fff7279f115 libdyld.dylib start + 1
    1. limitations:
  • breakpoint on fully qualified names work, eg: b test.fun but not on templates, eg: b test.fun1
  • why is LLDB tag marked as archived?
  • I couldn't find lldb as an option in Repository

Event Timeline

timotheecour created this revision.Mar 9 2018, 12:28 PM
timotheecour edited the summary of this revision. (Show Details)Mar 9 2018, 12:39 PM
timotheecour added a project: Restricted Project.
timotheecour edited the summary of this revision. (Show Details)
timotheecour edited the summary of this revision. (Show Details)Mar 9 2018, 12:48 PM
timotheecour edited the summary of this revision. (Show Details)Mar 9 2018, 1:11 PM
timotheecour set the repository for this revision to rL LLVM.
timotheecour added a subscriber: lldb-commits.
timotheecour edited the summary of this revision. (Show Details)Mar 9 2018, 1:17 PM
johanengelen added inline comments.Mar 9 2018, 1:40 PM
source/Plugins/Language/D/DLanguage.cpp
2

fix header, and also need to clang-format the file

21

add comment about the purpose of these fwd decls.

25

Did you look into using llvm/Support/DynamicLibrary.h ?
That would make the code crossplatform (notably: Windows).

36

typo: "if"

109

Would it help to initialize druntime using a static module constructor in the lldbdplugin dll?
(then you can also do de-init using a static module destructor)

source/Plugins/Language/D/DLanguage.h
2

fix header line

davide requested changes to this revision.Mar 9 2018, 1:56 PM
davide added a subscriber: davide.

This patch has no testcase. It shouldn't be particularly hard to write one, you can take inspiration from the one in lit/.

Thanks!

This revision now requires changes to proceed.Mar 9 2018, 1:56 PM
timotheecour marked 4 inline comments as done.
timotheecour edited the summary of this revision. (Show Details)
  • format
  • format
  • fixup
timotheecour added inline comments.Mar 10 2018, 1:54 PM
source/Plugins/Language/D/DLanguage.cpp
109

I don't really like static module constructor because it adds cyclic dependencies, see for vibe.d moving away from it: https://forum.dlang.org/post/qtabwblpaqwpteystwft@forum.dlang.org
explicit calling d_initialize is simple enough.

timotheecour marked an inline comment as done.Mar 10 2018, 1:55 PM
  • use llvm::sys::DynamicLibrary
  • format
timotheecour marked an inline comment as done.Mar 10 2018, 2:23 PM
timotheecour added inline comments.
source/Plugins/Language/D/DLanguage.cpp
25

thx for tip! done, used llvm::sys::DynamicLibrary

timotheecour marked an inline comment as done.Mar 10 2018, 2:23 PM

This patch has no testcase. It shouldn't be particularly hard to write one, you can take inspiration from the one in lit/.

Thanks!

5d29d3f0aa2f540c277a67eac5873feca8c62d51 (Support for OCaml native debugging) introduced a much larger change and had no test case either. If you really insist on a test case please provide more concrete guidance.

This patch has no testcase. It shouldn't be particularly hard to write one, you can take inspiration from the one in lit/.

Thanks!

5d29d3f0aa2f540c277a67eac5873feca8c62d51 (Support for OCaml native debugging) introduced a much larger change and had no test case either. If you really insist on a test case please provide more concrete guidance.

That was a while ago. In fact, the OCaml support is going to be removed. New languages need to meet an high(er) bar to be included. One of the requirements is, e.g. being tested.
It's a little more complicated for D because it's an out-of-tree compiler so it poses interesting challenges.
Let me think about it a little more and I'll come back to you.

Thanks,

Davide

  • format
  • use llvm::sys::DynamicLibrary
  • added DLanguageProperties

update: we can now set lldbdplugin in .lldbinit via settings set plugin.language.D.pluginfile so this avoids hardcoding that file

timotheecour edited the summary of this revision. (Show Details)Mar 10 2018, 8:17 PM

It's a little more complicated for D because it's an out-of-tree compiler so it poses interesting challenges.

the demangling itself is thoroughly tested in https://github.com/dlang/druntime/blob/master/src/core/demangle.d

Note that the D plugin is not loaded by default (so default behavior is not affected) ; it's only loaded if user adds to his .lldbinit:
settings set plugin.language.D.pluginfile "path_to/liblldbdplugin.dylib"

johanengelen added inline comments.Mar 11 2018, 4:57 AM
source/Plugins/Language/D/DLanguage.cpp
109

Module ctors don't add cyclic dependencies by themselves. There is also no danger of that here.
How do you de-initialize druntime? (without de-init, there is a big mem leak)

It's a little more complicated for D because it's an out-of-tree compiler so it poses interesting challenges.

the demangling itself is thoroughly tested in https://github.com/dlang/druntime/blob/master/src/core/demangle.d

It's not the correct demangling of strings that needs to be tested. What needs testing:

  • recognising D language and D mangled name
  • loading of dynamic lib (plus testing that when loading fails lldb doesn't crash/exit; note: errmsg can be improved to e.g. "Failed to load D language plugin...")
  • test that indeed the demangling function of the dyn lib is called

To test this, I think you can make a mock dynamic lib that implements the required interface, but in C++. The mock demangler could be something like this or even simpler:

extern "C" char* lldbd_demangle(size_t length, const char* mangled) {
   if (mangled == "_D3fooFZv") // pseudo code
       return "void foo()";
   else
       return mangled;
}

Then build that library in the test, and test loading and use on a fabricated piece of code (probably easiest to use C code with this in it: extern "C" _D3fooFZv() { trap(); }.

extern "C" char* lldbd_demangle(size_t length, const char* mangled) {
   if (mangled == "_D3fooFZv") // pseudo code
       return "void foo()";
   else
       return mangled;
}

Actually, this is not the correct interface is it? The returned pointer should point to newly allocated memory using malloc, right?
Good to document that somewhere.

  • document C interface

Actually, this is not the correct interface is it? The returned pointer should point to newly allocated memory using malloc, right?
Good to document that somewhere.

not 100% sure what you mean in that comment but I just pushed a commit that clarifies that point: 'The returned pointer should be allocated via malloc and owned by the caller' ; the reference implementation of liblldbplugin does the right thing.

  • added d_terminate
  • format
This comment was removed by timotheecour.
timotheecour edited the summary of this revision. (Show Details)Mar 14 2018, 4:17 PM

How do you de-initialize druntime? (without de-init, there is a big mem leak)

There is no memory leak because d_initialize once (using c++11 static initialization pattern) and is intended to last for duration of application; so druntime will be initialized only once, upon 1st use.

When druntime is initialized, a number of resources are allocated (e.g. memory and mutex). Yes you initialize druntime once, I can see that. You don't deinitialize druntime at all: that's the resource leak.

timotheecour added inline comments.Mar 14 2018, 4:37 PM
source/Plugins/Language/D/DLanguage.cpp
109

Module ctors don't add cyclic dependencies by themselves. There is also no danger of that here.

  • Also, current code allows for liblldbplugin.d to be imported from another module whereas it would cause complications if it contained a module ctor that would initialize druntime.

calling d_initialize as I'm doing works fine.

109

How do you de-initialize druntime? (without de-init, there is a big mem leak)

There is no memory leak because d_initialize once (using c++11 static initialization pattern) and is intended to last for duration of application; so druntime will be initialized only once, upon 1st use.

I've however added d_terminate in case a future PR wants to de-initialize druntime (eg in DLanguage::Terminate)

When druntime is initialized, a number of resources are allocated (e.g. memory and mutex). Yes you initialize druntime once, I can see that. You don't deinitialize druntime at all: that's the resource leak.

Where would you want me to deinit? inside DLanguage::Terminate ?

  • fix assert fail when plugin.language.D.pluginfile empty

When druntime is initialized, a number of resources are allocated (e.g. memory and mutex). Yes you initialize druntime once, I can see that. You don't deinitialize druntime at all: that's the resource leak.

Where would you want me to deinit? inside DLanguage::Terminate ?

I don't know enough of LLDB to know where to put the deinitialization. Sorry, can't help you there, except for making it easy and doing the deinitialization using a dtor in the library...

labath added a subscriber: labath.Mar 21 2018, 4:23 AM

The Terminate function is a good place to do this kind of cleanup.

source/Plugins/Language/D/DLanguage.cpp
34–36

Instead of using decltype everywhere, you could just define the type of these functions with typedef and use that.

I sat down to write my own plugin today and found this.

@timotheecour , thanks for writing this -- can I help with anything to get this across the finish line?