[compiler-rt][test] Fix flake in symbolize_stack test

[paulkirth]

Summary

Addresses tests flakes described in
https://github.com/llvm/llvm-project/issues/55460

The test being updated can fail in FileCheck to match when given long
enough stack traces. This can be problematic when file system paths
become long enough to cause the majority of the long function name to
become truncated. We found in our CI that the truncated output would
often fail to match, thereby causing the test to fail when it should not.

Here we change the test to match on sybolizer output that should be more
reliable than matching inside the long function name.

[paulkirth]

For context: https://discourse.llvm.org/t/post-mortem-on-test-flake-in-sanitizer-common/62657 describes how I got here.

I'm open to other suggestions, bu this flake is occuring quite frequently in Fuchsia's clang CI, and the only purpose of the test is to ensure it doesn't crash when function names are long.

I also considered using GTest, but that has its own issues, since this test links against all the various sanitizers, and I didn't see a clean way to do that.

Removing the FileCheck bit altogether is an option but may be a step too far.

[paulkirth]

Just match on any frame in the symbolizer output.

[leonardchan]

compiler-rt/test/sanitizer_common/TestCases/symbolize_stack.cpp

35

Will this just match on any digit we see in the output?

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/symbolize_stack.cpp

35

From https://discourse.llvm.org/t/post-mortem-on-test-flake-in-sanitizer-common/62657 it looks like a bug which needs to be fixed

[paulkirth]

compiler-rt/test/sanitizer_common/TestCases/symbolize_stack.cpp

35

Will this just match on any digit we see in the output?

it will match when the symbolizer prints a stack frame, which is #<frame number>, so it should match that.

[paulkirth]

@vitalybuka I'm hoping to just address the flaky test for now. There's some value in ensuring that long output doesn't crash symbolization w/in sanitizer runtimes, so I've opted to keep the test, and try to make it less prone to flake. For now, I'm happy just to remove the flake so we can update our Toolchain.

Is there something specific you think we should do to address the underlying problem?

[vitalybuka]

In D126102#3536037, @paulkirth wrote:

@vitalybuka I'm hoping to just address the flaky test for now. There's some value in ensuring that long output doesn't crash symbolization w/in sanitizer runtimes, so I've opted to keep the test, and try to make it less prone to flake. For now, I'm happy just to remove the flake so we can update our Toolchain.

I would prefer you just disable the test for your platform.
We still want to see that the tool produce some meaningful output.

Is there something specific you think we should do to address the underlying problem?

No particular ideas yet. That's just my impression from your investigation.

[paulkirth]

In D126102#3536038, @vitalybuka wrote:

I would prefer you just disable the test for your platform.

The failures we see are for AArch64 and x86_64 Linux... so are you saying we should disable the test for Linux?

We still want to see that the tool produce some meaningful output.

I'm not convinced that output is meaningful at all. The correctness of the symbolizer's output is already tested elsewhere in a more reliable way. Can you explain why matching part of the long function name makes the test better? Especially given that we see it isn't reliably output?

[vitalybuka]

In D126102#3536097, @paulkirth wrote:

In D126102#3536038, @vitalybuka wrote:

I would prefer you just disable the test for your platform.

The failures we see are for AArch64 and x86_64 Linux... so are you saying we should disable the test for Linux?

Yes, even "// UNSUPPORTED: *" with FIXME is fine

We still want to see that the tool produce some meaningful output.

I'm not convinced that output is meaningful at all. The correctness of the symbolizer's output is already tested elsewhere in a more reliable way. Can you explain why matching part of the long function name makes the test better? Especially given that we see it isn't reliably output?

We test the bad case where output is huge. E.g. another way to fail that is to output nothing without crash.

[paulkirth]

Restore exisiting checks, and mark the test unsupported on Linux

[paulkirth]

@vitalybuka what if we just changed the implementation to return whatever it had read so far?

so in https://github.com/llvm/llvm-project/blob/06fee478d217a9fbd2ba31f92bc595ed327635a5/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp#L535, we just break instead of setting the read_len = 0, and allow the filled buffer to be output?

[vitalybuka]

In D126102#3537519, @paulkirth wrote:

@vitalybuka what if we just changed the implementation to return whatever it had read so far?

so in https://github.com/llvm/llvm-project/blob/06fee478d217a9fbd2ba31f92bc595ed327635a5/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp#L535, we just break instead of setting the read_len = 0, and allow the filled buffer to be output?

It's internal API, we probably can just use InternalScopedString and append as needed

compiler-rt/test/sanitizer_common/TestCases/symbolize_stack.cpp

 // RUN: %clangxx -O0 %s -o %t && %run %t 2>&1 | FileCheck %s
 
 // Test that symbolizer does not crash on frame with large function name.
 
 // On Darwin LSan reports a false positive
 // XFAIL: darwin && lsan
 
+// FIXME: https://github.com/llvm/llvm-project/issues/55460
+// On Linux its possible for symbolizer output to be truncated and to match the
+// check below. Remove when the underlying problem has been addressed.
+// UNSUPPORTED: linux
+
 #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);
 }