remove treebuilder pointer validation

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)) {
+        StoreData(1, returnArg);
         getSizeType(*(s_ptr), returnArg);
+      } else {
+        StoreData(0, returnArg);
       }
     }
 

src/TreeBuilder.cpp

@@ -39,6 +39,17 @@ extern "C" {
 #include <sys/types.h>
 }
 
+/* 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<msgpack::sbuffer>();
 
@@ -205,7 +216,6 @@ void TreeBuilder::build(const std::vector<uint64_t>& 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<uint64_t>& 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 {
+      // 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
+          // potentially shared.
           node.pointer.reset();
-        // Account for Small String Optimization (SSO)
-        const int fbStringSsoCutOff = 23;
-        constexpr size_t ssoCutoff = fbStringSsoCutOff;
-        contentsStoredInline = containerStats.capacity <= ssoCutoff;
+        }
       }
       break;
     case STRING_TYPE:

src/TreeBuilder.h

@@ -99,13 +99,11 @@ class TreeBuilder {
    */
   std::unique_ptr<msgpack::sbuffer> buffer;
   rocksdb::DB* db = nullptr;
-  std::unordered_set<uintptr_t> 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 <class T>

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<UniqueNode>&"]
     setup = '''
       auto first = std::make_unique<UniqueNode>();
@@ -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<SharedNode>&"]
     setup = '''
       auto first = std::make_shared<SharedNode>();

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 = '''

types/fb_string_type.toml

@@ -24,12 +24,15 @@ void getSizeType(const %1%<E, T, A, Storage> &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%<E, T, A, Storage>)))
+    bool inlined = ((uintptr_t)t.data() < (uintptr_t)(&t + sizeof(%1%<E, T, A, Storage>)))
         &&
-      ((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);
+    }
 }
 """

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)
+    if (head && pointers.add((uintptr_t)head)) {
+        SAVE_DATA(1);
         getSizeType(*head, returnArg);
+    } else {
+        SAVE_DATA(0);
+    }
 }
 """

types/ref_wrapper_type.toml

@@ -20,6 +20,11 @@ void getSizeType(const %1%<T> &ref, size_t& returnArg)
 {
     SAVE_SIZE(sizeof(%1%<T>));
     SAVE_DATA((uintptr_t)&(ref.get()));
+    if (pointers.add((uintptr_t)&ref.get())) {
+        SAVE_DATA(1);
         getSizeType(ref.get(), returnArg);
+    } else {
+        SAVE_DATA(0);
+    }
 }
 """

types/shrd_ptr_type.toml

@@ -24,7 +24,10 @@ void getSizeType(const %1%<T> &s_ptr, size_t& returnArg)
         SAVE_DATA((uintptr_t)(s_ptr.get()));
 
         if (s_ptr && pointers.add((uintptr_t)(s_ptr.get()))) {
+            SAVE_DATA(1);
             getSizeType(*(s_ptr.get()), returnArg);
+        } else {
+            SAVE_DATA(0);
         }
     }
 }

types/uniq_ptr_type.toml

@@ -24,7 +24,10 @@ void getSizeType(const %1%<T,Deleter> &s_ptr, size_t& returnArg)
         SAVE_DATA((uintptr_t)(s_ptr.get()));
 
         if (s_ptr && pointers.add((uintptr_t)(s_ptr.get()))) {
+            SAVE_DATA(1);
             getSizeType(*(s_ptr.get()), returnArg);
+        } else {
+            SAVE_DATA(0);
         }
     }
 }