[cpu-profiler] Fix stack iterability during deopt

Add a bit on the isolate which indicates that the stack is currently
not iterable for the SafeStackFrameIterator.

This is needed during deoptimization, when we do a fast C call without
a return address on the stack, meaning we can't iterate the stack
frames.

Re-enable DeoptAtFirstLevelInlinedSource which is fixed by this CL.

Bug: v8:9057
Change-Id: I76379a2dd38023be7e6f5153edeb1f838e9ac4d6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1688049
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62634}

src/codegen/external-reference.cc

@@ -775,6 +775,12 @@ ExternalReference ExternalReference::fast_c_call_caller_pc_address(
       isolate->isolate_data()->fast_c_call_caller_pc_address());
 }
 
+ExternalReference ExternalReference::stack_is_iterable_address(
+    Isolate* isolate) {
+  return ExternalReference(
+      isolate->isolate_data()->stack_is_iterable_address());
+}
+
 FUNCTION_REFERENCE(call_enqueue_microtask_function,
                    MicrotaskQueue::CallEnqueueMicrotask)
 

src/codegen/external-reference.h

@@ -72,6 +72,7 @@ class StatsCounter;
     "IsolateData::fast_c_call_caller_fp_address")                              \
   V(fast_c_call_caller_pc_address,                                             \
     "IsolateData::fast_c_call_caller_pc_address")                              \
+  V(stack_is_iterable_address, "IsolateData::stack_is_iterable_address")       \
   V(address_of_regexp_stack_limit, "RegExpStack::limit_address()")             \
   V(address_of_regexp_stack_memory_address, "RegExpStack::memory_address()")   \
   V(address_of_regexp_stack_memory_size, "RegExpStack::memory_size()")         \

src/deoptimizer/deoptimizer.cc

