Skip to content

Commit

Permalink
[wip] changes to support Node.js v16
Browse files Browse the repository at this point in the history
This isn't quite ready for review yet, I just want to get it out there
so people can see that progress is being made. The biggest problem
currently is that this currently breaks support for Node.js v14.

With this diff, our tests are much better. Previously they were entirely
failing on Node 16. Now:

* `stack-test` passes (12/12)
* `inspect-test` mostly passes (55/59)
* `frame-test` somewhat passes (7/25)

There are a lot of changes in this diff. I plan on splitting the commits
before the review, but here's a summary of the changes:

1. `ScopeInfo` is no longer a `FixedArray` as of v8/v8@ecaac329. The
   `Length` field was also removed in v8/v8@f731e13f.
   - The length field shifted everything over by a slot, meaning that a
     bunch of offsets changed.
   - Since `ScopeInfo` is no longer a `FixedArray`, I changed callsites
     to use `HeapObject::LoadFieldValue` rather than `FixedArray::Get`.

2. Changes to V8 calling conventions
   - The arguments adaptor frame no longer exists. Modified
     `frame-test.js` to match.
   - Arguments are no longer pushed "in reverse". Modified
     `JSFrame::LeaParamSlot` to match.

3. Postmortem data fixes
   - `class_Map__constructor_or_backpointer__Object` is now
     `class_Map__constructor_or_back_pointer__Object` (note the extra
     `_`).
   - `class_Script__source__Object` is weirdly _not_ present in Node 16
     but _is_ present in both Node 14 and Node 18. I added a default to
     the correct value of 8.
  • Loading branch information
