Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[Demangle] Add minimal support for D programming language
ClosedPublic

Authored by ljmf00 on Oct 8 2021, 8:18 AM.

Details

Summary
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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ljmf00 updated this revision to Diff 380215.Oct 16 2021, 2:15 PM
ljmf00 updated this revision to Diff 380216.Oct 16 2021, 2:23 PM

@jhenderson @dblaikie Can you re-review this?

jhenderson added inline comments.
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.

ljmf00 updated this revision to Diff 381436.Oct 21 2021, 5:01 PM
ljmf00 added inline comments.
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.

ljmf00 updated this revision to Diff 383843.Nov 1 2021, 11:16 AM

@jhenderson @dblaikie can you re-review?

llvm/unittests/Demangle/DLangDemangleTest.cpp
26–28

Done

dblaikie added inline comments.Nov 1 2021, 4:57 PM
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.

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)?

ljmf00 updated this revision to Diff 384310.Nov 2 2021, 7:51 PM
ljmf00 edited the summary of this revision. (Show Details)
ljmf00 added inline comments.
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.
I just noticed that I should check if that failed or not. Added that check.

llvm/unittests/Demangle/DLangDemangleTest.cpp
31

That doesn't work, unfortunately.

Two nits still.

llvm/lib/Demangle/DLangDemangle.cpp
34

Nit: please run clang-format on the latest changed code.

45

Looks like this requested comment is still missing?

ljmf00 updated this revision to Diff 384382.Nov 3 2021, 3:12 AM
ljmf00 added inline comments.Nov 3 2021, 3:16 AM
llvm/lib/Demangle/DLangDemangle.cpp
34

Done

45

Sorry, I forgot it. Thanks for the reminder!

jhenderson accepted this revision.Nov 3 2021, 4:10 AM

LGTM, modulo the previously-discussed licensing question - have you heard anything from the LLVM foundation board yet?

llvm/lib/Demangle/DLangDemangle.cpp
40
This revision is now accepted and ready to land.Nov 3 2021, 4:10 AM
ljmf00 updated this revision to Diff 384411.Nov 3 2021, 6:04 AM
ljmf00 added a comment.Nov 3 2021, 6:18 AM

LGTM, modulo the previously-discussed licensing question - have you heard anything from the LLVM foundation board yet?

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.

LGTM, modulo the previously-discussed licensing question - have you heard anything from the LLVM foundation board yet?

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.

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?

lattner requested changes to this revision.Nov 3 2021, 1:09 PM

I'll respond through email.

This revision now requires changes to proceed.Nov 3 2021, 1:09 PM
ljmf00 updated this revision to Diff 385671.Nov 8 2021, 6:33 PM
ljmf00 edited the summary of this revision. (Show Details)
lattner accepted this revision.Nov 8 2021, 8:30 PM

looks great to me, thank you!

This revision is now accepted and ready to land.Nov 8 2021, 8:30 PM
ljmf00 added a comment.Nov 9 2021, 7:48 AM

@dblaikie Can you land this?

This revision was automatically updated to reflect the committed changes.

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).

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).

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.