[libunwind] Fix testing with sanitizers enabled

[ldionne]

Summary

When testing with sanitizers enabled, we need to link against a plethora
of system libraries. Using -nodefaultlibs like we used to breaks this,
and we would have to add all these system libraries manually, which is
not portable and error prone. Instead, stop using -nodefaultlibs so
that we get the libraries added by default by the compiler.

The only caveat with this approach is that we are now relying on the
fact that -L <path-to-local-libunwind> will cause the just built
libunwind to be selected before the system implementation (either of
libunwind or libgcc_s.so), which is somewhat fragile.

[MaskRay]

libunwind/test/configs/llvm-libunwind-shared.cfg.in

50

-ldl needs to be retained, otherwise on Linux x86-64:

ld.lld: error: undefined symbol: dladdr                                                                                                                                                                                                  
>>> referenced by forceunwind.pass.cpp                                                                                                                                                                                                   
>>>               /tmp/forceunwind-7489af.o:(stop(int, _Unwind_Action, unsigned long, _Unwind_Exception*, _Unwind_Context*, void*))                                                                                    
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)                                                            

error: command failed with exit status: 1                              

--                                                                     

********************                                                   
********************                                                   
Failed Tests (3):                                                      
  llvm-libunwind-shared.cfg.in :: forceunwind.pass.cpp                                                                                        
  llvm-libunwind-shared.cfg.in :: signal_unwind.pass.cpp                                                                                      
  llvm-libunwind-shared.cfg.in :: unwind_leaffunction.pass.cpp

libunwind/test/configs/llvm-libunwind-static.cfg.in

52

ditto for -ldl

[MaskRay]

When testing with sanitizers enabled, we need to link against a plethora of system libraries.

I do now know how to enable sanitizers for ninja check-unwind.

-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_USE_SANITIZER=Address -DLLVM_ENABLE_PROJECTS='clang' -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi;libunwind;compiler-rt' doesn't pass -fsanitize= to ninja check-unwind tests.

Using -nodefaultlibs like we used to breaks this, and we would have to add all these system libraries manually, which is not portable and error prone.

That said, I agree with removing -nodefaultlibs.

The tests will pass if -ldl is added back.

[ldionne]

Add back -ldl. Thanks for the review!

[ldionne]

Fix CI.

The unwind_leaffunction.pass.cpp test was failing with ubsan because it was
relying on undefined behavior to "do the right thing" and result in a SIGSEGV.
IMO it makes more sense to simply raise a SIGSEGV directly, since that is guaranteed
to work.

The only CI that will still be failing is the 32 bits CI. I traced it back to the fact
that there is no /usr/lib32/libstdc++.so provided on Ubuntu, which leads to ld being
unable to find the library for -lstdc++ (yes, we do link against the system's C++ library
unconditionally now -- we need to, otherwise the sanitizer builds can't work).

I don't know who relies on the 32 bits configuration. We have been supporting it historically,
but in practice it doesn't seem to be very well supported (for example the missing libstdc++
library on Ubuntu). Furthermore, we started seeing a bunch of novel and extremely hard to explain
issues when running 32 bits CI after upgrading our version of Ubuntu. I will create a separate
review with a proposal to remove support for building in 32 bits mode and see what happens there.

[ldionne]

I am going to make the 32 bit CI build a soft failure until we remove it entirely and ship this patch. See D114473 for removing the 32 bit multilib support.

[leonardchan]

The unwind_leaffunction.pass.cpp test was failing with ubsan because it was
relying on undefined behavior to "do the right thing" and result in a SIGSEGV.
IMO it makes more sense to simply raise a SIGSEGV directly, since that is guaranteed
to work.

It looks like using raise causes this test to fail on our aarch64 builder with exit code 255 (from the _Exit(-1) in signal_handler). I'm unsure why it would fail when using raise. These are a couple of things I noticed when debugging:

• If I replace raise(SIGSEGV) with kill(getpid(), SIGSEGV), the test passes normally. The man page for raise says this is functionally equivalent.
• If I manually print out the frame names from info.dli_sname, I notice with raise I only cover 2 frames (_Z14signal_handleri, __kernel_rt_sigreturn) but with kill I get 5 frames (_Z14signal_handleri, __kernel_rt_sigreturn, kill, _Z18crashing_leaf_funcv, main).
  • If I use the old UB approach, I get 4 frames (just excluding the call to kill).

Any ideas for why this might be the case?

[ldionne]

In D114385#3159448, @leonardchan wrote:

The unwind_leaffunction.pass.cpp test was failing with ubsan because it was
relying on undefined behavior to "do the right thing" and result in a SIGSEGV.
IMO it makes more sense to simply raise a SIGSEGV directly, since that is guaranteed
to work.

It looks like using raise causes this test to fail on our aarch64 builder with exit code 255 (from the _Exit(-1) in signal_handler). I'm unsure why it would fail when using raise. These are a couple of things I noticed when debugging:

