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

moko256 · 2022-09-16T12:52:27Z

This PR adds Windows support for //flutter/fml/backtrace.h.

This PR fixes flutter/flutter#111614.

No changes in the flutter/tests repo.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

moko256 · 2022-09-16T12:59:50Z

fml/backtrace.cc

+#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) {


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?

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):

In initialization it sets the SYMOPT_UNDNAME option

Symbolize calls SymFromAddr, which does the demangling.

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

loic-sharma · 2022-09-16T17:30:56Z

fml/backtrace.cc

    SIGSYS,   // non-existent system call invoked
    SIGPIPE,  // write on a pipe with no reader
    SIGALRM,  // real-time timer expired
-    SIGTERM,  // software termination signal
+


Suggested change

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?

fml/backtrace.cc

loic-sharma · 2022-09-16T18:17:34Z

fml/backtrace_unittests.cc

@@ -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);
+    std::cout << trace << std::endl;


Please remove these std::cout lines

loic-sharma · 2022-09-16T18:19:26Z

fml/backtrace_unittests.cc

@@ -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);


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?

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

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".

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.

loic-sharma · 2022-09-16T18:29:52Z

@moko256 Thanks for implementing this, it looks great! ~~Have you verified that the CI output now contains a stack traces on Window? If no, perhaps consider creating a draft PR on the engine that contains these changes but then crashes in a test.~~~

EDIT: See below: #36202 (comment)

loic-sharma · 2022-09-19T17:54:42Z

Here's example output from CI backtrace (see #36254):

[64/456] FlutterPlatformNodeDelegateTest.canGetAXNode (612 ms)
...
[----------] 1 test from FlutterPlatformNodeDelegateTest
[ RUN      ] FlutterPlatformNodeDelegateTest.canGetAXNode
[ERROR:accessibility_bridge.cc(146)] Check failed: false.
[ERROR:flutter/fml/backtrace.cc(135)] Caught signal SIGABRT during program execution.
Frame 0: 0000000142DBBDB9 abort
Frame 1: 0000000142AF8819 base::KillProcess
Frame 2: 0000000142AF87EB base::LogMessage::~LogMessage
Frame 3: 000000014023935A flutter::AccessibilityBridge::OnNodeCreated
Frame 4: 0000000142A1D8D9 ui::AXTree::NotifyNodeHasBeenReparentedOrCreated
Frame 5: 0000000142A1B6CC ui::AXTree::Unserialize
Frame 6: 000000014023840D flutter::AccessibilityBridge::CommitUpdates
Frame 7: 00000001401C0669 flutter::testing::FlutterPlatformNodeDelegateTest_canGetAXNode_Test::TestBody
Frame 8: 0000000142C80080 testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,void>
Frame 9: 0000000142C62511 testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
Frame 10: 0000000142C4882F testing::Test::Run
Frame 11: 0000000142C49038 testing::TestInfo::Run
Frame 12: 0000000142C496AC testing::TestSuite::Run
Frame 13: 0000000142C53985 testing::internal::UnitTestImpl::RunAllTests
Frame 14: 0000000142C853C0 testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
Frame 15: 0000000142C65171 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
Frame 16: 0000000142C53514 testing::UnitTest::Run
Frame 17: 000000014022CB41 RUN_ALL_TESTS
Frame 18: 000000014022C6C8 main
Frame 19: 0000000142D48FB4 invoke_main
Frame 20: 0000000142D48E9E __scrt_common_main_seh
Frame 21: 0000000142D48D5E __scrt_common_main
Frame 22: 0000000142D49049 mainCRTStartup
Frame 23: 00007FF86F127BD4 BaseThreadInitThunk
Frame 24: 00007FF86F2ECED1 RtlUserThreadStart

[64/456] FlutterPlatformNodeDelegateTest.canGetAXNode returned/aborted with exit code 3 (612 ms)

Looks great! 🎉

cbracken · 2022-09-19T22:54:08Z

