This patch adds minimal support for D programming language demangling on LLVM core based on the D name mangling spec. This will allow easier integration on a future LLDB plugin for D either in the upstream tree or outside of it. Minimal support includes recognizing D demangling encoding and at least one mangling name, which in this case is `_Dmain` mangle.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 11 | ||
| 17 | ||
| 27 | That using namespace shows that OutputString needs moving out of the itanium_demangle namespace, since D isn't Itanium! | |
| 31 | Move this down to close to its first usage. | |
| 33 | If I follow it correctly, the second half of this if is unnecessary, as the strncmp will fail instead. | |
| 39 | Just a thought, but would it make more sense to have this return the OutputString rather than modify it in-place? Probably rename it to createOutputString in that case (this would be a separate patch of course). | |
| 41–43 | LLVM doesn't use braces on single-line ifs. | |
| 45 | Perhaps add a comment or probably put this in a separate function, as it feels like a different level of abstraction to the rest of the code in this fucntion, and I don't really understand why it's necessary (NB: I haven't looked into the OutputString interface, but ideally, I shouldn't have to). | |
| llvm/lib/Demangle/Demangle.cpp | ||
| 24 | I'm wondering if we could reuse this function at the start of dlangDemangle instead of the second if. | |
| llvm/unittests/Demangle/DLangDemangleTest.cpp | ||
| 10 | This comment is probably going to end up out of place as we go forward, as I expect we'll find other test cases that need checking that aren't part fo the libiberty library. | |
| 21–23 | I'd probably just ditch the protected, and make this a struct. | |
| 26–28 | ||
| 31 | We need test cases for nullptr as the input string, and probably something that fails the strncmp, e.g. _ and _Z or similar. | |
| llvm/unittests/Demangle/DemangleTest.cpp | ||
| 25 | Probably move this above or below the two Rust mangling cases. | |
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 11 | Done | |
| 17 | Done | |
| 27 | This is the same on Rust demangler, that is primarily why I didn't write a patch. I can do the renaming on another separate patch. | |
| 31 | Removed. | |
| 33 | True. This can also be joined into the next if. Done | |
| 39 | I didn't write this API. I can change it on another patch if you wish. | |
| 41–43 | Ok, noted. I will try to change this on the stacked patches. | |
| 45 | OutputString/OutputBuffer is not null terminated therefore this is necessary. This is also being done on other demanglers to make the API conformat with null terminated strings. I don't belive such tiny thing need a separate function, but I can generalize this if it is really needed. | |
| llvm/lib/Demangle/Demangle.cpp | ||
| 24 | This can be done already since dlangDemangle performs that check too, I just followed what the other demanglers were doing. I think if this behaviour is documented I don't see the problem, although I also believe that the compiler optimized is smart enough to optimize this pre check. | |
Comment Actions
@jhenderson @dblaikie can you re-review?
| llvm/unittests/Demangle/DLangDemangleTest.cpp | ||
|---|---|---|
| 26–28 | Done | |
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 11 | Looks like this got mangled - ended up as "the for D programming language"" rather than "for the D programming language". But I'll probably leave the review for @jhenderson to come back around and check their feedback's been addressed. | |
Comment Actions
Just a few nits left from me.
A note on the commit message: we don't usually add Signed off by tags to the commit messages in LLVM.
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 27 | Sounds reasonable, thanks. | |
| 33 | Decl is a bit of a weird name to me. I'm assuming it's meant to stand for something like Declaration? I feel like Demangled or even DemangledName (for symmetry with MangledName) would be clearer. | |
| 39 | I guess it'll depend on how easy it is to change the API. If there is good reason for its current design, it's fine to remain as-is. Either way, not a blocker for this patch. | |
| 45 | The separate function thought was mostly a way of describing what this code is doing, as it's not immediately clear if you aren't familiar with the OutputBuffer interface. A simple comment at the start of the if is probably sufficient, if you prefer that though. | |
| llvm/unittests/Demangle/DLangDemangleTest.cpp | ||
| 31 | Just a thought: will brace initializers work here (i.e. {"_Dmain", "D main"} instead of std::make_pair(...) etc)? | |
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 11 | Oh, thanks for noticing! I amended in the right pos. | |
| 33 | I can change to Demangled, I don't really know the full rationale behind it but I believe that is a better name indeed. | |
| 39 | The current design relies on the common C error handling way. I think the rationale here is because this function can set multiple variables by reference, and returning a reference could be seen as inconsistent. I think returning the reference would be fine if only one value was set. | |
| llvm/unittests/Demangle/DLangDemangleTest.cpp | ||
| 31 | That doesn't work, unfortunately. | |
Comment Actions
LGTM, modulo the previously-discussed licensing question - have you heard anything from the LLVM foundation board yet?
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 40 | ||
Comment Actions
Updated the last comment. About licensing, I wrote to llvm-dev about that (here: https://lists.llvm.org/pipermail/llvm-dev/2021-October/153267.html ). Iain, the primary author of the code also wrote to the first patch. More context here https://reviews.llvm.org/D110576 . I don't know what is the next step in that regard if anything more is needed.
| llvm/lib/Demangle/DLangDemangle.cpp | ||
|---|---|---|
| 40 | Patch updated. | |
Comment Actions
I think I'd only be comfortable if we get something official from the foundation saying they are happy with Iain's statement, since I'm not a lawyer. I don't know if there's any prior art in this regard. @lattner?
Comment Actions
Looks like this patch https://lab.llvm.org/buildbot/#/builders/168/builds/3260/steps/10/logs/stdio
==40944==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 1024 byte(s) in 1 object(s) allocated from:
#0 0x387698 in malloc /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3
#1 0x4f47d8 in initializeOutputBuffer /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Demangle/Utility.h:199:31
#2 0x4f47d8 in llvm::dlangDemangle(char const*) /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Demangle/DLangDemangle.cpp:29:8
#3 0x3c9455 in DLangDemangleTestFixture::SetUp() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/unittests/Demangle/DLangDemangleTest.cpp:20:39
#4 0x5f710e in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
#5 0x5f710e in testing::Test::Run() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2503:3
#6 0x5fbcdb in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
#7 0x5fd14f in testing::TestSuite::Run() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
#8 0x62dacd in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
#9 0x62bc2c in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
#10 0x62bc2c in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
#11 0x5d95b0 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2473:46
#12 0x5d95b0 in main /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
#13 0x7f12de89a09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
SUMMARY: AddressSanitizer: 1024 byte(s) leaked in 1 allocation(s).Comment Actions
I don't have a sanitizer build setup currently, and couldn't reproduce this with valgrind (maybe used it incorrectly) - but hopefully 1bed03b5e3812dc6c659d285d77a2a577a2d113d addresses this issue.