diff --git a/src/OICodeGen.cpp b/src/OICodeGen.cpp index a8d7cea..d01d2f9 100644 --- a/src/OICodeGen.cpp +++ b/src/OICodeGen.cpp @@ -3284,7 +3284,10 @@ bool OICodeGen::generateJitCode(std::string& code) { JLOGPTR(s_ptr); StoreData((uintptr_t)(s_ptr), returnArg); if (s_ptr && pointers.add((uintptr_t)s_ptr)) { - getSizeType(*(s_ptr), returnArg); + StoreData(1, returnArg); + getSizeType(*(s_ptr), returnArg); + } else { + StoreData(0, returnArg); } } diff --git a/src/TreeBuilder.cpp b/src/TreeBuilder.cpp index 74040c4..1db4a5c 100644 --- a/src/TreeBuilder.cpp +++ b/src/TreeBuilder.cpp @@ -39,6 +39,17 @@ extern "C" { #include } +/* Tag indicating if the pointer has been followed or skipped */ +enum class TrackPointerTag: uint64_t { + /* The content has been skipped. + * It prevents double counting the footprint of a node the JIT code has seen + * before, double counting content being stored inline, or getting stuck in an + * infinite loop when processing circular linked list. + */ + skipped = 0, + followed = 1, +}; + TreeBuilder::TreeBuilder(Config c) : config{std::move(c)} { buffer = std::make_unique(); @@ -205,7 +216,6 @@ void TreeBuilder::build(const std::vector& data, th = &typeHierarchy; oidData = &data; - pointers.clear(); oidDataIndex = 3; // HACK: OID's first 3 outputs are dummy 0s ObjectIntrospection::Metrics::Tracing _("build_tree"); @@ -215,8 +225,9 @@ void TreeBuilder::build(const std::vector& data, auto& rootID = rootIDs.emplace_back(nextNodeID++); try { - // The first value is the address of the root object - pointers.insert(next()); + // The first value is the address of the root object. Drop it as we don't + // need to manage pointer deduplication anymore. + next(); process(rootID, {.type = type, .name = argName, .typePath = argName}); } catch (...) { // Mark the failure using the error node ID @@ -367,15 +378,6 @@ bool TreeBuilder::isPrimitive(struct drgn_type* type) { return drgn_type_primitive(type) != DRGN_NOT_PRIMITIVE_TYPE; } -bool TreeBuilder::shouldProcess(uintptr_t pointer) { - if (pointer == 0U) { - return false; - } - - auto [_, unprocessed] = pointers.emplace(pointer); - return unprocessed; -} - static std::string_view drgnKindStr(struct drgn_type* type) { auto kind = OICodeGen::drgnKindStr(type); // -1 is for the null terminator @@ -427,8 +429,10 @@ TreeBuilder::Node TreeBuilder::process(NodeID id, Variable variable) { auto innerTypeKind = drgn_type_kind(entry->second); if (innerTypeKind != DRGN_TYPE_FUNCTION) { node.pointer = next(); - if (innerTypeKind == DRGN_TYPE_VOID || - !shouldProcess(*node.pointer)) { + if (innerTypeKind == DRGN_TYPE_VOID) { + break; + } + if (next() == (uint64_t)TrackPointerTag::skipped) { break; } } @@ -626,7 +630,7 @@ void TreeBuilder::processContainer(const Variable& variable, Node& node) { node.pointer = next(); containerStats.length = *node.pointer ? 1 : 0; containerStats.capacity = 1; - if (!shouldProcess(*node.pointer)) { + if (next() == (uint64_t)TrackPointerTag::skipped) { return; } break; @@ -634,7 +638,7 @@ void TreeBuilder::processContainer(const Variable& variable, Node& node) { case REF_WRAPPER_TYPE: node.pointer = next(); containerStats.length = containerStats.capacity = 1; - if (!shouldProcess(*node.pointer)) { + if (next() == (uint64_t)TrackPointerTag::skipped) { return; } break; @@ -723,7 +727,7 @@ void TreeBuilder::processContainer(const Variable& variable, Node& node) { case FOLLY_IOBUFQUEUE_TYPE: node.pointer = next(); containerStats.length = containerStats.capacity = 0; - if (!shouldProcess(*node.pointer)) { + if (next() == (uint64_t)TrackPointerTag::skipped) { return; } // Fallthrough to the IOBuf data if we have a valid pointer @@ -736,20 +740,17 @@ void TreeBuilder::processContainer(const Variable& variable, Node& node) { node.pointer = next(); containerStats.capacity = next(); containerStats.length = next(); - // If this string's data is potentially shared (cutoff for sharing is 255) - if (containerStats.capacity >= 255) { - // Contents aren't actually stored inline in this case, - // but we set this to `true` so that we don't double-count - // this string's data if we have seen it before. - contentsStoredInline = !shouldProcess(*node.pointer); - } else { - // No sense in recording the pointer value if the string isn't - // potentially shared - node.pointer.reset(); - // Account for Small String Optimization (SSO) - const int fbStringSsoCutOff = 23; - constexpr size_t ssoCutoff = fbStringSsoCutOff; - contentsStoredInline = containerStats.capacity <= ssoCutoff; + // Contents are either stored inline or have been seen before in the JIT + // code. Set to true either way so as not to double count. + contentsStoredInline = next() == 0; + + { + constexpr int sharedCutOff = 255; + if (containerStats.capacity < sharedCutOff) { + // No sense in recording the pointer value if the string isn't + // potentially shared. + node.pointer.reset(); + } } break; case STRING_TYPE: diff --git a/src/TreeBuilder.h b/src/TreeBuilder.h index f12e0fc..ec8c63b 100644 --- a/src/TreeBuilder.h +++ b/src/TreeBuilder.h @@ -99,13 +99,11 @@ class TreeBuilder { */ std::unique_ptr buffer; rocksdb::DB* db = nullptr; - std::unordered_set pointers{}; uint64_t getDrgnTypeSize(struct drgn_type* type); uint64_t next(); bool isContainer(const Variable& variable); bool isPrimitive(struct drgn_type* type); - bool shouldProcess(uintptr_t pointer); Node process(NodeID id, Variable variable); void processContainer(const Variable& variable, Node& node); template diff --git a/test/integration/cycles.toml b/test/integration/cycles.toml index 2871aab..2bf2f3d 100644 --- a/test/integration/cycles.toml +++ b/test/integration/cycles.toml @@ -90,7 +90,6 @@ definitions = ''' ''' [cases.unique_ptr] - oil_skip = "OIL processes one node in the cycle twice" # https://github.com/facebookexperimental/object-introspection/issues/104 param_types = ["std::reference_wrapper&"] setup = ''' auto first = std::make_unique(); @@ -168,7 +167,6 @@ definitions = ''' ''' [cases.shared_ptr] - oil_skip = "OIL processes one node in the cycle twice" # https://github.com/facebookexperimental/object-introspection/issues/104 param_types = ["std::reference_wrapper&"] setup = ''' auto first = std::make_shared(); diff --git a/test/integration/fbstring.toml b/test/integration/fbstring.toml index 2fa27fe..a115555 100644 --- a/test/integration/fbstring.toml +++ b/test/integration/fbstring.toml @@ -76,7 +76,6 @@ includes = ["folly/FBString.h"] ''' [cases.string_pooled] - skip = "Potentially incorrect dynamic size" param_types = ["folly::fbstring&"] setup = "return folly::fbstring(1024, 'c');" expect_json = ''' diff --git a/types/fb_string_type.toml b/types/fb_string_type.toml index b8a246a..dbfb72e 100644 --- a/types/fb_string_type.toml +++ b/types/fb_string_type.toml @@ -24,12 +24,15 @@ void getSizeType(const %1% &t, size_t& returnArg) SAVE_DATA((uintptr_t)t.capacity()); SAVE_DATA((uintptr_t)t.size()); - // Check if the string is contained within the type (inlined) so as not to double count. - SAVE_SIZE( - ((uintptr_t)t.data() < (uintptr_t)(&t + sizeof(%1%))) + bool inlined = ((uintptr_t)t.data() < (uintptr_t)(&t + sizeof(%1%))) && - ((uintptr_t)t.data() >= (uintptr_t)&t) - ? 0 : (t.capacity() * sizeof(T)) - ); + ((uintptr_t)t.data() >= (uintptr_t)&t); + + if (!inlined && pointers.add((uintptr_t)t.data())) { + SAVE_SIZE(t.capacity() * sizeof(T)); + SAVE_DATA(1); + } else { + SAVE_DATA(0); + } } """ diff --git a/types/folly_iobuf_queue_type.toml b/types/folly_iobuf_queue_type.toml index 7f795bb..1e3da42 100644 --- a/types/folly_iobuf_queue_type.toml +++ b/types/folly_iobuf_queue_type.toml @@ -21,7 +21,11 @@ void getSizeType(const %1% &t, size_t& returnArg) const IOBuf *head = t.front(); SAVE_DATA((uintptr_t)head); - if (head) - getSizeType(*head, returnArg); + if (head && pointers.add((uintptr_t)head)) { + SAVE_DATA(1); + getSizeType(*head, returnArg); + } else { + SAVE_DATA(0); + } } """ diff --git a/types/ref_wrapper_type.toml b/types/ref_wrapper_type.toml index 2ef6026..497a057 100644 --- a/types/ref_wrapper_type.toml +++ b/types/ref_wrapper_type.toml @@ -20,6 +20,11 @@ void getSizeType(const %1% &ref, size_t& returnArg) { SAVE_SIZE(sizeof(%1%)); SAVE_DATA((uintptr_t)&(ref.get())); - getSizeType(ref.get(), returnArg); + if (pointers.add((uintptr_t)&ref.get())) { + SAVE_DATA(1); + getSizeType(ref.get(), returnArg); + } else { + SAVE_DATA(0); + } } """ diff --git a/types/shrd_ptr_type.toml b/types/shrd_ptr_type.toml index 85437fa..f58ed9f 100644 --- a/types/shrd_ptr_type.toml +++ b/types/shrd_ptr_type.toml @@ -21,11 +21,14 @@ void getSizeType(const %1% &s_ptr, size_t& returnArg) SAVE_SIZE(sizeof(%1%)); if constexpr (!std::is_void::value) { - SAVE_DATA((uintptr_t)(s_ptr.get())); + SAVE_DATA((uintptr_t)(s_ptr.get())); - if (s_ptr && pointers.add((uintptr_t)(s_ptr.get()))) { - getSizeType(*(s_ptr.get()), returnArg); - } + if (s_ptr && pointers.add((uintptr_t)(s_ptr.get()))) { + SAVE_DATA(1); + getSizeType(*(s_ptr.get()), returnArg); + } else { + SAVE_DATA(0); + } } } """ diff --git a/types/uniq_ptr_type.toml b/types/uniq_ptr_type.toml index 304f799..b386798 100644 --- a/types/uniq_ptr_type.toml +++ b/types/uniq_ptr_type.toml @@ -21,11 +21,14 @@ void getSizeType(const %1% &s_ptr, size_t& returnArg) SAVE_SIZE(sizeof(%1%)); if constexpr (!std::is_void::value) { - SAVE_DATA((uintptr_t)(s_ptr.get())); + SAVE_DATA((uintptr_t)(s_ptr.get())); - if (s_ptr && pointers.add((uintptr_t)(s_ptr.get()))) { - getSizeType(*(s_ptr.get()), returnArg); - } + if (s_ptr && pointers.add((uintptr_t)(s_ptr.get()))) { + SAVE_DATA(1); + getSizeType(*(s_ptr.get()), returnArg); + } else { + SAVE_DATA(0); + } } } """