Revert "[wasm][diagnostics] Support WasmCode in gdb JIT integration"

ngzhian · V8 LUCI CQ · commit b9c7fc2786de · 2021-08-12T23:59:14.000Z
This reverts commit a3b2c4e. Reason for revert: UBSan https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket/8839060153390139249/+/u/Check/gdbjit Original change's description: > [wasm][diagnostics] Support WasmCode in gdb JIT integration > > - Add new enum WASM_CODE to JitCodeEvent::CodeType > - Use AddressRegion instead of AddressRange (remove the latter) > - Change CodeDescription constructor to take an AddressRegion, > both JIT_CODE and WASM_CODE use this > - Add a simple mjsunit test that sets --gdbjit to check that > we don't crash. > - Add a api test for adding WASM_CODE > > Bug: v8:11908 > Change-Id: I6e87fadc2df67978144d78caf9800c3982bc3705 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3067754 > Reviewed-by: Adam Klein <adamk@chromium.org> > Reviewed-by: Clemens Backes <clemensb@chromium.org> > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> > Commit-Queue: Zhi An Ng <zhin@chromium.org> > Cr-Commit-Position: refs/heads/master@{#76271} Bug: v8:11908 Change-Id: Ic1a74a9239e8ef6107efd36f61c089ae6bfc5b6c No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3093365 Auto-Submit: Zhi An Ng <zhin@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#76274}
diff --git a/include/v8.h b/include/v8.h
@@ -7848,13 +7848,13 @@ struct JitCodeEvent {
   // statement, and is used to indicate possible break locations.
   enum PositionType { POSITION, STATEMENT_POSITION };
 
-  // There are three different kinds of CodeType, one for JIT code generated
-  // by the optimizing compiler, one for byte code generated for the
-  // interpreter, and one for code generated from Wasm. For JIT_CODE and
-  // WASM_CODE, |code_start| points to the beginning of jitted assembly code,
-  // while for BYTE_CODE events, |code_start| points to the first bytecode of
-  // the interpreted function.
-  enum CodeType { BYTE_CODE, JIT_CODE, WASM_CODE };
+  // There are two different kinds of JitCodeEvents, one for JIT code generated
+  // by the optimizing compiler, and one for byte code generated for the
+  // interpreter.  For JIT_CODE events, the |code_start| member of the event
+  // points to the beginning of jitted assembly code, while for BYTE_CODE
+  // events, |code_start| points to the first bytecode of the interpreted
+  // function.
+  enum CodeType { BYTE_CODE, JIT_CODE };
 
   // Type of event.
   EventType type;
diff --git a/src/diagnostics/gdb-jit.cc b/src/diagnostics/gdb-jit.cc
@@ -10,7 +10,6 @@
 
 #include "include/v8.h"
 #include "src/api/api-inl.h"
-#include "src/base/address-region.h"
 #include "src/base/bits.h"
 #include "src/base/hashmap.h"
 #include "src/base/platform/platform.h"
@@ -897,20 +896,17 @@ class CodeDescription {
   };
 #endif
 
-  CodeDescription(const char* name, base::AddressRegion region,
-                  SharedFunctionInfo shared, LineInfo* lineinfo,
-                  bool is_function)
-      : name_(name),
-        shared_info_(shared),
-        lineinfo_(lineinfo),
-        is_function_(is_function),
-        code_region_(region) {}
+  CodeDescription(const char* name, Code code, SharedFunctionInfo shared,
+                  LineInfo* lineinfo)
+      : name_(name), code_(code), shared_info_(shared), lineinfo_(lineinfo) {}
 
   const char* name() const { return name_; }
 
   LineInfo* lineinfo() const { return lineinfo_; }
 
-  bool is_function() const { return is_function_; }
+  bool is_function() const {
+    return CodeKindIsOptimizedJSFunction(code_.kind());
+  }
 
   bool has_scope_info() const { return !shared_info_.is_null(); }
 
@@ -919,11 +915,15 @@ class CodeDescription {
     return shared_info_.scope_info();
   }
 
-  uintptr_t CodeStart() const { return code_region_.begin(); }
+  uintptr_t CodeStart() const {
+    return static_cast<uintptr_t>(code_.InstructionStart());
+  }
 
-  uintptr_t CodeEnd() const { return code_region_.end(); }
+  uintptr_t CodeEnd() const {
+    return static_cast<uintptr_t>(code_.InstructionEnd());
+  }
 
-  uintptr_t CodeSize() const { return code_region_.size(); }
+  uintptr_t CodeSize() const { return CodeEnd() - CodeStart(); }
 
   bool has_script() {
     return !shared_info_.is_null() && shared_info_.script().IsScript();
@@ -933,8 +933,6 @@ class CodeDescription {
 
   bool IsLineInfoAvailable() { return lineinfo_ != nullptr; }
 
-  base::AddressRegion region() { return code_region_; }
-
 #if V8_TARGET_ARCH_X64
   uintptr_t GetStackStateStartAddress(StackState state) const {
     DCHECK(state < STACK_STATE_MAX);
@@ -967,10 +965,9 @@ class CodeDescription {
 
  private:
   const char* name_;
+  Code code_;
   SharedFunctionInfo shared_info_;
   LineInfo* lineinfo_;
-  bool is_function_;
-  base::AddressRegion code_region_;
 #if V8_TARGET_ARCH_X64
   uintptr_t stack_state_start_addresses_[STACK_STATE_MAX];
 #endif
@@ -1819,17 +1816,26 @@ static JITCodeEntry* CreateELFObject(CodeDescription* desc, Isolate* isolate) {
   return CreateCodeEntry(reinterpret_cast<Address>(w.buffer()), w.position());
 }
 
-// Like base::AddressRegion::StartAddressLess but also compares |end| when
-// |begin| is equal.
-struct AddressRegionLess {
-  bool operator()(const base::AddressRegion& a,
-                  const base::AddressRegion& b) const {
-    if (a.begin() == b.begin()) return a.end() < b.end();
-    return a.begin() < b.begin();
+struct AddressRange {
+  Address start;
+  Address end;
+};
+
+struct AddressRangeLess {
+  bool operator()(const AddressRange& a, const AddressRange& b) const {
+    if (a.start == b.start) return a.end < b.end;
+    return a.start < b.start;
   }
 };
 
-using CodeMap = std::map<base::AddressRegion, JITCodeEntry*, AddressRegionLess>;
+struct CodeMapConfig {
+  using Key = AddressRange;
+  using Value = JITCodeEntry*;
+  using Less = AddressRangeLess;
+};
+
+using CodeMap =
+    std::map<CodeMapConfig::Key, CodeMapConfig::Value, CodeMapConfig::Less>;
 
 static CodeMap* GetCodeMap() {
   // TODO(jgruber): Don't leak.
@@ -1903,37 +1909,36 @@ static void AddUnwindInfo(CodeDescription* desc) {
 
 static base::LazyMutex mutex = LAZY_MUTEX_INITIALIZER;
 
-// Remove entries from the map that intersect the given address region,
+// Remove entries from the map that intersect the given address range,
 // and deregister them from GDB.
-static void RemoveJITCodeEntries(CodeMap* map,
-                                 const base::AddressRegion region) {
-  DCHECK_LT(region.begin(), region.end());
+static void RemoveJITCodeEntries(CodeMap* map, const AddressRange& range) {
+  DCHECK(range.start < range.end);
 
   if (map->empty()) return;
 
   // Find the first overlapping entry.
 
-  // If successful, points to the first element not less than `region`. The
+  // If successful, points to the first element not less than `range`. The
   // returned iterator has the key in `first` and the value in `second`.
-  auto it = map->lower_bound(region);
+  auto it = map->lower_bound(range);
   auto start_it = it;
 
   if (it == map->end()) {
     start_it = map->begin();
   } else if (it != map->begin()) {
     for (--it; it != map->begin(); --it) {
-      if ((*it).first.end() <= region.begin()) break;
+      if ((*it).first.end <= range.start) break;
       start_it = it;
     }
   }
 
-  DCHECK_NE(start_it, map->end());
+  DCHECK(start_it != map->end());
 
-  // Find the first non-overlapping entry after `region`.
+  // Find the first non-overlapping entry after `range`.
 
-  const auto end_it = map->lower_bound({region.end(), 0});
+  const auto end_it = map->lower_bound({range.end, 0});
 
-  // Evict intersecting regions.
+  // Evict intersecting ranges.
 
   if (std::distance(start_it, end_it) < 1) return;  // No overlapping entries.
 
@@ -1947,7 +1952,7 @@ static void RemoveJITCodeEntries(CodeMap* map,
 }
 
 // Insert the entry into the map and register it with GDB.
-static void AddJITCodeEntry(CodeMap* map, const base::AddressRegion region,
+static void AddJITCodeEntry(CodeMap* map, const AddressRange& range,
                             JITCodeEntry* entry, bool dump_if_enabled,
                             const char* name_hint) {
 #if defined(DEBUG) && !V8_OS_WIN
@@ -1964,28 +1969,32 @@ static void AddJITCodeEntry(CodeMap* map, const base::AddressRegion region,
   }
 #endif
 
-  auto result = map->emplace(region, entry);
+  auto result = map->emplace(range, entry);
   DCHECK(result.second);  // Insertion happened.
   USE(result);
 
   RegisterCodeEntry(entry);
 }
 
-static void AddCode(const char* name, base::AddressRegion region,
-                    SharedFunctionInfo shared, LineInfo* lineinfo,
-                    Isolate* isolate, bool is_function) {
+static void AddCode(const char* name, Code code, SharedFunctionInfo shared,
+                    LineInfo* lineinfo) {
   DisallowGarbageCollection no_gc;
-  CodeDescription code_desc(name, region, shared, lineinfo, is_function);
 
   CodeMap* code_map = GetCodeMap();
-  RemoveJITCodeEntries(code_map, region);
+  AddressRange range;
+  range.start = code.address();
+  range.end = code.address() + code.CodeSize();
+  RemoveJITCodeEntries(code_map, range);
+
+  CodeDescription code_desc(name, code, shared, lineinfo);
 
   if (!FLAG_gdbjit_full && !code_desc.IsLineInfoAvailable()) {
     delete lineinfo;
     return;
   }
 
   AddUnwindInfo(&code_desc);
+  Isolate* isolate = code.GetIsolate();
   JITCodeEntry* entry = CreateELFObject(&code_desc, isolate);
 
   delete lineinfo;
@@ -2001,40 +2010,25 @@ static void AddCode(const char* name, base::AddressRegion region,
       should_dump = (name_hint != nullptr);
     }
   }
-  AddJITCodeEntry(code_map, region, entry, should_dump, name_hint);
+  AddJITCodeEntry(code_map, range, entry, should_dump, name_hint);
 }
 
 void EventHandler(const v8::JitCodeEvent* event) {
   if (!FLAG_gdbjit) return;
-  if ((event->code_type != v8::JitCodeEvent::JIT_CODE) &&
-      (event->code_type != v8::JitCodeEvent::WASM_CODE)) {
-    return;
-  }
+  if (event->code_type != v8::JitCodeEvent::JIT_CODE) return;
   base::MutexGuard lock_guard(mutex.Pointer());
   switch (event->type) {
     case v8::JitCodeEvent::CODE_ADDED: {
       Address addr = reinterpret_cast<Address>(event->code_start);
+      Isolate* isolate = reinterpret_cast<Isolate*>(event->isolate);
+      Code code = isolate->heap()->GcSafeFindCodeForInnerPointer(addr);
       LineInfo* lineinfo = GetLineInfo(addr);
       std::string event_name(event->name.str, event->name.len);
       // It's called UnboundScript in the API but it's a SharedFunctionInfo.
       SharedFunctionInfo shared = event->script.IsEmpty()
                                       ? SharedFunctionInfo()
                                       : *Utils::OpenHandle(*event->script);
-      Isolate* isolate = reinterpret_cast<Isolate*>(event->isolate);
-      bool is_function = false;
-      // TODO(zhin): See if we can use event->code_type to determine
-      // is_function, the difference currently is that JIT_CODE is SparkPlug,
-      // TurboProp, TurboFan, whereas CodeKindIsOptimizedJSFunction is only
-      // TurboProp and TurboFan. is_function is used for AddUnwindInfo, and the
-      // prologue that SP generates probably matches that of TP/TF, so we can
-      // use event->code_type here instead of finding the Code.
-      // TODO(zhin): Rename is_function to be more accurate.
-      if (event->code_type == v8::JitCodeEvent::JIT_CODE) {
-        Code code = isolate->heap()->GcSafeFindCodeForInnerPointer(addr);
-        is_function = CodeKindIsOptimizedJSFunction(code.kind());
-      }
-      AddCode(event_name.c_str(), {addr, event->code_len}, shared, lineinfo,
-              isolate, is_function);
+      AddCode(event_name.c_str(), code, shared, lineinfo);
       break;
     }
     case v8::JitCodeEvent::CODE_MOVED:
diff --git a/src/logging/log.cc b/src/logging/log.cc
@@ -730,7 +730,7 @@ void JitLogger::LogRecordedBuffer(const wasm::WasmCode* code, const char* name,
                                   int length) {
   JitCodeEvent event = {};
   event.type = JitCodeEvent::CODE_ADDED;
-  event.code_type = JitCodeEvent::WASM_CODE;
+  event.code_type = JitCodeEvent::JIT_CODE;
   event.code_start = code->instructions().begin();
   event.code_len = code->instructions().length();
   event.name.str = name;
@@ -1558,14 +1558,12 @@ void Logger::CodeLinePosInfoRecordEvent(Address code_start,
   CodeLinePosEvent(*jit_logger_, code_start, iter, code_type);
 }
 
-#if V8_ENABLE_WEBASSEMBLY
-void Logger::WasmCodeLinePosInfoRecordEvent(
+void Logger::CodeLinePosInfoRecordEvent(
     Address code_start, base::Vector<const byte> source_position_table) {
   if (!jit_logger_) return;
   SourcePositionTableIterator iter(source_position_table);
-  CodeLinePosEvent(*jit_logger_, code_start, iter, JitCodeEvent::WASM_CODE);
+  CodeLinePosEvent(*jit_logger_, code_start, iter, JitCodeEvent::JIT_CODE);
 }
-#endif  // V8_ENABLE_WEBASSEMBLY
 
 void Logger::CodeNameEvent(Address addr, int pos, const char* code_name) {
   if (code_name == nullptr) return;  // Not a code object.
diff --git a/src/logging/log.h b/src/logging/log.h
@@ -241,10 +241,8 @@ class Logger : public CodeEventListener {
   void CodeLinePosInfoRecordEvent(Address code_start,
                                   ByteArray source_position_table,
                                   JitCodeEvent::CodeType code_type);
-#if V8_ENABLE_WEBASSEMBLY
-  void WasmCodeLinePosInfoRecordEvent(
+  void CodeLinePosInfoRecordEvent(
       Address code_start, base::Vector<const byte> source_position_table);
-#endif  // V8_ENABLE_WEBASSEMBLY
 
   void CodeNameEvent(Address addr, int pos, const char* code_name);
 
diff --git a/src/wasm/wasm-code-manager.cc b/src/wasm/wasm-code-manager.cc
@@ -267,17 +267,14 @@ void WasmCode::LogCode(Isolate* isolate, const char* source_url,
                  "wasm-function[%d]", index()));
     name = base::VectorOf(name_buffer);
   }
-
-  // Record source positions before adding code, otherwise when code is added,
-  // there are no source positions to associate with the added code.
-  if (!source_positions().empty()) {
-    LOG_CODE_EVENT(isolate, WasmCodeLinePosInfoRecordEvent(instruction_start(),
-                                                           source_positions()));
-  }
-
   int code_offset = module->functions[index_].code.offset();
   PROFILE(isolate, CodeCreateEvent(CodeEventListener::FUNCTION_TAG, this, name,
                                    source_url, code_offset, script_id));
+
+  if (!source_positions().empty()) {
+    LOG_CODE_EVENT(isolate, CodeLinePosInfoRecordEvent(instruction_start(),
+                                                       source_positions()));
+  }
 }
 
 void WasmCode::Validate() const {
diff --git a/test/cctest/cctest.status b/test/cctest/cctest.status
@@ -580,7 +580,6 @@
   'test-api/TurboAsmDisablesDetach': [SKIP],
   'test-api/WasmI32AtomicWaitCallback': [SKIP],
   'test-api/WasmI64AtomicWaitCallback': [SKIP],
-  'test-api/WasmSetJitCodeEventHandler': [SKIP],
   'test-api-wasm/WasmStreaming*': [SKIP],
   'test-backing-store/Run_WasmModule_Buffer_Externalized_Regression_UseAfterFree': [SKIP],
   'test-c-wasm-entry/*': [SKIP],
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
diff --git a/test/mjsunit/wasm/gdbjit.js b/test/mjsunit/wasm/gdbjit.js
