From 2c5fb5d8453f447cd30a4dbce5d0ad444f1abca1 Mon Sep 17 00:00:00 2001 From: Alastair Robertson Date: Thu, 2 Nov 2023 06:18:32 -0700 Subject: [PATCH] TypeGraph: Stop identifying containers in DrgnParser Leave it to the new mutator pass IdentifyContainers to replace Class nodes with Container nodes where appropriate. This will allow us to run passes over the type graph before identifying containers, and therefore before we have lost information about the internal details of the container (e.g. alignment of member variables). --- oi/CodeGen.cpp | 16 ++- oi/CodeGen.h | 1 + oi/type_graph/DrgnParser.cpp | 32 +---- oi/type_graph/DrgnParser.h | 17 +-- oi/type_graph/TypeIdentifier.cpp | 1 + test/test_add_children.cpp | 22 ++-- test/test_codegen.cpp | 41 +++++-- test/test_drgn_parser.cpp | 196 ++++++++++--------------------- test/test_drgn_parser.h | 9 +- 9 files changed, 131 insertions(+), 204 deletions(-) diff --git a/oi/CodeGen.cpp b/oi/CodeGen.cpp index fef0a47..2d0c0eb 100644 --- a/oi/CodeGen.cpp +++ b/oi/CodeGen.cpp @@ -32,6 +32,7 @@ #include "type_graph/DrgnParser.h" #include "type_graph/EnforceCompatibility.h" #include "type_graph/Flattener.h" +#include "type_graph/IdentifyContainers.h" #include "type_graph/KeyCapture.h" #include "type_graph/NameGen.h" #include "type_graph/Prune.h" @@ -55,6 +56,7 @@ using type_graph::DrgnParserOptions; using type_graph::EnforceCompatibility; using type_graph::Enum; using type_graph::Flattener; +using type_graph::IdentifyContainers; using type_graph::KeyCapture; using type_graph::Member; using type_graph::NameGen; @@ -1137,21 +1139,25 @@ bool CodeGen::codegenFromDrgn(struct drgn_type* drgnType, std::string& code) { return true; } +void CodeGen::registerContainer(std::unique_ptr info) { + VLOG(1) << "Registered container: " << info->typeName; + containerInfos_.emplace_back(std::move(info)); +} + void CodeGen::registerContainer(const fs::path& path) { auto info = std::make_unique(path); if (info->requiredFeatures != (config_.features & info->requiredFeatures)) { VLOG(1) << "Skipping container (feature conflict): " << info->typeName; return; } - VLOG(1) << "Registered container: " << info->typeName; - containerInfos_.emplace_back(std::move(info)); + registerContainer(std::move(info)); } void CodeGen::addDrgnRoot(struct drgn_type* drgnType, TypeGraph& typeGraph) { DrgnParserOptions options{ .chaseRawPointers = config_.features[Feature::ChaseRawPointers], }; - DrgnParser drgnParser{typeGraph, containerInfos_, options}; + DrgnParser drgnParser{typeGraph, options}; Type& parsedRoot = drgnParser.parse(drgnType); typeGraph.addRoot(parsedRoot); } @@ -1162,6 +1168,7 @@ void CodeGen::transform(TypeGraph& typeGraph) { // Simplify the type graph first so there is less work for later passes pm.addPass(RemoveTopLevelPointer::createPass()); pm.addPass(Flattener::createPass()); + pm.addPass(IdentifyContainers::createPass(containerInfos_)); pm.addPass(TypeIdentifier::createPass(config_.passThroughTypes)); if (config_.features[Feature::PruneTypeGraph]) pm.addPass(Prune::createPass()); @@ -1171,11 +1178,12 @@ void CodeGen::transform(TypeGraph& typeGraph) { DrgnParserOptions options{ .chaseRawPointers = config_.features[Feature::ChaseRawPointers], }; - DrgnParser drgnParser{typeGraph, containerInfos_, options}; + DrgnParser drgnParser{typeGraph, options}; pm.addPass(AddChildren::createPass(drgnParser, symbols_)); // Re-run passes over newly added children pm.addPass(Flattener::createPass()); + pm.addPass(IdentifyContainers::createPass(containerInfos_)); pm.addPass(TypeIdentifier::createPass(config_.passThroughTypes)); if (config_.features[Feature::PruneTypeGraph]) pm.addPass(Prune::createPass()); diff --git a/oi/CodeGen.h b/oi/CodeGen.h index c6f2a5d..186ec34 100644 --- a/oi/CodeGen.h +++ b/oi/CodeGen.h @@ -53,6 +53,7 @@ class CodeGen { std::string linkageName, std::string& code); + void registerContainer(std::unique_ptr containerInfo); void registerContainer(const std::filesystem::path& path); void addDrgnRoot(struct drgn_type* drgnType, type_graph::TypeGraph& typeGraph); diff --git a/oi/type_graph/DrgnParser.cpp b/oi/type_graph/DrgnParser.cpp index 61f7a89..42a5033 100644 --- a/oi/type_graph/DrgnParser.cpp +++ b/oi/type_graph/DrgnParser.cpp @@ -25,8 +25,6 @@ extern "C" { #include } -#include - namespace oi::detail::type_graph { namespace { @@ -148,31 +146,7 @@ Type& DrgnParser::enumerateType(struct drgn_type* type) { return *t; } -/* - * enumerateContainer - * - * Attempts to parse a drgn_type as a Container. Returns nullptr if not - * sucessful. - */ -Container* DrgnParser::enumerateContainer(struct drgn_type* type, - const std::string& fqName) { - auto size = get_drgn_type_size(type); - - for (const auto& containerInfo : containers_) { - if (!std::regex_search(fqName, containerInfo->matcher)) { - continue; - } - - VLOG(2) << "Matching container `" << containerInfo->typeName << "` from `" - << fqName << "`" << std::endl; - auto& c = makeType(type, *containerInfo, size); - enumerateClassTemplateParams(type, c.templateParams); - return &c; - } - return nullptr; -} - -Type& DrgnParser::enumerateClass(struct drgn_type* type) { +Class& DrgnParser::enumerateClass(struct drgn_type* type) { std::string fqName; char* nameStr = nullptr; size_t length = 0; @@ -181,10 +155,6 @@ Type& DrgnParser::enumerateClass(struct drgn_type* type) { fqName = nameStr; } - auto* container = enumerateContainer(type, fqName); - if (container) - return *container; - const char* typeTag = drgn_type_tag(type); std::string name = typeTag ? typeTag : ""; diff --git a/oi/type_graph/DrgnParser.h b/oi/type_graph/DrgnParser.h index 9b00928..5b8fbe3 100644 --- a/oi/type_graph/DrgnParser.h +++ b/oi/type_graph/DrgnParser.h @@ -46,25 +46,19 @@ struct DrgnParserOptions { * - Performance: reading no more info from drgn than necessary * * For the most part we try to move logic out of DrgnParser and have later - * passes clean up the type graph, e.g. flattening parents. However, we do - * incorporate some extra logic here when it would allow us to read less DWARF - * information, e.g. matching containers in DrngParser means we don't read - * details about container internals. + * passes clean up the type graph, e.g. flattening parents and identifying + * containers. */ class DrgnParser { public: - DrgnParser(TypeGraph& typeGraph, - const std::vector>& containers, - DrgnParserOptions options) - : typeGraph_(typeGraph), containers_(containers), options_(options) { + DrgnParser(TypeGraph& typeGraph, DrgnParserOptions options) + : typeGraph_(typeGraph), options_(options) { } Type& parse(struct drgn_type* root); private: Type& enumerateType(struct drgn_type* type); - Container* enumerateContainer(struct drgn_type* type, - const std::string& fqName); - Type& enumerateClass(struct drgn_type* type); + Class& enumerateClass(struct drgn_type* type); Enum& enumerateEnum(struct drgn_type* type); Typedef& enumerateTypedef(struct drgn_type* type); Type& enumeratePointer(struct drgn_type* type); @@ -98,7 +92,6 @@ class DrgnParser { drgn_types_; TypeGraph& typeGraph_; - const std::vector>& containers_; int depth_; DrgnParserOptions options_; }; diff --git a/oi/type_graph/TypeIdentifier.cpp b/oi/type_graph/TypeIdentifier.cpp index 568b27d..0fd8696 100644 --- a/oi/type_graph/TypeIdentifier.cpp +++ b/oi/type_graph/TypeIdentifier.cpp @@ -86,6 +86,7 @@ void TypeIdentifier::visit(Container& c) { } c.templateParams[i] = *dummy; replaced = true; + break; } } diff --git a/test/test_add_children.cpp b/test/test_add_children.cpp index 0e6322c..5d563cb 100644 --- a/test/test_add_children.cpp +++ b/test/test_add_children.cpp @@ -66,7 +66,7 @@ TEST_F(AddChildrenTest, InheritanceStatic) { } TEST_F(AddChildrenTest, InheritancePolymorphic) { - testMultiCompiler("oid_test_case_inheritance_polymorphic_a_as_a", R"( + testMultiCompilerGlob("oid_test_case_inheritance_polymorphic_a_as_a", R"( [1] Pointer [0] Class: A (size: 16) Member: _vptr$A (offset: 0) @@ -78,11 +78,11 @@ TEST_F(AddChildrenTest, InheritancePolymorphic) { Function: A Function: A Child -[8] Class: B (size: 40) +[17] Class: B (size: 40) Parent (offset: 0) [0] Member: vec_b (offset: 16) -[4] Container: std::vector (size: 24) +[4] Class: vector > (size: 24) Param Primitive: int32_t Param @@ -105,14 +105,15 @@ TEST_F(AddChildrenTest, InheritancePolymorphic) { Function: ~allocator Function: allocate Function: deallocate + * Function: ~B (virtual) Function: myfunc (virtual) Function: B Function: B Child -[10] Class: C (size: 48) +[19] Class: C (size: 48) Parent (offset: 0) - [8] + [17] Member: int_c (offset: 40) Primitive: int32_t Function: ~C (virtual) @@ -120,7 +121,7 @@ TEST_F(AddChildrenTest, InheritancePolymorphic) { Function: C Function: C )", - R"( + R"( [1] Pointer [0] Class: A (size: 16) Member: _vptr.A (offset: 0) @@ -133,11 +134,11 @@ TEST_F(AddChildrenTest, InheritancePolymorphic) { Function: ~A (virtual) Function: myfunc (virtual) Child -[7] Class: B (size: 40) +[13] Class: B (size: 40) Parent (offset: 0) [0] Member: vec_b (offset: 16) -[4] Container: std::vector (size: 24) +[4] Class: vector > (size: 24) Param Primitive: int32_t Param @@ -157,15 +158,16 @@ TEST_F(AddChildrenTest, InheritancePolymorphic) { Function: ~allocator Function: allocate Function: deallocate + * Function: operator= Function: B Function: B Function: ~B (virtual) Function: myfunc (virtual) Child -[9] Class: C (size: 48) +[15] Class: C (size: 48) Parent (offset: 0) - [7] + [13] Member: int_c (offset: 40) Primitive: int32_t Function: operator= diff --git a/test/test_codegen.cpp b/test/test_codegen.cpp index c216007..94b6dd0 100644 --- a/test/test_codegen.cpp +++ b/test/test_codegen.cpp @@ -35,6 +35,9 @@ void testTransform(OICodeGen::Config& config, MockSymbolService symbols; CodeGen codegen{config, symbols}; + for (auto& info : getContainerInfos()) { + codegen.registerContainer(std::move(info)); + } codegen.transform(typeGraph); check(typeGraph, expectedAfter, "after transform"); @@ -48,7 +51,7 @@ void testTransform(std::string_view input, std::string_view expectedAfter) { TEST(CodeGenTest, TransformContainerAllocator) { testTransform(R"( -[0] Container: std::vector (size: 24) +[0] Class: std::vector (size: 24) Param Primitive: int32_t Param @@ -59,18 +62,18 @@ TEST(CodeGenTest, TransformContainerAllocator) { Function: deallocate )", R"( -[0] Container: std::vector> (size: 24) +[2] Container: std::vector> (size: 24) Param Primitive: int32_t Param -[2] DummyAllocator [MyAlloc] (size: 8) +[3] DummyAllocator [MyAlloc] (size: 8) Primitive: int32_t )"); } TEST(CodeGenTest, TransformContainerAllocatorParamInParent) { testTransform(R"( -[0] Container: std::map (size: 24) +[0] Class: std::map (size: 24) Param Primitive: int32_t Param @@ -80,7 +83,7 @@ TEST(CodeGenTest, TransformContainerAllocatorParamInParent) { Parent (offset: 0) [2] Struct: MyAllocBase> (size: 1) Param -[3] Container: std::pair (size: 8) +[3] Class: std::pair (size: 8) Param Primitive: int32_t Qualifiers: const @@ -92,14 +95,14 @@ TEST(CodeGenTest, TransformContainerAllocatorParamInParent) { Function: deallocate )", R"( -[0] Container: std::map, 0, 0, 4>> (size: 24) +[4] Container: std::map, 0, 0, 6>> (size: 24) Param Primitive: int32_t Param Primitive: int32_t Param -[4] DummyAllocator [MyAlloc>] (size: 0) -[3] Container: std::pair (size: 8) +[6] DummyAllocator [MyAlloc>] (size: 0) +[5] Container: std::pair (size: 8) Param Primitive: int32_t Qualifiers: const @@ -150,3 +153,25 @@ TEST(CodeGenTest, UnionMembersAlignment) { Primitive: int8_t )"); } + +TEST(CodeGenTest, ReplaceContainersAndDummies) { + testTransform(R"( +[0] Class: std::vector (size: 24) + Param + Primitive: uint32_t + Param +[1] Class: allocator (size: 1) + Param + Primitive: uint32_t + Function: allocate + Function: deallocate +)", + R"( +[2] Container: std::vector> (size: 24) + Param + Primitive: uint32_t + Param +[3] DummyAllocator [allocator] (size: 0) + Primitive: uint32_t +)"); +} diff --git a/test/test_drgn_parser.cpp b/test/test_drgn_parser.cpp index 8cdb976..d5b230d 100644 --- a/test/test_drgn_parser.cpp +++ b/test/test_drgn_parser.cpp @@ -1,16 +1,15 @@ +#include "test_drgn_parser.h" + #include #include -#include "oi/SymbolService.h" -// TODO needed?: -#include "oi/ContainerInfo.h" #include "oi/OIParser.h" +#include "oi/SymbolService.h" #include "oi/type_graph/NodeTracker.h" #include "oi/type_graph/Printer.h" #include "oi/type_graph/TypeGraph.h" #include "oi/type_graph/Types.h" -#include "test_drgn_parser.h" using namespace type_graph; @@ -19,25 +18,17 @@ using namespace type_graph; SymbolService* DrgnParserTest::symbols_ = nullptr; -namespace { -const std::vector>& getContainerInfos() { - static auto res = []() { - // TODO more container types, with various template parameter options - auto std_vector = - std::make_unique("std::vector", SEQ_TYPE, "vector"); - std_vector->stubTemplateParams = {1}; - - std::vector> containers; - containers.emplace_back(std::move(std_vector)); - return containers; - }(); - return res; +void DrgnParserTest::SetUpTestSuite() { + symbols_ = new SymbolService{TARGET_EXE_PATH}; +} + +void DrgnParserTest::TearDownTestSuite() { + delete symbols_; } -} // namespace DrgnParser DrgnParserTest::getDrgnParser(TypeGraph& typeGraph, DrgnParserOptions options) { - DrgnParser drgnParser{typeGraph, getContainerInfos(), options}; + DrgnParser drgnParser{typeGraph, options}; return drgnParser; } @@ -148,8 +139,9 @@ void DrgnParserTest::testGlob(std::string_view function, expected.remove_prefix(1); // Remove initial '\n' auto [expectedIdx, actualIdx] = globMatch(expected, actual); if (expected.size() != expectedIdx) { - ADD_FAILURE() << prefixLines(expected.substr(expectedIdx), "-", 50) << "\n" - << prefixLines(actual.substr(actualIdx), "+", 50); + ADD_FAILURE() << prefixLines(expected.substr(expectedIdx), "-", 10000) + << "\n" + << prefixLines(actual.substr(actualIdx), "+", 10000); } } @@ -261,54 +253,58 @@ TEST_F(DrgnParserTest, InheritanceMultiple) { } TEST_F(DrgnParserTest, Container) { - testMultiCompiler("oid_test_case_std_vector_int_empty", R"( -[4] Pointer -[0] Container: std::vector (size: 24) - Param - Primitive: int32_t - Param -[1] Class: allocator (size: 1) - Param - Primitive: int32_t - Parent (offset: 0) -[3] Typedef: __allocator_base -[2] Class: new_allocator (size: 1) - Param - Primitive: int32_t - Function: new_allocator - Function: new_allocator - Function: allocate - Function: deallocate - Function: _M_max_size - Function: allocator - Function: allocator - Function: operator= - Function: ~allocator - Function: allocate - Function: deallocate + testMultiCompilerGlob("oid_test_case_std_vector_int_empty", R"( +[13] Pointer +[0] Class: vector > (size: 24) + Param + Primitive: int32_t + Param +[1] Class: allocator (size: 1) + Param + Primitive: int32_t + Parent (offset: 0) +[3] Typedef: __allocator_base +[2] Class: new_allocator (size: 1) + Param + Primitive: int32_t + Function: new_allocator + Function: new_allocator + Function: allocate + Function: deallocate + Function: _M_max_size + Function: allocator + Function: allocator + Function: operator= + Function: ~allocator + Function: allocate + Function: deallocate + Parent (offset: 0) +* )", - R"( -[3] Pointer -[0] Container: std::vector (size: 24) - Param - Primitive: int32_t - Param -[1] Class: allocator (size: 1) - Parent (offset: 0) -[2] Class: new_allocator (size: 1) - Param - Primitive: int32_t - Function: new_allocator - Function: new_allocator - Function: allocate - Function: deallocate - Function: _M_max_size - Function: allocator - Function: allocator - Function: operator= - Function: ~allocator - Function: allocate - Function: deallocate + R"( +[9] Pointer +[0] Class: vector > (size: 24) + Param + Primitive: int32_t + Param +[1] Class: allocator (size: 1) + Parent (offset: 0) +[2] Class: new_allocator (size: 1) + Param + Primitive: int32_t + Function: new_allocator + Function: new_allocator + Function: allocate + Function: deallocate + Function: _M_max_size + Function: allocator + Function: allocator + Function: operator= + Function: ~allocator + Function: allocate + Function: deallocate + Parent (offset: 0) +* )"); } // TODO test vector with custom allocator @@ -455,70 +451,6 @@ TEST_F(DrgnParserTest, ClassTemplateInt) { )"); } -TEST_F(DrgnParserTest, ClassTemplateVector) { - testMultiCompiler("oid_test_case_templates_vector", R"( -[5] Pointer -[0] Class: TemplatedClass1 > > (size: 24) - Param -[1] Container: std::vector (size: 24) - Param - Primitive: int32_t - Param -[2] Class: allocator (size: 1) - Param - Primitive: int32_t - Parent (offset: 0) -[4] Typedef: __allocator_base -[3] Class: new_allocator (size: 1) - Param - Primitive: int32_t - Function: new_allocator - Function: new_allocator - Function: allocate - Function: deallocate - Function: _M_max_size - Function: allocator - Function: allocator - Function: operator= - Function: ~allocator - Function: allocate - Function: deallocate - Member: val (offset: 0) - [1] - Function: ~TemplatedClass1 - Function: TemplatedClass1 -)", - R"( -[4] Pointer -[0] Class: TemplatedClass1 > > (size: 24) - Param -[1] Container: std::vector (size: 24) - Param - Primitive: int32_t - Param -[2] Class: allocator (size: 1) - Parent (offset: 0) -[3] Class: new_allocator (size: 1) - Param - Primitive: int32_t - Function: new_allocator - Function: new_allocator - Function: allocate - Function: deallocate - Function: _M_max_size - Function: allocator - Function: allocator - Function: operator= - Function: ~allocator - Function: allocate - Function: deallocate - Member: val (offset: 0) - [1] - Function: TemplatedClass1 - Function: ~TemplatedClass1 -)"); -} - TEST_F(DrgnParserTest, ClassTemplateTwo) { test("oid_test_case_templates_two", R"( [3] Pointer diff --git a/test/test_drgn_parser.h b/test/test_drgn_parser.h index 0c70244..ec07df5 100644 --- a/test/test_drgn_parser.h +++ b/test/test_drgn_parser.h @@ -16,13 +16,8 @@ using namespace oi::detail; class DrgnParserTest : public ::testing::Test { protected: - static void SetUpTestSuite() { - symbols_ = new SymbolService{TARGET_EXE_PATH}; - } - - static void TearDownTestSuite() { - delete symbols_; - } + static void SetUpTestSuite(); + static void TearDownTestSuite(); static type_graph::DrgnParser getDrgnParser( type_graph::TypeGraph& typeGraph, type_graph::DrgnParserOptions options);