Make all reserved heap snapshot IDs odd

Summary:
v8 uses odd IDs to mean JS objects, and uses even IDs to mean
native objects that don't live on the JS heap.

v8 also chooses to use only odd IDs for its "synthetic" nodes that
show sections of GC roots.

I don't think anything relies on this, but I've found matching v8's behavior
as close as possible is the best way to ensure that no bugs occur.
Do this by having all reserved IDs get accessed through a function,
`IDTracker::reserved`, that changes the simple 0-based integer sequence
into a sequence that only uses odd numbers.
Changing all the node IDs to be odd and ascend by 2's doesn't seem to be possible
in the immediate declaration without having to explicitly list all of them, which defeats
part of the usefulness of an enum.

Reviewed By: rshest

Differential Revision: D25044648

fbshipit-source-id: 5da60a33e9ea12493ea5159d0fecb3e9d9dd1e31

include/hermes/VM/GCBase.h

@@ -453,8 +453,6 @@ class GCBase {
    public:
     /// These are IDs that are reserved for special objects.
     enum class ReservedObjectID : HeapSnapshot::NodeID {
-      // For any object where an ID cannot be found.
-      NoID = 0,
       // The ID for the super root object.
       SuperRoot,
       // The ID for the (GC roots) object.
@@ -468,6 +466,16 @@ class GCBase {
       FirstNonReservedID,
     };
 
+    // 0 is guaranteed to never be a valid node ID.
+    static constexpr HeapSnapshot::NodeID kInvalidNode = 0;
+
+    static constexpr HeapSnapshot::NodeID reserved(ReservedObjectID id) {
+      // All reserved IDs should be odd numbers to signify that they're JS
+      // objects and not native objects. This follows v8's output.
+      return static_cast<std::underlying_type<ReservedObjectID>::type>(id) * 2 +
+          1;
+    }
+
     /// A comparator for doubles that allows NaN.
     struct DoubleComparator {
       static double getEmptyKey() {
@@ -574,11 +582,9 @@ class GCBase {
 
     /// The last ID assigned to a non-native object. Object IDs are not
     /// recycled so that snapshots don't confuse two objects with each other.
-    /// NOTE: Need to ensure that this starts on an odd number, so check if
-    /// the first non-reserved ID is odd, if not add one.
-    HeapSnapshot::NodeID lastID_{static_cast<HeapSnapshot::NodeID>(
-                                     ReservedObjectID::FirstNonReservedID) |
-                                 1};
+    /// NOTE: Reserved guarantees that this is an odd number.
+    HeapSnapshot::NodeID lastID_{
+        reserved(ReservedObjectID::FirstNonReservedID)};
 
     /// Map of object pointers to IDs. Only populated once the first heap
     /// snapshot is requested, or the first time the memory profiler is turned

lib/VM/GCBase.cpp

@@ -144,14 +144,15 @@ std::error_code GCBase::createSnapshotToFile(const std::string &fileName) {
 
 namespace {
 
-constexpr GCBase::IDTracker::ReservedObjectID objectIDForRootSection(
+constexpr HeapSnapshot::NodeID objectIDForRootSection(
     RootAcceptor::Section section) {
   // Since root sections start at zero, and in IDTracker the root sections
   // start one past the reserved GC root, this number can be added to
   // do conversions.
-  return static_cast<GCBase::IDTracker::ReservedObjectID>(
-      static_cast<uint64_t>(GCBase::IDTracker::ReservedObjectID::GCRoots) + 1 +
-      static_cast<uint64_t>(section));
+  return GCBase::IDTracker::reserved(
+      static_cast<GCBase::IDTracker::ReservedObjectID>(
+          static_cast<uint64_t>(GCBase::IDTracker::ReservedObjectID::GCRoots) +
+          1 + static_cast<uint64_t>(section)));
 }
 
 // Abstract base class for all snapshot acceptors.
@@ -194,32 +195,29 @@ struct PrimitiveNodeAcceptor : public SnapshotAcceptor {
     snap_.endNode(
         HeapSnapshot::NodeType::Object,
         "undefined",
-        static_cast<HeapSnapshot::NodeID>(
+        GCBase::IDTracker::reserved(
             GCBase::IDTracker::ReservedObjectID::Undefined),
         0,
         0);
     snap_.beginNode();
     snap_.endNode(
         HeapSnapshot::NodeType::Object,
         "null",
-        static_cast<HeapSnapshot::NodeID>(
-            GCBase::IDTracker::ReservedObjectID::Null),
+        GCBase::IDTracker::reserved(GCBase::IDTracker::ReservedObjectID::Null),
         0,
         0);
     snap_.beginNode();
     snap_.endNode(
         HeapSnapshot::NodeType::Object,
         "true",
-        static_cast<HeapSnapshot::NodeID>(
-            GCBase::IDTracker::ReservedObjectID::True),
+        GCBase::IDTracker::reserved(GCBase::IDTracker::ReservedObjectID::True),
         0,
         0);
     snap_.beginNode();
     snap_.endNode(
         HeapSnapshot::NodeType::Object,
         "false",
-        static_cast<HeapSnapshot::NodeID>(
-            GCBase::IDTracker::ReservedObjectID::False),
+        GCBase::IDTracker::reserved(GCBase::IDTracker::ReservedObjectID::False),
         0,
         0);
     for (double num : seenNumbers_) {
@@ -322,7 +320,7 @@ struct SnapshotRootSectionAcceptor : public SnapshotAcceptor,
     snap_.addIndexedEdge(
         HeapSnapshot::EdgeType::Element,
         rootSectionNum_++,
-        static_cast<HeapSnapshot::NodeID>(objectIDForRootSection(section)));
+        objectIDForRootSection(section));
   }
 
   void endRootSection() override {
@@ -382,8 +380,7 @@ struct SnapshotRootAcceptor : public SnapshotAcceptor,
     snap_.endNode(
         HeapSnapshot::NodeType::Synthetic,
         rootNames[static_cast<unsigned>(currentSection_)],
-        static_cast<HeapSnapshot::NodeID>(
-            objectIDForRootSection(currentSection_)),
+        objectIDForRootSection(currentSection_),
         // The heap visualizer doesn't like it when these synthetic nodes have a
         // size (it describes them as living in the heap).
         0,
@@ -447,13 +444,11 @@ void GCBase::createSnapshot(GC *gc, llvh::raw_ostream &os) {
       snap.addIndexedEdge(
           HeapSnapshot::EdgeType::Element,
           1,
-          static_cast<HeapSnapshot::NodeID>(
-              IDTracker::ReservedObjectID::GCRoots));
+          IDTracker::reserved(IDTracker::ReservedObjectID::GCRoots));
       snap.endNode(
           HeapSnapshot::NodeType::Synthetic,
           "",
-          static_cast<HeapSnapshot::NodeID>(
-              IDTracker::ReservedObjectID::SuperRoot),
+          IDTracker::reserved(IDTracker::ReservedObjectID::SuperRoot),
           0,
           0);
       snap.beginNode();
@@ -463,7 +458,7 @@ void GCBase::createSnapshot(GC *gc, llvh::raw_ostream &os) {
           HeapSnapshot::NodeType::Synthetic,
           "(GC roots)",
           static_cast<HeapSnapshot::NodeID>(
-              IDTracker::ReservedObjectID::GCRoots),
+              IDTracker::reserved(IDTracker::ReservedObjectID::GCRoots)),
           0,
           0);
     }
@@ -954,14 +949,14 @@ void GCBase::AllocationLocationTracker::enable(
   // The first fragment has all objects that were live before the profiler was
   // enabled.
   // The ID and timestamp will be filled out via flushCallback.
-  fragments_.emplace_back(Fragment{
-      static_cast<HeapSnapshot::NodeID>(IDTracker::ReservedObjectID::NoID),
-      std::chrono::microseconds(),
-      numObjects,
-      numBytes,
-      // Say the fragment is touched here so it is written out
-      // automatically by flushCallback.
-      true});
+  fragments_.emplace_back(
+      Fragment{IDTracker::kInvalidNode,
+               std::chrono::microseconds(),
+               numObjects,
+               numBytes,
+               // Say the fragment is touched here so it is written out
+               // automatically by flushCallback.
+               true});
   // Immediately flush the first fragment.
   flushCallback();
 }
@@ -987,9 +982,7 @@ void GCBase::AllocationLocationTracker::newAlloc(const void *ptr, uint32_t sz) {
     (void)id;
     Fragment &lastFrag = fragments_.back();
     HERMES_SLOW_ASSERT(
-        lastFrag.lastSeenObjectID_ ==
-            static_cast<HeapSnapshot::NodeID>(
-                IDTracker::ReservedObjectID::NoID) &&
+        lastFrag.lastSeenObjectID_ == IDTracker::kInvalidNode &&
         "Last fragment should not have an ID assigned yet");
     lastFrag.numObjects_++;
     lastFrag.numBytes_ += sz;
@@ -1062,9 +1055,7 @@ void GCBase::AllocationLocationTracker::flushCallback() {
   const auto duration = std::chrono::duration_cast<std::chrono::microseconds>(
       std::chrono::steady_clock::now() - startTime_);
   assert(
-      lastFrag.lastSeenObjectID_ ==
-          static_cast<HeapSnapshot::NodeID>(
-              IDTracker::ReservedObjectID::NoID) &&
+      lastFrag.lastSeenObjectID_ == IDTracker::kInvalidNode &&
       "Last fragment should not have an ID assigned yet");
   // In case a flush happens without any allocations occurring, don't add a new
   // fragment.
@@ -1073,11 +1064,7 @@ void GCBase::AllocationLocationTracker::flushCallback() {
     lastFrag.timestamp_ = duration;
     // Place an empty fragment at the end, for any new allocs.
     fragments_.emplace_back(Fragment{
-        static_cast<HeapSnapshot::NodeID>(IDTracker::ReservedObjectID::NoID),
-        std::chrono::microseconds(),
-        0,
-        0,
-        false});
+        IDTracker::kInvalidNode, std::chrono::microseconds(), 0, 0, false});
   }
   if (fragmentCallback_) {
     std::vector<HeapStatsUpdate> updatedFragments;
@@ -1101,18 +1088,19 @@ llvh::Optional<HeapSnapshot::NodeID> GCBase::getSnapshotID(HermesValue val) {
     // This should be rare, but is occasionally used by some parts of the VM.
     return val.getPointer()
         ? getObjectID(val.getPointer())
-        : static_cast<HeapSnapshot::NodeID>(IDTracker::ReservedObjectID::Null);
+        : IDTracker::reserved(IDTracker::ReservedObjectID::Null);
   } else if (val.isNumber()) {
     return idTracker_.getNumberID(val.getNumber());
   } else if (val.isUndefined()) {
-    return static_cast<HeapSnapshot::NodeID>(
-        IDTracker::ReservedObjectID::Undefined);
+    return IDTracker::reserved(IDTracker::ReservedObjectID::Undefined);
   } else if (val.isNull()) {
-    return static_cast<HeapSnapshot::NodeID>(IDTracker::ReservedObjectID::Null);
+    return static_cast<HeapSnapshot::NodeID>(
+        IDTracker::reserved(IDTracker::ReservedObjectID::Null));
   } else if (val.isBool()) {
     return static_cast<HeapSnapshot::NodeID>(
-        val.getBool() ? IDTracker::ReservedObjectID::True
-                      : IDTracker::ReservedObjectID::False);
+        val.getBool()
+            ? IDTracker::reserved(IDTracker::ReservedObjectID::True)
+            : IDTracker::reserved(IDTracker::ReservedObjectID::False));
   } else {
     return llvh::None;
   }

lib/VM/Runtime.cpp

@@ -1875,7 +1875,7 @@ void Runtime::recordHiddenClass(
   ClassId objectHiddenClassId = heap_.getObjectID(objectHiddenClass);
   // prevent cached hidden class from being GC-ed
   ClassId cachedHiddenClassId =
-      static_cast<ClassId>(GCBase::IDTracker::ReservedObjectID::NoID);
+      static_cast<ClassId>(GCBase::IDTracker::kInvalidNode);
   if (cachedHiddenClass != nullptr) {
     preventHCGC(cachedHiddenClass);
     cachedHiddenClassId = heap_.getObjectID(cachedHiddenClass);
@@ -1890,8 +1890,7 @@ void Runtime::getInlineCacheProfilerInfo(llvh::raw_ostream &ostream) {
 }
 
 HiddenClass *Runtime::resolveHiddenClassId(ClassId classId) {
-  if (classId ==
-      static_cast<ClassId>(GCBase::IDTracker::ReservedObjectID::NoID)) {
+  if (classId == static_cast<ClassId>(GCBase::IDTracker::kInvalidNode)) {
     return nullptr;
   }
   auto &classIdToIdxMap = inlineCacheProfiler_.getClassIdtoIndexMap();

unittests/VMRuntime/HeapSnapshotTest.cpp

@@ -520,54 +520,52 @@ TEST(HeapSnapshotTest, TestNodesAndEdgesForDummyObjects) {
   EXPECT_EQ(llvh::cast<JSONArray>(root->at("locations"))->size(), 0);
 
   // Common nodes.
-  Node gcRoots{HeapSnapshot::NodeType::Synthetic,
-               "(GC roots)",
-               static_cast<HeapSnapshot::NodeID>(
-                   GC::IDTracker::ReservedObjectID::GCRoots),
-               0,
-               1};
-  Node rootSection{HeapSnapshot::NodeType::Synthetic,
-                   "(Custom)",
-                   static_cast<HeapSnapshot::NodeID>(
-                       GC::IDTracker::ReservedObjectID::Custom),
-                   0,
-                   1};
+  Node gcRoots{
+      HeapSnapshot::NodeType::Synthetic,
+      "(GC roots)",
+      GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::GCRoots),
+      0,
+      1};
+  Node rootSection{
+      HeapSnapshot::NodeType::Synthetic,
+      "(Custom)",
+      GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::Custom),
+      0,
+      1};
   Node firstDummy{HeapSnapshot::NodeType::Object,
                   cellKindStr(dummy->getKind()),
                   gc.getObjectID(dummy.get()),
                   blockSize,
                   // One edge to the second dummy, 4 for primitive singletons.
                   5};
-  Node undefinedNode{HeapSnapshot::NodeType::Object,
-                     "undefined",
-                     static_cast<HeapSnapshot::NodeID>(
-                         GCBase::IDTracker::ReservedObjectID::Undefined),
-                     0,
-                     0};
+  Node undefinedNode{
+      HeapSnapshot::NodeType::Object,
+      "undefined",
+      GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::Undefined),
+      0,
+      0};
   Node nullNode{HeapSnapshot::NodeType::Object,
                 "null",
-                static_cast<HeapSnapshot::NodeID>(
-                    GCBase::IDTracker::ReservedObjectID::Null),
+                GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::Null),
                 0,
                 0};
   Node trueNode(
       HeapSnapshot::NodeType::Object,
       "true",
-      static_cast<HeapSnapshot::NodeID>(
-          GCBase::IDTracker::ReservedObjectID::True),
+      GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::True),
       0,
       0);
   Node numberNode{HeapSnapshot::NodeType::Number,
                   "3.14",
                   gc.getIDTracker().getNumberID(dummy->hvDouble.getNumber()),
                   0,
                   0};
-  Node falseNode{HeapSnapshot::NodeType::Object,
-                 "false",
-                 static_cast<HeapSnapshot::NodeID>(
-                     GCBase::IDTracker::ReservedObjectID::False),
-                 0,
-                 0};
+  Node falseNode{
+      HeapSnapshot::NodeType::Object,
+      "false",
+      GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::False),
+      0,
+      0};
   Node secondDummy{HeapSnapshot::NodeType::Object,
                    cellKindStr(dummy->getKind()),
                    gc.getObjectID(dummy->other),
@@ -594,8 +592,7 @@ TEST(HeapSnapshotTest, TestNodesAndEdgesForDummyObjects) {
   expectedNodes.emplace(
       HeapSnapshot::NodeType::Synthetic,
       "",
-      static_cast<HeapSnapshot::NodeID>(
-          GC::IDTracker::ReservedObjectID::SuperRoot),
+      GC::IDTracker::reserved(GC::IDTracker::ReservedObjectID::SuperRoot),
       0,
       1);
   expectedNodes.emplace(gcRoots);