@@ -633,6 +633,12 @@ bool ShouldPadArguments(int arg_count) {
 // We rely on this function not causing a GC.  It is called from generated code
 // without having a real stack frame in place.
 void Deoptimizer::DoComputeOutputFrames() {
+  // When we call this function, the return address of the previous frame has
+  // been removed from the stack by GenerateDeoptimizationEntries() so the stack
+  // is not iterable by the SafeStackFrameIterator.
+#if V8_TARGET_ARCH_STORES_RETURN_ADDRESS_ON_STACK
+  DCHECK_EQ(0, isolate()->isolate_data()->stack_is_iterable());
+#endif
   base::ElapsedTimer timer;
 
   // Determine basic deoptimization information.  The optimized frame is

src/deoptimizer/ia32/deoptimizer-ia32.cc

@@ -116,6 +116,12 @@ void Deoptimizer::GenerateDeoptimizationEntries(MacroAssembler* masm,
   // and check that the generated code never deoptimizes with unbalanced stack.
   __ fnclex();
 
+  // Mark the stack as not iterable for the CPU profiler which won't be able to
+  // walk the stack without the return address.
+  __ mov_b(__ ExternalReferenceAsOperand(
+               ExternalReference::stack_is_iterable_address(isolate), edx),
+           Immediate(0));
+
   // Remove the return address and the double registers.
   __ add(esp, Immediate(kDoubleRegsSize + 1 * kSystemPointerSize));
 
@@ -194,6 +200,10 @@ void Deoptimizer::GenerateDeoptimizationEntries(MacroAssembler* masm,
     __ push(Operand(esi, offset));
   }
 
+  __ mov_b(__ ExternalReferenceAsOperand(
+               ExternalReference::stack_is_iterable_address(isolate), edx),
+           Immediate(1));
+
   // Restore the registers from the stack.
   __ popad();
 

src/deoptimizer/x64/deoptimizer-x64.cc

@@ -129,6 +129,12 @@ void Deoptimizer::GenerateDeoptimizationEntries(MacroAssembler* masm,
     __ popq(Operand(rbx, dst_offset));
   }
 
+  // Mark the stack as not iterable for the CPU profiler which won't be able to
+  // walk the stack without the return address.
+  __ movb(__ ExternalReferenceAsOperand(
+              ExternalReference::stack_is_iterable_address(isolate)),
+          Immediate(0));
+
   // Remove the return address from the stack.
   __ addq(rsp, Immediate(kPCOnStackSize));
 
@@ -218,6 +224,10 @@ void Deoptimizer::GenerateDeoptimizationEntries(MacroAssembler* masm,
     __ popq(r);
   }
 
+  __ movb(__ ExternalReferenceAsOperand(
+              ExternalReference::stack_is_iterable_address(isolate)),
+          Immediate(1));
+
   // Return to the continuation point.
   __ ret(0);
 }

src/execution/frames.cc

@@ -278,6 +278,11 @@ SafeStackFrameIterator::SafeStackFrameIterator(Isolate* isolate, Address pc,
   bool advance_frame = true;
 
   Address fast_c_fp = isolate->isolate_data()->fast_c_call_caller_fp();
+  uint8_t stack_is_iterable = isolate->isolate_data()->stack_is_iterable();
+  if (!stack_is_iterable) {
+    frame_ = nullptr;
+    return;
+  }
   // 'Fast C calls' are a special type of C call where we call directly from JS
   // to C without an exit frame inbetween. The CEntryStub is responsible for
   // setting Isolate::c_entry_fp, meaning that it won't be set for fast C calls.

src/execution/frames.h

@@ -147,7 +147,13 @@ class StackFrame {
   // the type of the value on the stack.
   static Type MarkerToType(intptr_t marker) {
     DCHECK(IsTypeMarker(marker));
-    return static_cast<Type>(marker >> kSmiTagSize);
+    intptr_t type = marker >> kSmiTagSize;
+    // TODO(petermarshall): There is a bug in the arm64 simulator that causes
+    // invalid frame markers.
+#if !(defined(USE_SIMULATOR) && V8_TARGET_ARCH_ARM64)
+    DCHECK_LT(static_cast<uintptr_t>(type), Type::NUMBER_OF_TYPES);
+#endif
+    return static_cast<Type>(type);
   }
 
   // Check if a marker is a stack frame type marker or a tagged pointer.

src/execution/isolate-data.h

@@ -81,8 +81,10 @@ class IsolateData final {
   // The FP and PC that are saved right before TurboAssembler::CallCFunction.
   Address* fast_c_call_caller_fp_address() { return &fast_c_call_caller_fp_; }
   Address* fast_c_call_caller_pc_address() { return &fast_c_call_caller_pc_; }
+  uint8_t* stack_is_iterable_address() { return &stack_is_iterable_; }
   Address fast_c_call_caller_fp() { return fast_c_call_caller_fp_; }
   Address fast_c_call_caller_pc() { return fast_c_call_caller_pc_; }
+  uint8_t stack_is_iterable() { return stack_is_iterable_; }
 
   // Returns true if this address points to data stored in this instance.
   // If it's the case then the value can be accessed indirectly through the
@@ -121,6 +123,7 @@ class IsolateData final {
   V(kVirtualCallTargetRegisterOffset, kSystemPointerSize)                     \
   V(kFastCCallCallerFPOffset, kSystemPointerSize)                             \
   V(kFastCCallCallerPCOffset, kSystemPointerSize)                             \
+  V(kStackIsIterableOffset, kUInt8Size)                                       \
   /* This padding aligns IsolateData size by 8 bytes. */                      \
   V(kPaddingOffset,                                                           \
     8 + RoundUp<8>(static_cast<int>(kPaddingOffset)) - kPaddingOffset)        \
@@ -172,6 +175,9 @@ class IsolateData final {
   // instruction in compiled code.
   Address fast_c_call_caller_fp_ = kNullAddress;
   Address fast_c_call_caller_pc_ = kNullAddress;
+  // Whether the SafeStackFrameIterator can successfully iterate the current
+  // stack. Only valid values are 0 or 1.
+  uint8_t stack_is_iterable_ = 1;
 
   // Ensure the size is 8-byte aligned in order to make alignment of the field
   // following the IsolateData field predictable. This solves the issue with
@@ -219,6 +225,8 @@ void IsolateData::AssertPredictableLayout() {
                 kFastCCallCallerFPOffset);
   STATIC_ASSERT(offsetof(IsolateData, fast_c_call_caller_pc_) ==
                 kFastCCallCallerPCOffset);
+  STATIC_ASSERT(offsetof(IsolateData, stack_is_iterable_) ==
+                kStackIsIterableOffset);
   STATIC_ASSERT(sizeof(IsolateData) == IsolateData::kSize);
 }
 

test/cctest/cctest.status

@@ -81,9 +81,6 @@
   'test-cpu-profiler/SampleWhenFrameIsNotSetup': [SKIP],
   'test-sampler/LibSamplerCollectSample': [SKIP],
 
-  # BUG(v8:9057). Flaky, maybe only on UBSan
-  'test-cpu-profiler/DeoptAtFirstLevelInlinedSource': [SKIP],
-
   # BUG(7202). The test is flaky.
   'test-cpu-profiler/NativeFrameStackTrace': [SKIP],
 

test/cctest/test-cpu-profiler.cc

@@ -2433,6 +2433,7 @@ TEST(DeoptAtFirstLevelInlinedSource) {
       "\n"
       "startProfiling();\n"
       "\n"
+      "%EnsureFeedbackVectorForFunction(opt_function);\n"
       "%PrepareFunctionForOptimization(test);\n"
       "\n"
       "test(10, 10);\n"