Uniform and precise source positions for inlining

src/source-position.h

Index: src/source-position.h
diff --git a/src/source-position.h b/src/source-position.h
index 2d36e97521a9d0fe7673f1b32f8eb8ff71219ee7..abb485cf0d7450857fc54ae48a034469078297f4 100644
--- a/src/source-position.h
+++ b/src/source-position.h
@@ -9,77 +9,122 @@
 
 #include "src/flags.h"
 #include "src/globals.h"
+#include "src/handles.h"
 #include "src/utils.h"
 
 namespace v8 {
 namespace internal {
 
-// This class encapsulates encoding and decoding of sources positions from
-// which hydrogen values originated.
-// When FLAG_track_hydrogen_positions is set this object encodes the
-// identifier of the inlining and absolute offset from the start of the
-// inlined function.
-// When the flag is not set we simply track absolute offset from the
-// script start.
-class SourcePosition {
+class SharedFunctionInfo;
+class Code;
+class CompilationInfo;
+class Script;
+
+struct SourcePositionInfo;
+
+class SourcePosition final {
  public:
-  static SourcePosition Unknown() {
-    return SourcePosition::FromRaw(kNoPosition);
+  SourcePosition() = default;

[vogelheim, 2016/10/31 17:34:40]

Wouldn't this leave value_ uninitialized? That doesn't seem like a good idea,
especially in a public method.

+
+  explicit SourcePosition(int script_offset, int inlining_id = kUnknown)
+      : value_(0) {
+    SetScriptOffset(script_offset);
+    SetInliningId(inlining_id);
   }
 
-  bool IsUnknown() const { return value_ == kNoPosition; }
+  static SourcePosition Unknown() { return SourcePosition(); }
+  bool IsUnknown() const { return !IsKnown(); }
+  bool IsKnown() const { return ScriptOffset() != kUnknown; }
 
-  uint32_t position() const { return PositionField::decode(value_); }
-  void set_position(uint32_t position) {
-    if (FLAG_hydrogen_track_positions) {
-      value_ = static_cast<uint32_t>(PositionField::update(value_, position));
-    } else {
-      value_ = position;
+  SourcePositionInfo Info(Handle<SharedFunctionInfo> script) const;
+  std::vector<SourcePositionInfo> Info(Handle<Code> code) const;
+  std::vector<SourcePositionInfo> Info(CompilationInfo* code) const;
+
+  void Print(std::ostream& out, Code* function) const;
+
+  int ScriptOffset() const { return ScriptOffsetField::decode(value_) - 1; }
+  int InliningId() const { return InliningIdField::decode(value_) - 1; }
+
+  void SetScriptOffset(int script_offset) {

[vogelheim, 2016/10/31 17:34:40]

This claims to set the script offset, but for certain values it will actually
silently do nothing. Is this your intent?

Should this maybe have been DCHECKs; or should the SourcePosition have some sort
of 'invalid' state?

+    if (script_offset == kNoSourcePosition ||
+        script_offset >= ScriptOffsetField::kMax)
+      script_offset = kUnknown;
+    if (script_offset != kUnknown) {

[vogelheim, 2016/10/31 17:34:40]

Why not merge the ifs?

if (script_offset != kUnknown &&
    script_offset != kNoSourcePosition &&
    script_offset < ScriptOffsetField::kMax) { ... 


[There's a similar thing below. Note this is largely a matter of taste, so
please decide as you see fit.]

+      DCHECK(script_offset >= 0);
+      value_ = ScriptOffsetField::update(value_, script_offset + 1);
     }
   }
-
-  uint32_t inlining_id() const { return InliningIdField::decode(value_); }
-  void set_inlining_id(uint32_t inlining_id) {
-    if (FLAG_hydrogen_track_positions) {
-      value_ =
-          static_cast<uint32_t>(InliningIdField::update(value_, inlining_id));
+  void SetInliningId(int inlining_id) {
+    if (inlining_id >= InliningIdField::kMax) inlining_id = kUnknown;
+    if (inlining_id != kUnknown) {
+      DCHECK(inlining_id >= 0);
+      value_ = InliningIdField::update(value_, inlining_id + 1);
     }
   }
 
-  uint32_t raw() const { return value_; }
-
- private:
-  static const uint32_t kNoPosition = static_cast<uint32_t>(kNoSourcePosition);
-  typedef BitField<uint32_t, 0, 9> InliningIdField;
-
-  // Offset from the start of the inlined function.
-  typedef BitField<uint32_t, 9, 23> PositionField;
+  int EncodeScriptOffset() { return ScriptOffset() + 2; }
+  int EncodeInliningId() { return -(InliningId() + 2); }

[vogelheim, 2016/10/31 17:34:40]

why?

+  void Decode(int enc) {
+    if (enc > 0) {
+      SetScriptOffset(enc - 2);
+    } else {
+      DCHECK(enc < 0);
+      SetInliningId(-enc - 2);
+    }
+  }
 
-  friend class HPositionInfo;
-  friend class Deoptimizer;
+  static const int kUnknown = -1;
+  STATIC_ASSERT(kUnknown == kNoSourcePosition);
 
-  static SourcePosition FromRaw(uint32_t raw_position) {
+  uint64_t raw() const { return value_; }
+  static SourcePosition FromRaw(uint64_t raw) {
     SourcePosition position;
-    position.value_ = raw_position;
+    position.value_ = raw;
     return position;
   }
 
-  // If FLAG_hydrogen_track_positions is set contains bitfields InliningIdField
-  // and PositionField.
-  // Otherwise contains absolute offset from the script start.
-  uint32_t value_;
+ private:
+  void Print(std::ostream& out) const;
+  void Print(std::ostream& out, SharedFunctionInfo* function) const;
+
+  // ids are in the hight bits for better compression in SourcePositionTable

[vogelheim, 2016/10/31 17:34:40]

hight -> high

+  typedef BitField64<int, 0, 31> ScriptOffsetField;
+  typedef BitField64<int, 31, 16> InliningIdField;
+  // leaving the highest bit untouched to allow for signed conversion
+  uint64_t value_;
 };
 
-inline std::ostream& operator<<(std::ostream& os, const SourcePosition& p) {
-  if (p.IsUnknown()) {
-    return os << "<?>";
-  } else if (FLAG_hydrogen_track_positions) {
-    return os << "<" << p.inlining_id() << ":" << p.position() << ">";
-  } else {
-    return os << "<0:" << p.raw() << ">";
-  }
+inline bool operator==(const SourcePosition& lhs, const SourcePosition& rhs) {
+  return lhs.raw() == rhs.raw();
 }
 
+inline bool operator!=(const SourcePosition& lhs, const SourcePosition& rhs) {
+  return !(lhs == rhs);
+}
+
+struct InliningPosition {
+  // these two ints correspond to the fields in SourcePosition

[vogelheim, 2016/10/31 17:34:40]

"these two ints"? I'm only seeing one int and a SourcePosition.

+  SourcePosition position;
+
+  // references position in DeoptimizationInputData::literals()
+  int inlined_function_id;
+};
+
+struct SourcePositionInfo {
+  explicit SourcePositionInfo(SourcePosition position) : position(position) {}
+
+  SourcePosition position;
+  MaybeHandle<SharedFunctionInfo> function;
+  int line = -1;
+  int column = -1;
+};
+
+std::ostream& operator<<(std::ostream& out, const SourcePosition& pos);
+
+std::ostream& operator<<(std::ostream& out, const SourcePositionInfo& pos);
+std::ostream& operator<<(std::ostream& out,
+                         const std::vector<SourcePositionInfo>& stack);
+
 }  // namespace internal
 }  // namespace v8