[tools] Add in-object properties to debug_helper

Until now, the in-object properties on JSObject have been invisible to
tools using the postmortem debugging library. With this change, those
tools will get enough information to show a flat list of property
values. This is still less powerful than the runtime printers, which can
show the corresponding key for each value, but it's a big step up from
manually inspecting memory.

This change basically requires a reimplementation of
Map::GetInObjectProperties for postmortem debugging. I'm not
enthusiastic about duplicating this logic, but it's pretty small and I
don't see any good alternatives.

As a drive-by cleanup, I moved some inline string literals into a batch
of constexpr char arrays.

Bug: v8:9376
Change-Id: Ia24c05f6e823086babaa07882d0d320ab9a225db
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1930174Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#65183}

test/cctest/test-debug-helper.cc

@@ -78,15 +78,21 @@ void CheckStructProp(const d::StructProperty& property,
   CHECK_EQ(property.offset, expected_offset);
 }
 
-template <typename TValue>
-TValue ReadProp(const d::ObjectPropertiesResult& props, std::string name) {
+const d::ObjectProperty& FindProp(const d::ObjectPropertiesResult& props,
+                                  std::string name) {
   for (size_t i = 0; i < props.num_properties; ++i) {
     if (name == props.properties[i]->name) {
-      return *reinterpret_cast<TValue*>(props.properties[i]->address);
+      return *props.properties[i];
     }
   }
   CHECK_WITH_MSG(false, ("property '" + name + "' not found").c_str());
-  return {};
+  UNREACHABLE();
+}
+
+template <typename TValue>
+TValue ReadProp(const d::ObjectPropertiesResult& props, std::string name) {
+  const d::ObjectProperty& prop = FindProp(props, name);
+  return *reinterpret_cast<TValue*>(prop.address);
 }
 
 }  // namespace
