This revision was automatically updated to reflect the committed changes.
Closed by commit rL337931: Use LLVMs new ItaniumPartialDemangler in LLDB
(authored by stefan.graenitz, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.
Given these numbers I think we can land this patch as is and figure out the IDP
stuff on the mailing list and/or a future patch. LGTM. Thanks Stefan!
sgraenitz marked an inline comment as done.
sgraenitz added a comment.
I did a little investigation of the performance impact of not reusing the IPD.
For this I used the unit test `PartialDemangleTest.TestNameChopping`, where I
added an outer loop:
TEST(PartialDemangleTest, TestNameChopping)
sgraenitz marked an inline comment as done.
sgraenitz added inline comments.
Comment at: unittests/Core/CMakeLists.txt:4
DataExtractorTest.cpp
+ MangledTest.cpp
ListenerTest.cpp
teemperor wrote:
> This should be after ListenerTest.cpp to keep this file in
sgraenitz updated this revision to Diff 157062.
sgraenitz added a comment.
Keep alphabetical order of files in CMakeLists.txt
https://reviews.llvm.org/D49612
Files:
cmake/modules/LLDBConfig.cmake
lldb.xcodeproj/project.pbxproj
source/Core/Mangled.cpp
unittests/Core/CMakeLists.txt
teemperor added inline comments.
Comment at: unittests/Core/CMakeLists.txt:4
DataExtractorTest.cpp
+ MangledTest.cpp
ListenerTest.cpp
This should be after ListenerTest.cpp to keep this file in alphabetical order.
https://reviews.llvm.org/D49612
sgraenitz updated this revision to Diff 157061.
sgraenitz added a comment.
Change EXPECTED_EQ to EXPECTED_STREQ, because ConstString::operator==()
compares identity of pointers, not equality of strings. While "they must come
from the same pool in order to be equal" (as stated in the
sgraenitz marked 2 inline comments as done.
sgraenitz added inline comments.
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+llvm::ItaniumPartialDemangler IPD;
+bool demangle_err = IPD.partialDemangle(mangled_name);
sgraenitz updated this revision to Diff 157001.
sgraenitz added a comment.
Minor improvements on naming and comments
https://reviews.llvm.org/D49612
Files:
cmake/modules/LLDBConfig.cmake
lldb.xcodeproj/project.pbxproj
source/Core/Mangled.cpp
unittests/Core/CMakeLists.txt
JDevlieghere added inline comments.
Comment at: source/Core/Mangled.cpp:30
#include "llvm/ADT/StringRef.h"// for StringRef
+#include "llvm/Demangle/Demangle.h"
While you're here I'd remove these redundant comments so this block looks more
consistent.
labath added a comment.
The patch makes sense to me. It's nice to get a performance boost, while
reducing the number of demanglers at the same time.
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+llvm::ItaniumPartialDemangler IPD;
+
erik.pilkington added inline comments.
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+llvm::ItaniumPartialDemangler IPD;
+bool demangle_err = IPD.partialDemangle(mangled_name);
sgraenitz wrote:
> erik.pilkington
sgraenitz added reviewers: erik.pilkington, labath, clayborg, mgorny, davide,
JDevlieghere.
sgraenitz added a comment.
Sorry if the review is a little bumpy, it's my first one. Added all subscribers
as reviewers now. Hope that's ok.
The current version is a rather simple change, that slightly
sgraenitz added a comment.
Well when repeating this test, the values are not always that far apart from
each other, but on average the old USE_BUILTIN_DEMANGLER path the slower one.
Maybe FastDemangle is still faster than IPD in success case, but the overhead
from the fallback cases is adding
erik.pilkington added a comment.
In https://reviews.llvm.org/D49612#1171550, @sgraenitz wrote:
> Quick local performance check doing `target create clang` in current review
> version vs. master HEAD version (both loading the exact same release build of
> clang) looks promising. It's faster
sgraenitz updated this revision to Diff 156778.
sgraenitz added a comment.
Remove CMake options LLDB_USE_BUILTIN_DEMANGLER and LLDB_USE_LLVM_DEMANGLER.
https://reviews.llvm.org/D49612
Files:
cmake/modules/LLDBConfig.cmake
lldb.xcodeproj/project.pbxproj
source/Core/Mangled.cpp
sgraenitz added inline comments.
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+llvm::ItaniumPartialDemangler IPD;
+bool demangle_err = IPD.partialDemangle(mangled_name);
sgraenitz wrote:
> sgraenitz wrote:
> >
sgraenitz marked an inline comment as done.
sgraenitz added a comment.
Quick local performance check doing `target create clang` in current review
version vs. master HEAD version (both loading the exact same release build of
clang) looks promising. It's faster already now, so I would remove the
sgraenitz marked an inline comment as done.
sgraenitz added inline comments.
Comment at: cmake/modules/LLDBConfig.cmake:417-423
+option(LLDB_USE_BUILTIN_DEMANGLER "Use lldb's builtin demangler" OFF)
+option(LLDB_USE_LLVM_DEMANGLER "Use llvm's new partial demangler" ON)
sgraenitz updated this revision to Diff 156745.
sgraenitz added a comment.
Enable LLDB_USE_LLVM_DEMANGLER on MSVC platforms.
https://reviews.llvm.org/D49612
Files:
cmake/modules/LLDBConfig.cmake
lldb.xcodeproj/project.pbxproj
source/Core/Mangled.cpp
unittests/Core/CMakeLists.txt
sgraenitz updated this revision to Diff 156743.
sgraenitz added a comment.
Fix: Use malloc instead of new for allocating the demangled_name buffer.
https://reviews.llvm.org/D49612
Files:
cmake/modules/LLDBConfig.cmake
lldb.xcodeproj/project.pbxproj
source/Core/Mangled.cpp
labath added a comment.
In https://reviews.llvm.org/D49612#1171395, @sgraenitz wrote:
> > That's fine. It just needs to be able to demangle itanium names when
> > running on an MSVC platform.
>
> IIUC `cstring_mangling_scheme()` should return `eManglingSchemeItanium` in
> this case and switch
sgraenitz added a comment.
> That's fine. It just needs to be able to demangle itanium names when running
> on an MSVC platform.
IIUC `cstring_mangling_scheme()` should return `eManglingSchemeItanium` in this
case and switch to the case label without the `#if defined(_MSC_VER)`.
labath added a comment.
In https://reviews.llvm.org/D49612#1171363, @sgraenitz wrote:
> > this new demangler should also be available in the MSVC case, should it not?
>
> I don't think the Itanium mangler supports MSVC mangling.
That's fine. It just needs to be able to demangle itanium names
sgraenitz added a comment.
Hi all, thanks for your feedback. I will update the review with code fixes in a
bit.
> Also we should compare demangled names between the two to ensure there are no
> differences as we are doing this.
Yes I can definitely do it manually. When it comes to writing
labath added inline comments.
Comment at: cmake/modules/LLDBConfig.cmake:417-423
+option(LLDB_USE_BUILTIN_DEMANGLER "Use lldb's builtin demangler" OFF)
+option(LLDB_USE_LLVM_DEMANGLER "Use llvm's new partial demangler" ON)
endif()
if(LLDB_USE_BUILTIN_DEMANGLER)
clayborg added a comment.
We do need to test the performance of this as demangling is a hot spot when we
parse any object file. If it is faster, then we should just replace it and not
add any options to be able to switch. Also we should compare demangled names
between the two to ensure there
davide added a comment.
Thanks for doing this.
We may consider doing some A-B testing between the two demanglers.
A strategy that worked very well for similar purposes was that of running `nm`
on a large executable (e.g. clang or lldb itself) and see whether we demangle
in the same exact way
sgraenitz added inline comments.
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+llvm::ItaniumPartialDemangler IPD;
+bool demangle_err = IPD.partialDemangle(mangled_name);
sgraenitz wrote:
> erik.pilkington wrote:
friss added a comment.
Stefan, you should always add lldb-commits to the subscribers of your phab
reviews. I added it now, but IIRC there are issues with adding subscribers
after the fact.
https://reviews.llvm.org/D49612
___
lldb-commits mailing
30 matches
Mail list logo
