[PATCH] D35085: Respect exitcode sanitizer option in UBSan

[Francis Ricci via Phabricator via llvm-commits]

fjricci added inline comments.


================
Comment at: lib/ubsan/ubsan_init.cc:44
   InitializeSuppressions();
+  Atexit(ReportFailure);
 }
----------------
vsk wrote:
> fjricci wrote:
> > rsmith wrote:
> > > Is this the right place to do this? If UBSan is hosted inside another sanitizer, does that sanitizer already set up such a callback?
> > That's a good question. I know that all of the tests pass for me on Linux and Darwin (both for UBSan hosted inside other sanitizers and for the other sanitizers themselves), but that's been the extent of my testing. Should that be sufficient, or should I investigate further?
> Asan implements exitcode support by calling Die after the first error. Tsan has an interceptor for exit, and implements exitcode support in that interceptor.
> 
> I only took a quick look but I don't think any of the sanitizers call exit() in an atexit() handler, which seems a bit suspicious to me. Couldn't that cause an infinite loop?
> 
> If calling exit() in an atexit() handler has safe/defined behavior, I'm fine with adding the handler here (though we should only add it if common_flags()->exitcode is set).
> 
LSan uses Atext(DoLeakCheck), and DoLeakCheck() calls Die(), so it should be safe.


================
Comment at: test/ubsan/TestCases/Float/cast-overflow.cpp:3
 // RUN: %run %t _
-// RUN: %env_ubsan_opts=print_summary=1:report_error_type=1 %run %t 0 2>&1 | FileCheck %s --check-prefix=CHECK-0
-// RUN: %run %t 1 2>&1 | FileCheck %s --check-prefix=CHECK-1
-// RUN: %run %t 2 2>&1 | FileCheck %s --check-prefix=CHECK-2
-// RUN: %run %t 3 2>&1 | FileCheck %s --check-prefix=CHECK-3
-// RUN: %run %t 4 2>&1 | FileCheck %s --check-prefix=CHECK-4
-// RUN: %run %t 5 2>&1 | FileCheck %s --check-prefix=CHECK-5
-// RUN: %run %t 6 2>&1 | FileCheck %s --check-prefix=CHECK-6
+// RUN: %env_ubsan_opts=print_summary=1:report_error_type=1 not %run %t 0 2>&1 | FileCheck %s --check-prefix=CHECK-0
+// RUN: not %run %t 1 2>&1 | FileCheck %s --check-prefix=CHECK-1
----------------
vsk wrote:
> This test doesn't use the exitcode flag, so why does this test need to change? I have the same question about the other test case changes here.
The exitcode flag defaults to returning failure on error (`lib/sanitizer_common/sanitizer_flags.inc`), which I think makes sense.


https://reviews.llvm.org/D35085