-
-
Notifications
You must be signed in to change notification settings - Fork 421
Add cppdemangle in druntime #3002
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @ErnyTech! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + druntime#3002" |
36d80f7
to
1be511b
Compare
cb69e57
to
0de39b0
Compare
It is now ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first round of review
src/core/internal/cppdemangle.d
Outdated
version (Posix) | ||
{ | ||
int result; | ||
auto demangledNamePtr = _instance.__cxa_demangle(mangledName, null, null, &result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of copying the result when you could just pass the pointer to the array here ?
Thanks for the extensive review! |
bbe2da0
to
f7e3b10
Compare
@Geod24, this is ready for second round ;-) |
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]>
/** | ||
* Demangles C++ mangled names. | ||
* | ||
* The output buffer and return will be contains the demangled name of C++ symbol if the demangling is successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* The output buffer and return will be contains the demangled name of C++ symbol if the demangling is successful. | |
* The output buffer and return value will contain the demangled name of C++ symbol if the demangling is successful. |
* | ||
* The output buffer and return will be contains the demangled name of C++ symbol if the demangling is successful. | ||
* | ||
* This function will fail and the mangled name in input will be returned if: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "return input on failure" part should be mentioned in the previous sentence as it describes the general function contract
* | ||
* Params: | ||
* mangledName = The string to demangle. | ||
* outputBuffer = The destination buffer, if the size of the destination buffer is <= the size of cppdemangle output the return string would be incomplete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant size requirement, it's already mentioned above
* Returns: | ||
* The demangled name or the original string if the name is not a mangled C++ name. | ||
*/ | ||
char[] cppdemangle(const(char)[] mangledName, char[] outputBuffer, string prefix = "") @trusted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this function should accept a prefix, it's an unrelated complication and a user can always do that manually:
buffer[0..prefix.length] = prefix;
cppdemangle(mangling, buffer[prefix.length .. $]);
* Returns: | ||
* The demangled name or the original string if the name is not a mangled C++ name. | ||
*/ | ||
char[] cppdemangle(const(char)[] mangledName, char[] outputBuffer, string prefix = "") @trusted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The symbol should be in camelCase: cppDemangle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trusted
should probably be used in smaller scope(s).
@nogc nothrow extern(System) | ||
{ | ||
private UnDecorateSymbolNameFunc UnDecorateSymbolName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nogc nothrow extern(System)
doesn't affect the type of UnDecorateSymbolName
and extern(System)
is redundant anyway
} | ||
} | ||
|
||
version (Windows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version (Windows) | |
else version (Windows) |
Now that my exams are over I can continue with this! |
Ping @ErnyTech |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note to tick later: Missing tests.
private static immutable names = ["libc++abi.dylib", "libstdc++.dylib"]; | ||
else | ||
{ | ||
private static immutable names = ["libstdc++.so", "libstdc++.so.6", "libstdc++.so.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about fallbacks to other major versions of libstdc++
? It shouldn't be required for new systems but could be good for compatibility.
|
||
@nogc nothrow extern(System) | ||
{ | ||
private UnDecorateSymbolNameFunc UnDecorateSymbolName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UnDecorateSymbolName
is a symbol from dbghelp.dll
and that doesn't come with a Windows stock installation, being instead, part of Windows Debugging Kit (WDK). Also, considering MinGW systems, aren't libstdc++
shipped anyway? Notwithstanding, we should have fallbacks for those too, just in case.
{ | ||
import core.sys.posix.dlfcn : dlsym, dlopen, dlclose, RTLD_LAZY; | ||
|
||
auto handle = dlopen(null, RTLD_LAZY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add null
to the fallback list instead of doing code duplication? Is there any additional logic I'm missing here?
return null; | ||
} | ||
|
||
auto mangledNamePtr = cast(char*) pureCalloc(mangledName.length + 1, char.sizeof); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if the allocation succeed.
} | ||
} | ||
|
||
return copyResult(demangledName[0..strlen(demangledName)], outputBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't do strlen
. __cxa_demangle
is capable of returning the length. See https://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-4.3/a01696.html
Why do we use a copyResult
function in the first place? We shouldn't hang if the buffer is not large enough, and could return the allocated buffer directly if the provided output buffer is null
.
version (Posix) | ||
{ | ||
int status; | ||
auto demangledName = _instance.__cxa_demangle(mangledNamePtr, null, null, &status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please feed the output buffer to __cxa_demangle
since a preallocated buffer can help avoid unnecessary reallocations.
Add cppdemangle in druntime
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)