From db93feb18024433f170eb0f66e03c85e84b4b625 Mon Sep 17 00:00:00 2001 From: Jake Hillion Date: Thu, 4 Jan 2024 17:01:52 +0000 Subject: [PATCH] incomplete: name type in compiler errors Summary: We have a good type representation in the Type Graph of an incomplete type and the underlying type that represents. However, this incomplete type still ends up in the generated code as `void` which loses information. For example, a container that can't contain void may fail to compile because it was initialised with `void` but really its because the type it was supposed to be initialised with (say, `Foo`) had incomplete debug information. This change identifies that a type is incomplete in the output by generating it as an incomplete type `struct Incomplete`. This allows us to name the type correctly in the TreeBuilder output and filter for incomplete types, as well as getting appropriate compiler errors if it mustn't be incomplete. Test Plan: - CI - Added a unit test to namegen. - Enabled and added an extra pointers_incomplete test. This change is tricky to test because it isn't really user visible. The types still use their `inputName` which is unchanged in any successful output - this change is used so the compiler fails with a more detailed error. --- oi/CodeGen.cpp | 31 +++++++++--------- oi/OITraceCode.cpp | 16 ++++++++++ oi/type_graph/DrgnExporter.cpp | 2 ++ oi/type_graph/NameGen.cpp | 23 +++++++++++++ oi/type_graph/NameGen.h | 1 + oi/type_graph/Printer.cpp | 4 ++- oi/type_graph/TopoSorter.cpp | 4 +++ oi/type_graph/TopoSorter.h | 1 + oi/type_graph/Types.cpp | 2 -- oi/type_graph/Types.h | 39 +++++++++++++++-------- test/TypeGraphParser.cpp | 5 +-- test/integration/pointers_incomplete.toml | 32 ++++++++++++++++--- test/test_drgn_parser.cpp | 4 +-- test/test_flattener.cpp | 2 +- test/test_name_gen.cpp | 13 ++++++++ test/test_remove_members.cpp | 2 +- types/shrd_ptr_type.toml | 6 ++-- types/uniq_ptr_type.toml | 6 ++-- 18 files changed, 144 insertions(+), 49 deletions(-) diff --git a/oi/CodeGen.cpp b/oi/CodeGen.cpp index 9614a1f..84fb9b4 100644 --- a/oi/CodeGen.cpp +++ b/oi/CodeGen.cpp @@ -60,6 +60,7 @@ using type_graph::EnforceCompatibility; using type_graph::Enum; using type_graph::Flattener; using type_graph::IdentifyContainers; +using type_graph::Incomplete; using type_graph::KeyCapture; using type_graph::Member; using type_graph::NameGen; @@ -415,8 +416,6 @@ void addStandardGetSizeFuncDecls(std::string& code) { template void getSizeType(/*const*/ T* s_ptr, size_t& returnArg); - void getSizeType(/*const*/ void *s_ptr, size_t& returnArg); - template void getSizeType(const OIArray& container, size_t& returnArg); )"; @@ -438,24 +437,24 @@ void addStandardGetSizeFuncDefs(std::string& code) { template void getSizeType(/*const*/ T* s_ptr, size_t& returnArg) { - JLOG("ptr val @"); - JLOGPTR(s_ptr); - StoreData((uintptr_t)(s_ptr), returnArg); - if (s_ptr && ctx.pointers.add((uintptr_t)s_ptr)) { - StoreData(1, returnArg); - getSizeType(*(s_ptr), returnArg); + if constexpr (!oi_is_complete) { + JLOG("incomplete ptr @"); + JLOGPTR(s_ptr); + StoreData((uintptr_t)(s_ptr), returnArg); + return; } else { - StoreData(0, returnArg); + JLOG("ptr val @"); + JLOGPTR(s_ptr); + StoreData((uintptr_t)(s_ptr), returnArg); + if (s_ptr && ctx.pointers.add((uintptr_t)s_ptr)) { + StoreData(1, returnArg); + getSizeType(*(s_ptr), returnArg); + } else { + StoreData(0, returnArg); + } } } - void getSizeType(/*const*/ void *s_ptr, size_t& returnArg) - { - JLOG("void ptr @"); - JLOGPTR(s_ptr); - StoreData((uintptr_t)(s_ptr), returnArg); - } - template void getSizeType(const OIArray& container, size_t& returnArg) { diff --git a/oi/OITraceCode.cpp b/oi/OITraceCode.cpp index 31a873b..a100900 100644 --- a/oi/OITraceCode.cpp +++ b/oi/OITraceCode.cpp @@ -182,3 +182,19 @@ bool isStorageInline(const auto& c) { return (uintptr_t)std::data(c) < (uintptr_t)(&c + sizeof(c)) && (uintptr_t)std::data(c) >= (uintptr_t)&c; } + +namespace OIInternal { +namespace { + +template +struct Incomplete; + +template +constexpr bool oi_is_complete = true; +template <> +constexpr bool oi_is_complete = false; +template +constexpr bool oi_is_complete> = false; + +} // namespace +} // namespace OIInternal diff --git a/oi/type_graph/DrgnExporter.cpp b/oi/type_graph/DrgnExporter.cpp index df9722d..39292e5 100644 --- a/oi/type_graph/DrgnExporter.cpp +++ b/oi/type_graph/DrgnExporter.cpp @@ -100,6 +100,8 @@ drgn_type* DrgnExporter::visit(Container& c) { if (auto* p = dynamic_cast(¶mType); p && p->kind() == Primitive::Kind::Void) { return drgnType; + } else if (auto* i = dynamic_cast(¶mType)) { + return drgnType; } } diff --git a/oi/type_graph/NameGen.cpp b/oi/type_graph/NameGen.cpp index 725ab59..0cc2a61 100644 --- a/oi/type_graph/NameGen.cpp +++ b/oi/type_graph/NameGen.cpp @@ -219,4 +219,27 @@ void NameGen::visit(CaptureKeys& c) { c.regenerateName(); } +void NameGen::visit(Incomplete& i) { + constexpr std::string_view kPrefix{"Incomplete visited_; diff --git a/oi/type_graph/Types.cpp b/oi/type_graph/Types.cpp index c6219b5..9b53f49 100644 --- a/oi/type_graph/Types.cpp +++ b/oi/type_graph/Types.cpp @@ -35,8 +35,6 @@ namespace oi::detail::type_graph { OI_TYPE_LIST #undef X -const std::string Incomplete::kName = "void"; - std::string Primitive::getName(Kind kind) { switch (kind) { case Kind::Int8: diff --git a/oi/type_graph/Types.h b/oi/type_graph/Types.h index 1d5dd79..f09f926 100644 --- a/oi/type_graph/Types.h +++ b/oi/type_graph/Types.h @@ -29,6 +29,8 @@ * debugging. */ +#include +#include #include #include #include @@ -211,29 +213,33 @@ struct TemplateParam { */ class Incomplete : public Type { public: - Incomplete(Type& underlyingType) : underlyingType_(underlyingType) { + Incomplete(NodeId id, Type& underlyingType) + : id_(id), underlyingType_(underlyingType) { } - Incomplete(std::string underlyingTypeName) - : underlyingType_(std::move(underlyingTypeName)) { + Incomplete(NodeId id, std::string underlyingTypeName) + : id_(id), underlyingType_(std::move(underlyingTypeName)) { } - static inline constexpr bool has_node_id = false; + static inline constexpr bool has_node_id = true; DECLARE_ACCEPT const std::string& name() const override { - return kName; + return name_; } std::string_view inputName() const override { - if (std::holds_alternative(underlyingType_)) { - return std::get(underlyingType_); - } - - return std::get>(underlyingType_) - .get() - .inputName(); + return std::visit( + [](const auto& el) -> std::string_view { + using T = std::decay_t; + if constexpr (std::is_same_v) { + return el; + } else { + return el.get().inputName(); + } + }, + underlyingType_); } size_t size() const override { @@ -245,7 +251,11 @@ class Incomplete : public Type { } NodeId id() const override { - return -1; + return id_; + } + + void setName(std::string name) { + name_ = std::move(name); } std::optional> underlyingType() const { @@ -257,8 +267,9 @@ class Incomplete : public Type { } private: + NodeId id_ = -1; std::variant> underlyingType_; - static const std::string kName; + std::string name_ = "void"; }; /* diff --git a/test/TypeGraphParser.cpp b/test/TypeGraphParser.cpp index f54fe08..d70236c 100644 --- a/test/TypeGraphParser.cpp +++ b/test/TypeGraphParser.cpp @@ -215,10 +215,11 @@ Type& TypeGraphParser::parseType(std::string_view& input, size_t rootIndent) { auto nameEndPos = line.find(']', nameStartPos); auto underlyingTypeName = line.substr(nameStartPos, nameEndPos - nameStartPos); - type = &typeGraph_.makeType(std::string(underlyingTypeName)); + type = + &typeGraph_.makeType(id, std::string(underlyingTypeName)); } else { auto& underlyingType = parseType(input, indent + 2); - type = &typeGraph_.makeType(underlyingType); + type = &typeGraph_.makeType(id, underlyingType); } } else if (nodeTypeName == "Class" || nodeTypeName == "Struct" || nodeTypeName == "Union") { diff --git a/test/integration/pointers_incomplete.toml b/test/integration/pointers_incomplete.toml index 0690511..2d34865 100644 --- a/test/integration/pointers_incomplete.toml +++ b/test/integration/pointers_incomplete.toml @@ -55,7 +55,6 @@ definitions = ''' }]''' [cases.unique_ptr] - oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/299 param_types = ["const std::unique_ptr&"] setup = ''' auto raw_ptr = static_cast(::operator new(5)); @@ -63,28 +62,29 @@ definitions = ''' raw_ptr, &incomplete_type_deleter); ''' expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]' + expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]' [cases.unique_ptr_null] - oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/299 param_types = ["const std::unique_ptr&"] setup = ''' return std::unique_ptr( nullptr, &incomplete_type_deleter); ''' expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]' + expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]' [cases.shared_ptr] - oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/300 param_types = ["const std::shared_ptr&"] setup = ''' auto raw_ptr = static_cast(::operator new(5)); return std::shared_ptr(raw_ptr , &incomplete_type_deleter); ''' expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]' + expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]' [cases.shared_ptr_null] - oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/300 param_types = ["const std::shared_ptr"] setup = "return nullptr;" expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]' + expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]' [cases.containing_struct] oil_disable = "oil can't chase raw pointers safely" @@ -116,3 +116,27 @@ definitions = ''' } ] }]''' + + [cases.containing_struct_no_follow] + param_types = ["const IncompleteTypeContainer&"] + setup = "return IncompleteTypeContainer{};" + expect_json = '''[{ + "staticSize": 88, + "members": [ + { "name": "ptrundef", "staticSize": 8 }, + { "name": "__makePad1", "staticSize": 1 }, + { "name": "shundef", "staticSize": 16 }, + { "name": "__makePad2", "staticSize": 1 }, + { "name": "shoptundef", + "staticSize": 24, + "length": 0, + "capacity": 1 + }, + { "name": "__makePad3", "staticSize": 1 }, + { "name": "optundef", + "staticSize": 16, + "length": 0, + "capacity": 1 + } + ] + }]''' diff --git a/test/test_drgn_parser.cpp b/test/test_drgn_parser.cpp index e8d03e3..c70f5a9 100644 --- a/test/test_drgn_parser.cpp +++ b/test/test_drgn_parser.cpp @@ -426,8 +426,8 @@ TEST_F(DrgnParserTest, PointerNoFollow) { TEST_F(DrgnParserTest, PointerIncomplete) { test("oid_test_case_pointers_incomplete_raw", R"( -[0] Pointer - Incomplete: [IncompleteType] +[1] Pointer +[0] Incomplete: [IncompleteType] )"); } diff --git a/test/test_flattener.cpp b/test/test_flattener.cpp index 808f4ee..c8944bf 100644 --- a/test/test_flattener.cpp +++ b/test/test_flattener.cpp @@ -1007,7 +1007,7 @@ TEST(FlattenerTest, IncompleteParent) { R"( [0] Class: MyClass (size: 4) Parent (offset: 0) - Incomplete: [IncompleteParent] +[1] Incomplete: [IncompleteParent] )", R"( [0] Class: MyClass (size: 4) diff --git a/test/test_name_gen.cpp b/test/test_name_gen.cpp index d0cda26..a83c180 100644 --- a/test/test_name_gen.cpp +++ b/test/test_name_gen.cpp @@ -501,3 +501,16 @@ TEST(NameGenTest, AnonymousMembers) { EXPECT_EQ(myclass.members[0].inputName, "__oi_anon_0"); EXPECT_EQ(myclass.members[1].inputName, "__oi_anon_1"); } + +TEST(NameGenTest, IncompleteTypes) { + auto myincompletevector = Incomplete{0, "std::vector"}; + + auto myint = Primitive{Primitive::Kind::Int32}; + auto myincompleteint = Incomplete{1, myint}; + + NameGen nameGen; + nameGen.generateNames({myincompletevector, myincompleteint}); + + EXPECT_EQ(myincompletevector.name(), "Incomplete"); + EXPECT_EQ(myincompleteint.name(), "Incomplete"); +} diff --git a/test/test_remove_members.cpp b/test/test_remove_members.cpp index 0d4a54e..9eff4ed 100644 --- a/test/test_remove_members.cpp +++ b/test/test_remove_members.cpp @@ -191,7 +191,7 @@ TEST(RemoveMembersTest, Incomplete) { Member: a (offset: 0) Primitive: int32_t Member: b (offset: 4) - Incomplete: [MyIncompleteType] +[1] Incomplete: [MyIncompleteType] Member: c (offset: 8) Primitive: int32_t )", diff --git a/types/shrd_ptr_type.toml b/types/shrd_ptr_type.toml index bb3a622..3439bf3 100644 --- a/types/shrd_ptr_type.toml +++ b/types/shrd_ptr_type.toml @@ -21,7 +21,7 @@ void getSizeType(const %1% &s_ptr, size_t& returnArg) { SAVE_SIZE(sizeof(%1%)); - if constexpr (!std::is_void::value) { + if constexpr (oi_is_complete) { SAVE_DATA((uintptr_t)(s_ptr.get())); if (s_ptr && ctx.pointers.add((uintptr_t)(s_ptr.get()))) { @@ -37,7 +37,7 @@ void getSizeType(const %1% &s_ptr, size_t& returnArg) traversal_func = """ auto tail = returnArg.write((uintptr_t)container.get()); -if constexpr (std::is_void::value) { +if constexpr (!oi_is_complete) { return tail.template delegate<0>(std::identity()); } else { bool do_visit = container && ctx.pointers.add((uintptr_t)container.get()); @@ -85,7 +85,7 @@ el.container_stats.emplace(result::Element::ContainerStats { }); // Must be in a `if constexpr` or the compiler will complain about make_field -if constexpr (!std::is_void::value) { +if constexpr (oi_is_complete) { if (sum.index == 1) { static constexpr auto element = make_field("ptr_val"); stack_ins(element); diff --git a/types/uniq_ptr_type.toml b/types/uniq_ptr_type.toml index f723f80..f71cff4 100644 --- a/types/uniq_ptr_type.toml +++ b/types/uniq_ptr_type.toml @@ -22,7 +22,7 @@ void getSizeType(const %1% &u_ptr, size_t& returnArg) { SAVE_SIZE(sizeof(%1%)); - if constexpr (!std::is_void::value) { + if constexpr (oi_is_complete) { SAVE_DATA((uintptr_t)(u_ptr.get())); if (u_ptr && ctx.pointers.add((uintptr_t)(u_ptr.get()))) { @@ -38,7 +38,7 @@ void getSizeType(const %1% &u_ptr, size_t& returnArg) traversal_func = """ auto tail = returnArg.write((uintptr_t)container.get()); -if constexpr (std::is_void::value) { +if constexpr (!oi_is_complete) { return tail.template delegate<0>(std::identity()); } else { bool do_visit = container && ctx.pointers.add((uintptr_t)container.get()); @@ -71,7 +71,7 @@ el.container_stats.emplace(result::Element::ContainerStats { }); // Must be in a `if constexpr` or the compiler will complain about make_field -if constexpr (!std::is_void::value) { +if constexpr (oi_is_complete) { if (sum.index == 1) { static constexpr auto element = make_field("ptr_val"); stack_ins(element);