Skip to content

Commit

Permalink
deps: cherry-pick ba752ea from upstream V8
Browse files Browse the repository at this point in the history
Original commit message:

    [cpu-profiler] Use instruction start as the key for the CodeMap

    Previously we used the start address of the AbstractCode object. This
    doesn't make sense for off-heap builtins, where the code isn't contained
    in the object itself. It also hides other potential problems - sometimes
    the sample.pc is inside the AbstractCode object header - this is
    never valid.

    There were a few changes necessary to make this happen:
      - Change the interface of CodeMoveEvent. Now 'to' and 'from' are both
        AbstractCode objects, which is nice because many users were taking
        'to' and adding the header offset to it to try and find the
        instruction start address. This isn't valid for off-heap builtins.
      - Fix a bug in CodeMap::MoveCode where we didn't update the CodeEntry
        object to reflect the new instruction_start.
      - Rename the 'start' field in all of the CodeEventRecord sub-classes
        to make it clear that this is the address of the first instruction.
      - Fix the confusion in RecordTickSample between 'tos' and 'pc' which
        caused pc_offset to be calculated incorrectly.

    Bug: v8:7983
    Change-Id: I3e9dddf74e4b2e96a5f031d216ef7008d6f184d1
    Reviewed-on: https://chromium-review.googlesource.com/1148457
    Commit-Queue: Peter Marshall <petermarshall@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54749}

Refs: v8/v8@ba752ea

PR-URL: #21983
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
targos committed Sep 7, 2018
1 parent 56d7411 commit 7766baf
Show file tree
Hide file tree
Showing 18 changed files with 102 additions and 88 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.5',
'v8_embedder_string': '-node.6',

