[wasm][debug] Skip scopes on uninspectable frames

Frames that have not been compiled by Liftoff for debugging are
uninspectable. Instead of reporting an empty local scope and stack scope
in this case, just don't report these two scopes at all.

This also fixes a case missed in https://crrev.com/c/2196349, where we
would still try to generate the stack scope for non-debugging code.

Drive-by: Use {WasmFrame} instead of {StandardFrame} in the
{DebugWasmScopeIterator}, and use the {FrameInspectionScope}
consistently.

R=thibaudm@chromium.org, bmeurer@chromium.org
CC=kimanh@chromium.org

Bug: v8:10359, chromium:1071757, chromium:1079328, chromium:1072839
Change-Id: I3a3731a0bd9f582f94458500252922b4146e394f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2198982Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67777}

src/debug/debug-frames.h

@@ -11,13 +11,13 @@
 #include "src/execution/isolate.h"
 #include "src/execution/v8threads.h"
 #include "src/objects/objects.h"
-#include "src/wasm/wasm-interpreter.h"
 
 namespace v8 {
 namespace internal {
 
 class JavaScriptFrame;
 class StandardFrame;
+class WasmFrame;
 
 class FrameInspector {
  public:

src/debug/debug-scope-iterator.cc

@@ -129,7 +129,7 @@ bool DebugScopeIterator::SetVariableValue(v8::Local<v8::String> name,
 }
 
 DebugWasmScopeIterator::DebugWasmScopeIterator(Isolate* isolate,
-                                               StandardFrame* frame)
+                                               WasmFrame* frame)
     : isolate_(isolate),
       frame_(frame),
       type_(debug::ScopeIterator::ScopeTypeModule) {}
@@ -142,7 +142,10 @@ void DebugWasmScopeIterator::Advance() {
   DCHECK(!Done());
   switch (type_) {
     case ScopeTypeModule:
-      type_ = debug::ScopeIterator::ScopeTypeLocal;
+      // Skip local scope and expression stack scope if the frame is not
+      // inspectable.
+      type_ = frame_->is_inspectable() ? debug::ScopeIterator::ScopeTypeLocal
+                                       : debug::ScopeIterator::ScopeTypeWith;
       break;
     case ScopeTypeLocal:
       type_ = debug::ScopeIterator::ScopeTypeWasmExpressionStack;
@@ -171,15 +174,13 @@ v8::Local<v8::Object> DebugWasmScopeIterator::GetObject() {
     }
     case debug::ScopeIterator::ScopeTypeLocal: {
       DCHECK(frame_->is_wasm());
-      wasm::DebugInfo* debug_info =
-          WasmFrame::cast(frame_)->native_module()->GetDebugInfo();
+      wasm::DebugInfo* debug_info = frame_->native_module()->GetDebugInfo();
       return Utils::ToLocal(debug_info->GetLocalScopeObject(
           isolate_, frame_->pc(), frame_->fp(), frame_->callee_fp()));
     }
     case debug::ScopeIterator::ScopeTypeWasmExpressionStack: {
       DCHECK(frame_->is_wasm());
-      wasm::DebugInfo* debug_info =
-          WasmFrame::cast(frame_)->native_module()->GetDebugInfo();
+      wasm::DebugInfo* debug_info = frame_->native_module()->GetDebugInfo();
       return Utils::ToLocal(debug_info->GetStackScopeObject(
           isolate_, frame_->pc(), frame_->fp(), frame_->callee_fp()));
     }

src/debug/debug-scope-iterator.h

@@ -39,7 +39,7 @@ class DebugScopeIterator final : public debug::ScopeIterator {
 
 class DebugWasmScopeIterator final : public debug::ScopeIterator {
  public:
-  DebugWasmScopeIterator(Isolate* isolate, StandardFrame* frame);
+  DebugWasmScopeIterator(Isolate* isolate, WasmFrame* frame);
 
   bool Done() override;
   void Advance() override;
@@ -55,7 +55,7 @@ class DebugWasmScopeIterator final : public debug::ScopeIterator {
                         v8::Local<v8::Value> value) override;
  private:
   Isolate* isolate_;
-  StandardFrame* frame_;
+  WasmFrame* frame_;
   ScopeType type_;
 };
 }  // namespace internal

src/debug/debug-stack-trace-iterator.cc

@@ -169,7 +169,7 @@ DebugStackTraceIterator::GetScopeIterator() const {
   StandardFrame* frame = iterator_.frame();
   if (frame->is_wasm()) {
     return std::make_unique<DebugWasmScopeIterator>(isolate_,
-                                                    iterator_.frame());
+                                                    WasmFrame::cast(frame));
   }
   return std::make_unique<DebugScopeIterator>(isolate_, frame_inspector_.get());
 }

src/execution/frames.cc

@@ -1885,6 +1885,11 @@ int WasmFrame::byte_offset() const {
   return code->GetSourcePositionBefore(offset);
 }
 
+bool WasmFrame::is_inspectable() const {
+  wasm::WasmCodeRefScope code_ref_scope;
+  return wasm_code()->is_inspectable();
+}
+
 Object WasmFrame::context() const { return wasm_instance().native_context(); }
 
 void WasmFrame::Summarize(std::vector<FrameSummary>* functions) const {

src/execution/frames.h

@@ -924,6 +924,7 @@ class WasmFrame : public StandardFrame {
   bool at_to_number_conversion() const;
   // Byte offset in the function.
   int byte_offset() const;
+  bool is_inspectable() const;
 
   void Summarize(std::vector<FrameSummary>* frames) const override;
 

src/wasm/wasm-code-manager.h

@@ -174,6 +174,10 @@ class V8_EXPORT_PRIVATE WasmCode final {
            pc < reinterpret_cast<Address>(instructions_ + instructions_size_);
   }
 
+  // Only Liftoff code that was generated for debugging can be inspected
+  // (otherwise debug side table positions would not match up).
+  bool is_inspectable() const { return is_liftoff() && for_debugging(); }
+
   Vector<const uint8_t> protected_instructions_data() const {
     return {meta_data_.get(),
             static_cast<size_t>(protected_instructions_size_)};

src/wasm/wasm-debug.cc

@@ -359,7 +359,7 @@ class DebugInfoImpl {
 
   int GetNumLocals(Isolate* isolate, Address pc) {
     FrameInspectionScope scope(this, isolate, pc);
-    if (!scope.code->is_liftoff()) return 0;
+    if (!scope.is_inspectable()) return 0;
     return scope.debug_side_table->num_locals();
   }
 
@@ -371,7 +371,7 @@ class DebugInfoImpl {
 
   int GetStackDepth(Isolate* isolate, Address pc) {
     FrameInspectionScope scope(this, isolate, pc);
-    if (!scope.code->is_liftoff()) return 0;
+    if (!scope.is_inspectable()) return 0;
     int num_locals = static_cast<int>(scope.debug_side_table->num_locals());
     int value_count = scope.debug_side_table_entry->num_values();
     return value_count - num_locals;
@@ -389,27 +389,17 @@ class DebugInfoImpl {
 
   Handle<JSObject> GetLocalScopeObject(Isolate* isolate, Address pc, Address fp,
                                        Address debug_break_fp) {
+    FrameInspectionScope scope(this, isolate, pc);
     Handle<JSObject> local_scope_object =
         isolate->factory()->NewJSObjectWithNullProto();
 
-    wasm::WasmCodeRefScope wasm_code_ref_scope;
-    wasm::WasmCode* code =
-        isolate->wasm_engine()->code_manager()->LookupCode(pc);
-    // Only Liftoff code that was generated for debugging can be inspected
-    // (otherwise debug side table positions would not match up).
-    if (!code->is_liftoff() || !code->for_debugging()) {
-      return local_scope_object;
-    }
+    if (!scope.is_inspectable()) return local_scope_object;
 
     auto* module = native_module_->module();
-    auto* function = &module->functions[code->index()];
-    auto* debug_side_table = GetDebugSideTable(code, isolate->allocator());
-    int pc_offset = static_cast<int>(pc - code->instruction_start());
-    auto* debug_side_table_entry = debug_side_table->GetEntry(pc_offset);
-    DCHECK_NOT_NULL(debug_side_table_entry);
+    auto* function = &module->functions[scope.code->index()];
 
     // Fill parameters and locals.
-    int num_locals = static_cast<int>(debug_side_table->num_locals());
+    int num_locals = static_cast<int>(scope.debug_side_table->num_locals());
     DCHECK_LE(static_cast<int>(function->sig->parameter_count()), num_locals);
     if (num_locals > 0) {
       Handle<JSObject> locals_obj =
@@ -426,7 +416,7 @@ class DebugInfoImpl {
           name = PrintFToOneByteString<true>(isolate, "var%d", i);
         }
         WasmValue value =
-            GetValue(debug_side_table_entry, i, fp, debug_break_fp);
+            GetValue(scope.debug_side_table_entry, i, fp, debug_break_fp);
         Handle<Object> value_obj = WasmValueToValueObject(isolate, value);
         // {name} can be a string representation of an element index.
         LookupIterator::Key lookup_key{isolate, name};
@@ -444,28 +434,21 @@ class DebugInfoImpl {
 
   Handle<JSObject> GetStackScopeObject(Isolate* isolate, Address pc, Address fp,
                                        Address debug_break_fp) {
+    FrameInspectionScope scope(this, isolate, pc);
     Handle<JSObject> stack_scope_obj =
         isolate->factory()->NewJSObjectWithNullProto();
-    wasm::WasmCodeRefScope wasm_code_ref_scope;
-
-    wasm::WasmCode* code =
-        isolate->wasm_engine()->code_manager()->LookupCode(pc);
-    // Only Liftoff code can be inspected.
-    if (!code->is_liftoff()) return stack_scope_obj;
 
-    auto* debug_side_table = GetDebugSideTable(code, isolate->allocator());
-    int pc_offset = static_cast<int>(pc - code->instruction_start());
-    auto* debug_side_table_entry = debug_side_table->GetEntry(pc_offset);
-    DCHECK_NOT_NULL(debug_side_table_entry);
+    if (!scope.is_inspectable()) return stack_scope_obj;
 
     // Fill stack values.
     // Use an object without prototype instead of an Array, for nicer displaying
     // in DevTools. For Arrays, the length field and prototype is displayed,
     // which does not make too much sense here.
-    int num_locals = static_cast<int>(debug_side_table->num_locals());
-    int value_count = debug_side_table_entry->num_values();
+    int num_locals = static_cast<int>(scope.debug_side_table->num_locals());
+    int value_count = scope.debug_side_table_entry->num_values();
     for (int i = num_locals; i < value_count; ++i) {
-      WasmValue value = GetValue(debug_side_table_entry, i, fp, debug_break_fp);
+      WasmValue value =
+          GetValue(scope.debug_side_table_entry, i, fp, debug_break_fp);
       Handle<Object> value_obj = WasmValueToValueObject(isolate, value);
       JSObject::AddDataElement(stack_scope_obj,
                                static_cast<uint32_t>(i - num_locals), value_obj,
@@ -513,7 +496,7 @@ class DebugInfoImpl {
     WasmCode* new_code = native_module_->PublishCode(
         native_module_->AddCompiledCode(std::move(result)));
 
-    DCHECK(new_code->is_liftoff() && new_code->for_debugging());
+    DCHECK(new_code->is_inspectable());
     bool added =
         debug_side_tables_.emplace(new_code, std::move(debug_sidetable)).second;
     DCHECK(added);
@@ -642,12 +625,17 @@ class DebugInfoImpl {
                          Address pc)
         : code(isolate->wasm_engine()->code_manager()->LookupCode(pc)),
           pc_offset(static_cast<int>(pc - code->instruction_start())),
-          debug_side_table(code->is_liftoff() ? debug_info->GetDebugSideTable(
-                                                    code, isolate->allocator())
+          debug_side_table(
+              code->is_inspectable()
+                  ? debug_info->GetDebugSideTable(code, isolate->allocator())
                   : nullptr),
           debug_side_table_entry(debug_side_table
                                      ? debug_side_table->GetEntry(pc_offset)
-                                     : nullptr) {}
+                                     : nullptr) {
+      DCHECK_IMPLIES(code->is_inspectable(), debug_side_table_entry != nullptr);
+    }
+
+    bool is_inspectable() const { return debug_side_table_entry; }
 
     wasm::WasmCodeRefScope wasm_code_ref_scope;
     wasm::WasmCode* code;
@@ -658,7 +646,7 @@ class DebugInfoImpl {
 
   const DebugSideTable* GetDebugSideTable(WasmCode* code,
                                           AccountingAllocator* allocator) {
-    DCHECK(code->is_liftoff() && code->for_debugging());
+    DCHECK(code->is_inspectable());
     {
       // Only hold the mutex temporarily. We can't hold it while generating the
       // debug side table, because compilation takes the {NativeModule} lock.