This is an archive of the discontinued LLVM Phabricator instance.

[llvm-cxxfilt] Unable to demangle a template argument which happens to be a null pointer
ClosedPublic

Authored by gbreynoo on Apr 19 2022, 9:05 AM.

Details

Summary

As seen in https://github.com/llvm/llvm-project/issues/51854 llvm-cxxfilt was having trouble demangling the case below:

When attempting to demangle "_Z1fIDnLDn0EEvv" llvm-cxxfilt does not demangle the input. According to GCC we should see similar to "void f<decltype(nullptr), (decltype(nullptr))0>()"

After investigating the issue I discovered the following in the Itanium C++ ABI demangling section:

The pointer literal expression nullptr is encoded as "LDnE". In contrast, a template argument which happens to be a null pointer (an extension made standard in C++11) is mangled as if it were a literal 0 of the appropriate pointer type; for example, "LPi0E" or "LDn0E". This inconsistency is an unfortunate accident.

We handle the "LDNE" case and "LPi0E" but not "LDn0E". This change adds that handling.

Diff Detail

Event Timeline

gbreynoo created this revision.Apr 19 2022, 9:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 9:05 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
gbreynoo requested review of this revision.Apr 19 2022, 9:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 9:05 AM

I am not so familiar with this part of LLVM, I think this testing may belong elsewhere but I am unsure where that would be.

Should Clang be changing to not use this mangling, so it uses a mangling that matches GCC?

And the demangle AST you're creating here is for an enum, and a cast from 0 to nullptr - but I'm not sure that's portable (even if it is inconsistent between producers) - nullptr_t isn't necessarily an enum, and I don't think C++ requires that you can cast 0 to nullptr_t?

urnathan requested changes to this revision.Apr 25 2022, 5:59 AM

You need to modify libcxxabi/src/demangle/ItaniumDemangle.h and deploy the cp-to-llvm.sh script therein.

llvm/include/llvm/Demangle/ItaniumDemangle.h
4345–4348

Why demangle this differently to just 'nullptr'? I notice GNU's demangler does that:

zathras:11>c++filt _Z1fIDnLDn0EEvv
void f<decltype(nullptr), (decltype(nullptr))0>()

It would seem

if (consumeIf("Dn") && (consumeIf ('0'), consumeIf ('E'))
  return make<NameType>("nullptr");
return nullptr;

would suffice?

llvm/test/tools/llvm-cxxfilt/null-pointer-template-arg.test
1–12 ↗(On Diff #423642)

demangler tests are in libcxxabi/test/test_demangle.pass.cpp you'll see there's a massive array of tuples.

This revision now requires changes to proceed.Apr 25 2022, 5:59 AM

Should Clang be changing to not use this mangling, so it uses a mangling that matches GCC?

That would be good. I see D122663 is also changing the mangling, so this particular change wouldn't be in isolation.

gbreynoo updated this revision to Diff 425243.Apr 26 2022, 9:11 AM

Move testing to area suggested by @urnathan.

Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 9:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Apologies for the delay in response and thank you for your input @dblaikie and @urnathan.

Should Clang be changing to not use this mangling, so it uses a mangling that matches GCC?

And the demangle AST you're creating here is for an enum, and a cast from 0 to nullptr - but I'm not sure that's portable (even if it is inconsistent between producers) - nullptr_t isn't necessarily an enum, and I don't think C++ requires that you can cast 0 to nullptr_t?

I'm not sure what you mean regarding clang's use of this mangling. From my understanding this is legal in the specification and so should be handled appropriately as input?

llvm/include/llvm/Demangle/ItaniumDemangle.h
4345–4348

This would output nullptr rather than decltype(nullptr) or nullptr_t. Looking at the spec "a template argument which happens to be a null pointer should mangled as if it were a literal 0 of the appropriate pointer type" which in the case of LDn0E is nullptr_t.

llvm/test/tools/llvm-cxxfilt/null-pointer-template-arg.test
1–12 ↗(On Diff #423642)

Thanks @urnathan the LPi0E and LDnE cases were already present so I have added the LDn0E case.

gbreynoo added inline comments.Apr 26 2022, 9:20 AM
llvm/include/llvm/Demangle/ItaniumDemangle.h
4373

@dblaikie regarding the use of EnumLiteral this looked strange to me too but when following the code path for demangling _Z1fIPiLPi0EEvv which does appear to be output correctly, this line is used to demangle LPi0E to (int*)0. Looking at the spec "a template argument which happens to be a null pointer should mangled as if it were a literal 0 of the appropriate pointer type" which in the case of LDn0E would be nullptr_t. I'm not sure which Kind of AST should be used in this case?

Apologies for the delay in response and thank you for your input @dblaikie and @urnathan.

Should Clang be changing to not use this mangling, so it uses a mangling that matches GCC?

And the demangle AST you're creating here is for an enum, and a cast from 0 to nullptr - but I'm not sure that's portable (even if it is inconsistent between producers) - nullptr_t isn't necessarily an enum, and I don't think C++ requires that you can cast 0 to nullptr_t?

I'm not sure what you mean regarding clang's use of this mangling. From my understanding this is legal in the specification and so should be handled appropriately as input?

Oh, sorry, I wasn't saying clang should be changed instead of this patch, but in addition to/as well as this patch. It may be legal, but if Clang and GCC use different manglings I assume that means there would be ABI incompatibility in some cases, where a function defined in code compiled with GCC couldn't be correctly called from code compiled with Clang, for instance.

llvm/include/llvm/Demangle/ItaniumDemangle.h
4345–4348

I think @urnathan is intending to suggest that what this patch currently produces "(decltype(nullptr))0" instead we could/should produce "nullptr". This seems like it would be valid, yeah?

I now see what you mean @dblaikie . I believe the mangling is functioning correctly and it's output matches GCC, it is the demangling that is the issue. By following @urnathan's code suggestion the demangled output will differ but I don't think this should be a problem as they are equivalent?

llvm/include/llvm/Demangle/ItaniumDemangle.h
4345–4348

Hi @dblaikie, I misunderstood the issue. I believe you are both correct and will update the patch appropriately.

gbreynoo updated this revision to Diff 425555.Apr 27 2022, 10:17 AM

Updated with urnathan's suggestion.

urnathan accepted this revision.Apr 28 2022, 4:00 AM

Thanks! Do you want to file a PR about the mangling difference between GCC & clang -- I've not looked sufficiently carefully to determine who is wrong[*] here, but we can work with GCC to figure that out. [*] wrong may mean 'pragmatically incorrect' rather than 'technically incorrect', I mean no blame, it's just unfortunate.

This revision is now accepted and ready to land.Apr 28 2022, 4:00 AM
gbreynoo added a comment.EditedApr 28 2022, 7:51 AM

@urnathan I've created the ticket https://github.com/llvm/llvm-project/issues/55171 regarding the mangling issue. Just to note here, above I mentioned that I thought the mangled output matched GCC but this is not the case.

This revision was landed with ongoing or failed builds.Apr 28 2022, 8:00 AM
This revision was automatically updated to reflect the committed changes.