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..1a2bc375db85beecdbc13d485efc3ee5a91b038b 100644
--- a/src/source-position.h
+++ b/src/source-position.h
@@ -9,77 +9,119 @@
 
 #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;

[vogelheim, 2016/11/04 10:57:41]

nitpick: Code < Comp... < Script < SharedFunc...

+class Code;
+class CompilationInfo;
+class Script;
+
+struct SourcePositionInfo;
+
+class SourcePosition final {

[vogelheim, 2016/11/04 10:57:41]

Please add a one-line comment on what this class does.

This class *looks* simple, but I'm rather confused on its value space. So maybe
add a class comment on that, too:

The value space of this class appears to be:
  ( script_offset | kNoSourcePosition ) x ( inline_id | kNotInlined )
  with:
    script_offset = 0..2**31 - 2
    inline_id = 0..2**16 - 2

Can a kNoSourcePosition have a proper inline_id?

[Tobias Tebbi, 2016/11/07 15:10:34]

It has a proper interpretation (unknown position inside of an inlined call), and
it is currently used.

  public:
-  static SourcePosition Unknown() {
-    return SourcePosition::FromRaw(kNoPosition);
+  explicit SourcePosition(int script_offset = kNoSourcePosition,
+                          int inlining_id = kNotInlined)
+      : 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() != kNoSourcePosition; }

[vogelheim, 2016/11/04 10:57:41]

I'm confused.... The CL comment says, "If SourcePosition::InliningId() is
SourcePosition::kUnknown (-1), it refers to the outer (non-inlined) function."
There is no kUnknown. But there is Is[Unk|K]nown(). But those deal with the
script offset, not the inline id.

[vogelheim, 2016/11/04 10:57:42]

Naming (likely connected to my confusion above):

The internal 'magic' values are kNoSourcePosition + kNotInlined. The externally
visible 'magic' values are called known/unknown. That.. doesn't make much sense
to me. If you want special accessors for the 'magic' values - which is probably
a good idea - maybe make them for both. Also, it helps understanding the code if
the naming is consistent.

 
-  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;
 
-  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 Print(std::ostream& out, Code* function) const;
 
-  uint32_t raw() const { return value_; }
+  int ScriptOffset() const { return ScriptOffsetField::decode(value_) - 1; }
+  int InliningId() const { return InliningIdField::decode(value_) - 1; }
 
- private:
-  static const uint32_t kNoPosition = static_cast<uint32_t>(kNoSourcePosition);
-  typedef BitField<uint32_t, 0, 9> InliningIdField;
+  void SetScriptOffset(int script_offset) {
+    if (script_offset >= ScriptOffsetField::kMax)
+      script_offset = kNoSourcePosition;

[vogelheim, 2016/11/04 10:57:41]

What does this if statement guard against? Should it be a DCHECK?

(Generally, we prefer to make bugs explicit, because silently 'fixing' bugs
tends to make vastly more difficult to find & fix them. If this is meant to
protect against a caller putting in non-sensical values, a DCHECK would likely
be better.)

+    DCHECK(script_offset >= kNoSourcePosition);
+    value_ = ScriptOffsetField::update(value_, script_offset + 1);
+  }
+  void SetInliningId(int inlining_id) {
+    if (inlining_id >= InliningIdField::kMax) inlining_id = kNotInlined;

[vogelheim, 2016/11/04 10:57:41]

... as above.

Also, if kNotInlined has a specific meaning (... "refers to the outer ..
function" ...), then using it as a 'default' value for invalid input might not
be a good idea.

+    DCHECK(inlining_id >= kNotInlined);
+    value_ = InliningIdField::update(value_, inlining_id + 1);
+  }
 
-  // Offset from the start of the inlined function.
-  typedef BitField<uint32_t, 9, 23> PositionField;
+  // This encodes a SourcePosition in two non-overlapping 32bit integer ranges.
+  // The encoding is used for the RelocInfo.

[vogelheim, 2016/11/04 10:57:42]

Why the requirement of "non-overlapping" ints? From a cursory look at the
RelocInfo, it seems like they're distinguishably by position. So why also encode
their 'function' in their sign of the encoded value?

[vogelheim, 2016/11/04 10:57:42]

If this is specifically about 32bit integers, why is the type 'int', rather than
int32_t, or something?

[Yang, 2016/11/04 14:32:59]

I would also suggest using uint32_t

[Tobias Tebbi, 2016/11/07 15:10:34]

I don't have a very good reason to do this indeed, I switched to a positional
encoding now.

+  int EncodeScriptOffset() { return ScriptOffset() + 2; }

[vogelheim, 2016/11/04 10:57:41]

const function? (also below)

[vogelheim, 2016/11/04 10:57:41]

Super nitpick, but you're technically constraining the value space further,
since here you forfit the 0 and therefor you can encode one value less.

That's fine IMHO, but if so then your previous checks on the value space should
also check against this limit, not 2**31.

+  int EncodeInliningId() { return -(InliningId() + 2); }
+  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 kNotInlined = -1;
+  STATIC_ASSERT(kNoSourcePosition == -1);
 
-  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;
+
+  // InliningId is in the high bits for better compression in
+  // SourcePositionTable.
+  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 {
+  // position of the inlined call
+  SourcePosition position;
+
+  // references position in DeoptimizationInputData::literals()
+  int inlined_function_id;

[vogelheim, 2016/11/04 10:57:42]

What's the difference/relationship between inlined_function_id and
position.InliningId()?

[Yang, 2016/11/04 14:32:59]

I'm also surprised that this needs to be an additional information. Isn't the
point of the inlining id in SourcePosition to be an index into the literals
array?

[Tobias Tebbi, 2016/11/07 15:10:34]

DeoptimizationInputData::literals() can have different positions than  the
inlining ids, because the existing code removes self-inlinings from it. To avoid
changing this part I don't know the motivations for and to avoid adding another
brittle (and already violated!) global invariant, I kept these separate.

+};
+
+struct SourcePositionInfo {
+  explicit SourcePositionInfo(SourcePosition position) : position(position) {}

[vogelheim, 2016/11/04 10:57:41]

Ah.... honestly unsure about the C++ subtleties here, but does
position(position) do the right thing=

[Yang, 2016/11/04 14:32:59]

Yeah. Let's rename the parameter here for readability.

+
+  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