src: introduce Constant class

mmarchini · mmarchini · commit 2afd59edfb41 · 2019-11-07T16:41:47.000-08:00
Same idea as CheckedType: instead of relying on the default value for constants to determine if they were loaded correctly or not (default value usually was -1), have a class which will have information about the loaded constant. This can help us: - Check if a constant was properly loaded before using it (otherwise we'd get into undefined behavior, and there are a lot of places where this happens. - Check if a constant value was loaded or if we're looking at the default value. - Explicitly define a constant as optional (constants which are not relevant except for specific V8 versions). This will help us improve reliability and debugability of llnode. PR-URL: #303 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
diff --git a/src/constants.cc b/src/constants.cc
@@ -1,4 +1,7 @@
 #include <cinttypes>
+#include <initializer_list>
+#include <iterator>
+#include <sstream>
 
 #include <lldb/API/SBExpressionOptions.h>
 
@@ -14,59 +17,51 @@ namespace llnode {
 
 template <typename T>
 T ReadSymbolFromTarget(SBTarget& target, SBAddress& start, const char* name,
-                       Error& err) {
-  SBError sberr;
+                       SBError& sberr) {
   T res = 0;
   target.ReadMemory(start, &res, sizeof(T), sberr);
-  if (!sberr.Fail()) {
-    err = Error::Ok();
-  } else {
-    err = Error::Failure("Failed to read symbol %s", name);
-  }
   return res;
 }
 
-int64_t Constants::LookupConstant(SBTarget target, const char* name,
-                                  int64_t def, Error& err) {
-  int64_t res = 0;
-  res = def;
+Constant<int64_t> Constants::LookupConstant(SBTarget target, const char* name) {
+  int64_t res;
 
   SBSymbolContextList context_list = target.FindSymbols(name);
 
   if (!context_list.IsValid() || context_list.GetSize() == 0) {
-    err = Error::Failure("Failed to find symbol %s", name);
-    return res;
+    return Constant<int64_t>();
   }
 
   SBSymbolContext context = context_list.GetContextAtIndex(0);
   SBSymbol symbol = context.GetSymbol();
   if (!symbol.IsValid()) {
-    err = Error::Failure("Failed to fetch symbol %s from context", name);
-    return res;
+    return Constant<int64_t>();
   }
 
   SBAddress start = symbol.GetStartAddress();
   SBAddress end = symbol.GetEndAddress();
   uint32_t size = end.GetOffset() - start.GetOffset();
 
   // NOTE: size could be bigger for at the end symbols
+  SBError sberr;
   if (size >= 8) {
-    res = ReadSymbolFromTarget<int64_t>(target, start, name, err);
+    res = ReadSymbolFromTarget<int64_t>(target, start, name, sberr);
   } else if (size == 4) {
-    int32_t tmp = ReadSymbolFromTarget<int32_t>(target, start, name, err);
+    int32_t tmp = ReadSymbolFromTarget<int32_t>(target, start, name, sberr);
     res = static_cast<int64_t>(tmp);
   } else if (size == 2) {
-    int16_t tmp = ReadSymbolFromTarget<int16_t>(target, start, name, err);
+    int16_t tmp = ReadSymbolFromTarget<int16_t>(target, start, name, sberr);
     res = static_cast<int64_t>(tmp);
   } else if (size == 1) {
-    int8_t tmp = ReadSymbolFromTarget<int8_t>(target, start, name, err);
+    int8_t tmp = ReadSymbolFromTarget<int8_t>(target, start, name, sberr);
     res = static_cast<int64_t>(tmp);
   } else {
-    err = Error::Failure("Unexpected symbol size %" PRIu32 " of symbol %s",
-                         size, name);
+    return Constant<int64_t>();
   }
 
-  return res;
+  if (sberr.Fail()) return Constant<int64_t>();
+
+  return Constant<int64_t>(res, name);
 }
 
 void Constants::Assign(SBTarget target) {
@@ -76,44 +71,71 @@ void Constants::Assign(SBTarget target) {
 
 
 int64_t Constants::LoadRawConstant(const char* name, int64_t def) {
-  Error err;
-  int64_t v = Constants::LookupConstant(target_, name, def, err);
-  if (err.Fail()) {
+  auto constant = Constants::LookupConstant(target_, name);
+  if (!constant.Check()) {
     PRINT_DEBUG("Failed to load raw constant %s, default to %" PRId64, name,
                 def);
+    return def;
   }
 
-  return v;
-}
-
-int64_t Constants::LoadConstant(const char* name, Error& err, int64_t def) {
-  int64_t v = Constants::LookupConstant(
-      target_, (constant_prefix() + name).c_str(), def, err);
-  return v;
+  return *constant;
 }
 
 int64_t Constants::LoadConstant(const char* name, int64_t def) {
-  Error err;
-  int64_t v = LoadConstant(name, err, def);
-  if (err.Fail()) {
-    PRINT_DEBUG("Failed to load constant %s, default to %" PRId64, name, def);
+  auto constant =
+      Constants::LookupConstant(target_, (constant_prefix() + name).c_str());
+  if (!constant.Check()) {
+    PRINT_DEBUG("Failed to load constant %s, default to %" PRId64, name);
+    return def;
   }
 
-  return v;
+  return *constant;
 }
 
 int64_t Constants::LoadConstant(const char* name, const char* fallback,
                                 int64_t def) {
-  Error err;
-  int64_t v = LoadConstant(name, err, def);
-  if (err.Fail()) v = LoadConstant(fallback, err, def);
-  if (err.Fail()) {
+  auto constant =
+      Constants::LookupConstant(target_, (constant_prefix() + name).c_str());
+  if (!constant.Check())
+    constant = Constants::LookupConstant(
+        target_, (constant_prefix() + fallback).c_str());
+  if (!constant.Check()) {
     PRINT_DEBUG("Failed to load constant %s, fallback %s, default to %" PRId64,
                 name, fallback, def);
+    return def;
   }
 
-  return v;
+  return *constant;
 }
 
+Constant<int64_t> Constants::LoadConstant(
+    std::initializer_list<const char*> names) {
+  for (std::string name : names) {
+    auto constant =
+        Constants::LookupConstant(target_, (constant_prefix() + name).c_str());
+    if (constant.Check()) return constant;
+  }
+
+  if (Error::IsDebugMode()) {
+    std::string joined = "";
+    for (std::string name : names) {
+      joined += (joined.empty() ? "'" : ", '") + name + "'";
+    }
+    PRINT_DEBUG("Failed to load constants: %s", joined.c_str());
+  }
+
+  return Constant<int64_t>();
+}
+
+Constant<int64_t> Constants::LoadOptionalConstant(
+    std::initializer_list<const char*> names, int def) {
+  for (std::string name : names) {
+    auto constant =
+        Constants::LookupConstant(target_, (constant_prefix() + name).c_str());
+    if (constant.Check()) return constant;
+  }
+
+  return Constant<int64_t>(def);
+}
 
 }  // namespace llnode
diff --git a/src/constants.h b/src/constants.h
@@ -10,6 +10,56 @@ using lldb::SBTarget;
 
 namespace llnode {
 
+enum ConstantStatus { kInvalid, kValid, kLoaded };
+
+// Class representing a constant which is used to interpret memory data. Most
+// constants represent offset of fields on an object, bit-masks or "tags" which
+// are used to identify types, but there are some constants with different
+// meanings as well.
+//
+// By default, constants are loaded from the binary debug symbols, and usually
+// those debug symbols are generated by V8's gen-postmortem-metadata.py or by
+// Node.js' node-postmortem-metadata.cc. Some constants can also come from C++
+// generated debug symbols.
+//
+// When a constant is successfully loaded, Check() and Loaded() will return
+// true, which means we can safely use this constant and make assumptions based
+// on its existence. In some cases, it's safe to assume defaults for a given
+// constant. If that's the case, the constant will return false on Loaded() but
+// true on Check(). A constant returning false for both Check() and Loaded() is
+// not safe to use.
+//
+// Use the dereference operator (*constant) to access a constant value.
+template <typename T>
+class Constant {
+ public:
+  Constant() : value_(-1), status_(kInvalid) {}
+  inline bool Check() {
+    return (status_ == ConstantStatus::kValid ||
+            status_ == ConstantStatus::kLoaded);
+  }
+
+  inline bool Loaded() { return status_ == kLoaded; }
+
+  T operator*() {
+    // TODO(mmarchini): Check()
+    return value_;
+  }
+
+  inline std::string name() { return name_; }
+
+ protected:
+  friend class Constants;
+  explicit Constant(T value) : value_(value), status_(kValid), name_("") {}
+  Constant(T value, std::string name)
+      : value_(value), status_(kLoaded), name_(name) {}
+
+ private:
+  T value_;
+  ConstantStatus status_;
+  std::string name_;
+};
+
 #define CONSTANTS_DEFAULT_METHODS(NAME) \
   inline NAME* operator()() {           \
     if (loaded_) return this;           \
@@ -28,15 +78,16 @@ class Constants {
 
   inline virtual std::string constant_prefix() { return ""; };
 
-  static int64_t LookupConstant(SBTarget target, const char* name, int64_t def,
-                                Error& err);
+  static Constant<int64_t> LookupConstant(SBTarget target, const char* name);
 
  protected:
   int64_t LoadRawConstant(const char* name, int64_t def = -1);
-  int64_t LoadConstant(const char* name, Error& err, int64_t def = -1);
   int64_t LoadConstant(const char* name, int64_t def = -1);
   int64_t LoadConstant(const char* name, const char* fallback,
                        int64_t def = -1);
+  Constant<int64_t> LoadConstant(std::initializer_list<const char*> names);
+  Constant<int64_t> LoadOptionalConstant(
+      std::initializer_list<const char*> names, int def);
 
   lldb::SBTarget target_;
   bool loaded_;
diff --git a/src/error.h b/src/error.h
@@ -30,6 +30,7 @@ class Error {
   inline const char* GetMessage() { return msg_.c_str(); }
 
   static void SetDebugMode(bool mode) { is_debug_mode = mode; }
+  static bool IsDebugMode() { return is_debug_mode; }
 
  private:
   bool failed_;
diff --git a/src/llv8-constants.cc b/src/llv8-constants.cc
@@ -81,10 +81,10 @@ void HeapObject::Load() {
 
 void Map::Load() {
   Error err;
-  kInstanceAttrsOffset =
-      LoadConstant("class_Map__instance_attributes__int", err);
-  if (err.Fail()) {
-    kInstanceAttrsOffset = LoadConstant("class_Map__instance_type__uint16_t");
+  kInstanceAttrsOffset = LoadConstant({"class_Map__instance_attributes__int",
+                                       "class_Map__instance_type__uint16_t"});
+  if (kInstanceAttrsOffset.name() ==
+      "v8dbg_class_Map__instance_type__uint16_t") {
     kMapTypeMask = 0xffff;
   } else {
     kMapTypeMask = 0xff;
@@ -93,8 +93,9 @@ void Map::Load() {
   kMaybeConstructorOffset =
       LoadConstant("class_Map__constructor_or_backpointer__Object",
                    "class_Map__constructor__Object");
-  kInstanceDescriptorsOffset =
-      LoadConstant("class_Map__instance_descriptors__DescriptorArray");
+  kInstanceDescriptorsOffset = LoadConstant({
+      "class_Map__instance_descriptors__DescriptorArray",
+  });
   kBitField3Offset =
       LoadConstant("class_Map__bit_field3__int", "class_Map__bit_field3__SMI");
   kInObjectPropertiesOffset = LoadConstant(
diff --git a/src/llv8-constants.h b/src/llv8-constants.h
@@ -69,9 +69,9 @@ class Map : public Module {
   CONSTANTS_DEFAULT_METHODS(Map);
 
   int64_t kMapTypeMask;
-  int64_t kInstanceAttrsOffset;
+  Constant<int64_t> kInstanceAttrsOffset;
   int64_t kMaybeConstructorOffset;
-  int64_t kInstanceDescriptorsOffset;
+  Constant<int64_t> kInstanceDescriptorsOffset;
   int64_t kBitField3Offset;
   int64_t kInObjectPropertiesOffset;
   int64_t kInObjectPropertiesStartOffset;
diff --git a/src/llv8-inl.h b/src/llv8-inl.h
@@ -165,9 +165,11 @@ inline bool HeapObject::IsJSErrorType(Error& err) {
 }
 
 
+// TODO(mmarchini): return CheckedType
 inline int64_t Map::GetType(Error& err) {
-  int64_t type =
-      v8()->LoadUnsigned(LeaField(v8()->map()->kInstanceAttrsOffset), 2, err);
+  RETURN_IF_INVALID(v8()->map()->kInstanceAttrsOffset, -1);
+  int64_t type = v8()->LoadUnsigned(
+      LeaField(*(v8()->map()->kInstanceAttrsOffset)), 2, err);
   if (err.Fail()) return -1;
 
   return type & v8()->map()->kMapTypeMask;
@@ -230,12 +232,19 @@ inline int64_t Map::NumberOfOwnDescriptors(Error& err) {
     return LoadFieldValue<TYPE>(v8()->OFF, err); \
   }
 
+#define SAFE_ACCESSOR(CLASS, METHOD, OFF, TYPE)     \
+  inline TYPE CLASS::METHOD(Error& err) {           \
+    if (!Check()) return TYPE();                    \
+    if (!v8()->OFF.Check()) return TYPE();          \
+    return LoadFieldValue<TYPE>(*(v8()->OFF), err); \
+  }
+
 
 ACCESSOR(HeapObject, GetMap, heap_obj()->kMapOffset, HeapObject)
 
 ACCESSOR(Map, MaybeConstructor, map()->kMaybeConstructorOffset, HeapObject)
-ACCESSOR(Map, InstanceDescriptors, map()->kInstanceDescriptorsOffset,
-         HeapObject)
+SAFE_ACCESSOR(Map, InstanceDescriptors, map()->kInstanceDescriptorsOffset,
+              HeapObject)
 
 ACCESSOR(Symbol, Name, symbol()->kNameOffset, HeapObject)
 
diff --git a/src/llv8.h b/src/llv8.h
@@ -37,9 +37,10 @@ class CodeMap;
   NAME(Value* v) : PARENT(v->v8(), v->raw()) {}    \
   static inline const char* ClassName() { return #NAME; }
 
-#define RETURN_IF_INVALID(var, ret) \
-  if (!var.Check()) {               \
-    return ret;                     \
+#define RETURN_IF_INVALID(var, ret)                            \
+  if (!var.Check()) {                                          \
+    PRINT_DEBUG("Unable to load variable %s correctly", #var); \
+    return ret;                                                \
   }
 
 #define RETURN_IF_THIS_INVALID(ret) \
