[sanitizer] Fall back to fast unwinder

[MaskRay]

Summary

-fno-exceptions -fno-asynchronous-unwind-tables compiled programs don't
produce .eh_frame on Linux and other ELF platforms, so the slow unwinder cannot
print stack traces. Just fall back to the fast unwinder: this allows
-fno-asynchronous-unwind-tables without requiring the sanitizer option
fast_unwind_on_fatal=1

[vitalybuka]

compiler-rt/lib/lsan/lsan.cpp

38 ↗

(On Diff #343576)

what do you think if we have a single
void __sanitizer::BufferedStackTrace::UnwindImpl in sanitizer_common

and only make sanitizers to implement own GetStackBounds()

we have common GetThreadStackTopAndBottom but it's going to slower without info some sanitizers have

[MaskRay]

compiler-rt/lib/lsan/lsan.cpp

38 ↗

(On Diff #343576)

Happy to investigate the code sharing idea after this patch.

Doing the simplification in this patch would change too many things.

[vitalybuka]

compiler-rt/lib/lsan/lsan.cpp

38 ↗

(On Diff #343576)

Could you land BufferedStackTrace::UnwindImpl change in a separate first patch
and BufferedStackTrace::Unwind in the second
I believe the first one is expected to be NFC
Can be useful to bisect if I am wrong and it actually break stuff somewhere.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp

87

if (size > 2 || size >= max_depth)

[MaskRay]

Pushed the simplification as 261d6e05d5574bec753ea6b7e9a7f99229927753

Will wait til tomorrow to ensure the first commit is stable.

[uweigand]

The new test case fails on s390x with "WARNING: Symbolizer buffer too small" from SymbolizerProcess::ReadFromSymbolizer, and it is indeed true that the full backtrace at this point does not fit in the default buffer size of 16 KB. In the innermost function we have 1024 instances of the string "std::vector<int, std::allocator<int> >" in the function type, which in itself is already 40 KB. Plus the higher stack frames as well ...

I can make it pass again by increasing the buffer size to 128 KB. But I'm not quite sure if that is the intended behavior here?

[MaskRay]

In D102046#2766100, @uweigand wrote:

The new test case fails on s390x with "WARNING: Symbolizer buffer too small" from SymbolizerProcess::ReadFromSymbolizer, and it is indeed true that the full backtrace at this point does not fit in the default buffer size of 16 KB. In the innermost function we have 1024 instances of the string "std::vector<int, std::allocator<int> >" in the function type, which in itself is already 40 KB. Plus the higher stack frames as well ...

I can make it pass again by increasing the buffer size to 128 KB. But I'm not quite sure if that is the intended behavior here?

I copied the test code from ../symbolize_stack.cpp, so presumably that file may have a similar issue?
Both give me WARNING: Symbolizer buffer too small on my x86-64 Linux machine.

How about s/A<10>()/A<7>()/? Does that pass on s390x?

[uweigand]

In D102046#2766553, @MaskRay wrote:

In D102046#2766100, @uweigand wrote:

The new test case fails on s390x with "WARNING: Symbolizer buffer too small" from SymbolizerProcess::ReadFromSymbolizer, and it is indeed true that the full backtrace at this point does not fit in the default buffer size of 16 KB. In the innermost function we have 1024 instances of the string "std::vector<int, std::allocator<int> >" in the function type, which in itself is already 40 KB. Plus the higher stack frames as well ...

I can make it pass again by increasing the buffer size to 128 KB. But I'm not quite sure if that is the intended behavior here?

I copied the test code from ../symbolize_stack.cpp, so presumably that file may have a similar issue?
Both give me WARNING: Symbolizer buffer too small on my x86-64 Linux machine.

Well, the symbolize_stack.cpp passes for me on s390x for some reason.

How about s/A<10>()/A<7>()/? Does that pass on s390x?

This passes, yes.

[uweigand]

In D102046#2767927, @uweigand wrote:

In D102046#2766553, @MaskRay wrote:

How about s/A<10>()/A<7>()/? Does that pass on s390x?

This passes, yes.

Can we make that change in mainline? I'd really like to get our build bots green again ...

[MaskRay]

In D102046#2779171, @uweigand wrote:

In D102046#2767927, @uweigand wrote:

In D102046#2766553, @MaskRay wrote:

How about s/A<10>()/A<7>()/? Does that pass on s390x?

This passes, yes.

Can we make that change in mainline? I'd really like to get our build bots green again ...

Pushed 3a678fe3e29fe48d9b4efc936edd2ac43c720bae

You may want to investigate ../symbolize_stack.cpp further why that test passes while this fails...
And why "WARNING: Symbolizer buffer too small" is benign on other arches while being a hard error on s390x...

[uweigand]

In D102046#2780372, @MaskRay wrote:

In D102046#2779171, @uweigand wrote:

In D102046#2767927, @uweigand wrote:

In D102046#2766553, @MaskRay wrote:

How about s/A<10>()/A<7>()/? Does that pass on s390x?

This passes, yes.

Can we make that change in mainline? I'd really like to get our build bots green again ...

Pushed 3a678fe3e29fe48d9b4efc936edd2ac43c720bae

Thanks! With a bunch unrelated issues also resolved, all our build bots are finally green again!

You may want to investigate ../symbolize_stack.cpp further why that test passes while this fails...
And why "WARNING: Symbolizer buffer too small" is benign on other arches while being a hard error on s390x...

I've looked into this a bit more, and the actual problem seems to be that for some reason the unwinder fails to unwind beyond the innermost RecursiveTemplateFunction frame. Now, in the A<7> case, the line printed for that innermost frame already matches the {{vector<.*vector<.*vector<.*vector<.*vector<}} string that FileCheck checks for, and therefore the test passes. In the A<10> case, for the innermost frame, no decoded line is printed (because it exceeds the buffer size), and because unwind fails to display outer frames, that FileCheck string never matches anywhere, and the test fails. On other target where the unwinder (presumably) works, the innermost frame will still have no decoded line, but the FileCheck string will match in one of the outer frames, and therefore the test passes again.

I'll have a look why the unwinder doesn't work here.

[uweigand]

Ah wait: the test case uses -fno-asynchronous-unwind-tables ! This explains why unwind cannot work - on s390x the ABI requires .eh_frame everywhere, and there is no alternative unwind mechanism via a frame pointer. The "fast unwinder" instead uses the (deprecated) "backchain" mechanism, but that is off by default and requires use of the -mbackchain option. That option is actually set by the various .lit.cfg files, but I guess the special invocation in this test case ignores those flags?

I can confirm that the A<10> version also passes on s390x when adding -mbackchain to the compiler command line.

[DavidSpickett]

We have the same test failing on thumb because the fast unwinder doesn't (can't, for various reasons) work. Perhaps:

// REQUIRES: fast-unwinder-works

[DavidSpickett]

I have implemented that requires for Arm in https://reviews.llvm.org/D103512.

Perhaps the same could apply to s390x?

[uweigand]

In D102046#2793430, @DavidSpickett wrote:

I have implemented that requires for Arm in https://reviews.llvm.org/D103512.

Perhaps the same could apply to s390x?

I mean, the fast unwinder does work on s390x (as opposed to Arm), it just requires passing the -mbackchain option at compiler time. So I think this is a bit different.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp

if (max_depth == 1) {
     return;
   }
   if (!WillUseFastUnwind(request_fast_unwind)) {
 #if SANITIZER_CAN_SLOW_UNWIND
     if (context)
       UnwindSlow(pc, context, max_depth);
     else
       UnwindSlow(pc, max_depth);
+    // If there are too few frames, the program may be built with
+    // -fno-asynchronous-unwind-tables. Fall back to fast unwinder below.
+    if (size > 2)
+      return;
 #else
     UNREACHABLE("slow unwind requested but not available");
 #endif
-  } else {
-    UnwindFast(pc, bp, stack_top, stack_bottom, max_depth);
   }
+  UnwindFast(pc, bp, stack_top, stack_bottom, max_depth);
 }
 
 static int GetModuleAndOffsetForPc(uptr pc, char *module_name,
                                    uptr module_name_len, uptr *pc_offset) {
   const char *found_module_name = nullptr;
   bool ok = Symbolizer::GetOrInit()->GetModuleNameAndOffsetForPC(
       pc, &found_module_name, pc_offset);
 

compiler-rt/test/sanitizer_common/TestCases/Linux/symbolize_stack_fp.cpp

+/// When the main program doesn't use .eh_frame, the slow unwinder does not work.
+/// Test that we can fall back to the fast unwinder.
+// RUN: %clangxx -O0 -g1 -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer %s -o %t
+// RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=SEC
+// RUN: %run %t 2>&1 | FileCheck %s
+
+/// No .eh_frame && -g => .debug_frame
+// SEC: .debug_frame
+
+#include <sanitizer/common_interface_defs.h>
+#include <vector>
+
+template <int N>
+struct A {
+  template <class T>
+  void RecursiveTemplateFunction(const T &t);
+};
+
+template <int N>
+template <class T>
+__attribute__((noinline)) void A<N>::RecursiveTemplateFunction(const T &) {
+  std::vector<T> t;
+  return A<N - 1>().RecursiveTemplateFunction(t);
+}
+
+template <>
+template <class T>
+__attribute__((noinline)) void A<0>::RecursiveTemplateFunction(const T &) {
+  __sanitizer_print_stack_trace();
+}
+
+int main() {
+  // CHECK: {{vector<.*vector<.*vector<.*vector<.*vector<}}
+  A<10>().RecursiveTemplateFunction(0);
+}