From 15fee822470654e722879e1e5d6b50008637af0c Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris@google.com>
Date: Mon, 12 Sep 2022 18:00:10 -0700
Subject: [PATCH] Fix thread unwind in CallStack.

The CallStack unwind of a different thread was broken since it
wasn't properly setting the tid value.

Fix this problem and add new unit tests to verify the behavior.

Bug: 246405269

Test: New unit tests pass.
Test: Ran unit tests for 1000 operations to verify not flaky.
Change-Id: I00342e6cdcdb4bcb68f29734dadee6c987c98040
---
 libutils/Android.bp         | 19 ++++++++----
 libutils/CallStack.cpp      |  2 +-
 libutils/CallStack_test.cpp | 62 +++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 libutils/CallStack_test.cpp

diff --git a/libutils/Android.bp b/libutils/Android.bp
index 7939e82d7..c744b5316 100644
--- a/libutils/Android.bp
+++ b/libutils/Android.bp
@@ -299,6 +299,7 @@ cc_test {
 
     srcs: [
         "BitSet_test.cpp",
+        "CallStack_test.cpp",
         "Errors_test.cpp",
         "FileMap_test.cpp",
         "LruCache_test.cpp",
@@ -319,11 +320,14 @@ cc_test {
                 "SystemClock_test.cpp",
             ],
             shared_libs: [
-                "libz",
-                "liblog",
-                "libcutils",
-                "libutils",
                 "libbase",
+                "libcutils",
+                "liblog",
+                "liblzma",
+                "libunwindstack",
+                "libutils",
+                "libutilscallstack",
+                "libz",
             ],
         },
         linux: {
@@ -334,9 +338,12 @@ cc_test {
         },
         host: {
             static_libs: [
-                "libutils",
-                "liblog",
                 "libbase",
+                "liblog",
+                "liblzma",
+                "libunwindstack_no_dex",
+                "libutils",
+                "libutilscallstack",
             ],
         },
     },
diff --git a/libutils/CallStack.cpp b/libutils/CallStack.cpp
index f19ba6a3c..4dcb35b36 100644
--- a/libutils/CallStack.cpp
+++ b/libutils/CallStack.cpp
@@ -49,7 +49,7 @@ void CallStack::update(int32_t ignoreDepth, pid_t tid) {
     unwindstack::AndroidUnwinderData data;
     std::optional<pid_t> tid_val;
     if (tid != -1) {
-        *tid_val = tid;
+        tid_val = tid;
     }
     if (!unwinder.Unwind(tid_val, data)) {
         ALOGW("%s: Failed to unwind callstack: %s", __FUNCTION__, data.GetErrorString().c_str());
diff --git a/libutils/CallStack_test.cpp b/libutils/CallStack_test.cpp
new file mode 100644
index 000000000..2ea191168
--- /dev/null
+++ b/libutils/CallStack_test.cpp
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2022 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 <unistd.h>
+
+#include <thread>
+
+#include <android-base/threads.h>
+#include <gtest/gtest.h>
+#include <utils/CallStack.h>
+
+[[clang::noinline]] extern "C" void CurrentCaller(android::String8& backtrace) {
+    android::CallStack cs;
+    cs.update();
+    backtrace = cs.toString();
+}
+
+TEST(CallStackTest, current_backtrace) {
+    android::String8 backtrace;
+    CurrentCaller(backtrace);
+
+    ASSERT_NE(-1, backtrace.find("(CurrentCaller")) << "Full backtrace:\n" << backtrace;
+}
+
+[[clang::noinline]] extern "C" void ThreadBusyWait(std::atomic<pid_t>* tid, volatile bool* done) {
+    *tid = android::base::GetThreadId();
+    while (!*done) {
+    }
+}
+
+TEST(CallStackTest, thread_backtrace) {
+    // Use a volatile to avoid any problems unwinding since sometimes
+    // accessing a std::atomic does not include unwind data at every
+    // instruction and leads to failed unwinds.
+    volatile bool done = false;
+    std::atomic<pid_t> tid = -1;
+    std::thread thread([&tid, &done]() { ThreadBusyWait(&tid, &done); });
+
+    while (tid == -1) {
+    }
+
+    android::CallStack cs;
+    cs.update(0, tid);
+
+    done = true;
+    thread.join();
+
+    ASSERT_NE(-1, cs.toString().find("(ThreadBusyWait")) << "Full backtrace:\n" << cs.toString();
+}