Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Add Windows support for //flutter/fml/backtrace.h #36202

Merged
merged 26 commits into from
Oct 20, 2022
Merged

Add Windows support for //flutter/fml/backtrace.h #36202

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
760e866
Add Windows support for `//flutter/fml/backtrace.h`
moko256 Sep 16, 2022
6b26b25
remove empty break
moko256 Sep 20, 2022
aa92903
use absl::GetStackTrace
moko256 Sep 20, 2022
1d5b4fc
remove __cxxabiv1::__cxa_demangle
moko256 Sep 20, 2022
09649c1
replace with absil's signal handler
moko256 Sep 20, 2022
dca3616
fix format
moko256 Sep 21, 2022
dba4017
cleanup unused vars
moko256 Sep 21, 2022
a5b212c
fix test to enable to run without symbol
moko256 Sep 21, 2022
4fe24c3
mark BacktraceTest as DeathTest
moko256 Sep 21, 2022
f67dcf6
restore BacktraceHere
moko256 Sep 23, 2022
393233e
add comment
moko256 Sep 23, 2022
edfe8c1
test if stacktrace is supported
moko256 Sep 23, 2022
30105e7
Revert "test if stacktrace is supported"
moko256 Sep 23, 2022
51eaf5d
Rollback to 6b26b25
moko256 Sep 25, 2022
8df181b
Restore some changes.
moko256 Sep 25, 2022
6d5f7c2
Remove unused include.
moko256 Sep 25, 2022
a5a124a
Unite adding to offset
moko256 Sep 25, 2022
763d211
Remove test output
moko256 Sep 25, 2022
987eee5
Add test for BacktraceHere(0)
moko256 Sep 25, 2022
d6c28d9
Skip test if binary has no symbol
moko256 Sep 25, 2022
20b6dc4
Use gmock's matcher
moko256 Sep 25, 2022
ce81ec5
Fix private header
moko256 Sep 25, 2022
a4f3850
Force inlining
moko256 Sep 25, 2022
b6c657e
Skip if backtrace is not supported
moko256 Sep 25, 2022
a99f0dc
Inline manually
moko256 Sep 26, 2022
24b2aa5
Rollback to 763d211 and fix comment
moko256 Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions fml/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ source_set("fml") {
"wakeable.h",
]

