[Demangle] Add minimal support for D simple basic types

[ljmf00]

Summary

This patch implements simple demangling of two basic types to add minimal type functionality. This will be later used in function type parsing. After that being implemented we can add the rest of the types and test the result of the type name.

[dblaikie]

Missing test coverage - all the cases added should be tested.

[ljmf00]

In D111416#3052693, @dblaikie wrote:

Missing test coverage - all the cases added should be tested.

Well, I thought about this but currently, the demangler is not prepending the type to the demangled symbol so, there is no way to visually test this until https://reviews.llvm.org/D111421. That is why those tests get introduced there instead (types are shown in function parameters). I can add tests to check if this, at least, doesn't fail (nullptr) although I thought that adding tests for all of the types expecting the same output was useless when https://reviews.llvm.org/D111421 get introduced.

[dblaikie]

In D111416#3056708, @ljmf00 wrote:

In D111416#3052693, @dblaikie wrote:

Missing test coverage - all the cases added should be tested.

Well, I thought about this but currently, the demangler is not prepending the type to the demangled symbol so, there is no way to visually test this until https://reviews.llvm.org/D111421. That is why those tests get introduced there instead (types are shown in function parameters). I can add tests to check if this, at least, doesn't fail (nullptr) although I thought that adding tests for all of the types expecting the same output was useless when https://reviews.llvm.org/D111421 get introduced.

Perhaps the patches are out of order then, or split too much - code should be tested when it is introduced.

[ljmf00]

In D111416#3059334, @dblaikie wrote:

In D111416#3056708, @ljmf00 wrote:

In D111416#3052693, @dblaikie wrote:

Missing test coverage - all the cases added should be tested.

Well, I thought about this but currently, the demangler is not prepending the type to the demangled symbol so, there is no way to visually test this until https://reviews.llvm.org/D111421. That is why those tests get introduced there instead (types are shown in function parameters). I can add tests to check if this, at least, doesn't fail (nullptr) although I thought that adding tests for all of the types expecting the same output was useless when https://reviews.llvm.org/D111421 get introduced.

Perhaps the patches are out of order then, or split too much - code should be tested when it is introduced.