@@ -278,12 +284,25 @@ TEST(GetObjectProperties) {
   props = d::GetObjectProperties(o->ptr(), &ReadMemory, heap_addresses);
   CHECK(std::string(props->brief).substr(79, 7) == std::string("aa...\" "));
 
-  // Verify the result for a heap object field which is itself a struct: the
-  // "descriptors" field on a DescriptorArray.
-  // First we need to construct an object and get its map's descriptor array.
+  // Build a basic JS object and get its properties.
   v = CompileRun("({a: 1, b: 2})");
   o = v8::Utils::OpenHandle(*v);
   props = d::GetObjectProperties(o->ptr(), &ReadMemory, heap_addresses);
+
+  // Objects constructed from literals get their properties placed inline, so
+  // the GetObjectProperties response should include an array.
+  const d::ObjectProperty& prop = FindProp(*props, "in-object properties");
+  CheckProp(prop, "v8::internal::Object", "in-object properties",
+            d::PropertyKind::kArrayOfKnownSize, 2);
+  // The second item in that array is the SMI value 2 from the object literal.
+  props2 =
+      d::GetObjectProperties(reinterpret_cast<i::Tagged_t*>(prop.address)[1],
+                             &ReadMemory, heap_addresses);
+  CHECK(props2->brief == std::string("2 (0x2)"));
+
+  // Verify the result for a heap object field which is itself a struct: the
+  // "descriptors" field on a DescriptorArray.
+  // Start by getting the object's map and the map's descriptor array.
   props = d::GetObjectProperties(ReadProp<i::Tagged_t>(*props, "map"),
                                  &ReadMemory, heap_addresses);
   props = d::GetObjectProperties(

tools/debug_helper/get-object-properties.cc

@@ -16,6 +16,16 @@ namespace i = v8::internal;
 
 namespace v8_debug_helper_internal {
 
+constexpr char kObject[] = "v8::internal::Object";
+constexpr char kTaggedValue[] = "v8::internal::TaggedValue";
+constexpr char kSmi[] = "v8::internal::Smi";
+constexpr char kHeapObject[] = "v8::internal::HeapObject";
+#ifdef V8_COMPRESS_POINTERS
+constexpr char kObjectAsStoredInHeap[] = "v8::internal::TaggedValue";
+#else
+constexpr char kObjectAsStoredInHeap[] = "v8::internal::Object";
+#endif
+
 std::string AppendAddressAndType(const std::string& brief, uintptr_t address,
                                  const char* type) {
   std::stringstream brief_stream;
@@ -24,6 +34,10 @@ std::string AppendAddressAndType(const std::string& brief, uintptr_t address,
                        : brief + " (" + brief_stream.str() + ")";
 }
 
+std::string JoinWithSpace(const std::string& a, const std::string& b) {
+  return a.empty() || b.empty() ? a + b : a + " " + b;
+}
+
 struct TypedObject {
   TypedObject(d::TypeCheckResult type_check_result,
               std::unique_ptr<TqObject> object)
@@ -334,27 +348,78 @@ class ReadStringVisitor : public TqObjectVisitor {
   bool done_;      // Whether to stop further work.
 };
 
-// An object visitor that adds extra debugging information for some types.
+// An object visitor that supplies extra information for some types.
 class AddInfoVisitor : public TqObjectVisitor {
  public:
-  AddInfoVisitor(const std::string& brief, d::MemoryAccessor accessor,
-                 const d::HeapAddresses& heap_addresses)
-      : accessor_(accessor), brief_(brief), heap_addresses_(heap_addresses) {}
-
-  // Returns the brief object description, once visiting is complete.
-  const std::string& GetBrief() { return brief_; }
+  // Returns a descriptive string and a list of properties for the given object.
+  // Both may be empty, and are meant as an addition to, not a replacement for,
+  // the Torque-generated data about the object.
+  static std::pair<std::string, std::vector<std::unique_ptr<ObjectProperty>>>
+  Visit(const TqObject* object, d::MemoryAccessor accessor,
+        const d::HeapAddresses& heap_addresses) {
+    AddInfoVisitor visitor(accessor, heap_addresses);
+    object->Visit(&visitor);
+    return {std::move(visitor.brief_), std::move(visitor.properties_)};
+  }
 
   void VisitString(const TqString* object) override {
     ReadStringVisitor visitor(accessor_, heap_addresses_);
     object->Visit(&visitor);
-    if (!brief_.empty()) brief_ += " ";
-    brief_ += "\"" + visitor.GetString() + "\"";
+    brief_ = "\"" + visitor.GetString() + "\"";
+  }
+
+  void VisitJSObject(const TqJSObject* object) override {
+    // JSObject and its subclasses can be followed directly by an array of
+    // property values. The start and end offsets of those values are described
+    // by a pair of values in its Map.
+    auto map_ptr = object->GetMapValue(accessor_);
+    if (map_ptr.validity != d::MemoryAccessResult::kOk) {
+      return;  // Can't read the JSObject. Nothing useful to do.
+    }
+    TqMap map(map_ptr.value);
+
+    // On JSObject instances, this value is the start of in-object properties.
+    // The constructor function index option is only for primitives.
+    auto start_offset =
+        map.GetInObjectPropertiesStartOrConstructorFunctionIndexValue(
+            accessor_);
+
+    // The total size of the object in memory. This may include over-allocated
+    // expansion space that doesn't correspond to any user-accessible property.
+    auto instance_size = map.GetInstanceSizeInWordsValue(accessor_);
+
+    if (start_offset.validity != d::MemoryAccessResult::kOk ||
+        instance_size.validity != d::MemoryAccessResult::kOk) {
+      return;  // Can't read the Map. Nothing useful to do.
+    }
+    int num_properties = instance_size.value - start_offset.value;
+    if (num_properties > 0) {
+      properties_.push_back(std::make_unique<ObjectProperty>(
+          "in-object properties", kObjectAsStoredInHeap, kObject,
+          object->GetMapAddress() + start_offset.value * i::kTaggedSize,
+          num_properties, i::kTaggedSize,
+          std::vector<std::unique_ptr<StructProperty>>(),
+          d::PropertyKind::kArrayOfKnownSize));
+    }
   }
 
  private:
+  AddInfoVisitor(d::MemoryAccessor accessor,
+                 const d::HeapAddresses& heap_addresses)
+      : accessor_(accessor), heap_addresses_(heap_addresses) {}
+
+  // Inputs used by this visitor:
+
   d::MemoryAccessor accessor_;
-  std::string brief_;
   const d::HeapAddresses& heap_addresses_;
+
+  // Outputs generated by this visitor:
+
+  // A brief description of the object.
+  std::string brief_;
+  // A list of extra properties to append after the automatic ones that are
+  // created for all Torque-defined class fields.
+  std::vector<std::unique_ptr<ObjectProperty>> properties_;
 };
 
 std::unique_ptr<ObjectPropertiesResult> GetHeapObjectPropertiesNotCompressed(
@@ -366,16 +431,15 @@ std::unique_ptr<ObjectPropertiesResult> GetHeapObjectPropertiesNotCompressed(
 
   TypedObject typed =
       GetTypedHeapObject(address, accessor, type_hint, heap_addresses);
+  auto props = typed.object->GetProperties(accessor);
 
-  // TODO(v8:9376): Many object types need additional data that is not included
-  // in their Torque layout definitions. For example, JSObject has an array of
-  // in-object properties after its Torque-defined fields, which at a minimum
-  // should be represented as an array in this response. If the relevant memory
-  // is available, we should instead represent those properties (and any out-of-
-  // object properties) using their JavaScript property names.
-  AddInfoVisitor visitor(brief, accessor, heap_addresses);
-  typed.object->Visit(&visitor);
-  brief = visitor.GetBrief();
+  // Use the AddInfoVisitor to get any extra properties or descriptive text that
+  // can't be directly derived from Torque class definitions.
+  auto extra_info =
+      AddInfoVisitor::Visit(typed.object.get(), accessor, heap_addresses);
+  brief = JoinWithSpace(brief, extra_info.first);
+  props.insert(props.end(), std::make_move_iterator(extra_info.second.begin()),
+               std::make_move_iterator(extra_info.second.end()));
 
   brief = AppendAddressAndType(brief, address, typed.object->GetName());
 
@@ -387,8 +451,8 @@ std::unique_ptr<ObjectPropertiesResult> GetHeapObjectPropertiesNotCompressed(
   }
 
   return std::make_unique<ObjectPropertiesResult>(
-      typed.type_check_result, brief, typed.object->GetName(),
-      typed.object->GetProperties(accessor), std::move(guessed_types));
+      typed.type_check_result, brief, typed.object->GetName(), std::move(props),
+      std::move(guessed_types));
 }
 
 std::unique_ptr<ObjectPropertiesResult> GetHeapObjectPropertiesMaybeCompressed(
@@ -410,10 +474,9 @@ std::unique_ptr<ObjectPropertiesResult> GetHeapObjectPropertiesMaybeCompressed(
   if (any_uncompressed_ptr == 0) {
     // We can't figure out the heap range. Just check for known objects.
     std::string brief = FindKnownObject(address, heap_addresses);
-    brief = AppendAddressAndType(brief, address, "v8::internal::TaggedValue");
+    brief = AppendAddressAndType(brief, address, kTaggedValue);
     return std::make_unique<ObjectPropertiesResult>(
-        d::TypeCheckResult::kUnableToDecompress, brief,
-        "v8::internal::TaggedValue");
+        d::TypeCheckResult::kUnableToDecompress, brief, kTaggedValue);
   }
 
   address = EnsureDecompressed(address, any_uncompressed_ptr);
@@ -427,8 +490,7 @@ std::unique_ptr<ObjectPropertiesResult> GetObjectProperties(
     const d::HeapAddresses& heap_addresses, const char* type_hint) {
   if (static_cast<uint32_t>(address) == i::kClearedWeakHeapObjectLower32) {
     return std::make_unique<ObjectPropertiesResult>(
-        d::TypeCheckResult::kWeakRef, "cleared weak ref",
-        "v8::internal::HeapObject");
+        d::TypeCheckResult::kWeakRef, "cleared weak ref", kHeapObject);
   }
   bool is_weak = (address & i::kHeapObjectTagMask) == i::kWeakHeapObjectTag;
   if (is_weak) {
@@ -449,8 +511,8 @@ std::unique_ptr<ObjectPropertiesResult> GetObjectProperties(
   int32_t value = i::PlatformSmiTagging::SmiToInt(address);
   std::stringstream stream;
   stream << value << " (0x" << std::hex << value << ")";
-  return std::make_unique<ObjectPropertiesResult>(
-      d::TypeCheckResult::kSmi, stream.str(), "v8::internal::Smi");
+  return std::make_unique<ObjectPropertiesResult>(d::TypeCheckResult::kSmi,
+                                                  stream.str(), kSmi);
 }
 
 }  // namespace v8_debug_helper_internal