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

[gbreynoo]

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.

[gbreynoo]

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.

[dblaikie]

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]

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.

[urnathan]

In D124010#3459710, @dblaikie wrote:

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]

Move testing to area suggested by @urnathan.

[gbreynoo]

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

In D124010#3459710, @dblaikie wrote:

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]

llvm/include/llvm/Demangle/ItaniumDemangle.h

4369

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

[dblaikie]

In D124010#3474930, @gbreynoo wrote:

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

In D124010#3459710, @dblaikie wrote:

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?

[gbreynoo]

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 with urnathan's suggestion.

[urnathan]

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.

[gbreynoo]

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

libcxxabi/src/demangle/ItaniumDemangle.h

case 'A': {
     if (T == nullptr)
       return nullptr;
     // FIXME: We need to include the string contents in the mangling.
     if (consumeIf('E'))
       return make<StringLiteral>(T);
     return nullptr;
   }
   case 'D':
-    if (consumeIf("DnE"))
+    if (consumeIf("Dn") && (consumeIf('0'), consumeIf('E')))
       return make<NameType>("nullptr");
     return nullptr;
   case 'T':
     // Invalid mangled name per
     //   http://sourcerytools.com/pipermail/cxx-abi-dev/2011-August/002422.html
     return nullptr;
   case 'U': {
     // FIXME: Should we support LUb... for block literals?

libcxxabi/test/test_demangle.pass.cpp

const char* cases[][2] =
     {"_ZN11ImageLoader19fgTotalWeakBindTimeE", "ImageLoader::fgTotalWeakBindTime"},
     {"_ZN11ImageLoader10fgTotalDOFE", "ImageLoader::fgTotalDOF"},
     {"_ZN11ImageLoader15fgTotalInitTimeE", "ImageLoader::fgTotalInitTime"},
     {"_ZN11ImageLoader23fgDynamicImageReExportsE", "ImageLoader::fgDynamicImageReExports"},
     {"_ZN11ImageLoader19fgInterposingTuplesE", "ImageLoader::fgInterposingTuples"},
     {"_ZNSs4_Rep20_S_empty_rep_storageE", "std::string::_Rep::_S_empty_rep_storage"},
     {"_ZZN23ImageLoaderMachOClassic26bindIndirectSymbolPointersERKN11ImageLoader11LinkContextEbbE13alreadyWarned", "ImageLoaderMachOClassic::bindIndirectSymbolPointers(ImageLoader::LinkContext const&, bool, bool)::alreadyWarned"},
     {"_ZN12_GLOBAL__N_115emergency_mutexE", "(anonymous namespace)::emergency_mutex"},
+    {"_Z1fIDnLDn0EEvv", "void f<std::nullptr_t, nullptr>()"},
 
     // clang
 
     {"_ZN4llvm8DenseMapIjcNS_12DenseMapInfoIjEENS1_IcEEE6insertERKSt4pairIjcE", "llvm::DenseMap<unsigned int, char, llvm::DenseMapInfo<unsigned int>, llvm::DenseMapInfo<char> >::insert(std::pair<unsigned int, char> const&)"},
     {"_ZN4llvm8DenseMapIjcNS_12DenseMapInfoIjEENS1_IcEEE16InsertIntoBucketERKjRKcPSt4pairIjcE", "llvm::DenseMap<unsigned int, char, llvm::DenseMapInfo<unsigned int>, llvm::DenseMapInfo<char> >::InsertIntoBucket(unsigned int const&, char const&, std::pair<unsigned int, char>*)"},
     {"_ZN4llvm8DenseMapIjcNS_12DenseMapInfoIjEENS1_IcEEE4growEj", "llvm::DenseMap<unsigned int, char, llvm::DenseMapInfo<unsigned int>, llvm::DenseMapInfo<char> >::grow(unsigned int)"},
     {"_ZN4llvm19MachinePassRegistry3AddEPNS_23MachinePassRegistryNodeE", "llvm::MachinePassRegistry::Add(llvm::MachinePassRegistryNode*)"},
     {"_ZN4llvm2cl6valuesIiEENS0_11ValuesClassIT_EEPKcS3_S6_z", "llvm::cl::ValuesClass<int> llvm::cl::values<int>(char const*, int, char const*, ...)"},

llvm/include/llvm/Demangle/ItaniumDemangle.h

case 'A': {
     if (T == nullptr)
       return nullptr;
     // FIXME: We need to include the string contents in the mangling.
     if (consumeIf('E'))
       return make<StringLiteral>(T);
     return nullptr;
   }
   case 'D':
-    if (consumeIf("DnE"))
+    if (consumeIf("Dn") && (consumeIf('0'), consumeIf('E')))
       return make<NameType>("nullptr");
     return nullptr;
   case 'T':
     // Invalid mangled name per
     //   http://sourcerytools.com/pipermail/cxx-abi-dev/2011-August/002422.html
     return nullptr;
   case 'U': {
     // FIXME: Should we support LUb... for block literals?
     if (look(1) != 'l')
       return nullptr;
     Node *T = parseUnnamedTypeName(nullptr);
     if (!T || !consumeIf('E'))
       return nullptr;
     return make<LambdaExpr>(T);
   }
   default: {
     // might be named type
     Node *T = getDerived().parseType();
     if (T == nullptr)
       return nullptr;
     StringView N = parseNumber(/*AllowNegative=*/true);
     if (N.empty())
       return nullptr;
     if (!consumeIf('E'))
       return nullptr;
     return make<EnumLiteral>(T, N);
   }
   }
 }
 
 // <braced-expression> ::= <expression>
 //                     ::= di <field source-name> <braced-expression>    # .name = expr
 //                     ::= dx <index expression> <braced-expression>     # [expr] = expr
 //                     ::= dX <range begin expression> <range end expression> <braced-expression>