• If I replace raise(SIGSEGV) with kill(getpid(), SIGSEGV), the test passes normally. The man page for raise says this is functionally equivalent.
• If I manually print out the frame names from info.dli_sname, I notice with raise I only cover 2 frames (_Z14signal_handleri, __kernel_rt_sigreturn) but with kill I get 5 frames (_Z14signal_handleri, __kernel_rt_sigreturn, kill, _Z18crashing_leaf_funcv, main).
  • If I use the old UB approach, I get 4 frames (just excluding the call to kill).

Any ideas for why this might be the case?

No, that doesn't ring a bell. I think this is a question for whoever provides the C Standard Library on your aarch64 bot. Perhaps libunwind is somehow unable to understand the stack frames below raise before it gets to the kernel? Honestly I'm kind of shooting in the dark.

I'd be fine with switching to kill(getpid(), SIGSEGV) temporarily to unbreak your bot, but I'd like to avoid doing a blind fix and get the ball rolling on this issue with the C library folks. Do you think you can provide a patch to switch to kill?

[leonardchan]

No, that doesn't ring a bell. I think this is a question for whoever provides the C Standard Library on your aarch64 bot. Perhaps libunwind is somehow unable to understand the stack frames below raise before it gets to the kernel? Honestly I'm kind of shooting in the dark.

I'd be fine with switching to kill(getpid(), SIGSEGV) temporarily to unbreak your bot, but I'd like to avoid doing a blind fix and get the ball rolling on this issue with the C library folks. Do you think you can provide a patch to switch to kill?

I have D114743 up for review. Asking around to see why we might be running into this issue before submitting.

[leonardchan]

Ok I did more digging and it looks like the issue is that my host libc doesn't have an FDE for raise, but it does have one for kill (which I don't have an explanation for). I suppose that would mean D114743 isn't a bad alternative, but I guess this means it could also fail again for any user with a libc that doesn't happen to have unwind info for kill (could be a wait-and-see scenario). If we want to avoid the potential missing unwind info altogether, we could perhaps having some inline assembly that does a bad memory reference, but that might not be very portable. Thoughts?

[phosek]

In D114385#3160079, @leonardchan wrote:

Ok I did more digging and it looks like the issue is that my host libc doesn't have an FDE for raise, but it does have one for kill (which I don't have an explanation for). I suppose that would mean D114743 isn't a bad alternative, but I guess this means it could also fail again for any user with a libc that doesn't happen to have unwind info for kill (could be a wait-and-see scenario). If we want to avoid the potential missing unwind info altogether, we could perhaps having some inline assembly that does a bad memory reference, but that might not be very portable. Thoughts?

I'd just point out that the host libc in this case is glibc shipped with Ubuntu 18.04 on arm64 which is a popular distribution (although it's possible that arm64 version doesn't receive as much testing as amd64).

[danielkiss]

libunwind/test/unwind_leaffunction.pass.cpp

41–43

adding a call here turns the leaf function into a not-leaf function which changes the intention of this test.

[ldionne]

libunwind/test/unwind_leaffunction.pass.cpp

41–43

Ugh, that's a good point. I'm not sure how to trigger SIGSEGV without invoking a function nor invoking undefined behavior. Do you have a suggestion?

Would using __builtin_trap() do any kind of good?

[ldionne]

In D114385#3159617, @leonardchan wrote:

I have D114743 up for review. Asking around to see why we might be running into this issue before submitting.

Thanks a lot. Per @danielkiss 's comment, I think we'll need to find a way to do what we want without calling any other function, otherwise we're missing the point of this test entirely. Go ahead and ship D114743 just so it unbreaks the bots until we find a real solution.

[danielkiss]

libunwind/test/unwind_leaffunction.pass.cpp

41–43

__builtin_trap() looks good from codegen / unwind info point of view.

it generates SIGILL on X86, while SIGSEGV on Aarch64 but with the below change it should work.

