[ItaniumDemangle] Fix substitution failure of _BitInt

[zsrkmyn]

a soft ping

[zsrkmyn]

ping

[erichkeane]

I don't own the demangler nor the libcxxabi stuff, so I can't really help here. However, you might have trouble getting review until folks start returning to work from the christmas/newyear break.

[zsrkmyn]

In D140359#4023107, @erichkeane wrote:

I don't own the demangler nor the libcxxabi stuff, so I can't really help here. However, you might have trouble getting review until folks start returning to work from the christmas/newyear break.

Oh, thanks! got it. I'm sorry that I forgot it's now Christmas/new year holiday.

[aaron.ballman]

Adding @rjmccall for Itanium ABI code owner opinions.

llvm/include/llvm/Demangle/ItaniumDemangle.h

3929–3930

The comments below say:

// If we parsed a type, insert it into the substitution table. Note that all
// <builtin-type>s and <substitution>s have already bailed out, because they
// don't get substitutions.

_BitInt is a builtin type, so I don't think it should be in the substitution table, right?

[rjmccall]

That's an interesting question. The ABI says:

There are two exceptions that appear to be substitution candidates from the grammar, but are explicitly excluded:

- `<builtin-type>` other than vendor extended types, and
- function and operator names other than `extern "C"` functions.

The intent here, I think, is to not waste substitution entries on builtin types, most of which are encoded more compactly than substitutions anyway. That is why vendor extended types are an exception to the exception: they're not particularly compact and so are still useful to compress. (I don't know if we reliably get that right in the frontend, actually.) It's in keeping with that intent for _BigInt to be a substitution candidate, since it's a relatively long encoding. That's not what the ABI actually says, though, so we should fix it either in the frontend or in the ABI document.

[zsrkmyn]

I just noticed that DF isn't substituted by clang.

$ echo 'void foo(_Float16 a, _Float16 b) {}' | ./clang -xc++ - -S -o - -emit-llvm | grep def
define dso_local void @_Z3fooDF16_DF16_(half noundef %a, half noundef %b) #0 {

It would look good if we keep _FloatN and _Bitint are substituted (or not) in the same way no matter if we fix it in ABI doc or frontend.

I'd prefer fixing it in the ABI for the intent for shorter encoding. I can submit a fix in either clang or ABI if you like :-)

[rjmccall]

If it's not substituted by Clang, then Clang is actually matching the ABI, and it would need to be fixed in both places. What other implementation is substituting it?

[zsrkmyn]

I also checked how GCC trunck handles _Float16 on godbolt, but it seems it hasn't been supported yet. What other implementations should I check?

[zsrkmyn]

Filed a PR here to update the ABI :-)

libcxxabi/src/demangle/ItaniumDemangle.h

         return nullptr;
       if (!consumeIf('_'))
         return nullptr;
-      return make<BitIntType>(Size, Signed);
+      Result = make<BitIntType>(Size, Signed);
+      break;
     }
     //                ::= Di   # char32_t
     case 'i':

libcxxabi/test/test_demangle.pass.cpp

     {"_Z1Av", "A()"},
     {"_Z1A1B1C", "A(B, C)"},
     {"_Z1fDB3_", "f(_BitInt(3))"},
+    {"_Z1fDB3_S_", "f(_BitInt(3), _BitInt(3))"},
     {"_Z1fDU10_", "f(unsigned _BitInt(10))"},
     {"_Z1fIfEvDUstPT__", "void f<float>(unsigned _BitInt(sizeof (float*)))"},
     {"_Z1fIiEvDBstPT__", "void f<int>(_BitInt(sizeof (int*)))"},

llvm/include/llvm/Demangle/ItaniumDemangle.h

       bool Signed = look(1) == 'B';
       First += 2;
       Node *Size = std::isdigit(look()) ? make<NameType>(parseNumber())
                                         : getDerived().parseExpr();
       if (!Size)
         return nullptr;
       if (!consumeIf('_'))
         return nullptr;
-      return make<BitIntType>(Size, Signed);
+      Result = make<BitIntType>(Size, Signed);
+      break;
     }
     //                ::= Di   # char32_t
     case 'i':
       First += 2;
       return make<NameType>("char32_t");
     //                ::= Ds   # char16_t
     case 's':
       First += 2;