if (is_mac || is_linux || (is_ios && is_debug)) {
if (is_mac || is_linux || is_win || (is_ios && is_debug)) {
sources += [ "backtrace.cc" ]
} else {
sources += [ "backtrace_stub.cc" ]
Expand All @@ -115,9 +115,12 @@ source_set("fml") {
"//third_party/icu",
]

if (is_mac || is_linux || (is_ios && is_debug)) {
if (is_mac || is_linux || is_win || (is_ios && is_debug)) {
# This abseil dependency is only used by backtrace.cc.
deps += [ "//third_party/abseil-cpp/absl/debugging:symbolize" ]
deps += [
"//third_party/abseil-cpp/absl/debugging:failure_signal_handler",
"//third_party/abseil-cpp/absl/debugging:symbolize",
]
}

configs += [ "//third_party/icu:icu_config" ]
Expand Down
127 changes: 4 additions & 123 deletions fml/backtrace.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,133 +4,13 @@

#include "flutter/fml/backtrace.h"

#include <cxxabi.h>
#include <dlfcn.h>
#include <execinfo.h>

#include <csignal>
#include <sstream>

#if FML_OS_WIN
#include <crtdbg.h>
#include <debugapi.h>
#endif

#include "flutter/fml/logging.h"
#include "flutter/fml/build_config.h"
#include "flutter/fml/paths.h"
#include "third_party/abseil-cpp/absl/debugging/failure_signal_handler.h"
#include "third_party/abseil-cpp/absl/debugging/symbolize.h"

namespace fml {

static std::string kKUnknownFrameName = "Unknown";

static std::string DemangleSymbolName(const std::string& mangled) {
if (mangled == kKUnknownFrameName) {
return kKUnknownFrameName;
}

int status = 0;
size_t length = 0;
char* demangled = __cxxabiv1::__cxa_demangle(
mangled.data(), // mangled name
nullptr, // output buffer (malloc-ed if nullptr)
&length, // demangled length
&status);

if (demangled == nullptr || status != 0) {
return mangled;
}

auto demangled_string = std::string{demangled, length};
free(demangled);
return demangled_string;
}

static std::string GetSymbolName(void* symbol) {
char name[1024];
if (!absl::Symbolize(symbol, name, sizeof(name))) {
return kKUnknownFrameName;
}

return DemangleSymbolName({name});
}

std::string BacktraceHere(size_t offset) {
constexpr size_t kMaxFrames = 256;
void* symbols[kMaxFrames];
const auto available_frames = ::backtrace(symbols, kMaxFrames);
if (available_frames <= 0) {
return "";
}

std::stringstream stream;
for (int i = 1 + offset; i < available_frames; ++i) {
stream << "Frame " << i - 1 - offset << ": " << symbols[i] << " "
<< GetSymbolName(symbols[i]) << std::endl;
}
return stream.str();
}

static size_t kKnownSignalHandlers[] = {
SIGABRT, // abort program
SIGFPE, // floating-point exception
SIGBUS, // bus error
SIGSEGV, // segmentation violation
SIGSYS, // non-existent system call invoked
SIGPIPE, // write on a pipe with no reader
SIGALRM, // real-time timer expired
SIGTERM, // software termination signal
};

static std::string SignalNameToString(int signal) {
switch (signal) {
case SIGABRT:
return "SIGABRT";
case SIGFPE:
return "SIGFPE";
case SIGBUS:
return "SIGBUS";
case SIGSEGV:
return "SIGSEGV";
case SIGSYS:
return "SIGSYS";
case SIGPIPE:
return "SIGPIPE";
case SIGALRM:
return "SIGALRM";
case SIGTERM:
return "SIGTERM";
};
return std::to_string(signal);
}

static void ToggleSignalHandlers(bool set);

static void SignalHandler(int signal) {
// We are a crash signal handler. This can only happen once. Since we don't
// want to catch crashes while we are generating the crash reports, disable
// all set signal handlers to their default values before reporting the crash
// and re-raising the signal.
ToggleSignalHandlers(false);

FML_LOG(ERROR) << "Caught signal " << SignalNameToString(signal)
<< " during program execution." << std::endl
<< BacktraceHere(3);

::raise(signal);
}

static void ToggleSignalHandlers(bool set) {
for (size_t i = 0; i < sizeof(kKnownSignalHandlers) / sizeof(size_t); ++i) {
auto signal_name = kKnownSignalHandlers[i];
auto handler = set ? &SignalHandler : SIG_DFL;

if (::signal(signal_name, handler) == SIG_ERR) {
FML_LOG(ERROR) << "Could not attach signal handler for " << signal_name;
}
}
}

void InstallCrashHandler() {
#if FML_OS_WIN
if (!IsDebuggerPresent()) {
Expand All @@ -142,7 +22,8 @@ void InstallCrashHandler() {
if (exe_path.first) {
absl::InitializeSymbolizer(exe_path.second.c_str());
}
ToggleSignalHandlers(true);
absl::FailureSignalHandlerOptions options;
absl::InstallFailureSignalHandler(options);
}

bool IsCrashHandlingSupported() {
Expand Down
4 changes: 0 additions & 4 deletions fml/backtrace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,10 @@
#ifndef FLUTTER_FML_BACKTRACE_H_
#define FLUTTER_FML_BACKTRACE_H_

#include <string>

#include "flutter/fml/macros.h"

namespace fml {

std::string BacktraceHere(size_t offset = 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this function? I presume the intent was for use when debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the #16450, which introduced BacktraceHere, this is only used in signal handler (and test for BacktraceHere). I thought it was made public to test.
The handler from absl::InstallFailureSignalHandler also shows stacktrace when receive signal, same as from fml's InstallCrashHandler before. The stacktrace's format is a little different from BacktraceHere, but it seems that the existing tests doesn't depend on the format.

I just intended to avoid bitrot to follow the style guide. I know this can be used to debug. If any engine's contributors use it to debug, I'll restore it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm -- I don't use it to debug, so personally I'm fine if we remove it (we can always restore it in a followup patch with appropriate tweaks if desired)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have code that isn't used, but I do end up using this call frequently when debugging/testing locally. Does absl have a similar function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is not blocking for landing, but I'd like to know for my own edification)

Copy link
Member

@cbracken cbracken Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people are using it for development, and if absl doesn't have an equivalent then let's restore it (the existing test will avoid bitrot, at least to some extent).

If absl does have an equivalent call, maybe we could add a "// See: absl::SomethingOrOther" type comment to the class docs as a little breadcrumb to help developers find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


void InstallCrashHandler();

bool IsCrashHandlingSupported();
Expand Down
6 changes: 0 additions & 6 deletions fml/backtrace_stub.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@

namespace fml {

static std::string kKUnknownFrameName = "Unknown";

std::string BacktraceHere(size_t offset) {
return "";
}

void InstallCrashHandler() {
// Not supported.
}
Expand Down
24 changes: 5 additions & 19 deletions fml/backtrace_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,20 @@

#include "backtrace.h"

#include <csignal>

#include "gtest/gtest.h"
#include "logging.h"

namespace fml {
namespace testing {

TEST(BacktraceTest, CanGatherBacktrace) {
TEST(BacktraceDeathTest, CanGatherBacktrace) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does nothing useful now. Let's remove this as well as IsCrashHandlingSupported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (!IsCrashHandlingSupported()) {
GTEST_SKIP();
return;
}
{
auto trace = BacktraceHere(0);
ASSERT_GT(trace.size(), 0u);
ASSERT_NE(trace.find("Frame 0"), std::string::npos);
}

{
auto trace = BacktraceHere(1);
ASSERT_GT(trace.size(), 0u);
ASSERT_NE(trace.find("Frame 0"), std::string::npos);
}

{
auto trace = BacktraceHere(2);
ASSERT_GT(trace.size(), 0u);
ASSERT_NE(trace.find("Frame 0"), std::string::npos);
}
::testing::FLAGS_gtest_death_test_style = "threadsafe";
EXPECT_DEATH_IF_SUPPORTED({ std::abort(); }, "SIGABRT");
}

} // namespace testing
Expand Down