src: boxed double fields for V8 8.1

mmarchini · mmarchini · commit dd57bfb8af3c · 2020-04-21T16:31:15.000-07:00
V8 8.1 disabled unboxed double fieds, which means all double fields are now stored on memory as references to HeapNumber objects instead of the double value. In theory V8 could store boxed doubles before, but we didn't take it into account in our dobule fields code. In the future, V8 intends to re-enable unboxed doubles, which means we'll need to add logic to handle both types of fields in the same llnode session. This also fixes a bug where llnode was not handling double fields at all in some cases. Fixes: #353 Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #358 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
diff --git a/src/error.h b/src/error.h
@@ -1,12 +1,18 @@
 #ifndef SRC_ERROR_H_
 #define SRC_ERROR_H_
 
+#include <iostream>
 #include <string>
 #include <typeinfo>
 
 #define PRINT_DEBUG(format, ...) \
   Error::PrintInDebugMode(__FILE__, __LINE__, __func__, format, ##__VA_ARGS__)
 
+[[noreturn]] inline void unreachable() {
+  std::cerr << "unreachable" << std::endl;
+  std::abort();
+}
+
 namespace llnode {
 
 class Error {
diff --git a/src/llv8-constants.cc b/src/llv8-constants.cc
@@ -128,9 +128,24 @@ void Map::Load() {
     kNumberOfOwnDescriptorsMask = (1 << kDescriptorIndexBitCount) - 1;
     kNumberOfOwnDescriptorsMask <<= kNumberOfOwnDescriptorsShift;
   }
+  kLayoutDescriptor =
+      LoadConstant({"class_Map__layout_descriptor__LayoutDescriptor"});
 }
 
 
+bool Map::HasUnboxedDoubleFields() {
+  // LayoutDescriptor is used by V8 to define which fields are not tagged
+  // (unboxed). In preparation for pointer compressions, V8 disabled unboxed
+  // doubles everywhere, which means Map doesn't have a layout_descriptor
+  // field, but because of how gen-postmortem-metadata works and how Torque
+  // generates the offsets file, we get a constant for it anyway. In the future
+  // unboxing will be enabled again, in which case this field will be used.
+  // Until then, we use the presence of this field as version (if the field is
+  // present, it's safe to assume we're on V8 8.1+, at least on supported
+  // release lines).
+  return !kLayoutDescriptor.Loaded();
+}
+
 void JSObject::Load() {
   kPropertiesOffset =
       LoadConstant("class_JSReceiver__raw_properties_or_hash__Object",
diff --git a/src/llv8-constants.h b/src/llv8-constants.h
@@ -77,11 +77,14 @@ class Map : public Module {
   int64_t kInObjectPropertiesStartOffset;
   int64_t kInstanceSizeOffset;
   int64_t kInstanceTypeOffset;
+  Constant<int64_t> kLayoutDescriptor;
 
   int64_t kNumberOfOwnDescriptorsMask;
   int64_t kNumberOfOwnDescriptorsShift;
   int64_t kDictionaryMapShift;
 
+  bool HasUnboxedDoubleFields();
+
  protected:
   void Load();
 };
diff --git a/src/llv8-inl.h b/src/llv8-inl.h
@@ -89,6 +89,10 @@ inline bool HeapObject::Check() const {
          (raw() & v8()->heap_obj()->kTagMask) == v8()->heap_obj()->kTag;
 }
 
+inline bool HeapNumber::Check() const {
+  if (unboxed_double_) return Value::Check();
+  return HeapObject::Check();
+}
 
 int64_t HeapObject::LeaField(int64_t off) const {
   return raw() - v8()->heap_obj()->kTag + off;
@@ -388,8 +392,46 @@ inline T JSObject::GetInObjectValue(int64_t size, int index, Error& err) {
   return LoadFieldValue<T>(size + index * v8()->common()->kPointerSize, err);
 }
 
+inline HeapNumber JSObject::GetDoubleField(int64_t index, Error err) {
+  HeapObject map_obj = GetMap(err);
+  if (err.Fail()) HeapNumber();
+
+  Map map(map_obj);
+  int64_t instance_size = map.InstanceSize(err);
+  if (err.Fail()) return HeapNumber();
+
+  // TODO(mmarchini): Explain why index might be lower than zero.
+  if (index < 0) {
+    // When unboxed doubles are not allowed, all double fields are stored as
+    // HeapNumber objects.
+    if (v8()->map()->HasUnboxedDoubleFields()) {
+      // TODO(mmarchini): GetInObjectValue call chain should be
+      // CheckedType-aware instead of relying on Error.
+      Error get_in_object_value_err;
+      double result = GetInObjectValue<double>(instance_size, index,
+                                               get_in_object_value_err);
+      if (get_in_object_value_err.Fail()) {
+        return HeapNumber(v8(), CheckedType<double>());
+      } else {
+        return HeapNumber(v8(), CheckedType<double>(result));
+      }
+    }
+    return GetInObjectValue<HeapNumber>(instance_size, index, err);
+  }
+  HeapObject extra_properties_obj = Properties(err);
+  if (err.Fail()) return HeapNumber();
+
+  FixedArray extra_properties(extra_properties_obj);
+
+  return HeapNumber(v8(), extra_properties.Get<double>(index, err));
+}
+
+inline const CheckedType<double> HeapNumber::GetValue(Error& err) {
+  if (unboxed_double_) return unboxed_value_;
+  return GetHeapNumberValue(err);
+};
 
-ACCESSOR(HeapNumber, GetValue, heap_number()->kValueOffset, double)
+ACCESSOR(HeapNumber, GetHeapNumberValue, heap_number()->kValueOffset, double)
 
 ACCESSOR(JSArray, Length, js_array()->kLengthOffset, Smi)
 
diff --git a/src/llv8.cc b/src/llv8.cc
@@ -693,8 +693,12 @@ std::string HeapObject::GetTypeName(Error& err) {
 std::string HeapNumber::ToString(bool whole, Error& err) {
   char buf[128];
   const char* fmt = whole ? "%f" : "%.2f";
-  snprintf(buf, sizeof(buf), fmt, GetValue(err));
-  err = Error::Ok();
+  auto val = GetValue(err);
+  if (err.Fail() || !val.Check()) {
+    err = Error::Ok();
+    return "???";
+  }
+  snprintf(buf, sizeof(buf), fmt, val);
   return buf;
 }
 
@@ -1176,15 +1180,7 @@ Value JSObject::GetDescriptorProperty(std::string key_name, Map map,
     int64_t index = descriptors.FieldIndex(details) - in_object_count;
 
     if (descriptors.IsDoubleField(details)) {
-      double value;
-      if (index < 0) {
-        value = GetInObjectValue<double>(instance_size, index, err);
-      } else {
-        value = extra_properties.Get<double>(index, err);
-      }
-
-      if (err.Fail()) return Value();
-
+      return GetDoubleField(index, err);
     } else {
       Value value;
       if (index < 0) {
diff --git a/src/llv8.h b/src/llv8.h
@@ -277,13 +277,35 @@ class ThinString : public String {
   inline std::string ToString(Error& err);
 };
 
+// Numbers on the heap can be a boxed HeapNumber or a unboxed double.
 class HeapNumber : public HeapObject {
  public:
   V8_VALUE_DEFAULT_METHODS(HeapNumber, HeapObject)
 
-  inline double GetValue(Error& err);
+  HeapNumber(LLV8* v8, CheckedType<double> value)
+      : HeapObject(v8, 0), unboxed_value_(value), unboxed_double_(true) {}
+
+  inline bool Check() const;
+
+  inline int64_t raw() const {
+    if (unboxed_double_) {
+      PRINT_DEBUG("Shouldn't try to access raw() of unboxed double");
+#if DEBUG
+      unreachable();
+#endif
+    }
+    return HeapObject::raw();
+  }
+
+  inline const CheckedType<double> GetValue(Error& err);
 
   std::string ToString(bool whole, Error& err);
+
+ private:
+  inline double GetHeapNumberValue(Error& err);
+
+  const CheckedType<double> unboxed_value_;
+  const bool unboxed_double_ = false;
 };
 
 class JSObject : public HeapObject {
@@ -309,6 +331,8 @@ class JSObject : public HeapObject {
 
   static inline bool IsObjectType(LLV8* v8, int64_t type);
 
+  inline HeapNumber GetDoubleField(int64_t index, Error err);
+
  protected:
   friend class llnode::Printer;
   template <class T>
diff --git a/src/printer.cc b/src/printer.cc
@@ -1030,17 +1030,10 @@ std::string Printer::StringifyDescriptors(v8::JSObject js_object, v8::Map map,
     int64_t index = descriptors.FieldIndex(details) - in_object_count;
 
     if (descriptors.IsDoubleField(details)) {
-      double value;
-      if (index < 0)
-        value = js_object.GetInObjectValue<double>(instance_size, index, err);
-      else
-        value = extra_properties.Get<double>(index, err);
-
-      if (err.Fail()) return std::string();
+      v8::HeapNumber value = js_object.GetDoubleField(index, err);
 
-      char tmp[100];
-      snprintf(tmp, sizeof(tmp), "%f", value);
-      res += tmp;
+      Error value_err;
+      res += value.ToString(true, value_err);
     } else {
       v8::Value value;
       if (index < 0)