fml/backtrace.cc

 namespace fml {

 static std::string kKUnknownFrameName = "Unknown";

 static std::string DemangleSymbolName(const std::string& mangled) {
+#if FML_OS_WIN


@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:

https://github.com/flutter/engine/blob/main/fml/file.h

https://github.com/flutter/engine/blob/main/fml/platform/posix/file_posix.cc

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

cbracken

I haven't looked but do the existing tests cover Windows?

loic-sharma · 2022-09-19T22:58:36Z

fml/backtrace.cc

 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);


Should/could we use abseil's GetStackTrace here?

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

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

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

moko256 · 2022-09-20T17:07:13Z

I am seeing the current master's behavior on host_linux_unopt, but many functions haven't symbolized.
I ran readelf -s -W out/host_debug_unopt/fml_unittests, but it seems that only gtest and std's symbol are contained.

auto trace = BacktraceHere(0);
std::cout << trace << std::endl;

Frame 0: 0x55655013e0ec Unknown
Frame 1: 0x55655225eebb Unknown
Frame 2: 0x556552251a67 Unknown
Frame 3: 0x556552240eb3 testing::Test::Run()
Frame 4: 0x556552241574 testing::TestInfo::Run()
Frame 5: 0x556552241b7d testing::TestSuite::Run()
Frame 6: 0x55655224b321 testing::internal::UnitTestImpl::RunAllTests()
Frame 7: 0x55655226203b Unknown
Frame 8: 0x556552253457 Unknown
Frame 9: 0x55655224af1f testing::UnitTest::Run()
Frame 10: 0x5565502c61e1 Unknown
Frame 11: 0x5565502c60b2 Unknown
Frame 12: 0x7f442f1be09b __libc_start_main
Frame 13: 0x55655013ab6a _start

cbracken · 2022-09-20T20:19:21Z

@moko256 can you try running with out/host_debug_unopt/exe.unstripped/fml_unittests instead? The default Linux binaries under the build directory are stripped.

moko256 · 2022-09-21T06:50:19Z

@cbracken I find it. Thanks.

moko256 · 2022-09-21T15:58:03Z

I replaced signal handling with absl::InstallFailureSignalHandler and added a death test to check it is installed. It also works on Windows.

cbracken

lgtm other than the removal of BacktraceHere, though I'll admit I hadn't actually used it in my own debugging (I usually just use lldb if I want to grab a backtrace).

@chinmaygarde may have thoughts on that.

cbracken · 2022-09-21T17:19:44Z

fml/backtrace.h

 #include "flutter/fml/macros.h"

 namespace fml {

-std::string BacktraceHere(size_t offset = 0);


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

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.

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)

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?

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

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.

loic-sharma

LGTM but please also get approval from @cbracken or @chinmaygarde before merging as they're more familiar with the history here

chinmaygarde · 2022-09-22T20:07:27Z

fml/backtrace_unittests.cc


 namespace fml {
 namespace testing {

-TEST(BacktraceTest, CanGatherBacktrace) {
+TEST(BacktraceDeathTest, CanGatherBacktrace) {


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

This reverts commit edfe8c1.

moko256 · 2022-09-23T18:09:01Z

Acoording to CI results, somehow on macOS ,the absl::GetStackTrace writes no stacktrace and
absl::debugging_internal::StackTraceWorksForTest returns false.
If absl::GetStackTrace doesn't support on macOS, the absl's signal handler doesn't support as well.

It looks like the absil's source code contains the implementation using execinfo, and the ifdefs for macOS, but I can't investigate the actual reason right away. I'll revert the PR to the first commit, or defer to other people on investigation on macOS.

zanderso · 2022-09-29T20:02:33Z

@cbracken @loic-sharma @chinmaygarde @dnfield From PR review triage: It looks like this PR is ready for another review pass.

…r/engine#36202)

moko256 added the platform-windows label Sep 16, 2022

moko256 commented Sep 16, 2022

View reviewed changes

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

760e866

moko256 force-pushed the moko256_win_fml_backtrace branch from 450e5f2 to 760e866 Compare September 16, 2022 13:08

loic-sharma reviewed Sep 16, 2022

View reviewed changes

fml/backtrace.cc Show resolved Hide resolved

loic-sharma reviewed Sep 16, 2022

View reviewed changes

loic-sharma mentioned this pull request Sep 19, 2022

Please ignore: testing Windows stack trace on CI #36254

Closed

8 tasks

cbracken reviewed Sep 19, 2022

View reviewed changes

loic-sharma reviewed Sep 19, 2022

View reviewed changes

moko256 added 3 commits September 20, 2022 15:51

remove empty break

6b26b25

use absl::GetStackTrace

aa92903

remove __cxxabiv1::__cxa_demangle

1d5b4fc

replace with absil's signal handler

09649c1

moko256 added 4 commits September 21, 2022 15:59

fix format

dca3616

cleanup unused vars

dba4017

fix test to enable to run without symbol

a5b212c

mark BacktraceTest as DeathTest

4fe24c3

cbracken reviewed Sep 21, 2022

View reviewed changes

loic-sharma approved these changes Sep 21, 2022

View reviewed changes

cbracken approved these changes Sep 21, 2022

View reviewed changes

chinmaygarde approved these changes Sep 22, 2022

View reviewed changes

moko256 added 3 commits September 24, 2022 01:02

add comment

393233e

test if stacktrace is supported

edfe8c1

Revert "test if stacktrace is supported"

30105e7

This reverts commit edfe8c1.

moko256 added 3 commits September 25, 2022 23:24

Rollback to 6b26b25

51eaf5d

Restore some changes.

8df181b

Remove unused include.

6d5f7c2

moko256 requested review from cbracken, loic-sharma and chinmaygarde September 25, 2022 14:29

moko256 added 10 commits September 26, 2022 01:04

Unite adding to offset

a5a124a

Remove test output

763d211

Add test for BacktraceHere(0)

987eee5

Skip test if binary has no symbol

d6c28d9

Use gmock's matcher

20b6dc4

Fix private header

ce81ec5

Force inlining

a4f3850

Skip if backtrace is not supported

b6c657e

Inline manually

a99f0dc

Rollback to 763d211 and fix comment

24b2aa5

chinmaygarde approved these changes Oct 6, 2022

View reviewed changes

chinmaygarde added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Oct 20, 2022

auto-submit bot merged commit 7c84844 into flutter:main Oct 20, 2022

engine-flutter-autoroll mentioned this pull request Oct 20, 2022

Roll Flutter Engine from 3b6ecf9f1a66 to 7c8484437d88 (2 revisions) flutter/flutter#113795

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2022

7c8484437 Add Windows support for //flutter/fml/backtrace.h (flutte…

09de4ec

…r/engine#36202)

engine-flutter-autoroll mentioned this pull request Oct 20, 2022

Roll Flutter Engine from 3b6ecf9f1a66 to 2b21a3125ca2 (6 revisions) flutter/flutter#113798

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2022

7c8484437 Add Windows support for //flutter/fml/backtrace.h (flutte…

fda75a9

…r/engine#36202)

cbracken mentioned this pull request Jul 24, 2023

Install backtrace handlers on Windows on all unit-test runs. flutter/flutter#98432

Closed

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

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

Uh oh!

Conversation

moko256 commented Sep 16, 2022

Pre-launch Checklist

Uh oh!

moko256 Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moko256 Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loic-sharma commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moko256 Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moko256 Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moko256 commented Sep 20, 2022

Uh oh!

cbracken commented Sep 20, 2022

Uh oh!

moko256 commented Sep 21, 2022

Uh oh!

moko256 commented Sep 21, 2022

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

moko256 Sep 16, 2022 •

edited

Loading

loic-sharma Sep 16, 2022 •

edited

Loading

moko256 Sep 20, 2022 •

edited

Loading

loic-sharma Sep 16, 2022 •

edited

Loading

loic-sharma commented Sep 16, 2022 •

edited

Loading

loic-sharma commented Sep 19, 2022 •

edited

Loading

cbracken Sep 19, 2022 •

edited

Loading

moko256 Sep 20, 2022 •

edited

Loading

moko256 Sep 20, 2022 •

edited

Loading

cbracken Sep 22, 2022 •

edited

Loading