int main(int, char**) {
  signal(SIGSEGV, signal_handler);
  signal(SIGILL, signal_handler);

[danielkiss]

libunwind/test/unwind_leaffunction.pass.cpp

41–43

mybad, it is SIGTRAP on linux/aarch64.

signal(SIGTRAP, signal_handler);
signal(SIGILL, signal_handler);

libcxx/utils/ci/buildkite-pipeline.yml

artifact_paths:
       - "**/test-results.xml"
     agents:
       queue: "libcxx-builders"
       os: "linux"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2
+    soft_fail:
+        - exit_status: 1
     timeout_in_minutes: 120
 
   - label: "Single-threaded"
     command: "libcxx/utils/ci/run-buildbot generic-singlethreaded"
     artifact_paths:
       - "**/test-results.xml"
     agents:
       queue: "libcxx-builders"

libunwind/test/configs/llvm-libunwind-shared.cfg.in

 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{flags}',
     '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
     '-nostdinc++ -I {}/include {}'.format('@LIBUNWIND_SOURCE_DIR@', ' '.join(compile_flags))
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nodefaultlibs -L {0} -Wl,-rpath,{0} -lunwind -ldl -lc {1}'.format('@LIBUNWIND_LIBRARY_DIR@', ' '.join(link_flags))
+    '-L {0} -Wl,-rpath,{0} -lunwind -ldl {1}'.format('@LIBUNWIND_LIBRARY_DIR@', ' '.join(link_flags))
 ))
 config.substitutions.append(('%{exec}', ''))
 
 import os, site
 site.addsitedir(os.path.join('@LIBUNWIND_LIBCXX_PATH@', 'utils'))
 import libcxx.test.params, libcxx.test.newconfig, libcxx.test.newconfig
 libcxx.test.newconfig.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libunwind/test/configs/llvm-libunwind-static.cfg.in

 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{flags}',
     '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
     '-nostdinc++ -I {}/include {}'.format('@LIBUNWIND_SOURCE_DIR@', ' '.join(compile_flags))
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nodefaultlibs {}/libunwind.a -ldl -lc {}'.format('@LIBUNWIND_LIBRARY_DIR@', ' '.join(link_flags))
+    '{}/libunwind.a -ldl {}'.format('@LIBUNWIND_LIBRARY_DIR@', ' '.join(link_flags))
 ))
 config.substitutions.append(('%{exec}', ''))
 
 import os, site
 site.addsitedir(os.path.join('@LIBUNWIND_LIBCXX_PATH@', 'utils'))
 import libcxx.test.params, libcxx.test.newconfig, libcxx.test.newconfig
 libcxx.test.newconfig.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libunwind/test/forceunwind.pass.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // REQUIRES: linux
 
-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 // Basic test for _Unwind_ForcedUnwind.
 // See libcxxabi/test/forced_unwind* tests too.
 
 #include <assert.h>
 #include <dlfcn.h>
 #include <signal.h>
 #include <stdint.h>
 #include <stdio.h>

libunwind/test/frameheadercache_test.pass.cpp

-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 // The other libunwind tests don't test internal interfaces, so the include path
 // is a little wonky.
 #include "../src/config.h"
 
 // Only run this test under supported configurations.
 
 #if defined(_LIBUNWIND_USE_DL_ITERATE_PHDR) &&                                 \
     defined(_LIBUNWIND_USE_FRAME_HEADER_CACHE)

libunwind/test/libunwind_01.pass.cpp

-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 // TODO: Investigate these failures on x86_64 macOS back deployment
 // UNSUPPORTED: target=x86_64-apple-darwin{{.+}}
 
 #include <libunwind.h>
 #include <stdlib.h>
 #include <string.h>
 
 void backtrace(int lower_bound) {

libunwind/test/libunwind_02.pass.cpp

-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 #include <assert.h>
 #include <stdlib.h>
 #include <unwind.h>
 
 #define EXPECTED_NUM_FRAMES 50
 #define NUM_FRAMES_UPPER_BOUND 100
 
 _Unwind_Reason_Code callback(_Unwind_Context *context, void *cnt) {

libunwind/test/remember_state_leak.pass.sh.s

 # REQUIRES: target={{x86_64-.+-linux-gnu}}
 # RUN: %{build}
 # RUN: %{run}
 
-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 // TODO: Investigate this failure
 // XFAIL: 32bits-on-64bits
 
 # The following assembly is a translation of this code:
 #
 #   _Unwind_Reason_Code callback(int, _Unwind_Action, long unsigned int,
 #                                _Unwind_Exception*, _Unwind_Context*, void*) {
 #     return _Unwind_Reason_Code(0);

libunwind/test/signal_frame.pass.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Ensure that functions marked as signal frames are reported as such.
 
-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 // TODO: Investigate this failure on macOS
 // XFAIL: target={{.+}}-apple-darwin{{.+}}
 
 // UNSUPPORTED: libunwind-arm-ehabi
 
 // The AIX assembler does not support CFI directives, which
 // are necessary to run this test.
 // UNSUPPORTED: target=powerpc{{(64)?}}-ibm-aix

libunwind/test/signal_unwind.pass.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Ensure that the unwinder can cope with the signal handler.
 // REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}})
 
-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 #include <assert.h>
 #include <dlfcn.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
 #include <unistd.h>

libunwind/test/unw_getcontext.pass.cpp

-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 #include <assert.h>
 #include <libunwind.h>
 
 int main(int, char**) {
   unw_context_t context;
   int ret = unw_getcontext(&context);
   assert(ret == UNW_ESUCCESS);
   return 0;
 }

libunwind/test/unwind_leaffunction.pass.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Ensure that leaf function can be unwund.
 // REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}})
 
-// TODO: Investigate these failures
-// XFAIL: asan, tsan, ubsan
-
 #include <assert.h>
 #include <dlfcn.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
 #include <unistd.h>

 }
 
 void signal_handler(int signum) {
   (void)signum;
   _Unwind_Backtrace(frame_handler, NULL);
   _Exit(-1);
 }
 
-int* faultyPointer = NULL;
-
 __attribute__((noinline)) void crashing_leaf_func(void) {
-  *faultyPointer = 0;
+  raise(SIGSEGV);
 }
 
 int main(int, char**) {
   signal(SIGSEGV, signal_handler);
   crashing_leaf_func();
   return -2;
-}
+}