This is an archive of the discontinued LLVM Phabricator instance.

[llvm-cxxfilt] Enable support for D programming language demangling
AbandonedPublic

Authored by ljmf00 on Sep 27 2021, 1:00 PM.

Details

Reviewers
jhenderson
Summary

This is part of https://github.com/dlang/projects/issues/81

Enable support for demangling D symbols using llvm-cxxfilt tool

Signed-off-by: Luís Ferreira <contact@lsferreira.net>

Diff Detail

Event Timeline

ljmf00 created this revision.Sep 27 2021, 1:00 PM
ljmf00 created this object with edit policy "Administrators".
ljmf00 requested review of this revision.Sep 27 2021, 1:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 1:00 PM
ljmf00 changed the edit policy from "Administrators" to "All Users".Sep 28 2021, 8:43 AM
jhenderson accepted this revision.Sep 29 2021, 12:10 AM

One nit, otherwise LGTM.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

I don't think you need to include this header - strncmp is already used in this file, so it's clearly being included through the existing include hierarchy. See also https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible.

This revision is now accepted and ready to land.Sep 29 2021, 12:10 AM
ljmf00 updated this revision to Diff 375889.Sep 29 2021, 7:48 AM
ljmf00 marked an inline comment as not done.Sep 29 2021, 5:59 PM
ljmf00 added inline comments.
llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

Yeah, my editor LSP plugin probably added it automatically by mistake, and no apparent duplication was detected. I'm going to remove it!

jhenderson accepted this revision.Sep 30 2021, 12:14 AM
jhenderson added inline comments.
llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

No problem!

dblaikie added inline comments.
llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

Generally we should include anything we depend on - not rely on indirect inclusions. (the style guide gives some allowance for this - like if some other header just has to have provided a definition/inclusion of a thing, that's fine - but in this case there's no strong connection between whatever header includes something for strncmp so it's suitable to include it here, so if that other header is refactored to remove the need/use of the header it doesn't break this code)

So this #include is probably good/proper here.

The advice in the style guide is about not including unused headers and not including headers when a forward declaration would suffice.

79–100

Perhaps this code could be refactored to be shared with llvm-symbolizer? (eg: the code being updated in D110664 here: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp)

79–100

Maybe the refactoring would be better done in D110664 and then that could be the basis for this patch to make one change that'd apply to both llvm-cxxfilt and llvm-symbolizer/symbolizing library.

jblachly removed a subscriber: jblachly.
ljmf00 added inline comments.Sep 30 2021, 5:53 PM
llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

I can agree on that, I just also agreed with @jhenderson more likely because itanium and rust code parts are also using strncmp without such include.

79–100

I can wait for that refactor on https://reviews.llvm.org/D110664 and make a patch to symbolize code

dblaikie added inline comments.Sep 30 2021, 6:06 PM
llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

Yeah, I think good tooling probably trumps whatever people did before - most of us in the absence of tooling just add #includes whenever we notice they're needed, I think. So good tooling probably is doing a better job than us mere humans. It sounds like the include is appropriate to me.

But I'm not super fussed.

79–100

Thanks!

ljmf00 updated this revision to Diff 378260.Oct 8 2021, 8:57 AM

Updated the diff to add the requested header. Also added a simpler test to be able to change the parent diff and stack with the diff implementing simple symbols.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
21

I added the header again.

ljmf00 added a subscriber: Geod24.Oct 8 2021, 8:59 AM
jhenderson accepted this revision.Oct 11 2021, 12:25 AM

LGTM again.

ljmf00 abandoned this revision.Oct 16 2021, 12:55 PM

Since support for llvm-cxxfilt was synced with Demangle.cpp logic on https://reviews.llvm.org/D110664, this doesn't makes sense anymore. Tests are going to be added on https://reviews.llvm.org/D111414 instead.