They are not out of order, according to the ABI specification grammar for name mangling: https://dlang.org/spec/abi.html#name_mangling
This is a problem on this particular demangler, since it doesn't include return types, or variable types. I can add tests for this, anyway, but as I described earlier, it won't be distinguishable at this point.
This behaviour is tested when function symbols are parsable (https://reviews.llvm.org/D111421). Function symbols are dependent of of TypeFunction node (https://dlang.org/spec/abi.html#TypeFunction) and subsequently dependent of Type node, which is what this patch implements.

There is two possible options I can think of:
A. Add intentional support for return types / variable types to the demangled symbol and add visible tests accordingly.
B. Add invisible tests to do increase coverage and remove them lately when https://reviews.llvm.org/D111421 get introduced.

To clarify what do I mean about invisible tests is:

{"_D8demangle1ai", "demangle.a"},
{"_D8demangle1aOi", "demangle.a"},
{"_D8demangle1ae", "demangle.a"},

Those three tests include different mangled symbols but the same demangled symbol.

What are the implications of A. ? It will introduce some complexity to the demangler and doesn't add much benefit to the end user, since you can't have same symbol name with different types.
I guess another point that can be raised is consistency among the other demanglers on LLVM: do they include the return type of a function symbol or type of a variable? For what I analyzed from Rust, it doesn't, but I'm not too familiar with Rust and can't read/understand its mangled names easily to know if it includes the type of a symbol. If you can help me on giving me examples, we can discuss better on what is better to do here :)

[dblaikie]

In D111416#3059466, @ljmf00 wrote:

In D111416#3059334, @dblaikie wrote:

In D111416#3056708, @ljmf00 wrote:

In D111416#3052693, @dblaikie wrote:

Missing test coverage - all the cases added should be tested.

Well, I thought about this but currently, the demangler is not prepending the type to the demangled symbol so, there is no way to visually test this until https://reviews.llvm.org/D111421. That is why those tests get introduced there instead (types are shown in function parameters). I can add tests to check if this, at least, doesn't fail (nullptr) although I thought that adding tests for all of the types expecting the same output was useless when https://reviews.llvm.org/D111421 get introduced.

Perhaps the patches are out of order then, or split too much - code should be tested when it is introduced.

They are not out of order, according to the ABI specification grammar for name mangling: https://dlang.org/spec/abi.html#name_mangling
This is a problem on this particular demangler, since it doesn't include return types, or variable types. I can add tests for this, anyway, but as I described earlier, it won't be distinguishable at this point.
This behaviour is tested when function symbols are parsable (https://reviews.llvm.org/D111421). Function symbols are dependent of of TypeFunction node (https://dlang.org/spec/abi.html#TypeFunction) and subsequently dependent of Type node, which is what this patch implements.

There is two possible options I can think of:
A. Add intentional support for return types / variable types to the demangled symbol and add visible tests accordingly.
B. Add invisible tests to do increase coverage and remove them lately when https://reviews.llvm.org/D111421 get introduced.

To clarify what do I mean about invisible tests is:

{"_D8demangle1ai", "demangle.a"},
{"_D8demangle1aOi", "demangle.a"},
{"_D8demangle1ae", "demangle.a"},

Those three tests include different mangled symbols but the same demangled symbol.

What are the implications of A. ? It will introduce some complexity to the demangler and doesn't add much benefit to the end user, since you can't have same symbol name with different types.
I guess another point that can be raised is consistency among the other demanglers on LLVM: do they include the return type of a function symbol or type of a variable? For what I analyzed from Rust, it doesn't, but I'm not too familiar with Rust and can't read/understand its mangled names easily to know if it includes the type of a symbol. If you can help me on giving me examples, we can discuss better on what is better to do here :)

I think we're getting into the weeds a bit.

A few things that might help: Whole grammar rules don't have to be implemented atomically - a patch could add support for, say, only a basic integer type if that's accessible/testable right now but other types aren't accessible/testable. Then when function types (basic functionality of that could be tested only with integer types?) are added and other types are accessible via that - other types could be added and tested.

When I said out of order, what I meant was maybe some other functionality could be added first (and tested) that would then ennable these type parsing cases to be tested when they're added on top. For instance, maybe function types can be added without these other types being supported - starting with a void() function.

But, to answer the other question, yes return types are rendered when they are mangled. In C++ templates get mangled return types, and they are rendered in the demangling:

$ cat mangle.cpp
void f1() { }
template<typename T>
void f2() { }
int main() {
  f2<int>();
}
$ clang++-tot mangle.cpp -c && nm mangle.o | llvm-cxxfilt
0000000000000010 T main
0000000000000000 T f1()
0000000000000000 W void f2<int>()

[ljmf00]

In D111416#3059520, @dblaikie wrote:

In D111416#3059466, @ljmf00 wrote:

In D111416#3059334, @dblaikie wrote:

In D111416#3056708, @ljmf00 wrote:

In D111416#3052693, @dblaikie wrote:

Missing test coverage - all the cases added should be tested.

Well, I thought about this but currently, the demangler is not prepending the type to the demangled symbol so, there is no way to visually test this until https://reviews.llvm.org/D111421. That is why those tests get introduced there instead (types are shown in function parameters). I can add tests to check if this, at least, doesn't fail (nullptr) although I thought that adding tests for all of the types expecting the same output was useless when https://reviews.llvm.org/D111421 get introduced.

Perhaps the patches are out of order then, or split too much - code should be tested when it is introduced.

They are not out of order, according to the ABI specification grammar for name mangling: https://dlang.org/spec/abi.html#name_mangling
This is a problem on this particular demangler, since it doesn't include return types, or variable types. I can add tests for this, anyway, but as I described earlier, it won't be distinguishable at this point.
This behaviour is tested when function symbols are parsable (https://reviews.llvm.org/D111421). Function symbols are dependent of of TypeFunction node (https://dlang.org/spec/abi.html#TypeFunction) and subsequently dependent of Type node, which is what this patch implements.

There is two possible options I can think of:
A. Add intentional support for return types / variable types to the demangled symbol and add visible tests accordingly.
B. Add invisible tests to do increase coverage and remove them lately when https://reviews.llvm.org/D111421 get introduced.

To clarify what do I mean about invisible tests is:

{"_D8demangle1ai", "demangle.a"},
{"_D8demangle1aOi", "demangle.a"},
{"_D8demangle1ae", "demangle.a"},

Those three tests include different mangled symbols but the same demangled symbol.

What are the implications of A. ? It will introduce some complexity to the demangler and doesn't add much benefit to the end user, since you can't have same symbol name with different types.
I guess another point that can be raised is consistency among the other demanglers on LLVM: do they include the return type of a function symbol or type of a variable? For what I analyzed from Rust, it doesn't, but I'm not too familiar with Rust and can't read/understand its mangled names easily to know if it includes the type of a symbol. If you can help me on giving me examples, we can discuss better on what is better to do here :)

I think we're getting into the weeds a bit.

A few things that might help: Whole grammar rules don't have to be implemented atomically - a patch could add support for, say, only a basic integer type if that's accessible/testable right now but other types aren't accessible/testable. Then when function types (basic functionality of that could be tested only with integer types?) are added and other types are accessible via that - other types could be added and tested.

When I said out of order, what I meant was maybe some other functionality could be added first (and tested) that would then ennable these type parsing cases to be tested when they're added on top. For instance, maybe function types can be added without these other types being supported - starting with a void() function.

Ok, that makes sense to me. So adding only a few types here to prove this functionality, adding basic testing for them and introduce the rest of the types after function types get introduced and test them.

But, to answer the other question, yes return types are rendered when they are mangled. In C++ templates get mangled return types, and they are rendered in the demangling:

$ cat mangle.cpp
void f1() { }
template<typename T>
void f2() { }
int main() {
  f2<int>();
}
$ clang++-tot mangle.cpp -c && nm mangle.o | llvm-cxxfilt
0000000000000010 T main
0000000000000000 T f1()
0000000000000000 W void f2<int>()

Ok, didn't know that. This can be introduced in a future patch then and do what you suggested, right now.

[MaskRay]

llvm/lib/Demangle/DLangDemangle.cpp

279

Prefer // for C++ (https://llvm.org/docs/CodingStandards.html#comment-formatting)

280

Prefer pre-increment ++Mangled (https://llvm.org/docs/CodingStandards.html#prefer-preincrement)

[ljmf00]

@dblaikie I removed all the uncovered types and added just minimal support just to prove type parsing functionality. I change the name of the patch to reflect the changes. I still have Demangled << "int"; and Demangled << "void"; that are covered but with untested behaviour. I can add a comment explaining why or remove it completely if you prefer.

llvm/lib/Demangle/DLangDemangle.cpp

280

Done

[ljmf00]

Added comment on nullptr testcases.

[dblaikie]

In D111416#3223399, @ljmf00 wrote:

@dblaikie I removed all the uncovered types and added just minimal support just to prove type parsing functionality. I change the name of the patch to reflect the changes. I still have Demangled << "int"; and Demangled << "void"; that are covered but with untested behaviour. I can add a comment explaining why or remove it completely if you prefer.

Yeah, I think best to remove them for now - introduce them when they're testable. Not sure what the minimal functionality si for now - if you need one of them, but to do nothing, could just have "case 'v': ++Mangled; return Mangled;" or the like.

[ljmf00]

In D111416#3233116, @dblaikie wrote:

In D111416#3223399, @ljmf00 wrote:

@dblaikie I removed all the uncovered types and added just minimal support just to prove type parsing functionality. I change the name of the patch to reflect the changes. I still have Demangled << "int"; and Demangled << "void"; that are covered but with untested behaviour. I can add a comment explaining why or remove it completely if you prefer.

Yeah, I think best to remove them for now - introduce them when they're testable. Not sure what the minimal functionality si for now - if you need one of them, but to do nothing, could just have "case 'v': ++Mangled; return Mangled;" or the like.

I added i (int) instead of v (void), since it makes more sense. A void variable is permitted by the ABI but feels weird, especially if adding support for function types before a function that receives a void parameter is also weird. I removed the untested Demangled. Can you please re-review it?

[dblaikie]

llvm/lib/Demangle/DLangDemangle.cpp

275

Looks like this function is currently never called with a null parameter (the one call is inside an if (Mangled != nullptr) block). So this should probably be omitted, or replaced with an assertion.

Is the other condition (== '\0') reachable/tested? (I guess it'd fall out naturally from the switch case below - so maybe that could be done instead and this if test/early return could be removed?)

[ljmf00]

llvm/lib/Demangle/DLangDemangle.cpp

275

Makes sense. I omitted the nullptr check, since this was contrarily tested on line 182 and this should never ever happen and an assert wouldn't benefit much as dereferencing will certainly happen right after anyway.

About the null terminator char check, I added a test for that.

[dblaikie]

Good stuff, thanks!

[ljmf00]

Added final dots on unit test comments.

llvm/lib/Demangle/DLangDemangle.cpp

private:
   /// \param Demangled Output buffer to write the demangled name.
   /// \param Mangled Mangled symbol to be demangled.
   ///
   /// \return The remaining string on success or nullptr on failure.
   ///
   /// \see https://dlang.org/spec/abi.html#QualifiedName .
   const char *parseQualified(OutputBuffer *Demangled, const char *Mangled);
 
+  /// Extract and demangle a type from a given mangled symbol append it to
+  /// the output string.
+  ///
+  /// \param Mangled mangled symbol to be demangled.
+  ///
+  /// \return the remaining string on success or nullptr on failure.
+  ///
+  /// \see https://dlang.org/spec/abi.html#Type .
+  const char *parseType(const char *Mangled);
+
   /// The string we are demangling.
   const char *Str;
 };
 
 } // namespace
 
 const char *Demangler::decodeNumber(const char *Mangled, unsigned long *Ret) {
   // Return nullptr if trying to extract something that isn't a digit.
const char *Demangler::parseMangle(OutputBuffer *Demangled,
 
   Mangled = parseQualified(Demangled, Mangled);
 
   if (Mangled != nullptr) {
     // Artificial symbols end with 'Z' and have no type.
     if (*Mangled == 'Z')
       ++Mangled;
     else {
-      // TODO: Implement symbols with types.
-      return nullptr;
+      Mangled = parseType(Mangled);
     }
   }
 
   return Mangled;
 }
 
 const char *Demangler::parseQualified(OutputBuffer *Demangled,
                                       const char *Mangled) {
if (Len >= 4 && Mangled[0] == '_' && Mangled[1] == '_' && Mangled[2] == 'S') {
     }
 
     // Else demangle it as a plain identifier.
   }
 
   return parseLName(Demangled, Mangled, Len);
 }
 
+const char *Demangler::parseType(const char *Mangled) {
+  if (*Mangled == '\0')
+    return nullptr;
+
+  switch (*Mangled) {
+  // TODO: Parse type qualifiers.
+  // TODO: Parse function types.
+  // TODO: Parse compound types.
+  // TODO: Parse delegate types.
+  // TODO: Parse tuple types.
+
+  // Basic types.
+  case 'i':
+    ++Mangled;
+    // TODO: Add type name dumping
+    return Mangled;
+
+    // TODO: Add support for the rest of the basic types.
+    // TODO: Parse back referenced types.
+
+  default: // unhandled.
+    return nullptr;
+  }
+}
+
 const char *Demangler::parseLName(OutputBuffer *Demangled, const char *Mangled,
                                   unsigned long Len) {
   switch (Len) {
   case 6:
     if (strncmp(Mangled, "__initZ", Len + 1) == 0) {
       // The static initializer for a given symbol.
       Demangled->prepend("initializer for ");
       Demangled->setCurrentPosition(Demangled->getCurrentPosition() - 1);

llvm/unittests/Demangle/DLangDemangleTest.cpp

testing::Values(
         std::make_pair("_D8demangle4test6__vtblZ", "vtable for demangle.test"),
         std::make_pair("_D8demangle4test7__ClassZ",
                        "ClassInfo for demangle.test"),
         std::make_pair("_D8demangle4test11__InterfaceZ",
                        "Interface for demangle.test"),
         std::make_pair("_D8demangle4test12__ModuleInfoZ",
                        "ModuleInfo for demangle.test"),
         std::make_pair("_D8demangle4__S14testZ", "demangle.test"),
-        std::make_pair("_D8demangle4__Sd4testZ", "demangle.__Sd.test")));
+        std::make_pair("_D8demangle4__Sd4testZ", "demangle.__Sd.test"),
+        std::make_pair("_D8demangle3fooi", "demangle.foo"),
+        std::make_pair("_D8demangle3foo",
+                       nullptr), // symbol without a type sequence.
+        std::make_pair("_D8demangle3fooinvalidtypeseq",
+                       nullptr))); // invalid type sequence.