Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

[WIP] [DONT MERGE] Enable demangling of C++ symbols in the stacktrace #3003

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ErnyTech
Copy link

@ErnyTech ErnyTech commented Mar 24, 2020

This allows you to view demangled and pretty printed C++ symbols
in the stacktrace.
Is especially useful considering that D is getting more and more support for C++.

Now the C++ symbols in the stacktrace will be printed like this:

        test.d:7 [C++] Test<int>::SomeName(int, long, int) [0x55711efee36f]
        test.d:18 [C++] testcpp() [0x557228e17978]

This need the PR #3002

@ErnyTech ErnyTech changed the title Enable demangling of C++ symbols in the stacktrace [WIP] [DONT MERGE] Enable demangling of C++ symbols in the stacktrace Mar 26, 2020
@ErnyTech ErnyTech force-pushed the cppstack branch 8 times, most recently from df6a61a to 3e03206 Compare March 26, 2020 17:00
@Geod24
Copy link
Member

Geod24 commented Mar 27, 2020

FYI we now have dlang/dmd#10964 in DMD.
We might be interested in having it here, too.

@rainers
Copy link
Member

rainers commented Mar 27, 2020

FYI there was a similar PR: #2083

I think this one has better separation. For Windows, the VC mangling should be supported via UnDecorateSymbolName. Please also add a test case.

res ~= demangle(tempSymName, demangleBuf);
auto demangledName = demangle(tempSymName, demangleBuf);

version (Shared)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why limit this to a shared build of the runtime?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, for Windows it is not necessary

Copy link
Member

@rainers rainers Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary on other platforms? "Shared" is not a version for dlopen support, but whether the runtime itself is a shared library.

Copy link
Author

@ErnyTech ErnyTech Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

druntime must be linked with -ldl, otherwise the linker will not find dlopen.
I don't know how i can check this in a better way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler already adds -ldl to the linker command line (at least for linux):
https://github.com/dlang/dmd/blob/e8f984995598543279c07b9800d19a9e19321562/src/dmd/link.d#L756

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but if I create the object files with dmd and then link it manually? I'm not sure it's a good idea to depend on libdl for druntime

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see that rt_loadLibrary is only implemented if version(Shared), so it's consistent to not use it here, too. That restriction seems a bit strange, though.

Copy link
Author

@ErnyTech ErnyTech Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could do better using weak symbols.
It would be enough to create a dlopen function that returns null marked as weak symbol, so if the user links libdl the real dlopen would be used

@ErnyTech ErnyTech force-pushed the cppstack branch 13 times, most recently from 9ccee09 to 18fd3ec Compare April 1, 2020 12:34
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments about the second commit

@@ -0,0 +1,51 @@
Added experimental `C++` symbol demanling in the stacktrace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Added experimental `C++` symbol demanling in the stacktrace
The runtime can now demangle C++ names in stack traces

A bit more selling to the user (we tend to mark everything as "experimental" and that doesn't inspire confidence).
You can just note later that it is not enabled by default.

@@ -0,0 +1,51 @@
Added experimental `C++` symbol demanling in the stacktrace

This feature is availableby passing the following switches to your executable (not to the compiler):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the release note should start by describing the feature itself. You've been working on it for quite some time, so you know exactly what it entails, but not everyone does. Something like:

When a stack trace was generated by the runtime (e.g. when calling `Throwable.toString`), C++ names were not previously demangled. Hence the following code:
---
<INSERT CODE>
---
Would lead to the following stack trace:
---
<INSERT MANGLED STACK TRACE>
---

Starting from this release, C++ names will be demangled whenever possible, hence the previous stack trace will now show as:
---
<INSERT DEMANGLED STACK TRACE>
---

This feature is still experimental and must be opted-in by <BLAH BLAH>.
Demangling is only possible when <BLAH BLAH>.

Note that this is just an example, but it illustrate a few points:

  • Always describe the feature first, what it brings to the user: imagine giving an elevator pitch about it;
  • Try to show the difference between the old behavior and the new one: remember users might not be familiar with the old behavior, especially if someone looks at this release note in a few year;
  • Include a code example whenever possible (which you did already);
  • Put restrictions / limitations at the end;


private struct CPPTrace
{
__gshared CPPDemangle instance;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we already have a singleton in the other module?

__gshared CPPDemangle instance;
__gshared Config config;

static this()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A static this() is run on each thread constructions. You want a shared static this here, no ?

Having a C++ symbol demangler is very useful, with this it will
be possible to implement the C++ symbol demangle in the stacktrace.

The demangling is done in Posix platform by function __cxa_demangle
presents in Itanium C++ ABI (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#demangler).

Itanium C++ ABI is used practically in all modern C++ compilers on Posix,
however old compilers (like GCC < 3.0) used a different name mangling,
this is not a problem because DMD in fact only supports Itanium C++ ABI on Posix.

For Windows instead the implementation it is provided by the UnDecorateSymbolName
function present in the Debug Help Library.
(https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-undecoratesymbolname)

Signed-off-by: Ernesto Castellotti <[email protected]>
This allows you to view demangled and pretty printed C++ symbols
in the stacktrace.
Is especially useful considering that D is getting more and more support for C++.

Now the C++ symbols in the stacktrace will be printed like this:
	test.d:7 [C++] Test<int>::SomeName(int, long, int) [0x55711efee36f]
        test.d:18 [C++] testcpp() [0x557228e17978]

Signed-off-by: Ernesto Castellotti <[email protected]>
@RazvanN7
Copy link
Contributor

@ErnyTech What's the status on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Rebase needs a `git rebase` performed Needs Work stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants