Revert "Fix stack-walking race"

This reverts commit 721e40283793649b4750c05da4fe972bd372f7f9.

Reason for revert: Wanted more discussion.

Test: none

Bug: 72608560
Bug: 29259363
Bug: 148166031
Change-Id: Id0fb201018a0d0bdca11d51ec156a41d6d7fe4ae

12 files changed, 162 insertions, 482 deletions

diff --git a/dex2oat/dex2oat_test.cc b/dex2oat/dex2oat_test.cc
index 6639d4b6c3..7ff9b734b1 100644
--- a/dex2oat/dex2oat_test.cc
+++ b/dex2oat/dex2oat_test.cc
@@ -458,9 +458,9 @@ TEST_F(Dex2oatSwapUseTest, CheckSwapUsage) {
   TEST_DISABLED_FOR_MEMORY_TOOL();
 
   // The `native_alloc_2_ >= native_alloc_1_` assertion below may not
-  // hold true on some x86 or x86_64 systems; disable this test while we
+  // hold true on some x86 systems; disable this test while we
   // investigate (b/29259363).
-  TEST_DISABLED();
+  TEST_DISABLED_FOR_X86();
 
   RunTest(/*use_fd=*/ false,
           /*expect_use=*/ false);
diff --git a/runtime/base/locks.h b/runtime/base/locks.h
index 806e84e773..c1667f3ea7 100644
--- a/runtime/base/locks.h
+++ b/runtime/base/locks.h
@@ -97,7 +97,6 @@ enum LockLevel : uint8_t {
   kTracingStreamingLock,
   kClassLoaderClassesLock,
   kDefaultMutexLevel,
-  kInstrumentationInstallLock,
   kDexLock,
   kMarkSweepLargeObjectLock,
   kJdwpObjectRegistryLock,
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index c9f3247002..011d947ea2 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -298,12 +298,6 @@ void InstrumentationInstallStack(Thread* thread, void* arg)
           last_return_pc_(0),
           force_deopt_id_(force_deopt_id) {}
 
-   protected:
-    bool IsStackInstrumentWalk() const override {
-      return true;
-    }
-
-   public:
     bool VisitFrame() override REQUIRES_SHARED(Locks::mutator_lock_) {
       ArtMethod* m = GetMethod();
       if (m == nullptr) {
@@ -402,14 +396,8 @@ void InstrumentationInstallStack(Thread* thread, void* arg)
             break;
           }
         }
-        {
-          // Increment the version count of the instrumentation stack so other threads can see it.
-          MutexLock mu2(Thread::Current(),
-                        *GetThread()->GetInstrumentationInstallSequenceNumberMutex());
-          instrumentation_stack_->insert(it, instrumentation_frame);
-          SetReturnPc(instrumentation_exit_pc_);
-          GetThread()->IncrementInstrumentationInstallSequenceNumber();
-        }
+        instrumentation_stack_->insert(it, instrumentation_frame);
+        SetReturnPc(instrumentation_exit_pc_);
       }
       uint32_t dex_pc = dex::kDexNoIndex;
       if (last_return_pc_ != 0 && GetCurrentOatQuickMethodHeader() != nullptr) {
@@ -429,7 +417,6 @@ void InstrumentationInstallStack(Thread* thread, void* arg)
     uintptr_t last_return_pc_;
     uint64_t force_deopt_id_;
   };
-  MutexLock mu(Thread::Current(), *thread->GetInstrumentationInstallMutex());
   if (kVerboseInstrumentation) {
     std::string thread_name;
     thread->GetThreadName(thread_name);
@@ -549,7 +536,6 @@ static void InstrumentationRestoreStack(Thread* thread, void* arg)
     thread->GetThreadName(thread_name);
     LOG(INFO) << "Removing exit stubs in " << thread_name;
   }
-  MutexLock mu(Thread::Current(), *thread->GetInstrumentationInstallMutex());
   std::deque<instrumentation::InstrumentationStackFrame>* stack = thread->GetInstrumentationStack();
   if (stack->size() > 0) {
     Instrumentation* instrumentation = reinterpret_cast<Instrumentation*>(arg);
@@ -557,11 +543,7 @@ static void InstrumentationRestoreStack(Thread* thread, void* arg)
         reinterpret_cast<uintptr_t>(GetQuickInstrumentationExitPc());
     RestoreStackVisitor visitor(thread, instrumentation_exit_pc, instrumentation);
     visitor.WalkStack(true);
-    MutexLock mu2(Thread::Current(), *thread->GetInstrumentationInstallSequenceNumberMutex());
-    thread->IncrementInstrumentationInstallSequenceNumber();
     CHECK_EQ(visitor.frames_removed_, stack->size());
-    // Since we restore the return-pcs first we don't really need to increment the instrumentation
-    // install sequence number, still good practice though.
     while (stack->size() > 0) {
       stack->pop_front();
     }
diff --git a/runtime/stack.cc b/runtime/stack.cc
index bc8f8cc648..20e97dc968 100644
--- a/runtime/stack.cc
+++ b/runtime/stack.cc
@@ -21,11 +21,9 @@
 
 #include "arch/context.h"
 #include "art_method-inl.h"
-#include "base/aborting.h"
 #include "base/callee_save_type.h"
 #include "base/enums.h"
 #include "base/hex_dump.h"
-#include "base/mutex.h"
 #include "dex/dex_file_types.h"
 #include "entrypoints/entrypoint_utils-inl.h"
 #include "entrypoints/quick/callee_save_frame.h"
@@ -73,7 +71,6 @@ StackVisitor::StackVisitor(Thread* thread,
       cur_oat_quick_method_header_(nullptr),
       num_frames_(num_frames),
       cur_depth_(0),
-      walk_started_(false),
       cur_inline_info_(nullptr, CodeInfo()),
       cur_stack_map_(0, StackMap()),
       context_(context),
@@ -848,277 +845,186 @@ void StackVisitor::WalkStack(bool include_transitions) {
     DCHECK(thread_ == Thread::Current() || thread_->IsSuspended());
   }
   CHECK_EQ(cur_depth_, 0U);
-  CHECK(!walk_started_);
-  walk_started_ = true;
-  bool retry = false;
-  size_t depth;
-  bool skip_to_depth = false;
-  uint64_t current_sequence_number;
-  Thread* self = Thread::Current();
-  // If the instrumentation stack changes due to DeoptimizeThreadStack or similar while we are
-  // running this stack-walk we need to restart and try again.
-  //
-  // This is because when we perform DeoptimizeThreadStack we replace the return-pc of the threads
-  // stack-frames with a trampoline, the actual return-address is placed in the threads
-  // InstrumentationStack. The instrumentation code will fill up this stack as it goes. Since there
-  // is no good way to map instrumentation stack frames to real ones (or vice versa) and we need to
-  // know which instrumentation stack frame we are on in order to find the real return-pc using that
-  // the next stack-frame's size we instead simply keep track of how many instrumentation frames we
-  // have used (as instrumentation_stack_depth). Each time we need a new instrumentation frame we
-  // simply increment this value. This works well enough but if InstrumentationInstallStack is
-  // called concurrently the InstrumentationStack might have frames inserted prior to the point this
-  // StackVisitor is currently at. This would cause the visitor to get the incorrect
-  // instrumentation_frame and start incorrectly walking the stack.
-  //
-  // To prevent this we keep track of when the last update that changes the indexs of
-  // instrumentation frames occurs and restart the stack walk if that changes. To simplify the
-  // process of building stack walkers we also keep track of how many times we have called
-  // VisitFrame and skip that many calls. This prevents any frames from being visited more than once
-  // per 'WalkStack' call.
-  do {
-    retry = false;
-    depth = 0;
-    // Keep track of how far in the stack-walk we are, making sure to also keep track of whether
-    // we've skipped enough to resume calling 'VisitFrame' after a retry.
-    auto increment_depth = [&]() {
-      DCHECK_LE(depth, cur_depth_);
-      ++depth;
-      if (skip_to_depth && depth == cur_depth_) {
-        skip_to_depth = false;
-      } else if (!skip_to_depth) {
-        ++cur_depth_;
-      }
-    };
-    // Wait for any current instrumentation installs to finish, Skip this if we are aborting.
-    if (LIKELY(gAborting == 0)) {
-      MutexLock mu(self, *GetThread()->GetInstrumentationInstallMutex());
-    }
-    // Get Current install state id.
-    {
-      MutexLock mu(self, *GetThread()->GetInstrumentationInstallSequenceNumberMutex());
-      current_sequence_number = GetThread()->GetInstrumentationInstallSequenceNumber();
-    }
-    bool exit_stubs_installed = Runtime::Current()->GetInstrumentation()->AreExitStubsInstalled();
-    uint32_t instrumentation_stack_depth = 0;
-    size_t inlined_frames_count = 0;
-
-    for (const ManagedStack* current_fragment = thread_->GetManagedStack();
-         current_fragment != nullptr;
-         current_fragment = current_fragment->GetLink()) {
-      cur_shadow_frame_ = current_fragment->GetTopShadowFrame();
-      cur_quick_frame_ = current_fragment->GetTopQuickFrame();
-      cur_quick_frame_pc_ = 0;
-      cur_oat_quick_method_header_ = nullptr;
-      if (cur_quick_frame_ != nullptr) {  // Handle quick stack frames.
-        // Can't be both a shadow and a quick fragment.
-        DCHECK(current_fragment->GetTopShadowFrame() == nullptr);
-        ArtMethod* method = *cur_quick_frame_;
-        DCHECK(method != nullptr);
-        bool header_retrieved = false;
-        if (method->IsNative()) {
-          // We do not have a PC for the first frame, so we cannot simply use
-          // ArtMethod::GetOatQuickMethodHeader() as we're unable to distinguish there
-          // between GenericJNI frame and JIT-compiled JNI stub; the entrypoint may have
-          // changed since the frame was entered. The top quick frame tag indicates
-          // GenericJNI here, otherwise it's either AOT-compiled or JNI-compiled JNI stub.
-          if (UNLIKELY(current_fragment->GetTopQuickFrameTag())) {
-            // The generic JNI does not have any method header.
-            cur_oat_quick_method_header_ = nullptr;
+  bool exit_stubs_installed = Runtime::Current()->GetInstrumentation()->AreExitStubsInstalled();
+  uint32_t instrumentation_stack_depth = 0;
+  size_t inlined_frames_count = 0;
+
+  for (const ManagedStack* current_fragment = thread_->GetManagedStack();
+       current_fragment != nullptr; current_fragment = current_fragment->GetLink()) {
+    cur_shadow_frame_ = current_fragment->GetTopShadowFrame();
+    cur_quick_frame_ = current_fragment->GetTopQuickFrame();
+    cur_quick_frame_pc_ = 0;
+    cur_oat_quick_method_header_ = nullptr;
+    if (cur_quick_frame_ != nullptr) {  // Handle quick stack frames.
+      // Can't be both a shadow and a quick fragment.
+      DCHECK(current_fragment->GetTopShadowFrame() == nullptr);
+      ArtMethod* method = *cur_quick_frame_;
+      DCHECK(method != nullptr);
+      bool header_retrieved = false;
+      if (method->IsNative()) {
+        // We do not have a PC for the first frame, so we cannot simply use
+        // ArtMethod::GetOatQuickMethodHeader() as we're unable to distinguish there
+        // between GenericJNI frame and JIT-compiled JNI stub; the entrypoint may have
+        // changed since the frame was entered. The top quick frame tag indicates
+        // GenericJNI here, otherwise it's either AOT-compiled or JNI-compiled JNI stub.
+        if (UNLIKELY(current_fragment->GetTopQuickFrameTag())) {
+          // The generic JNI does not have any method header.
+          cur_oat_quick_method_header_ = nullptr;
+        } else {
+          const void* existing_entry_point = method->GetEntryPointFromQuickCompiledCode();
+          CHECK(existing_entry_point != nullptr);
+          Runtime* runtime = Runtime::Current();
+          ClassLinker* class_linker = runtime->GetClassLinker();
+          // Check whether we can quickly get the header from the current entrypoint.
+          if (!class_linker->IsQuickGenericJniStub(existing_entry_point) &&
+              !class_linker->IsQuickResolutionStub(existing_entry_point) &&
+              existing_entry_point != GetQuickInstrumentationEntryPoint()) {
+            cur_oat_quick_method_header_ =
+                OatQuickMethodHeader::FromEntryPoint(existing_entry_point);
           } else {
-            const void* existing_entry_point = method->GetEntryPointFromQuickCompiledCode();
-            CHECK(existing_entry_point != nullptr);
-            Runtime* runtime = Runtime::Current();
-            ClassLinker* class_linker = runtime->GetClassLinker();
-            // Check whether we can quickly get the header from the current entrypoint.
-            if (!class_linker->IsQuickGenericJniStub(existing_entry_point) &&
-                !class_linker->IsQuickResolutionStub(existing_entry_point) &&
-                existing_entry_point != GetQuickInstrumentationEntryPoint()) {
-              cur_oat_quick_method_header_ =
-                  OatQuickMethodHeader::FromEntryPoint(existing_entry_point);
+            const void* code = method->GetOatMethodQuickCode(class_linker->GetImagePointerSize());
+            if (code != nullptr) {
+              cur_oat_quick_method_header_ = OatQuickMethodHeader::FromEntryPoint(code);
             } else {
-              const void* code = method->GetOatMethodQuickCode(class_linker->GetImagePointerSize());
-              if (code != nullptr) {
-                cur_oat_quick_method_header_ = OatQuickMethodHeader::FromEntryPoint(code);
-              } else {
-                // This must be a JITted JNI stub frame.
-                CHECK(runtime->GetJit() != nullptr);
-                code = runtime->GetJit()->GetCodeCache()->GetJniStubCode(method);
-                CHECK(code != nullptr) << method->PrettyMethod();
-                cur_oat_quick_method_header_ = OatQuickMethodHeader::FromCodePointer(code);
-              }
+              // This must be a JITted JNI stub frame.
+              CHECK(runtime->GetJit() != nullptr);
+              code = runtime->GetJit()->GetCodeCache()->GetJniStubCode(method);
+              CHECK(code != nullptr) << method->PrettyMethod();
+              cur_oat_quick_method_header_ = OatQuickMethodHeader::FromCodePointer(code);
             }
           }
-          header_retrieved = true;
         }
-        while (method != nullptr) {
-          if (!header_retrieved) {
-            cur_oat_quick_method_header_ = method->GetOatQuickMethodHeader(cur_quick_frame_pc_);
-          }
-          header_retrieved = false;  // Force header retrieval in next iteration.
-          SanityCheckFrame();
-
-          if ((walk_kind_ == StackWalkKind::kIncludeInlinedFrames) &&
-              (cur_oat_quick_method_header_ != nullptr) &&
-              cur_oat_quick_method_header_->IsOptimized() &&
-              !method->IsNative()  // JNI methods cannot have any inlined frames.
-              && CodeInfo::HasInlineInfo(cur_oat_quick_method_header_->GetOptimizedCodeInfoPtr())) {
-            DCHECK_NE(cur_quick_frame_pc_, 0u);
-            CodeInfo* code_info = GetCurrentInlineInfo();
-            StackMap* stack_map = GetCurrentStackMap();
-            if (stack_map->IsValid() && stack_map->HasInlineInfo()) {
-              DCHECK_EQ(current_inline_frames_.size(), 0u);
-              for (current_inline_frames_ = code_info->GetInlineInfosOf(*stack_map);
-                   !current_inline_frames_.empty();
-                   current_inline_frames_.pop_back()) {
-                if (!skip_to_depth) {
-                  bool should_continue = VisitFrame();
-                  if (UNLIKELY(!should_continue)) {
-                    return;
-                  }
-                }
-                increment_depth();
-                inlined_frames_count++;
+        header_retrieved = true;
+      }
+      while (method != nullptr) {
+        if (!header_retrieved) {
+          cur_oat_quick_method_header_ = method->GetOatQuickMethodHeader(cur_quick_frame_pc_);
+        }
+        header_retrieved = false;  // Force header retrieval in next iteration.
+        SanityCheckFrame();
+
+        if ((walk_kind_ == StackWalkKind::kIncludeInlinedFrames)
+            && (cur_oat_quick_method_header_ != nullptr)
+            && cur_oat_quick_method_header_->IsOptimized()
+            && !method->IsNative()  // JNI methods cannot have any inlined frames.
+            && CodeInfo::HasInlineInfo(cur_oat_quick_method_header_->GetOptimizedCodeInfoPtr())) {
+          DCHECK_NE(cur_quick_frame_pc_, 0u);
+          CodeInfo* code_info = GetCurrentInlineInfo();
+          StackMap* stack_map = GetCurrentStackMap();
+          if (stack_map->IsValid() && stack_map->HasInlineInfo()) {
+            DCHECK_EQ(current_inline_frames_.size(), 0u);
+            for (current_inline_frames_ = code_info->GetInlineInfosOf(*stack_map);
+                 !current_inline_frames_.empty();
+                 current_inline_frames_.pop_back()) {
+              bool should_continue = VisitFrame();
+              if (UNLIKELY(!should_continue)) {
+                return;
               }
+              cur_depth_++;
+              inlined_frames_count++;
             }
           }
+        }
 
-          if (!skip_to_depth) {
-            bool should_continue = VisitFrame();
-            if (UNLIKELY(!should_continue)) {
-              return;
-            }
-          }
-          if (kCount == CountTransitions::kYes || !method->IsRuntimeMethod()) {
-            increment_depth();
-          }
+        bool should_continue = VisitFrame();
+        if (UNLIKELY(!should_continue)) {
+          return;
+        }
 
-          // NB This uses cur_oat_quick_frame_info_ (and hence cur_quick_frame_pc_ & the previous
-          // return_pc_) to determine the frame information.
-          QuickMethodFrameInfo frame_info = GetCurrentQuickFrameInfo();
-          if (context_ != nullptr) {
-            context_->FillCalleeSaves(reinterpret_cast<uint8_t*>(cur_quick_frame_), frame_info);
-          }
-          // Compute PC for next stack frame from return PC.
-          size_t frame_size = frame_info.FrameSizeInBytes();
-          size_t return_pc_offset = frame_size - sizeof(void*);
-          uint8_t* return_pc_addr = reinterpret_cast<uint8_t*>(cur_quick_frame_) + return_pc_offset;
-          uintptr_t return_pc = *reinterpret_cast<uintptr_t*>(return_pc_addr);
-
-          if (UNLIKELY(exit_stubs_installed ||
-                       reinterpret_cast<uintptr_t>(GetQuickInstrumentationExitPc()) == return_pc)) {
-            // While profiling, the return pc is restored from the side stack, except when walking
-            // the stack for an exception where the side stack will be unwound in VisitFrame.
-            if (reinterpret_cast<uintptr_t>(GetQuickInstrumentationExitPc()) == return_pc) {
-              MutexLock mu(self, *GetThread()->GetInstrumentationInstallSequenceNumberMutex());
-              // Instrument-install walks will change the instrumentation-stack but only put things
-              // at the end, so we can continue regardless. We hold the InstrumentationInstallMutex
-              // so we know nothing can mess up the stack.
-              if (!IsStackInstrumentWalk() &&
-                  GetThread()->GetInstrumentationInstallSequenceNumber() !=
-                      current_sequence_number) {
-                if (kDebugStackWalk) {
-                  LOG(INFO) << "Retrying walk from begining due to instrumentation change. "
-                            << "Skipping to depth " << cur_depth_;
-                }
-                skip_to_depth = true;
-                retry = true;
-                // Annoyingly c++ doesn't have a labled continue. If they respected RAII this would
-                // be a good time to use goto but as is we'll just break twice.
-                break;  // From while (method != nullptr)
-              } else if (kIsDebugBuild && IsStackInstrumentWalk()) {
-                GetThread()->GetInstrumentationInstallMutex()->AssertExclusiveHeld(self);
-              }
-              CHECK_LT(instrumentation_stack_depth, thread_->GetInstrumentationStack()->size());
-              const instrumentation::InstrumentationStackFrame& instrumentation_frame =
-                  (*thread_->GetInstrumentationStack())[instrumentation_stack_depth];
-              instrumentation_stack_depth++;
-              if (GetMethod() ==
-                  Runtime::Current()->GetCalleeSaveMethod(CalleeSaveType::kSaveAllCalleeSaves)) {
-                // Skip runtime save all callee frames which are used to deliver exceptions.
-              } else if (instrumentation_frame.interpreter_entry_) {
-                ArtMethod* callee =
-                    Runtime::Current()->GetCalleeSaveMethod(CalleeSaveType::kSaveRefsAndArgs);
-                CHECK_EQ(GetMethod(), callee)
-                    << "Expected: " << ArtMethod::PrettyMethod(callee)
-                    << " Found: " << ArtMethod::PrettyMethod(GetMethod());
-              } else {
-                // Instrumentation generally doesn't distinguish between a method's obsolete and
-                // non-obsolete version.
-                CHECK_EQ(instrumentation_frame.method_->GetNonObsoleteMethod(),
-                          GetMethod()->GetNonObsoleteMethod())
-                    << "Expected: "
-                    << ArtMethod::PrettyMethod(
-                            instrumentation_frame.method_->GetNonObsoleteMethod())
-                    << " Found: " << ArtMethod::PrettyMethod(GetMethod()->GetNonObsoleteMethod());
-              }
-              return_pc = instrumentation_frame.return_pc_;
-              // TODO It would be nice to do this check but unfortunately it leads to a race since
-              // we need to perform a recursive stack-walk to calculate the frame-id.
-              // size_t expected_id = instrumentation_frame.frame_id_;
-              // if (num_frames_ != 0) {
-              //   // Check agreement of frame Ids only if num_frames_ is computed to avoid infinite
-              //   // recursion.
-              //   size_t frame_id = instrumentation::Instrumentation::ComputeFrameId(
-              //       thread_,
-              //       // We already incremented depth but we want to get the last visited frame.
-              //       depth - 1,
-              //       inlined_frames_count);
-              //   CHECK_EQ(expected_id, frame_id);
-              // }
+        QuickMethodFrameInfo frame_info = GetCurrentQuickFrameInfo();
+        if (context_ != nullptr) {
+          context_->FillCalleeSaves(reinterpret_cast<uint8_t*>(cur_quick_frame_), frame_info);
+        }
+        // Compute PC for next stack frame from return PC.
+        size_t frame_size = frame_info.FrameSizeInBytes();
+        size_t return_pc_offset = frame_size - sizeof(void*);
+        uint8_t* return_pc_addr = reinterpret_cast<uint8_t*>(cur_quick_frame_) + return_pc_offset;
+        uintptr_t return_pc = *reinterpret_cast<uintptr_t*>(return_pc_addr);
+
+        if (UNLIKELY(exit_stubs_installed ||
+                     reinterpret_cast<uintptr_t>(GetQuickInstrumentationExitPc()) == return_pc)) {
+          // While profiling, the return pc is restored from the side stack, except when walking
+          // the stack for an exception where the side stack will be unwound in VisitFrame.
+          if (reinterpret_cast<uintptr_t>(GetQuickInstrumentationExitPc()) == return_pc) {
+            CHECK_LT(instrumentation_stack_depth, thread_->GetInstrumentationStack()->size());
+            const instrumentation::InstrumentationStackFrame& instrumentation_frame =
+                (*thread_->GetInstrumentationStack())[instrumentation_stack_depth];
+            instrumentation_stack_depth++;
+            if (GetMethod() ==
+                Runtime::Current()->GetCalleeSaveMethod(CalleeSaveType::kSaveAllCalleeSaves)) {
+              // Skip runtime save all callee frames which are used to deliver exceptions.
+            } else if (instrumentation_frame.interpreter_entry_) {
+              ArtMethod* callee =
+                  Runtime::Current()->GetCalleeSaveMethod(CalleeSaveType::kSaveRefsAndArgs);
+              CHECK_EQ(GetMethod(), callee) << "Expected: " << ArtMethod::PrettyMethod(callee)
+                                            << " Found: " << ArtMethod::PrettyMethod(GetMethod());
+            } else {
+              // Instrumentation generally doesn't distinguish between a method's obsolete and
+              // non-obsolete version.
+              CHECK_EQ(instrumentation_frame.method_->GetNonObsoleteMethod(),
+                       GetMethod()->GetNonObsoleteMethod())
+                  << "Expected: "
+                  << ArtMethod::PrettyMethod(instrumentation_frame.method_->GetNonObsoleteMethod())
+                  << " Found: " << ArtMethod::PrettyMethod(GetMethod()->GetNonObsoleteMethod());
             }
+            if (num_frames_ != 0) {
+              // Check agreement of frame Ids only if num_frames_ is computed to avoid infinite
+              // recursion.
+              size_t frame_id = instrumentation::Instrumentation::ComputeFrameId(
+                  thread_,
+                  cur_depth_,
+                  inlined_frames_count);
+              CHECK_EQ(instrumentation_frame.frame_id_, frame_id);
+            }
+            return_pc = instrumentation_frame.return_pc_;
           }
+        }
 
-          cur_quick_frame_pc_ = return_pc;
-          uint8_t* next_frame = reinterpret_cast<uint8_t*>(cur_quick_frame_) + frame_size;
-          cur_quick_frame_ = reinterpret_cast<ArtMethod**>(next_frame);
-
-          if (kDebugStackWalk) {
-            LOG(INFO) << ArtMethod::PrettyMethod(method) << "@" << method << " size=" << frame_size
-                      << std::boolalpha << " optimized="
-                      << (cur_oat_quick_method_header_ != nullptr &&
-                          cur_oat_quick_method_header_->IsOptimized())
-                      << " native=" << method->IsNative() << std::noboolalpha
-                      << " entrypoints=" << method->GetEntryPointFromQuickCompiledCode() << ","
-                      << (method->IsNative() ? method->GetEntryPointFromJni() : nullptr)
-                      << " next=" << *cur_quick_frame_;
-          }
-
-          method = *cur_quick_frame_;
+        cur_quick_frame_pc_ = return_pc;
+        uint8_t* next_frame = reinterpret_cast<uint8_t*>(cur_quick_frame_) + frame_size;
+        cur_quick_frame_ = reinterpret_cast<ArtMethod**>(next_frame);
+
+        if (kDebugStackWalk) {
+          LOG(INFO) << ArtMethod::PrettyMethod(method) << "@" << method << " size=" << frame_size
+              << std::boolalpha
+              << " optimized=" << (cur_oat_quick_method_header_ != nullptr &&
+                                   cur_oat_quick_method_header_->IsOptimized())
+              << " native=" << method->IsNative()
+              << std::noboolalpha
+              << " entrypoints=" << method->GetEntryPointFromQuickCompiledCode()
+              << "," << (method->IsNative() ? method->GetEntryPointFromJni() : nullptr)
+              << " next=" << *cur_quick_frame_;
         }
-        // We want to start again. Break right now.
-        if (retry) {
-          break;  // From for (const ManagedStack* current_fragment = thread_->GetManagedStack();
+
+        if (kCount == CountTransitions::kYes || !method->IsRuntimeMethod()) {
+          cur_depth_++;
         }
-      } else if (cur_shadow_frame_ != nullptr) {
-        do {
-          SanityCheckFrame();
-          if (!skip_to_depth) {
-            bool should_continue = VisitFrame();
-            if (UNLIKELY(!should_continue)) {
-              return;
-            }
-          }
-          increment_depth();
-          cur_shadow_frame_ = cur_shadow_frame_->GetLink();
-        } while (cur_shadow_frame_ != nullptr);
+        method = *cur_quick_frame_;
       }
-      if (!skip_to_depth && include_transitions) {
+    } else if (cur_shadow_frame_ != nullptr) {
+      do {
+        SanityCheckFrame();
         bool should_continue = VisitFrame();
-        if (!should_continue) {
+        if (UNLIKELY(!should_continue)) {
           return;
         }
-      }
-      if (kCount == CountTransitions::kYes) {
-        increment_depth();
-      }
+        cur_depth_++;
+        cur_shadow_frame_ = cur_shadow_frame_->GetLink();
+      } while (cur_shadow_frame_ != nullptr);
     }
-    if (retry) {
-      continue;  // Real retry.
+    if (include_transitions) {
+      bool should_continue = VisitFrame();
+      if (!should_continue) {
+        return;
+      }
     }
-    if (num_frames_ != 0) {
-      CHECK_EQ(cur_depth_, num_frames_);
+    if (kCount == CountTransitions::kYes) {
+      cur_depth_++;
     }
-  } while (retry);
+  }
+  if (num_frames_ != 0) {
+    CHECK_EQ(cur_depth_, num_frames_);
+  }
 }
 
 template void StackVisitor::WalkStack<StackVisitor::CountTransitions::kYes>(bool);
diff --git a/runtime/stack.h b/runtime/stack.h
index 6f6c365f5d..ad73e75fb5 100644
--- a/runtime/stack.h
+++ b/runtime/stack.h
@@ -129,10 +129,6 @@ class StackVisitor {
   bool GetRegisterIfAccessible(uint32_t reg, VRegKind kind, uint32_t* val) const
       REQUIRES_SHARED(Locks::mutator_lock_);
 
-  virtual bool IsStackInstrumentWalk() const {
-    return false;
-  }
-
  public:
   virtual ~StackVisitor() {}
   StackVisitor(const StackVisitor&) = default;
@@ -364,8 +360,6 @@ class StackVisitor {
   size_t num_frames_;
   // Depth of the frame we're currently at.
   size_t cur_depth_;
-  // Whether we've received an initial WalkStack call.
-  bool walk_started_;
   // Current inlined frames of the method we are currently at.
   // We keep poping frames from the end as we visit the frames.
   BitTableRange<InlineInfo> current_inline_frames_;
diff --git a/runtime/thread.cc b/runtime/thread.cc
index be9bceddf7..c3e4afe768 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -2295,15 +2295,8 @@ void Thread::NotifyThreadGroup(ScopedObjectAccessAlreadyRunnable& soa, jobject t
 
 Thread::Thread(bool daemon)
     : tls32_(daemon),
-      instrumentation_install_sequence_number_(0),
       wait_monitor_(nullptr),
       is_runtime_thread_(false) {
-  instrumentation_install_mutex_ = new Mutex(
-      "a thread instrumentation install mutex",
-      LockLevel::kInstrumentationInstallLock,
-      /*recursive=*/true);
-  instrumentation_install_sequence_number_mutex_ = new Mutex(
-      "a thread instrumentation install sequence number mutex", LockLevel::kGenericBottomLock);
   wait_mutex_ = new Mutex("a thread wait mutex", LockLevel::kThreadWaitLock);
   wait_cond_ = new ConditionVariable("a thread wait condition variable", *wait_mutex_);
   tlsPtr_.instrumentation_stack = new std::deque<instrumentation::InstrumentationStackFrame>;
diff --git a/runtime/thread.h b/runtime/thread.h
index 0bee7e5add..34434cf68c 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -607,22 +607,6 @@ class Thread {
   void NotifyLocked(Thread* self) REQUIRES(wait_mutex_);
 
  public:
-  Mutex* GetInstrumentationInstallMutex() const LOCK_RETURNED(instrumentation_install_mutex_) {
-    return instrumentation_install_mutex_;
-  }
-  Mutex* GetInstrumentationInstallSequenceNumberMutex() const
-      LOCK_RETURNED(instrumentation_install_sequence_number_mutex_) {
-    return instrumentation_install_sequence_number_mutex_;
-  }
-  uint64_t GetInstrumentationInstallSequenceNumber() const
-      REQUIRES(instrumentation_install_sequence_number_mutex_) {
-    return instrumentation_install_sequence_number_;
-  }
-  void IncrementInstrumentationInstallSequenceNumber()
-      REQUIRES(instrumentation_install_sequence_number_mutex_) {
-    ++instrumentation_install_sequence_number_;
-  }
-
   Mutex* GetWaitMutex() const LOCK_RETURNED(wait_mutex_) {
     return wait_mutex_;
   }
@@ -1889,15 +1873,6 @@ class Thread {
   // All fields below this line should not be accessed by native code. This means these fields can
   // be modified, rearranged, added or removed without having to modify asm_support.h
 
-  // Mutex that guards instrumentation installation.
-  Mutex* instrumentation_install_mutex_ DEFAULT_MUTEX_ACQUIRED_AFTER;
-
-  // Mutex and id to enable stack-walking to recognize that a concurrent instrumentation-install has
-  // occurred and stack-walking will need to retry.
-  Mutex* instrumentation_install_sequence_number_mutex_ BOTTOM_MUTEX_ACQUIRED_AFTER;
-  uint64_t instrumentation_install_sequence_number_
-      GUARDED_BY(instrumentation_install_sequence_number_mutex_);
-
   // Guards the 'wait_monitor_' members.
   Mutex* wait_mutex_ DEFAULT_MUTEX_ACQUIRED_AFTER;
 
diff --git a/test/2011-stack-walk-concurrent-instrument/expected.txt b/test/2011-stack-walk-concurrent-instrument/expected.txt
deleted file mode 100644
index 77a1486479..0000000000
--- a/test/2011-stack-walk-concurrent-instrument/expected.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-JNI_OnLoad called
-Done
diff --git a/test/2011-stack-walk-concurrent-instrument/info.txt b/test/2011-stack-walk-concurrent-instrument/info.txt
deleted file mode 100644
index 91f0106279..0000000000
--- a/test/2011-stack-walk-concurrent-instrument/info.txt
+++ /dev/null
@@ -1,3 +0,0 @@
-Tests concurrently instrumenting a thread while walking a stack doesn't crash/break.
-
-Bug: 72608560
diff --git a/test/2011-stack-walk-concurrent-instrument/src/Main.java b/test/2011-stack-walk-concurrent-instrument/src/Main.java
deleted file mode 100644
index 8f96f937c9..0000000000
--- a/test/2011-stack-walk-concurrent-instrument/src/Main.java
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
- * Copyright 2020 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import java.util.concurrent.*;
-
-public class Main {
-  public Main() {
-  }
-
-  void $noinline$f(Runnable r) throws Exception {
-    $noinline$g(r);
-  }
-
-  void $noinline$g(Runnable r) {
-    $noinline$h(r);
-  }
-
-  void $noinline$h(Runnable r) {
-    r.run();
-  }
-
-  public native void resetTest();
-  public native void waitAndDeopt(Thread t);
-  public native void doSelfStackWalk();
-
-  void testConcurrent() throws Exception {
-    resetTest();
-    final Thread current = Thread.currentThread();
-    Thread t = new Thread(() -> {
-      try {
-        this.waitAndDeopt(current);
-      } catch (Exception e) {
-        throw new Error("Fail!", e);
-      }
-    });
-    t.start();
-    $noinline$f(() -> {
-      try {
-        this.doSelfStackWalk();
-      } catch (Exception e) {
-        throw new Error("Fail!", e);
-      }
-    });
-    t.join();
-  }
-
-  public static void main(String[] args) throws Exception {
-    System.loadLibrary(args[0]);
-    Main st = new Main();
-    st.testConcurrent();
-    System.out.println("Done");
-  }
-}
diff --git a/test/2011-stack-walk-concurrent-instrument/stack_walk_concurrent.cc b/test/2011-stack-walk-concurrent-instrument/stack_walk_concurrent.cc
deleted file mode 100644
index 68e58f4143..0000000000
--- a/test/2011-stack-walk-concurrent-instrument/stack_walk_concurrent.cc
+++ /dev/null
@@ -1,97 +0,0 @@
-/*
- * Copyright 2020 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include <atomic>
-#include <string_view>
-
-#include "arch/context.h"
-#include "art_method-inl.h"
-#include "jni.h"
-#include "scoped_thread_state_change.h"
-#include "stack.h"
-#include "thread.h"
-
-namespace art {
-namespace StackWalkConcurrentInstrument {
-
-std::atomic<bool> instrument_waiting = false;
-std::atomic<bool> instrumented = false;
-
-// Spin lock.
-static void WaitForInstrument() REQUIRES_SHARED(Locks::mutator_lock_) {
-  ScopedThreadSuspension sts(Thread::Current(), ThreadState::kWaitingForDeoptimization);
-  instrument_waiting = true;
-  while (!instrumented) {
-  }
-}
-
-class SelfStackWalkVisitor : public StackVisitor {
- public:
-  explicit SelfStackWalkVisitor(Thread* thread) REQUIRES_SHARED(Locks::mutator_lock_)
-      : StackVisitor(thread, Context::Create(), StackWalkKind::kIncludeInlinedFrames) {}
-
-  bool VisitFrame() override REQUIRES_SHARED(Locks::mutator_lock_) {
-    if (GetMethod()->GetNameView() == "$noinline$f") {
-      CHECK(!found_f_);
-      found_f_ = true;
-    } else if (GetMethod()->GetNameView() == "$noinline$g") {
-      CHECK(!found_g_);
-      found_g_ = true;
-      WaitForInstrument();
-    } else if (GetMethod()->GetNameView() == "$noinline$h") {
-      CHECK(!found_h_);
-      found_h_ = true;
-    }
-    return true;
-  }
-
-  bool found_f_ = false;
-  bool found_g_ = false;
-  bool found_h_ = false;
-};
-
-extern "C" JNIEXPORT void JNICALL Java_Main_resetTest(JNIEnv*, jobject) {
-  instrument_waiting = false;
-  instrumented = false;
-}
-
-extern "C" JNIEXPORT void JNICALL Java_Main_doSelfStackWalk(JNIEnv*, jobject) {
-  ScopedObjectAccess soa(Thread::Current());
-  SelfStackWalkVisitor sswv(Thread::Current());
-  sswv.WalkStack();
-  CHECK(sswv.found_f_);
-  CHECK(sswv.found_g_);
-  CHECK(sswv.found_h_);
-}
-extern "C" JNIEXPORT void JNICALL Java_Main_waitAndDeopt(JNIEnv*, jobject, jobject target) {
-  while (!instrument_waiting) {
-  }
-  bool timed_out = false;
-  Thread* other = Runtime::Current()->GetThreadList()->SuspendThreadByPeer(
-      target, true, SuspendReason::kInternal, &timed_out);
-  CHECK(!timed_out);
-  CHECK(other != nullptr);
-  ScopedObjectAccess soa(Thread::Current());
-  Runtime::Current()->GetInstrumentation()->InstrumentThreadStack(other);
-  MutexLock mu(Thread::Current(), *Locks::thread_suspend_count_lock_);
-  bool updated = other->ModifySuspendCount(Thread::Current(), -1, nullptr, SuspendReason::kInternal);
-  CHECK(updated);
-  instrumented = true;
-  return;
-}
-
-}  // namespace StackWalkConcurrentInstrument
-}  // namespace art
diff --git a/test/Android.bp b/test/Android.bp
index 53ed6ab1ca..89538c6751 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -589,7 +589,6 @@ cc_defaults {
         "1947-breakpoint-redefine-deopt/check_deopt.cc",
         "1972-jni-id-swap-indices/jni_id.cc",
         "1985-structural-redefine-stack-scope/stack_scope.cc",
-        "2011-stack-walk-concurrent-instrument/stack_walk_concurrent.cc",
         "common/runtime_state.cc",
         "common/stack_inspect.cc",
     ],