kvakil committed Aug 14, 2022
1 parent 39b38a9 commit 42c82fe
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 37 deletions.
7 changes: 6 additions & 1 deletion src/llv8-constants.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ void Map::Load() {
kMaybeConstructorOffset =
LoadConstant("class_Map__constructor_or_backpointer__Object",
"class_Map__constructor__Object");
if (kMaybeConstructorOffset == -1) {
kMaybeConstructorOffset =
LoadConstant("class_Map__constructor_or_back_pointer__Object");
}

kInstanceDescriptorsOffset = LoadConstant({
"class_Map__instance_descriptors__DescriptorArray",
"class_Map__instance_descriptors_offset",
Expand Down Expand Up @@ -300,7 +305,7 @@ void Context::Load() {
void Script::Load() {
kNameOffset = LoadConstant("class_Script__name__Object");
kLineOffsetOffset = LoadConstant("class_Script__line_offset__SMI");
kSourceOffset = LoadConstant("class_Script__source__Object");
kSourceOffset = LoadConstant("class_Script__source__Object", 8);
kLineEndsOffset = LoadConstant("class_Script__line_ends__Object");
}

Expand Down
64 changes: 38 additions & 26 deletions src/llv8-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ inline JSFunction JSFrame::GetFunction(Error& err) {

inline int64_t JSFrame::LeaParamSlot(int slot, int count) const {
return raw() + v8()->frame()->kArgsOffset +
(count - slot - 1) * v8()->common()->kPointerSize;
(slot + 1) * v8()->common()->kPointerSize;
}


Expand Down Expand Up @@ -483,7 +483,7 @@ inline CheckedType<int32_t> String::Length(Error& err) {

ACCESSOR(Script, Name, script()->kNameOffset, String)
ACCESSOR(Script, LineOffset, script()->kLineOffsetOffset, Smi)
ACCESSOR(Script, Source, script()->kSourceOffset, HeapObject)
ACCESSOR(Script, Source, script()->kSourceOffset, String)
ACCESSOR(Script, LineEnds, script()->kLineEndsOffset, HeapObject)

ACCESSOR(SharedFunctionInfo, function_data, shared_info()->kFunctionDataOffset,
Expand Down Expand Up @@ -722,21 +722,24 @@ inline CheckedType<uintptr_t> JSTypedArray::GetData() {
inline ScopeInfo::PositionInfo ScopeInfo::MaybePositionInfo(Error& err) {
ScopeInfo::PositionInfo position_info = {
.start_position = 0, .end_position = 0, .is_valid = false};
int proper_index = ContextLocalIndex(err);
auto kPointerSize = v8()->common()->kPointerSize;
int bytes_offset = kPointerSize * ContextLocalIndex(err);
if (err.Fail()) return position_info;

Smi context_local_count = ContextLocalCount(err);
if (err.Fail()) return position_info;
proper_index += context_local_count.GetValue() * 2;
bytes_offset += 2 * kPointerSize * context_local_count.GetValue();

int tries = 5;
while (tries > 0 && proper_index < (Length(err).GetValue() - 1)) {
while (tries > 0) {
err = Error();

Smi maybe_start_position = Get<Smi>(proper_index, err);
Smi maybe_start_position =
HeapObject::LoadFieldValue<Smi>(bytes_offset, err);
if (err.Success() && maybe_start_position.IsSmi(err)) {
proper_index++;
Smi maybe_end_position = Get<Smi>(proper_index, err);
bytes_offset += kPointerSize;
Smi maybe_end_position =
HeapObject::LoadFieldValue<Smi>(bytes_offset, err);
if (err.Success() && maybe_end_position.IsSmi(err)) {
position_info.start_position = maybe_start_position.GetValue();
position_info.end_position = maybe_end_position.GetValue();
Expand All @@ -746,7 +749,7 @@ inline ScopeInfo::PositionInfo ScopeInfo::MaybePositionInfo(Error& err) {
}

tries--;
proper_index++;
bytes_offset += kPointerSize;
}
return position_info;
}
Expand Down Expand Up @@ -1091,19 +1094,26 @@ inline Value Context::ContextSlot(int index, Error& err) {
}

inline Smi ScopeInfo::ParameterCount(Error& err) {
return FixedArray::Get<Smi>(v8()->scope_info()->kParameterCountOffset, err);
Smi res = HeapObject::LoadFieldValue<Smi>(
v8()->scope_info()->kParameterCountOffset * v8()->common()->kPointerSize,
err);
return res;
}

inline Smi ScopeInfo::StackLocalCount(Error& err) {
if (v8()->scope_info()->kStackLocalCountOffset == -1) {
return Smi(v8(), 0);
}
return FixedArray::Get<Smi>(v8()->scope_info()->kStackLocalCountOffset, err);
return HeapObject::LoadFieldValue<Smi>(
v8()->scope_info()->kStackLocalCountOffset * v8()->common()->kPointerSize,
err);
}

inline Smi ScopeInfo::ContextLocalCount(Error& err) {
return FixedArray::Get<Smi>(v8()->scope_info()->kContextLocalCountOffset,
err);
return HeapObject::LoadFieldValue<Smi>(
(v8()->scope_info()->kContextLocalCountOffset + 1) *
v8()->common()->kPointerSize,
err);
}

inline int ScopeInfo::ContextLocalIndex(Error& err) {
Expand All @@ -1122,30 +1132,32 @@ inline int ScopeInfo::ContextLocalIndex(Error& err) {
}

inline String ScopeInfo::ContextLocalName(int index, Error& err) {
int proper_index = ContextLocalIndex(err) + index;
int proper_index =
(ContextLocalIndex(err) + index + 1) * v8()->common()->kPointerSize;
if (err.Fail()) return String();
return FixedArray::Get<String>(proper_index, err);
return HeapObject::LoadFieldValue<String>(proper_index, err);
}

inline HeapObject ScopeInfo::MaybeFunctionName(Error& err) {
int proper_index = ContextLocalIndex(err);
if (err.Fail()) return HeapObject();

Smi context_local_count = ContextLocalCount(err);
if (err.Fail()) return HeapObject();
proper_index += context_local_count.GetValue() * 2;

// NOTE(mmarchini): FunctionName can be stored either in the first, second or
// third slot after ContextLocalCount. Since there are missing postmortem
// metadata to determine in which slot its being stored for the present
// ScopeInfo, we try to find it heuristically.
int tries = 3;
auto kPointerSize = v8()->common()->kPointerSize;
HeapObject likely_function_name;
while (tries > 0 && proper_index < Length(err).GetValue()) {
int bytes_offset = kPointerSize * ContextLocalIndex(err);
if (err.Fail()) return likely_function_name;

Smi context_local_count = ContextLocalCount(err);
if (err.Fail()) return likely_function_name;
bytes_offset += 2 * kPointerSize * context_local_count.GetValue();

int tries = 5;
while (tries > 0) {
err = Error();

HeapObject maybe_function_name =
FixedArray::Get<HeapObject>(proper_index, err);
HeapObject::LoadFieldValue<HeapObject>(bytes_offset, err);
if (err.Success() && String::IsString(v8(), maybe_function_name, err)) {
likely_function_name = maybe_function_name;
if (*String(likely_function_name).Length(err) > 0) {
Expand All @@ -1154,7 +1166,7 @@ inline HeapObject ScopeInfo::MaybeFunctionName(Error& err) {
}

tries--;
proper_index++;
bytes_offset += kPointerSize;
}

if (likely_function_name.Check()) {
Expand Down
5 changes: 2 additions & 3 deletions src/llv8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ void Script::GetLineColumnFromPos(int64_t pos, int64_t& line, int64_t& column,
line = 0;
column = 0;

HeapObject source = Source(err);
String source = Source(err);
if (err.Fail()) return;

int64_t type = source.GetType(err);
Expand All @@ -496,8 +496,7 @@ void Script::GetLineColumnFromPos(int64_t pos, int64_t& line, int64_t& column,
return;
}

String str(source);
std::string source_str = str.ToString(err);
std::string source_str = source.ToString(err);
int64_t limit = source_str.length();
if (limit > pos) limit = pos;

Expand Down
6 changes: 3 additions & 3 deletions src/llv8.h
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class Script : public HeapObject {

inline String Name(Error& err);
inline Smi LineOffset(Error& err);
inline HeapObject Source(Error& err);
inline String Source(Error& err);
inline HeapObject LineEnds(Error& err);

void GetLines(uint64_t start_line, std::string lines[], uint64_t line_limit,
Expand Down Expand Up @@ -509,9 +509,9 @@ class NameDictionary : public FixedArray {
inline int64_t Length(Error& err);
};

class ScopeInfo : public FixedArray {
class ScopeInfo : public HeapObject {
public:
V8_VALUE_DEFAULT_METHODS(ScopeInfo, FixedArray)
V8_VALUE_DEFAULT_METHODS(ScopeInfo, HeapObject)

struct PositionInfo {
int64_t start_position;
Expand Down
8 changes: 4 additions & 4 deletions test/plugin/frame-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ tape('v8 stack', async (t) => {
t.ok(lines.length > 4, 'frame count');

lines = lines.filter((s) => !/<builtin>|<stub>/.test(s));
const exit = lines[5];
const crasher = lines[4];
const adapter = lines[3];
const exit = lines[4];
const crasher = lines[3];
const fnInferredName = lines[2];
const fnInferredNamePrototype = lines[1];
const fnFunctionName = lines[0];
t.ok(/<exit>/.test(exit), 'exit frame');
t.ok(/crasher/.test(crasher), 'crasher frame');
t.ok(/<adaptor>/.test(adapter), 'arguments adapter frame');
if (nodejsVersion()[0] < 16)
t.ok(/<adaptor>/.test(adapter), 'arguments adapter frame');
if (nodejsVersion()[0] < 12)
t.ok(/\sfnInferredName\(/.test(fnInferredName), 'fnInferredName frame');
t.ok(/\sModule.fnInferredNamePrototype\(/.test(fnInferredNamePrototype),
Expand Down

0 comments on commit 42c82fe

Please sign in to comment.