-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[MsDemangle] Read entire chain of target names in special tables #155630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Ping |
zmodem
left a comment
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.
Sorry, I must have missed this before. Thanks for fixing this.
| if (!consumeFront(MangledName, '@')) | ||
| STSN->TargetName = demangleFullyQualifiedTypeName(MangledName); | ||
|
|
||
| NodeList *TargetCurrent = nullptr; |
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.
Instead of building a linked list here, could we just put these into a SmallVector and create the NodeArray from that directly?
zmodem
left a comment
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.
Thanks! I think that's much easier to read.
zmodem
left a comment
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.
lgtm
| #include "llvm/Demangle/MicrosoftDemangle.h" | ||
|
|
||
| #include "llvm/ADT/ArrayRef.h" | ||
| #include "llvm/ADT/SmallVector.h" |
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 cannot build LLVM with gcc+gnu ld after this patch. It introduces cyclic dependencies (LLVMSupport <-> LLVMDemangle).
FAILED: lib/libLLVMDemangle.so.22.0git
: && /usr/bin/c++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-dangling-reference -Wno-redundant-move -Wno-pessimizing-move -Wno-array-bounds -Wno-stringop-overread -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O2 -g -DNDEBUG -Wl,-z,defs -Wl,-z,nodelete -Wl,-rpath-link,/data/zyw/dev/llvm-build/./lib -Wl,--gc-sections -shared -Wl,-soname,libLLVMDemangle.so.22.0git -o lib/libLLVMDemangle.so.22.0git lib/Demangle/CMakeFiles/LLVMDemangle.dir/Demangle.cpp.o lib/Demangle/CMakeFiles/LLVMDemangle.dir/ItaniumDemangle.cpp.o lib/Demangle/CMakeFiles/LLVMDemangle.dir/MicrosoftDemangle.cpp.o lib/Demangle/CMakeFiles/LLVMDemangle.dir/MicrosoftDemangleNodes.cpp.o lib/Demangle/CMakeFiles/LLVMDemangle.dir/RustDemangle.cpp.o lib/Demangle/CMakeFiles/LLVMDemangle.dir/DLangDemangle.cpp.o -Wl,-rpath,"\$ORIGIN/../lib:" && :
/usr/bin/ld: lib/Demangle/CMakeFiles/LLVMDemangle.dir/MicrosoftDemangle.cpp.o: in function `llvm::SmallVectorTemplateCommon<llvm::ms_demangle::Node*, void>::grow_pod(unsigned long, unsigned long)':
/data/zyw/dev/llvm-project/llvm/include/llvm/ADT/SmallVector.h:140:(.text._ZN4llvm11ms_demangle9Demangler30demangleSpecialTableSymbolNodeERSt17basic_string_viewIcSt11char_traitsIcEENS0_20SpecialIntrinsicKindE+0x28c): undefined reference to `llvm::SmallVectorBase<unsigned int>::grow_pod(void*, unsigned long, unsigned long)'
collect2: error: ld returned 1 exit 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.
Sorry, opened #166586 to fix this.
|
I believe this one broke a few of our buildbots, e.g., https://lab.llvm.org/buildbot/#/builders/203/builds/28219 From the log |
Using `SmallVector` would introduce a dependency cycle (see #155630 (comment)), so this uses a NodeList.
…s (#166586) Using `SmallVector` would introduce a dependency cycle (see llvm/llvm-project#155630 (comment)), so this uses a NodeList.
When there's a deep inheritance hierarchy of multiple C++ classes (see below), then the mangled name of a VFTable can include multiple key nodes in the target name.
For example, in the following code, MSVC will generate mangled names for the VFTables that have up to three key classes in the context.
Code
This will include
??_7C@@6BInd1@@Ind4@@Ind5@@@(and every other combination). Microsoft's undname will demangle this to "const C::`vftable'{for `Ind1's `Ind4's `Ind5'}". Previously, LLVM would demangle this to "const C::`vftable'{for `Ind1'}".With this PR, the output of LLVM's undname will be identical to Microsoft's version. This changes
SpecialTableSymbolNode::TargetNameto a node array which contains each key from the name. Unlike namespaces, these keys are not in reverse order - they are in the same order as in the mangled name.