base::debug::StackTrace: return addresses using span.

Bug: 1490484 Change-Id: I9f69b53a9446d0760c5960f017aa5ee049146655 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4875963 Reviewed-by: Matthew Denton <mpdenton@chromium.org> Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Ayu Ishii <ayui@chromium.org> Cr-Commit-Position: refs/heads/main@{#1210332}
chromium · Oct 16, 2023 · a0e290d · a0e290d
1 parent 9011ff6
commit a0e290d
Show file tree

Hide file tree

Showing 9 changed files with 75 additions and 94 deletions.
diff --git a/base/debug/stack_trace.cc b/base/debug/stack_trace.cc
@@ -261,13 +261,6 @@ bool StackTrace::WillSymbolizeToStreamForTesting() {
 #endif
 }
 
-const void *const *StackTrace::Addresses(size_t* count) const {
-  *count = count_;
-  if (count_)
-    return trace_;
-  return nullptr;
-}
-
 void StackTrace::Print() const {
   PrintWithPrefix(nullptr);
 }

diff --git a/base/debug/stack_trace.h b/base/debug/stack_trace.h
@@ -11,6 +11,7 @@
 #include <string>
 
 #include "base/base_export.h"
+#include "base/containers/span.h"
 #include "base/debug/debugging_buildflags.h"
 #include "base/memory/raw_ptr.h"
 #include "build/build_config.h"
@@ -104,7 +105,9 @@ class BASE_EXPORT StackTrace {
   // number of elements in the returned array. Addresses()[0] will contain an
   // address from the leaf function, and Addresses()[count-1] will contain an
   // address from the root function (i.e.; the thread's entry point).
-  const void* const* Addresses(size_t* count) const;
+  span<const void* const> addresses() const {
+    return make_span(trace_, count_);
+  }
 
   // Prints the stack trace to stderr.
   void Print() const;

diff --git a/base/debug/stack_trace_nacl.cc b/base/debug/stack_trace_nacl.cc
@@ -11,10 +11,6 @@ StackTrace::StackTrace() = default;
 StackTrace::StackTrace(size_t count) : StackTrace() {}
 StackTrace::StackTrace(const void* const* trace, size_t count) : StackTrace() {}
 
-const void* const* StackTrace::Addresses(size_t* count) const {
-  return nullptr;
-}
-
 void StackTrace::Print() const {}
 
 void StackTrace::OutputToStream(std::ostream* os) const {}

diff --git a/base/debug/stack_trace_perftest.cc b/base/debug/stack_trace_perftest.cc
@@ -4,6 +4,7 @@
 
 #include <vector>
 
+#include "base/containers/span.h"
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
@@ -32,19 +33,19 @@ perf_test::PerfResultReporter SetUpReporter(const std::string& story_name) {
 
 class StackTracer {
  public:
-  StackTracer(size_t trace_count) : trace_count(trace_count) {}
+  StackTracer(size_t trace_count) : trace_count_(trace_count) {}
   void Trace() {
-    size_t tmp;
-    base::debug::StackTrace st(trace_count);
-    const void* addresses = st.Addresses(&tmp);
+    StackTrace st(trace_count_);
+    span<const void* const> addresses = st.addresses();
     // make sure a valid array of stack frames is returned
-    EXPECT_NE(addresses, nullptr);
+    ASSERT_FALSE(addresses.empty());
+    EXPECT_TRUE(addresses[0]);
     // make sure the test generates the intended count of stack frames
-    EXPECT_EQ(trace_count, tmp);
+    EXPECT_EQ(trace_count_, addresses.size());
   }
 
  private:
-  const size_t trace_count;
+  const size_t trace_count_;
 };
 
 void MultiObjTest(size_t trace_count) {

diff --git a/base/debug/stack_trace_unittest.cc b/base/debug/stack_trace_unittest.cc
@@ -48,8 +48,7 @@ TEST_F(StackTraceTest, OutputToStream) {
   // ToString() should produce the same output.
   EXPECT_EQ(backtrace_message, trace.ToString());
 
-  size_t frames_found = 0;
-  const void* const* addresses = trace.Addresses(&frames_found);
+  span<const void* const> addresses = trace.addresses();
 
 #if defined(OFFICIAL_BUILD) && \
     ((BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)) || BUILDFLAG(IS_FUCHSIA))
@@ -61,8 +60,8 @@ TEST_F(StackTraceTest, OutputToStream) {
         // ((BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)) ||
         // BUILDFLAG(IS_FUCHSIA))
 
-  ASSERT_TRUE(addresses);
-  ASSERT_GT(frames_found, 5u) << "Too few frames found.";
+  ASSERT_GT(addresses.size(), 5u) << "Too few frames found.";
+  ASSERT_TRUE(addresses[0]);
 
   if (!StackTrace::WillSymbolizeToStreamForTesting())
     return;
@@ -98,13 +97,10 @@ TEST_F(StackTraceTest, OutputToStream) {
 TEST_F(StackTraceTest, TruncatedTrace) {
   StackTrace trace;
 
-  size_t count = 0;
-  trace.Addresses(&count);
-  ASSERT_LT(2u, count);
+  ASSERT_LT(2u, trace.addresses().size());
 
   StackTrace truncated(2);
-  truncated.Addresses(&count);
-  EXPECT_EQ(2u, count);
+  EXPECT_EQ(2u, truncated.addresses().size());
 }
 #endif  // !defined(OFFICIAL_BUILD) && !defined(NO_UNWIND_TABLES)
 

diff --git a/base/debug/task_trace.cc b/base/debug/task_trace.cc
@@ -4,22 +4,20 @@
 
 #include "base/debug/task_trace.h"
 
+#include <algorithm>
+#include <iostream>
+#include <sstream>
+
+#include "base/pending_task.h"
 #include "base/ranges/algorithm.h"
+#include "base/task/common/task_annotator.h"
 #include "build/build_config.h"
 
 #if BUILDFLAG(IS_ANDROID)
 #include <android/log.h>
-#endif  // BUILDFLAG(IS_ANDROID)
 
-#include <iostream>
-#include <sstream>
-
-#if BUILDFLAG(IS_ANDROID)
 #include "base/no_destructor.h"
-#endif
-
-#include "base/pending_task.h"
-#include "base/task/common/task_annotator.h"
+#endif  // BUILDFLAG(IS_ANDROID)
 
 namespace base {
 namespace debug {
@@ -98,11 +96,11 @@ size_t TaskTrace::GetAddresses(span<const void*> addresses) const {
   if (empty()) {
     return count;
   }
-  const void* const* current_addresses = stack_trace_->Addresses(&count);
-  for (size_t i = 0; i < count && i < addresses.size(); ++i) {
-    addresses[i] = current_addresses[i];
-  }
-  return count;
+  span<const void* const> current_addresses = stack_trace_->addresses();
+  ranges::copy_n(current_addresses.begin(),
+                 std::min(current_addresses.size(), addresses.size()),
+                 addresses.begin());
+  return current_addresses.size();
 }
 
 std::ostream& operator<<(std::ostream& os, const TaskTrace& task_trace) {

diff --git a/components/crash/core/common/crash_key.cc b/components/crash/core/common/crash_key.cc
@@ -13,16 +13,17 @@ namespace internal {
 
 std::string FormatStackTrace(const base::debug::StackTrace& trace,
                              size_t max_length) {
-  size_t count = 0;
-  const void* const* addresses = trace.Addresses(&count);
+  base::span<const void* const> addresses = trace.addresses();
 
   std::string value;
-  for (size_t i = 0; i < count; ++i) {
-    std::string address = base::StringPrintf(
-        "0x%" PRIx64, reinterpret_cast<uint64_t>(addresses[i]));
-    if (value.size() + address.size() > max_length)
+  for (const void* address : addresses) {
+    std::string address_as_string =
+        base::StringPrintf("0x%" PRIx64, reinterpret_cast<uint64_t>(address));
+    if (value.size() + address_as_string.size() > max_length) {
       break;
-    value += address + " ";
+    }
+    value += address_as_string;
+    value += ' ';
   }
 
   if (!value.empty() && value.back() == ' ') {

diff --git a/components/gwp_asan/crash_handler/crash_analyzer_unittest.cc b/components/gwp_asan/crash_handler/crash_analyzer_unittest.cc
@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "base/debug/stack_trace.h"
 #include "base/functional/callback_helpers.h"
 #include "base/test/gtest_util.h"
@@ -198,33 +199,32 @@ TEST_F(CrashAnalyzerTest, StackTraceCollection) {
   ASSERT_TRUE(proto.has_deallocation());
 
   base::debug::StackTrace st;
-  size_t trace_len;
-  const void* const* trace = st.Addresses(&trace_len);
-  ASSERT_NE(trace, nullptr);
-  ASSERT_GT(trace_len, 0U);
+  base::span<const void* const> trace = st.addresses();
+  ASSERT_FALSE(trace.empty());
 
   // Adjust the stack trace to point to the entry above the current frame.
-  while (trace_len > 0) {
+  while (!trace.empty()) {
     if (trace[0] == __builtin_return_address(0))
       break;
 
-    trace++;
-    trace_len--;
+    trace = trace.subspan(1);
   }
 
-  ASSERT_GT(proto.allocation().stack_trace_size(), (int)trace_len);
-  ASSERT_GT(proto.deallocation().stack_trace_size(), (int)trace_len);
+  ASSERT_GT(proto.allocation().stack_trace_size(),
+            static_cast<int>(trace.size()));
+  ASSERT_GT(proto.deallocation().stack_trace_size(),
+            static_cast<int>(trace.size()));
 
   // Ensure that the allocation and deallocation stack traces match the stack
   // frames that we collected above the current frame.
-  for (size_t i = 1; i <= trace_len; i++) {
+  for (size_t i = 1; i <= trace.size(); i++) {
     SCOPED_TRACE(i);
     ASSERT_EQ(proto.allocation().stack_trace(
                   proto.allocation().stack_trace_size() - i),
-              reinterpret_cast<uintptr_t>(trace[trace_len - i]));
+              reinterpret_cast<uintptr_t>(trace[trace.size() - i]));
     ASSERT_EQ(proto.deallocation().stack_trace(
                   proto.deallocation().stack_trace_size() - i),
-              reinterpret_cast<uintptr_t>(trace[trace_len - i]));
+              reinterpret_cast<uintptr_t>(trace[trace.size() - i]));
   }
 }
 #endif

diff --git a/third_party/googletest/custom/gtest/internal/custom/stack_trace_getter.cc b/third_party/googletest/custom/gtest/internal/custom/stack_trace_getter.cc
@@ -7,18 +7,19 @@
 #include <algorithm>
 #include <iterator>
 
+#include "base/containers/adapters.h"
+#include "base/containers/span.h"
+#include "base/ranges/algorithm.h"
+
 std::string StackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) {
   base::debug::StackTrace stack_trace;
 
-  size_t departure_frame_count = 0;
-  const void* const* departure_addresses =
-      stack_trace_upon_leaving_gtest_.has_value()
-          ? stack_trace_upon_leaving_gtest_->Addresses(&departure_frame_count)
-          : nullptr;
+  base::span<const void* const> departure;
+  if (stack_trace_upon_leaving_gtest_.has_value()) {
+    departure = stack_trace_upon_leaving_gtest_->addresses();
+  }
 
-  size_t current_frame_count = 0;
-  const void* const* current_addresses =
-      stack_trace.Addresses(&current_frame_count);
+  base::span<const void* const> current = stack_trace.addresses();
 
   // Ignore the frames at the root of the current trace that match those of the
   // point of departure from GTest. These frames all relate to thread start and
@@ -27,46 +28,38 @@ std::string StackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) {
   // within the GTest function that called UponLeavingGTest, and is irrelevant
   // as well.
   {
-    auto departure_rbegin =
-        std::make_reverse_iterator(departure_addresses + departure_frame_count);
-    auto departure_rend = std::make_reverse_iterator(departure_addresses);
-    auto current_rbegin =
-        std::make_reverse_iterator(current_addresses + current_frame_count);
-    auto current_rend = std::make_reverse_iterator(current_addresses);
-    auto mismatch_pair = std::mismatch(departure_rbegin, departure_rend,
-                                       current_rbegin, current_rend);
-    if (mismatch_pair.second != current_rend)
-      current_frame_count -= (mismatch_pair.second - current_rbegin) + 1;
+    auto mismatch_pair = std::mismatch(departure.rbegin(), departure.rend(),
+                                       current.rbegin(), current.rend());
+    if (mismatch_pair.second != current.rend()) {
+      current = current.subspan(
+          0, current.size() - (mismatch_pair.second - current.rbegin() + 1));
+    }
   }
 
   // Ignore the frames at the leaf of the current trace that match those of the
   // point of departure from GTest. These frames are the call(s) into
   // StackTrace's constructor, which are irrelevant. Also ignore the very first
   // mismatch, as it identifies two instructions within current function.
   {
-    auto mismatch_pair = std::mismatch(
-        departure_addresses, departure_addresses + departure_frame_count,
-        current_addresses, current_addresses + current_frame_count);
-    if (mismatch_pair.second != current_addresses + current_frame_count) {
-      auto match_size = (mismatch_pair.second - current_addresses) + 1;
-      current_frame_count -= match_size;
-      current_addresses += match_size;
+    auto mismatch_pair = std::mismatch(departure.begin(), departure.end(),
+                                       current.begin(), current.end());
+    if (mismatch_pair.second != current.end()) {
+      auto match_size = (mismatch_pair.second - current.begin()) + 1;
+      current = current.subspan(match_size);
     }
   }
 
   // Ignore frames that the caller wishes to skip.
-  if (skip_count >= 0 &&
-      static_cast<size_t>(skip_count) < current_frame_count) {
-    current_frame_count -= skip_count;
-    current_addresses += skip_count;
+  if (skip_count >= 0 && static_cast<size_t>(skip_count) < current.size()) {
+    current = current.subspan(skip_count);
   }
 
   // Only return as many as requested.
-  if (max_depth >= 0 && static_cast<size_t>(max_depth) < current_frame_count)
-    current_frame_count = static_cast<size_t>(max_depth);
+  if (max_depth >= 0 && static_cast<size_t>(max_depth) < current.size()) {
+    current = current.subspan(0, static_cast<size_t>(max_depth));
+  }
 
-  return base::debug::StackTrace(current_addresses, current_frame_count)
-      .ToString();
+  return base::debug::StackTrace(current.data(), current.size()).ToString();
 }
 
 void StackTraceGetter::UponLeavingGTest() {