Return an empty stack, not a blank frame, when no trace is available (meta-pytorch#3855)

Summary:

`captureNativeErrorStack()` promises "an empty vector when it is unavailable",
but does not deliver it. `folly::split('\n', "", v)` yields a vector holding one
empty string, so an unavailable trace is reported to Scuba as a single blank
frame rather than as no frames.

Two paths reach that empty string, and only one was considered. The header
guarded on `__has_include(<dwarf.h>)`, treating a missing header as the only way
capture can be unavailable. But `Symbolizer.h:292-293` documents that the
**real** implementation also returns `""` when a trace is not available, so the
bug is reachable in a build with a fully working symbolizer. Gating on a header
was never the right test.

This replaces the header probe with a test of the actual result. `Symbolizer.h`
is now included unconditionally -- it already supplies an inline stub returning
`""` in builds without a symbolizer, so duplicating folly's capability macros
here served no purpose -- and the empty case is handled where it is observable.
`folly::split` also now ignores empty tokens, so a trailing newline no longer
contributes a blank trailing frame.

The split/demangle/`skipInternalFrames` sequence is unchanged and moves intact
into `detail::normalizeStackTrace`, which is exposed so the behaviour can be
tested directly. Without that seam the empty case could only be reached by
building against a folly without a symbolizer, which is not something a unit
test can arrange.

No behavioural change for a healthy capture: the same frames, in the same order,
with the same plumbing frames stripped.

Differential Revision: D117752032

Author: Scusemua

comms/utils/logger/ErrorStackUtil.cc

@@ -3,13 +3,10 @@
 #include "comms/utils/logger/ErrorStackUtil.h"
 
 #include <array>
-#include <sstream>
 #include <string_view>
 
-#if __has_include(<dwarf.h>)
-#include <folly/debugging/symbolizer/Symbolizer.h>
-#endif
 #include <folly/String.h>
+#include <folly/debugging/symbolizer/Symbolizer.h>
 
 namespace {
 // Leading native-stack frames that belong to the logging / Scuba plumbing
@@ -55,25 +52,33 @@ void skipInternalFrames(std::vector<std::string>& frames) {
 
 namespace meta::comms::logger {
 
-std::vector<std::string> captureNativeErrorStack() {
-  std::vector<std::string> stackTraceDemangled;
+namespace detail {
+
+std::vector<std::string> normalizeStackTrace(const std::string& trace) {
+  // folly returns "" both from the no-symbolizer stub and, per Symbolizer.h,
+  // from the real implementation when no trace is available. Splitting "" would
+  // yield one empty element -- a bogus frame where there should be none.
+  if (trace.empty()) {
+    return {};
+  }
 
-  // Get stack trace (requires elfutils/libdwarf for folly Symbolizer)
-#if __has_include(<dwarf.h>)
-  std::stringstream ss;
-  ss << folly::symbolizer::getStackTraceStr();
+  std::vector<std::string> frames;
   // @lint-ignore CLANGTIDY
-  folly::split('\n', ss.str(), stackTraceDemangled);
-  for (auto& line : stackTraceDemangled) {
+  folly::split('\n', trace, frames, /* ignoreEmpty */ true);
+  for (auto& line : frames) {
     auto demangledLine = folly::demangle(line.c_str()).toStdString();
     line.swap(demangledLine);
   }
   // Drop the leading logging / Scuba plumbing frames so the recorded stack
   // starts near the real error site.
-  skipInternalFrames(stackTraceDemangled);
-#endif
+  skipInternalFrames(frames);
+  return frames;
+}
 
-  return stackTraceDemangled;
+} // namespace detail
+
+std::vector<std::string> captureNativeErrorStack() {
+  return detail::normalizeStackTrace(folly::symbolizer::getStackTraceStr());
 }
 
 } // namespace meta::comms::logger

comms/utils/logger/ErrorStackUtil.h

@@ -8,10 +8,19 @@
 namespace meta::comms::logger {
 
 // Capture the native symbolized error stack once, dropping the leading logging
-// / Scuba plumbing frames. Returns the symbolized stack when symbolizer support
-// is available, and an empty vector when it is unavailable (no dwarf.h). The
-// result can be shared across all error reporters so the expensive capture runs
-// only once per error.
+// / Scuba plumbing frames. Returns an empty vector when the build has no
+// symbolizer support and when no trace could be obtained, so callers must treat
+// an empty result as "no stack available" rather than as an error. The result
+// can be shared across all error reporters so the expensive capture runs only
+// once per error.
 std::vector<std::string> captureNativeErrorStack();
 
+namespace detail {
+// Turns a raw symbolizer trace into frames: one per line, demangled, with the
+// leading logging / Scuba plumbing removed. An empty trace yields an empty
+// vector. Exposed so that case can be tested without building against a folly
+// that has no symbolizer.
+std::vector<std::string> normalizeStackTrace(const std::string& trace);
+} // namespace detail
+
 } // namespace meta::comms::logger

comms/utils/logger/tests/ErrorStackUtilTest.cc

@@ -0,0 +1,64 @@
+// Copyright (c) Meta Platforms, Inc. and affiliates.
+
+#include "comms/utils/logger/ErrorStackUtil.h"
+
+#include <folly/debugging/symbolizer/Symbolizer.h>
+#include <folly/portability/GTest.h>
+
+using meta::comms::logger::captureNativeErrorStack;
+using meta::comms::logger::detail::normalizeStackTrace;
+
+// This target exercises the real symbolizer path. If folly is ever configured
+// without one here, the capture test below would silently pass against the
+// inline stub instead, so assert the capability rather than infer it.
+static_assert(
+    FOLLY_HAVE_ELF && FOLLY_HAVE_DWARF,
+    "buck2 is expected to build folly with a real symbolizer; without it the "
+    "captureNativeErrorStack test would exercise the empty-string stub");
+
+TEST(ErrorStackUtilTest, EmptyTraceYieldsEmptyVector) {
+  // Both the no-symbolizer stub and the real implementation return "" when no
+  // trace is available; splitting it would yield one bogus frame.
+  EXPECT_TRUE(normalizeStackTrace("").empty());
+}
+
+TEST(ErrorStackUtilTest, SplitsFramesOnNewlines) {
+  const std::vector<std::string> expected = {"frame_one", "frame_two"};
+  EXPECT_EQ(normalizeStackTrace("frame_one\nframe_two"), expected);
+}
+
+TEST(ErrorStackUtilTest, IgnoresBlankLines) {
+  // A trailing newline must not produce an empty trailing frame.
+  const std::vector<std::string> expected = {"frame_one", "frame_two"};
+  EXPECT_EQ(normalizeStackTrace("frame_one\n\nframe_two\n"), expected);
+}
+
+TEST(ErrorStackUtilTest, DropsLeadingPlumbingFrames) {
+  // Leading logging / Scuba frames are stripped so the stack starts at the
+  // real error site.
+  const std::vector<std::string> expected = {"real_error_site", "caller"};
+  EXPECT_EQ(
+      normalizeStackTrace(
+          "folly::symbolizer::getStackTraceStr()\n"
+          "NcclScubaSample::setError\n"
+          "real_error_site\n"
+          "caller"),
+      expected);
+}
+
+TEST(ErrorStackUtilTest, KeepsInternalMarkerAppearingAfterRealFrame) {
+  // Only *leading* plumbing is dropped -- a marker deeper in the stack is a
+  // genuine frame.
+  const std::vector<std::string> expected = {
+      "real_error_site", "logErrorToScuba"};
+  EXPECT_EQ(normalizeStackTrace("real_error_site\nlogErrorToScuba"), expected);
+}
+
+TEST(ErrorStackUtilTest, CaptureProducesNoEmptyFrames) {
+  // Symbolizer.h documents that even the real implementation may return "",
+  // so the result is allowed to be empty; what must never happen is a frame
+  // that is itself empty.
+  for (const auto& frame : captureNativeErrorStack()) {
+    EXPECT_FALSE(frame.empty());
+  }
+}