[PATCH] D35085: Respect exitcode sanitizer option in UBSan

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a subscriber: eugenis.
vsk added a comment.

Could you explain how you intend to make use of these changes? As it stands I'm not convinced this patch is headed in the right direction, but I'd like to give some more constructive feedback.



================
Comment at: lib/ubsan/ubsan_init.cc:44
   InitializeSuppressions();
+  Atexit(ReportFailure);
 }
----------------
fjricci wrote:
> 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.
I'm still not sure that calling exit in an atexit handler is a good idea. Example: there are two instrumented shared libraries loaded into a program. Both would get atexit handlers. Calling exit() in one handler would prevent the other handlers from being run. This would break -fprofile-instr-generate, and user-provided atexit handlers.

CC'ing @eugenis for any comment since he reviewed D12120, which revamped exitcode handling.


================
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
----------------
fjricci wrote:
> 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.
I don't think that enabling a sanitizer should, by default, change the way a program's exit codes work. Users may intend to quit with specific exit codes in scenarios which trigger sanitizer diagnostics.



https://reviews.llvm.org/D35085