Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions fml/BUILD.gn
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,7 +115,7 @@ 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" ]
}
Expand Down
51 changes: 35 additions & 16 deletions fml/backtrace.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,32 @@

#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/build_config.h"
#include "flutter/fml/logging.h"
#include "flutter/fml/paths.h"
#include "third_party/abseil-cpp/absl/debugging/symbolize.h"

#ifdef FML_OS_WIN
#include <Windows.h>
#include <crtdbg.h>
#include <debugapi.h>
#else // FML_OS_WIN
#include <cxxabi.h>
#include <dlfcn.h>
#include <execinfo.h>
#endif // FML_OS_WIN

namespace fml {

static std::string kKUnknownFrameName = "Unknown";

static std::string DemangleSymbolName(const std::string& mangled) {
Copy link
Contributor Author

@moko256 moko256 Sep 16, 2022

Choose a reason for hiding this comment

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

According to docs, the absl::Symbolize is already doing demangling, and the mangled here would be demangled name by absl::Symbolize.

Can we remove this function for all platforms?

Copy link
Member

@loic-sharma loic-sharma Sep 16, 2022

Choose a reason for hiding this comment

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

It looks like this can be removed for all platforms. It looks like DemangleSymbolName was needed by the original implementation but should've been removed when we moved to abseil's implementation. @chinmaygarde @jason-simmons Is that correct?

FWIW, absl::Symbolize definitely demangles on Windows (referred to as undecorating by MSVC):

  1. In initialization it sets the SYMOPT_UNDNAME option
  2. Symbolize calls SymFromAddr, which does the demangling.

Copy link
Member

Choose a reason for hiding this comment

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

If the demangler is no longer necessary, let's remove it. It was needed when I first wired it up.

#if FML_OS_WIN
Copy link
Member

@cbracken cbracken Sep 19, 2022

Choose a reason for hiding this comment

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

@chinmaygarde for thoughts on preprocessor conditionals vs making a posix subclass and a windows subclass? That's something we do with things like files, mutexes etc. typically by adding a platform specific file in fml/platform that's conditionally compiled in via the BUILD file. e.g. For files we do:

We could also do that as a follow-up refactor to get support landed initially.

return mangled;
#else
if (mangled == kKUnknownFrameName) {
return kKUnknownFrameName;
}
Expand All @@ -44,6 +49,7 @@ static std::string DemangleSymbolName(const std::string& mangled) {
auto demangled_string = std::string{demangled, length};
free(demangled);
return demangled_string;
#endif // FML_OS_WIN
}

static std::string GetSymbolName(void* symbol) {
Expand All @@ -55,10 +61,18 @@ static std::string GetSymbolName(void* symbol) {
return DemangleSymbolName({name});
}

static int Backtrace(void** symbols, int size) {
#if FML_OS_WIN
return CaptureStackBackTrace(0, size, symbols, NULL);
#else
return ::backtrace(symbols, size);
#endif // FML_OS_WIN
}

std::string BacktraceHere(size_t offset) {
constexpr size_t kMaxFrames = 256;
void* symbols[kMaxFrames];
const auto available_frames = ::backtrace(symbols, kMaxFrames);
const auto available_frames = Backtrace(symbols, kMaxFrames);
Copy link
Member

Choose a reason for hiding this comment

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

Should/could we use abseil's GetStackTrace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks nice! I'm replacing with this. I'll check output on linux...

Copy link
Contributor Author

@moko256 moko256 Sep 20, 2022

Choose a reason for hiding this comment

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

I also find absl::InstallFailureSignalHandler. The engine doesn't use BacktraceHere except in signal handler, so we can replace backtrace.cc with absl::InstallFailureSignalHandler.

cc: @chinmaygarde @cbracken

Copy link
Contributor Author

@moko256 moko256 Sep 20, 2022

Choose a reason for hiding this comment

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

Source code (prototype): moko256@3ca3f10
Windows:

*** SIGABRT received at time=1663690051 ***
    @   000000014073933C  (unknown)  absl::AbslFailureSignalHandler
    @   0000000142957225  (unknown)  raise
    @   00000001428D5DE9  (unknown)  abort
    @   00000001400E74A9  (unknown)  fml::testing::BacktracePrinter
    @   00000001400E748E  (unknown)  fml::testing::BacktraceTest_BacktraceHere_Test::TestBody
    @   00000001427BA210  (unknown)  testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,void>
    @   00000001427A4201  (unknown)  testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
    @   000000014278A59F  (unknown)  testing::Test::Run
    @   000000014278ADA8  (unknown)  testing::TestInfo::Run
    @   000000014278B41C  (unknown)  testing::TestSuite::Run
    @   00000001427956E5  (unknown)  testing::internal::UnitTestImpl::RunAllTests
    @   00000001427BE6F0  (unknown)  testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
    @   00000001427A6E91  (unknown)  testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
    @   0000000142795274  (unknown)  testing::UnitTest::Run
    @   0000000140279BC1  (unknown)  RUN_ALL_TESTS
    @   0000000140279848  (unknown)  main
    @   00000001428636A9  (unknown)  invoke_main
    @   000000014286358E  (unknown)  __scrt_common_main_seh
    @   000000014286344E  (unknown)  __scrt_common_main
    @   000000014286373E  (unknown)  mainCRTStartup
    @   00007FFFC2AB7034  (unknown)  BaseThreadInitThunk
    @   00007FFFC3A426A1  (unknown)  RtlUserThreadStart

Linux:

*** SIGABRT received at time=1663721941 on cpu 2 ***
PC: @     0x7fc0709537bb  (unknown)  raise
    @     0x55a71acd5eec         64  absl::WriteFailureInfo()
    @     0x55a71acd5c02         64  absl::AbslFailureSignalHandler()
    @     0x7fc070c71730  1470387120  (unknown)
    @     0x55a71a74e941         32  fml::testing::BacktraceTest_BacktraceHere_Test::TestBody()
    @     0x55a71c87107b         96  testing::internal::HandleSehExceptionsInMethodIfSupported<>()
    @     0x55a71c863c27        112  testing::internal::HandleExceptionsInMethodIfSupported<>()
    @     0x55a71c853073         96  testing::Test::Run()
    @     0x55a71c853734        112  testing::TestInfo::Run()
    @     0x55a71c853d3d        112  testing::TestSuite::Run()
    @     0x55a71c85d4e1        208  testing::internal::UnitTestImpl::RunAllTests()
    @     0x55a71c8741fb         96  testing::internal::HandleSehExceptionsInMethodIfSupported<>()
    @     0x55a71c865617        112  testing::internal::HandleExceptionsInMethodIfSupported<>()
    @     0x55a71c85d0df         96  testing::UnitTest::Run()
    @     0x55a71a8d5961         16  RUN_ALL_TESTS()
    @     0x55a71a8d5832        720  main
    @     0x7fc07094009b  (unknown)  __libc_start_main
    @ 0x5541d68949564100  (unknown)  (unknown)
Aborted

if (available_frames <= 0) {
return "";
}
Expand All @@ -74,12 +88,15 @@ std::string BacktraceHere(size_t offset) {
static size_t kKnownSignalHandlers[] = {
SIGABRT, // abort program
SIGFPE, // floating-point exception
SIGBUS, // bus error
SIGTERM, // software termination signal
SIGSEGV, // segmentation violation
#if !FML_OS_WIN
SIGBUS, // bus error
SIGSYS, // non-existent system call invoked
SIGPIPE, // write on a pipe with no reader
SIGALRM, // real-time timer expired
SIGTERM, // software termination signal

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

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

Copy link
Contributor Author

@moko256 moko256 Sep 20, 2022

Choose a reason for hiding this comment

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

Without this line, the formatter makes additional whitespace to the comment below.

    SIGALRM,  // real-time timer expired
#endif        // !FML_OS_WIN
// Here ^^^

Should we keep this line to remove these space, or remove this line?

#endif // !FML_OS_WIN
};

static std::string SignalNameToString(int signal) {
Expand All @@ -88,18 +105,20 @@ static std::string SignalNameToString(int signal) {
return "SIGABRT";
case SIGFPE:
return "SIGFPE";
case SIGBUS:
return "SIGBUS";
case SIGSEGV:
return "SIGSEGV";
case SIGTERM:
return "SIGTERM";
#if !FML_OS_WIN
case SIGBUS:
return "SIGBUS";
case SIGSYS:
return "SIGSYS";
case SIGPIPE:
return "SIGPIPE";
case SIGALRM:
return "SIGALRM";
case SIGTERM:
return "SIGTERM";
#endif // !FML_OS_WIN
};
return std::to_string(signal);
}
Expand Down
3 changes: 3 additions & 0 deletions fml/backtrace_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,21 @@ TEST(BacktraceTest, CanGatherBacktrace) {
auto trace = BacktraceHere(0);
ASSERT_GT(trace.size(), 0u);
ASSERT_NE(trace.find("Frame 0"), std::string::npos);
Copy link
Member

@loic-sharma loic-sharma Sep 16, 2022

Choose a reason for hiding this comment

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

I'm curious, could these verify that BacktraceHere is in the string for offset 0, and CanGatherBacktrace is in the string for offsets 0 and 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On master, the frame 0 of offset 0 is actually CanGatherBacktrace. I fixed the PR and added the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some CI tests seems to run without symbols. I added skipping the test BacktraceStartsWithCallerName if the function name in the first line of stacktrace is "Unknown".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to make all CIs pass, but only on
Linux Unopt, execinfo's backtrace returns the backtrace which contains itself, and it makes hard to write this test.
I'll remove this test from PR.

std::cout << trace << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these std::cout lines

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

}

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

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

Expand Down