# Enable disassembler for `--print-code` v8 options
'v8_enable_disassembler': 1,
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/src/code-events.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class CodeEventListener {
virtual void GetterCallbackEvent(Name* name, Address entry_point) = 0;
virtual void SetterCallbackEvent(Name* name, Address entry_point) = 0;
virtual void RegExpCodeCreateEvent(AbstractCode* code, String* source) = 0;
virtual void CodeMoveEvent(AbstractCode* from, Address to) = 0;
virtual void CodeMoveEvent(AbstractCode* from, AbstractCode* to) = 0;
virtual void SharedFunctionInfoMoveEvent(Address from, Address to) = 0;
virtual void CodeMovingGCEvent() = 0;
virtual void CodeDisableOptEvent(AbstractCode* code,
Expand Down Expand Up @@ -154,7 +154,7 @@ class CodeEventDispatcher {
void RegExpCodeCreateEvent(AbstractCode* code, String* source) {
CODE_EVENT_DISPATCH(RegExpCodeCreateEvent(code, source));
}
void CodeMoveEvent(AbstractCode* from, Address to) {
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) {
CODE_EVENT_DISPATCH(CodeMoveEvent(from, to));
}
void SharedFunctionInfoMoveEvent(Address from, Address to) {
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/heap/mark-compact.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1149,7 +1149,7 @@ class ProfilingMigrationObserver final : public MigrationObserver {
int size) final {
if (dest == CODE_SPACE || (dest == OLD_SPACE && dst->IsBytecodeArray())) {
PROFILE(heap_->isolate(),
CodeMoveEvent(AbstractCode::cast(src), dst->address()));
CodeMoveEvent(AbstractCode::cast(src), AbstractCode::cast(dst)));
}
heap_->OnMoveEvent(dst, src, size);
}
Expand Down
25 changes: 10 additions & 15 deletions deps/v8/src/log.cc
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ class PerfBasicLogger : public CodeEventLogger {
explicit PerfBasicLogger(Isolate* isolate);
~PerfBasicLogger() override;

void CodeMoveEvent(AbstractCode* from, Address to) override {}
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override {}
void CodeDisableOptEvent(AbstractCode* code,
SharedFunctionInfo* shared) override {}

Expand Down Expand Up @@ -496,7 +496,7 @@ class LowLevelLogger : public CodeEventLogger {
LowLevelLogger(Isolate* isolate, const char* file_name);
~LowLevelLogger() override;

void CodeMoveEvent(AbstractCode* from, Address to) override;
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override;
void CodeDisableOptEvent(AbstractCode* code,
SharedFunctionInfo* shared) override {}
void SnapshotPositionEvent(HeapObject* obj, int pos);
Expand Down Expand Up @@ -615,11 +615,10 @@ void LowLevelLogger::LogRecordedBuffer(const wasm::WasmCode* code,
code->instructions().length());
}

void LowLevelLogger::CodeMoveEvent(AbstractCode* from, Address to) {
void LowLevelLogger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) {
CodeMoveStruct event;
event.from_address = from->InstructionStart();
size_t header_size = from->InstructionStart() - from->address();
event.to_address = to + header_size;
event.to_address = to->InstructionStart();
LogWriteStruct(event);
}

Expand All @@ -641,7 +640,7 @@ class JitLogger : public CodeEventLogger {
public:
JitLogger(Isolate* isolate, JitCodeEventHandler code_event_handler);

void CodeMoveEvent(AbstractCode* from, Address to) override;
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override;
void CodeDisableOptEvent(AbstractCode* code,
SharedFunctionInfo* shared) override {}
void AddCodeLinePosInfoEvent(void* jit_handler_data, int pc_offset,
Expand Down Expand Up @@ -700,7 +699,7 @@ void JitLogger::LogRecordedBuffer(const wasm::WasmCode* code, const char* name,
code_event_handler_(&event);
}

void JitLogger::CodeMoveEvent(AbstractCode* from, Address to) {
void JitLogger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) {
base::LockGuard<base::Mutex> guard(&logger_mutex_);

JitCodeEvent event;
Expand All @@ -709,12 +708,7 @@ void JitLogger::CodeMoveEvent(AbstractCode* from, Address to) {
from->IsCode() ? JitCodeEvent::JIT_CODE : JitCodeEvent::BYTE_CODE;
event.code_start = reinterpret_cast<void*>(from->InstructionStart());
event.code_len = from->InstructionSize();

// Calculate the header size.
const size_t header_size = from->InstructionStart() - from->address();

// Calculate the new start address of the instructions.
event.new_code_start = reinterpret_cast<void*>(to + header_size);
event.new_code_start = reinterpret_cast<void*>(to->InstructionStart());
event.isolate = reinterpret_cast<v8::Isolate*>(isolate_);

code_event_handler_(&event);
Expand Down Expand Up @@ -1431,9 +1425,10 @@ void Logger::RegExpCodeCreateEvent(AbstractCode* code, String* source) {
msg.WriteToLogFile();
}

void Logger::CodeMoveEvent(AbstractCode* from, Address to) {
void Logger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) {
if (!is_listening_to_code_events()) return;
MoveEventInternal(CodeEventListener::CODE_MOVE_EVENT, from->address(), to);
MoveEventInternal(CodeEventListener::CODE_MOVE_EVENT, from->address(),
to->address());
}

namespace {
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/src/log.h
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ class Logger : public CodeEventListener {
// Emits a code create event for a RegExp.
void RegExpCodeCreateEvent(AbstractCode* code, String* source);
// Emits a code move event.
void CodeMoveEvent(AbstractCode* from, Address to);
void CodeMoveEvent(AbstractCode* from, AbstractCode* to);
// Emits a code line info record event.
void CodeLinePosInfoRecordEvent(Address code_start,
ByteArray* source_position_table);
Expand Down Expand Up @@ -486,7 +486,7 @@ class ExternalCodeEventListener : public CodeEventListener {
void GetterCallbackEvent(Name* name, Address entry_point) override {}
void SetterCallbackEvent(Name* name, Address entry_point) override {}
void SharedFunctionInfoMoveEvent(Address from, Address to) override {}
void CodeMoveEvent(AbstractCode* from, Address to) override {}
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override {}
void CodeDisableOptEvent(AbstractCode* code,
SharedFunctionInfo* shared) override {}
void CodeMovingGCEvent() override {}
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/perf-jit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ void PerfJitLogger::LogWriteUnwindingInfo(Code* code) {
LogWriteBytes(padding_bytes, static_cast<int>(padding_size));
}

void PerfJitLogger::CodeMoveEvent(AbstractCode* from, Address to) {
void PerfJitLogger::CodeMoveEvent(AbstractCode* from, AbstractCode* to) {
// We may receive a CodeMove event if a BytecodeArray object moves. Otherwise
// code relocation is not supported.
CHECK(from->IsBytecodeArray());
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/src/perf-jit.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class PerfJitLogger : public CodeEventLogger {
explicit PerfJitLogger(Isolate* isolate);
virtual ~PerfJitLogger();

void CodeMoveEvent(AbstractCode* from, Address to) override;
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override;
void CodeDisableOptEvent(AbstractCode* code,
SharedFunctionInfo* shared) override {}

Expand Down Expand Up @@ -120,7 +120,7 @@ class PerfJitLogger : public CodeEventLogger {
public:
explicit PerfJitLogger(Isolate* isolate) : CodeEventLogger(isolate) {}

void CodeMoveEvent(AbstractCode* from, Address to) override {
void CodeMoveEvent(AbstractCode* from, AbstractCode* to) override {
UNIMPLEMENTED();
}

Expand Down
10 changes: 5 additions & 5 deletions deps/v8/src/profiler/cpu-profiler-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,25 @@ namespace v8 {
namespace internal {

void CodeCreateEventRecord::UpdateCodeMap(CodeMap* code_map) {
code_map->AddCode(start, entry, size);
code_map->AddCode(instruction_start, entry, instruction_size);
}


void CodeMoveEventRecord::UpdateCodeMap(CodeMap* code_map) {
code_map->MoveCode(from, to);
code_map->MoveCode(from_instruction_start, to_instruction_start);
}


void CodeDisableOptEventRecord::UpdateCodeMap(CodeMap* code_map) {
CodeEntry* entry = code_map->FindEntry(start);
CodeEntry* entry = code_map->FindEntry(instruction_start);
if (entry != nullptr) {
entry->set_bailout_reason(bailout_reason);
}
}


void CodeDeoptEventRecord::UpdateCodeMap(CodeMap* code_map) {
CodeEntry* entry = code_map->FindEntry(start);
CodeEntry* entry = code_map->FindEntry(instruction_start);
if (entry == nullptr) return;
std::vector<CpuProfileDeoptFrame> frames_vector(
deopt_frames, deopt_frames + deopt_frame_count);
Expand All @@ -44,7 +44,7 @@ void CodeDeoptEventRecord::UpdateCodeMap(CodeMap* code_map) {


void ReportBuiltinEventRecord::UpdateCodeMap(CodeMap* code_map) {
CodeEntry* entry = code_map->FindEntry(start);
CodeEntry* entry = code_map->FindEntry(instruction_start);
if (!entry) {
// Code objects for builtins should already have been added to the map but
// some of them have been filtered out by CpuProfiler.
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/profiler/cpu-profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ void CpuProfiler::LogBuiltins() {
CodeEventsContainer evt_rec(CodeEventRecord::REPORT_BUILTIN);
ReportBuiltinEventRecord* rec = &evt_rec.ReportBuiltinEventRecord_;
Builtins::Name id = static_cast<Builtins::Name>(i);
rec->start = builtins->builtin(id)->address();
rec->instruction_start = builtins->builtin(id)->InstructionStart();
rec->builtin_id = id;
processor_->Enqueue(evt_rec);
}
Expand Down
14 changes: 7 additions & 7 deletions deps/v8/src/profiler/cpu-profiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,26 @@ class CodeEventRecord {

class CodeCreateEventRecord : public CodeEventRecord {
public:
Address start;
Address instruction_start;
CodeEntry* entry;
unsigned size;
unsigned instruction_size;

V8_INLINE void UpdateCodeMap(CodeMap* code_map);
};


class CodeMoveEventRecord : public CodeEventRecord {
public:
Address from;
Address to;
Address from_instruction_start;
Address to_instruction_start;

V8_INLINE void UpdateCodeMap(CodeMap* code_map);
};


class CodeDisableOptEventRecord : public CodeEventRecord {
public:
Address start;
Address instruction_start;
const char* bailout_reason;

V8_INLINE void UpdateCodeMap(CodeMap* code_map);
Expand All @@ -81,7 +81,7 @@ class CodeDisableOptEventRecord : public CodeEventRecord {

class CodeDeoptEventRecord : public CodeEventRecord {
public:
Address start;
Address instruction_start;
const char* deopt_reason;
int deopt_id;
Address pc;
Expand All @@ -95,7 +95,7 @@ class CodeDeoptEventRecord : public CodeEventRecord {

class ReportBuiltinEventRecord : public CodeEventRecord {
public:
Address start;
Address instruction_start;
Builtins::Name builtin_id;

V8_INLINE void UpdateCodeMap(CodeMap* code_map);
Expand Down
41 changes: 28 additions & 13 deletions deps/v8/src/profiler/profile-generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,8 @@ void CodeMap::AddCode(Address addr, CodeEntry* entry, unsigned size) {
ClearCodesInRange(addr, addr + size);
unsigned index = AddCodeEntry(addr, entry);
code_map_.emplace(addr, CodeEntryMapInfo{index, size});
DCHECK(entry->instruction_start() == kNullAddress ||
addr == entry->instruction_start());
}

void CodeMap::ClearCodesInRange(Address start, Address end) {
Expand All @@ -550,8 +552,14 @@ CodeEntry* CodeMap::FindEntry(Address addr) {
auto it = code_map_.upper_bound(addr);
if (it == code_map_.begin()) return nullptr;
--it;
Address end_address = it->first + it->second.size;
return addr < end_address ? entry(it->second.index) : nullptr;
Address start_address = it->first;
Address end_address = start_address + it->second.size;
CodeEntry* ret = addr < end_address ? entry(it->second.index) : nullptr;
if (ret && ret->instruction_start() != kNullAddress) {
DCHECK_EQ(start_address, ret->instruction_start());
DCHECK(addr >= start_address && addr < end_address);
}
return ret;
}

void CodeMap::MoveCode(Address from, Address to) {
Expand All @@ -563,6 +571,9 @@ void CodeMap::MoveCode(Address from, Address to) {
DCHECK(from + info.size <= to || to + info.size <= from);
ClearCodesInRange(to, to + info.size);
code_map_.emplace(to, info);

CodeEntry* entry = code_entries_[info.index].entry;
entry->set_instruction_start(to);
}

unsigned CodeMap::AddCodeEntry(Address start, CodeEntry* entry) {
Expand Down Expand Up @@ -693,26 +704,29 @@ void ProfileGenerator::RecordTickSample(const TickSample& sample) {
if (sample.pc != nullptr) {
if (sample.has_external_callback && sample.state == EXTERNAL) {
// Don't use PC when in external callback code, as it can point
// inside callback's code, and we will erroneously report
// inside a callback's code, and we will erroneously report
// that a callback calls itself.
stack_trace.push_back(
{FindEntry(reinterpret_cast<Address>(sample.external_callback_entry)),
no_line_info});
} else {
CodeEntry* pc_entry = FindEntry(reinterpret_cast<Address>(sample.pc));
// If there is no pc_entry we're likely in native code.
// Find out, if top of stack was pointing inside a JS function
// meaning that we have encountered a frameless invocation.
Address attributed_pc = reinterpret_cast<Address>(sample.pc);
CodeEntry* pc_entry = FindEntry(attributed_pc);
// If there is no pc_entry, we're likely in native code. Find out if the
// top of the stack (the return address) was pointing inside a JS
// function, meaning that we have encountered a frameless invocation.
if (!pc_entry && !sample.has_external_callback) {
pc_entry = FindEntry(reinterpret_cast<Address>(sample.tos));
attributed_pc = reinterpret_cast<Address>(sample.tos);
pc_entry = FindEntry(attributed_pc);
}
// If pc is in the function code before it set up stack frame or after the
// frame was destroyed SafeStackFrameIterator incorrectly thinks that
// ebp contains return address of the current function and skips caller's
// frame. Check for this case and just skip such samples.
// frame was destroyed, SafeStackFrameIterator incorrectly thinks that
// ebp contains the return address of the current function and skips the
// caller's frame. Check for this case and just skip such samples.
if (pc_entry) {
int pc_offset = static_cast<int>(reinterpret_cast<Address>(sample.pc) -
pc_entry->instruction_start());
int pc_offset =
static_cast<int>(attributed_pc - pc_entry->instruction_start());
DCHECK_GE(pc_offset, 0);
src_line = pc_entry->GetSourceLine(pc_offset);
if (src_line == v8::CpuProfileNode::kNoLineNumberInfo) {
src_line = pc_entry->line_number();
Expand Down Expand Up @@ -744,6 +758,7 @@ void ProfileGenerator::RecordTickSample(const TickSample& sample) {
// Find out if the entry has an inlining stack associated.
int pc_offset =
static_cast<int>(stack_pos - entry->instruction_start());
DCHECK_GE(pc_offset, 0);
const std::vector<std::unique_ptr<CodeEntry>>* inline_stack =
entry->GetInlineStack(pc_offset);
if (inline_stack) {
Expand Down
2 changes: 2 additions & 0 deletions deps/v8/src/profiler/profile-generator.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ class CodeEntry {
const std::vector<std::unique_ptr<CodeEntry>>* GetInlineStack(
int pc_offset) const;

void set_instruction_start(Address start) { instruction_start_ = start; }
Address instruction_start() const { return instruction_start_; }

CodeEventListener::LogEventsAndTags tag() const {
return TagField::decode(bit_field_);
}
Expand Down
Loading

0 comments on commit 7766baf

Please sign in to comment.