DrgnParser: Handle enum values in template params

We want to use the fully qualified name for scoped enums to keep the C++
compiler happy. When a parameter expects an enum value, we must supply
an enum value and not its underlying integer value.

Before:
  isset_bitset<1, 0>

After:
  isset_bitset<1, apache::thrift::detail::IssetBitsetOption::Unpacked>

oi/type_graph/AddChildren.cpp

@@ -53,7 +53,7 @@ void AddChildren::visit(Type& type) {
 
 void AddChildren::visit(Class& c) {
   for (auto& param : c.templateParams) {
-    visit(*param.type);
+    visit(param.type);
   }
   for (auto& member : c.members) {
     visit(*member.type);

oi/type_graph/AddChildren.h

@@ -46,6 +46,8 @@ class AddChildren final : public RecursiveVisitor {
       : typeGraph_(typeGraph), drgnParser_(drgnParser) {
   }
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Class& c) override;
 

oi/type_graph/AddPadding.cpp

@@ -50,14 +50,14 @@ void AddPadding::visit(Class& c) {
   assert(c.parents.empty());
 
   for (auto& param : c.templateParams) {
-    visit(*param.type);
+    visit(param.type);
   }
   for (auto& member : c.members) {
     visit(*member.type);
   }
 
   if (c.kind() == Class::Kind::Union) {
-    // Don't padd unions
+    // Don't pad unions
     return;
   }
 

oi/type_graph/AddPadding.h

@@ -40,6 +40,8 @@ class AddPadding final : public RecursiveVisitor {
   explicit AddPadding(TypeGraph& typeGraph) : typeGraph_(typeGraph) {
   }
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Class& c) override;
 

oi/type_graph/AlignmentCalc.h

@@ -37,6 +37,8 @@ class AlignmentCalc final : public RecursiveVisitor {
   void calculateAlignments(
       const std::vector<std::reference_wrapper<Type>>& types);
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Class& c) override;
 

oi/type_graph/DrgnParser.cpp

@@ -132,7 +132,6 @@ Container* DrgnParser::enumerateContainer(struct drgn_type* type) {
     return nullptr;
   }
 
-  std::string name{nameStr};
   auto size = get_drgn_type_size(type);
 
   for (const auto& containerInfo : containers_) {
@@ -299,36 +298,76 @@ void DrgnParser::enumerateTemplateParam(drgn_type_template_parameter* tparams,
     params.emplace_back(ttype, qualifiers);
   } else {
     // This template parameter is a value
-    //    TODO why do we need the type of a value?
-    //    tparamQualType.type = obj->type;
-    //    tparamQualType.qualifiers = obj->qualifiers;
     std::string value;
-    if (obj->encoding == DRGN_OBJECT_ENCODING_BUFFER) {
-      uint64_t size = drgn_object_size(obj);
-      char* buf = nullptr;
-      if (size <= sizeof(obj->value.ibuf)) {
-        buf = (char*)&(obj->value.ibuf);
-      } else {
-        buf = obj->value.bufp;
+
+    if (drgn_type_kind(obj->type) == DRGN_TYPE_ENUM) {
+      char* nameStr = nullptr;
+      size_t length = 0;
+      auto* err = drgn_type_fully_qualified_name(obj->type, &nameStr, &length);
+      if (err != nullptr || nameStr == nullptr) {
+        throw DrgnParserError{"Failed to get enum's fully qualified name", err};
       }
 
-      if (buf != nullptr) {
-        value = std::string(buf);
+      uint64_t enumVal;
+      switch (obj->encoding) {
+        case DRGN_OBJECT_ENCODING_SIGNED:
+          enumVal = obj->value.svalue;
+          break;
+        case DRGN_OBJECT_ENCODING_UNSIGNED:
+          enumVal = obj->value.uvalue;
+          break;
+        default:
+          throw DrgnParserError{
+              "Unknown template parameter object encoding format: " +
+              std::to_string(obj->encoding)};
+      }
+
+      drgn_type_enumerator* enumerators = drgn_type_enumerators(obj->type);
+      size_t numEnumerators = drgn_type_num_enumerators(obj->type);
+      for (size_t j = 0; j < numEnumerators; j++) {
+        if (enumerators[j].uvalue == enumVal) {
+          value = std::string{nameStr} + "::" + enumerators[j].name;
+          break;
+        }
+      }
+
+      if (value.empty()) {
+        throw DrgnParserError{"Unable to find enum name for value: " +
+                              std::to_string(enumVal)};
       }
-    } else if (obj->encoding == DRGN_OBJECT_ENCODING_SIGNED) {
-      value = std::to_string(obj->value.svalue);
-    } else if (obj->encoding == DRGN_OBJECT_ENCODING_UNSIGNED) {
-      value = std::to_string(obj->value.uvalue);
-    } else if (obj->encoding == DRGN_OBJECT_ENCODING_FLOAT) {
-      value = std::to_string(obj->value.fvalue);
     } else {
-      throw DrgnParserError{
-          "Unknown template parameter object encoding format: " +
-          std::to_string(obj->encoding)};
+      switch (obj->encoding) {
+        case DRGN_OBJECT_ENCODING_BUFFER: {
+          uint64_t size = drgn_object_size(obj);
+          char* buf = nullptr;
+          if (size <= sizeof(obj->value.ibuf)) {
+            buf = (char*)&(obj->value.ibuf);
+          } else {
+            buf = obj->value.bufp;
+          }
+
+          if (buf != nullptr) {
+            value = std::string(buf);
+          }
+          break;
+        }
+        case DRGN_OBJECT_ENCODING_SIGNED:
+          value = std::to_string(obj->value.svalue);
+          break;
+        case DRGN_OBJECT_ENCODING_UNSIGNED:
+          value = std::to_string(obj->value.uvalue);
+          break;
+        case DRGN_OBJECT_ENCODING_FLOAT:
+          value = std::to_string(obj->value.fvalue);
+          break;
+        default:
+          throw DrgnParserError{
+              "Unknown template parameter object encoding format: " +
+              std::to_string(obj->encoding)};
+      }
     }
 
-    params.emplace_back(make_type<Primitive>(nullptr, Primitive::Kind::UInt8),
-                        value);
+    params.emplace_back(std::move(value));
   }
 }
 

oi/type_graph/Flattener.cpp

@@ -167,7 +167,7 @@ void Flattener::visit(Container& c) {
   // Containers themselves don't need to be flattened, but their template
   // parameters might need to be
   for (const auto& templateParam : c.templateParams) {
-    visit(*templateParam.type);
+    visit(templateParam.type);
   }
 }
 

oi/type_graph/Flattener.h

@@ -36,6 +36,8 @@ class Flattener : public RecursiveVisitor {
 
   void flatten(std::vector<std::reference_wrapper<Type>>& types);
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Class& c) override;
   void visit(Container& c) override;

oi/type_graph/NameGen.cpp

@@ -69,7 +69,7 @@ void NameGen::visit(Class& c) {
   }
 
   for (const auto& param : c.templateParams) {
-    visit(*param.type);
+    visit(param.type);
   }
   for (const auto& parent : c.parents) {
     visit(*parent.type);
@@ -88,7 +88,7 @@ void NameGen::visit(Container& c) {
   }
 
   for (const auto& template_param : c.templateParams) {
-    visit(*template_param.type);
+    visit(template_param.type);
   }
 
   std::string name = c.name();

oi/type_graph/NameGen.h

@@ -32,6 +32,8 @@ class NameGen final : public RecursiveVisitor {
 
   void generateNames(const std::vector<std::reference_wrapper<Type>>& types);
 
+  using RecursiveVisitor::visit;
+
   void visit(Class& c) override;
   void visit(Container& c) override;
   void visit(Typedef& td) override;

oi/type_graph/Printer.cpp

@@ -24,7 +24,7 @@ Printer::Printer(std::ostream& out, size_t numTypes) : out_(out) {
   baseIndent_ = static_cast<int>(log10(static_cast<double>(numTypes)) + 1) + 3;
 }
 
-void Printer::print(Type& type) {
+void Printer::print(const Type& type) {
   depth_++;
   type.accept(*this);
   depth_--;
@@ -189,7 +189,7 @@ void Printer::print_function(const Function& function) {
   depth_--;
 }
 
-void Printer::print_child(Type& child) {
+void Printer::print_child(const Type& child) {
   depth_++;
   prefix();
   out_ << "Child:" << std::endl;

oi/type_graph/Printer.h

@@ -30,7 +30,7 @@ class Printer : public ConstVisitor {
  public:
   Printer(std::ostream& out, size_t numTypes);
 
-  void print(Type& type);
+  void print(const Type& type);
 
   void visit(const Class& c) override;
   void visit(const Container& c) override;
@@ -48,7 +48,7 @@ class Printer : public ConstVisitor {
   void print_parent(const Parent& parent);
   void print_member(const Member& member);
   void print_function(const Function& function);
-  void print_child(Type& child);
+  void print_child(const Type& child);
   void print_value(const std::string& value);
   static std::string align_str(uint64_t align);
 

oi/type_graph/RemoveIgnored.h

@@ -42,6 +42,8 @@ class RemoveIgnored : public RecursiveVisitor {
       : typeGraph_(typeGraph), membersToIgnore_(membersToIgnore) {
   }
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Class& c) override;
 

oi/type_graph/TopoSorter.cpp

@@ -56,7 +56,7 @@ void TopoSorter::visit(Type& type) {
 
 void TopoSorter::visit(Class& c) {
   for (const auto& param : c.templateParams) {
-    visit(*param.type);
+    visit(param.type);
   }
   for (const auto& parent : c.parents) {
     visit(*parent.type);
@@ -75,7 +75,7 @@ void TopoSorter::visit(Class& c) {
 
 void TopoSorter::visit(Container& c) {
   for (const auto& param : c.templateParams) {
-    visit(*param.type);
+    visit(param.type);
   }
   sortedTypes_.push_back(c);
 }

oi/type_graph/TopoSorter.h

@@ -38,6 +38,8 @@ class TopoSorter : public RecursiveVisitor {
   void sort(const std::vector<std::reference_wrapper<Type>>& types);
   const std::vector<std::reference_wrapper<Type>>& sortedTypes() const;
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Class& c) override;
   void visit(Container& c) override;

oi/type_graph/TypeIdentifier.cpp

@@ -100,7 +100,7 @@ void TypeIdentifier::visit(Container& c) {
   }
 
   for (const auto& param : c.templateParams) {
-    visit(*param.type);
+    visit(param.type);
   }
 }
 

oi/type_graph/TypeIdentifier.h

@@ -37,6 +37,8 @@ class TypeIdentifier : public RecursiveVisitor {
   TypeIdentifier(TypeGraph& typeGraph) : typeGraph_(typeGraph) {
   }
 
+  using RecursiveVisitor::visit;
+
   void visit(Type& type) override;
   void visit(Container& c) override;
 

oi/type_graph/Types.h

@@ -105,14 +105,12 @@ struct TemplateParam {
   TemplateParam(Type* type, QualifierSet qualifiers)
       : type(type), qualifiers(qualifiers) {
   }
-  TemplateParam(Type* type, std::string value)
-      : type(type), value(std::move(value)) {
+  TemplateParam(std::string value) : value(std::move(value)) {
   }
 
-  Type* type;
+  Type* type = nullptr;  // Note: type is not always set
   QualifierSet qualifiers;
-  std::optional<std::string>
-      value;  // TODO is there any reason not to store all values as strings?
+  std::optional<std::string> value;
 };
 
 class Class : public Type {

oi/type_graph/Visitor.h

@@ -58,9 +58,13 @@ class RecursiveVisitor : public Visitor {
  public:
   virtual ~RecursiveVisitor() = default;
   virtual void visit(Type&) = 0;
+  virtual void visit(Type* type) {
+    if (type)
+      visit(*type);
+  }
   virtual void visit(Class& c) {
     for (const auto& param : c.templateParams) {
-      visit(*param.type);
+      visit(param.type);
     }
     for (const auto& parent : c.parents) {
       visit(*parent.type);
@@ -74,7 +78,7 @@ class RecursiveVisitor : public Visitor {
   }
   virtual void visit(Container& c) {
     for (const auto& param : c.templateParams) {
-      visit(*param.type);
+      visit(param.type);
     }
   }
   virtual void visit(Primitive&) {

test/integration/CMakeLists.txt

@@ -3,9 +3,9 @@ set(INTEGRATION_TEST_CONFIGS
   alignment.toml
   anonymous.toml
   arrays.toml
-  container_enums.toml
   cycles.toml
   enums.toml
+  enums_params.toml
   fbstring.toml
   ignored.toml
   inheritance_access.toml

test/integration/enums_params.toml

@@ -12,21 +12,51 @@ definitions = '''
     ONE = 1,
     TWO = 2,
   };
+
+  enum class EnumWithGaps {
+    Five = 5,
+    MinusTwo = -2,
+    Twenty = 20,
+  };
   } // MyNS
+
+  template <MyNS::ScopedEnum val>
+  class MyClass {
+    int n;
+  };
+
+  template <MyNS::EnumWithGaps val>
+  class ClassGaps {
+    int n;
+  };
 '''
 
 [cases]
   [cases.scoped_enum_type]
     param_types = ["const std::vector<MyNS::ScopedEnum>&"]
     setup = "return {};"
-  [cases.scoped_enum_val]
+  [cases.scoped_enum_val_cast]
     param_types = ["const std::array<int, static_cast<size_t>(MyNS::ScopedEnum::Two)>&"]
     setup = "return {};"
     expect_json = '[{"staticSize":8, "dynamicSize":0, "length":2, "capacity":2, "elementStaticSize":4}]'
+
+  [cases.scoped_enum_val]
+    param_types = ["const MyClass<MyNS::ScopedEnum::One>&"]
+    setup = "return {};"
+    expect_json = '[{"staticSize":4, "dynamicSize":0, "exclusiveSize":0}]'
+  [cases.scoped_enum_val_gaps]
+    param_types = ["const ClassGaps<MyNS::EnumWithGaps::Twenty>&"]
+    setup = "return {};"
+    expect_json = '[{"staticSize":4, "dynamicSize":0, "exclusiveSize":0}]'
+  [cases.scoped_enum_val_negative]
+    param_types = ["const ClassGaps<MyNS::EnumWithGaps::MinusTwo>&"]
+    setup = "return {};"
+    expect_json = '[{"staticSize":4, "dynamicSize":0, "exclusiveSize":0}]'
+
   [cases.unscoped_enum_type]
     param_types = ["const std::vector<MyNS::UNSCOPED_ENUM>&"]
     setup = "return {};"
-  [cases.unscoped_enum_val]
+  [cases.unscoped_enum_val_cast]
     param_types = ["const std::array<int, MyNS::ONE>&"]
     setup = "return {};"
     expect_json = '[{"staticSize":4, "dynamicSize":0, "length":1, "capacity":1, "elementStaticSize":4}]'

test/test_drgn_parser.cpp

@@ -1,3 +1,4 @@
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include <regex>
@@ -12,6 +13,7 @@
 #include "oi/type_graph/Types.h"
 
 using namespace type_graph;
+using ::testing::HasSubstr;
 
 // TODO setup google logging for tests so it doesn't appear on terminal by
 // default
@@ -26,22 +28,29 @@ class DrgnParserTest : public ::testing::Test {
     delete symbols_;
   }
 
+  std::string run(std::string_view function, bool chaseRawPointers);
   void test(std::string_view function,
             std::string_view expected,
             bool chaseRawPointers = true);
+  void testContains(std::string_view function,
+                    std::string_view expected,
+                    bool chaseRawPointers = true);
   void testMultiCompiler(std::string_view function,
                          std::string_view expectedClang,
                          std::string_view expectedGcc,
                          bool chaseRawPointers = true);
+  void testMultiCompilerContains(std::string_view function,
+                                 std::string_view expectedClang,
+                                 std::string_view expectedGcc,
+                                 bool chaseRawPointers = true);
 
   static SymbolService* symbols_;
 };
 
 SymbolService* DrgnParserTest::symbols_ = nullptr;
 
-void DrgnParserTest::test(std::string_view function,
-                          std::string_view expected,
-                          bool chaseRawPointers) {
+std::string DrgnParserTest::run(std::string_view function,
+                                bool chaseRawPointers) {
   irequest req{"entry", std::string{function}, "arg0"};
   auto drgnRoot = symbols_->getRootType(req);
 
@@ -60,9 +69,25 @@ void DrgnParserTest::test(std::string_view function,
   Printer printer{out, typeGraph.size()};
   printer.print(*type);
 
-  // TODO standardise expected-actual order
+  return out.str();
+}
+
+void DrgnParserTest::test(std::string_view function,
+                          std::string_view expected,
+                          bool chaseRawPointers) {
+  auto actual = run(function, chaseRawPointers);
+
   expected.remove_prefix(1);  // Remove initial '\n'
-  EXPECT_EQ(expected, out.str());
+  EXPECT_EQ(expected, actual);
+}
+
+void DrgnParserTest::testContains(std::string_view function,
+                                  std::string_view expected,
+                                  bool chaseRawPointers) {
+  auto actual = run(function, chaseRawPointers);
+
+  expected.remove_prefix(1);  // Remove initial '\n'
+  EXPECT_THAT(actual, HasSubstr(expected));
 }
 
 void DrgnParserTest::testMultiCompiler(
@@ -77,6 +102,18 @@ void DrgnParserTest::testMultiCompiler(
 #endif
 }
 
+void DrgnParserTest::testMultiCompilerContains(
+    std::string_view function,
+    [[maybe_unused]] std::string_view expectedClang,
+    [[maybe_unused]] std::string_view expectedGcc,
+    bool chaseRawPointers) {
+#if defined(__clang__)
+  testContains(function, expectedClang, chaseRawPointers);
+#else
+  testContains(function, expectedGcc, chaseRawPointers);
+#endif
+}
+
 TEST_F(DrgnParserTest, SimpleStruct) {
   test("oid_test_case_simple_struct", R"(
 [0] Pointer
@@ -430,6 +467,54 @@ TEST_F(DrgnParserTest, ClassTemplateValue) {
 )");
 }
 
+TEST_F(DrgnParserTest, TemplateEnumValue) {
+  testMultiCompilerContains("oid_test_case_enums_params_scoped_enum_val",
+                            R"(
+[0] Pointer
+[1]   Class: MyClass<ns_enums_params::MyNS::ScopedEnum::One> (size: 4)
+        Param
+          Value: ns_enums_params::MyNS::ScopedEnum::One
+)",
+                            R"(
+[0] Pointer
+[1]   Class: MyClass<(ns_enums_params::MyNS::ScopedEnum)1> (size: 4)
+        Param
+          Value: ns_enums_params::MyNS::ScopedEnum::One
+)");
+}
+
+TEST_F(DrgnParserTest, TemplateEnumValueGaps) {
+  testMultiCompilerContains("oid_test_case_enums_params_scoped_enum_val_gaps",
+                            R"(
+[0] Pointer
+[1]   Class: ClassGaps<ns_enums_params::MyNS::EnumWithGaps::Twenty> (size: 4)
+        Param
+          Value: ns_enums_params::MyNS::EnumWithGaps::Twenty
+)",
+                            R"(
+[0] Pointer
+[1]   Class: ClassGaps<(ns_enums_params::MyNS::EnumWithGaps)20> (size: 4)
+        Param
+          Value: ns_enums_params::MyNS::EnumWithGaps::Twenty
+)");
+}
+
+TEST_F(DrgnParserTest, TemplateEnumValueNegative) {
+  testMultiCompilerContains(
+      "oid_test_case_enums_params_scoped_enum_val_negative", R"(
+[0] Pointer
+[1]   Class: ClassGaps<ns_enums_params::MyNS::EnumWithGaps::MinusTwo> (size: 4)
+        Param
+          Value: ns_enums_params::MyNS::EnumWithGaps::MinusTwo
+)",
+      R"(
+[0] Pointer
+[1]   Class: ClassGaps<(ns_enums_params::MyNS::EnumWithGaps)-2> (size: 4)
+        Param
+          Value: ns_enums_params::MyNS::EnumWithGaps::MinusTwo
+)");
+}
+
 // TODO
 // TEST_F(DrgnParserTest, ClassFunctions) {
 //  test("TestClassFunctions", R"(

test/test_name_gen.cpp

@@ -183,6 +183,22 @@ TEST(NameGenTest, ContainerNoParams) {
   EXPECT_EQ(mycontainer.name(), "std::vector");
 }
 
+TEST(NameGenTest, ContainerParamsValue) {
+  auto myint = Primitive{Primitive::Kind::Int32};
+  auto myenum = Enum{"MyEnum", 4};
+
+  auto mycontainer = getVector();
+  mycontainer.templateParams.push_back(TemplateParam{"123"});
+  mycontainer.templateParams.push_back(TemplateParam{"MyEnum::OptionC"});
+
+  NameGen nameGen;
+  nameGen.generateNames({mycontainer});
+
+  EXPECT_EQ(myint.name(), "int32_t");
+  EXPECT_EQ(myenum.name(), "MyEnum");
+  EXPECT_EQ(mycontainer.name(), "std::vector<123, MyEnum::OptionC>");
+}
+
 TEST(NameGenTest, Array) {
   auto myparam1 = std::make_unique<Class>(Class::Kind::Struct, "MyParam", 13);
   auto myparam2 = std::make_unique<Class>(Class::Kind::Struct, "MyParam", 13);