[PATCH] D12318: [ASan] Enable optional ASan recovery
Yury Gribov via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 28 00:53:15 PDT 2015
ygribov added a comment.
> Still missing the Darwin interface symbols file.
Right, I iterated Phab's comments and completely forgot about this item. I can add a test but I don't have Mac at hand so I'll have to XFAIL it if problem arises after commit.
================
Comment at: lib/asan/asan_flags.inc:147
@@ +146,3 @@
+ "WARNING: USE AT YOUR OWN RISK!)")
+ASAN_FLAG(uptr, max_errors, 10,
+ "Maximum number of errors reported if halting-on-error is disabled")
----------------
kcc wrote:
> Shouldn't this go to sanitizer_common?
Hm, perhaps we should implement it for UBSan/MSan and then move to sanitizer_common?
================
Comment at: lib/asan/asan_interceptors.cc:52
@@ -51,2 +51,3 @@
#define ACCESS_MEMORY_RANGE(ctx, offset, size, isWrite) do { \
+ if (QuickCheckSilence(false)) break; \
uptr __offset = (uptr)(offset); \
----------------
kcc wrote:
> why do we need it here?
This enables fast path once error reporting limit is reached. Otherwise an error in a busy loop may cause significant (100x) performance degradation. One might argue that we should not be interested in continuing execution at all after we can't report more errors but in fully sanitized distribution it may be beneficial to continue execution to provoke errors in dependent processes.
================
Comment at: lib/asan/asan_interceptors.cc:73
@@ -71,3 +72,3 @@
GET_CURRENT_PC_BP_SP; \
- __asan_report_error(pc, bp, sp, __bad, isWrite, __size, 0); \
+ __asan_report_error(pc, bp, sp, __bad, isWrite, __size, 0, false);\
} \
----------------
filcab wrote:
> Why `false`? Shouldn't it be the flag for `halt_on_error`?
This is covered by quick check above.
================
Comment at: lib/asan/asan_internal.h:124
@@ +123,3 @@
+// unnecessary.
+static inline bool QuickCheckSilence(bool is_error) {
+ if (LIKELY(flags()->halt_on_error))
----------------
kcc wrote:
> Does it have to be static inline?
> Also, I don't like the name. But it depends on another question later.
It doesn't but I thought this would be more efficient.
================
Comment at: lib/asan/asan_internal.h:125
@@ +124,3 @@
+static inline bool QuickCheckSilence(bool is_error) {
+ if (LIKELY(flags()->halt_on_error))
+ return false;
----------------
filcab wrote:
> So, I see two things that can happen:
>
> `halt_on_error` is true:
> We go through the `if` once (maybe more times, if we find several errors in several threads).
> `QuickCheckSilence` returns false
> `LIKELY` Isn't really doing much. Tweaking a jump that is executed once doesn't really matter.
>
> `halt_on_error` is false:
> We go through the `if` roughly `max_errors` times (let's say around 100, which seems to me is already a too big number for `max_errors` (if you actually manage to skip 100 errors and still execute code, that's a huge amount of luck!)
>
> From these two, I would actually say that, at most, you should have an `UNLIKELY`, and not a `LIKELY`. *If* we execute that `if` more than once, then I should always expect it to be false.
>
> Still, even in the case where we execute it "a bunch of times" we're still "almost never" executing it. `QuickCheckSilence` is called only when reporting errors, which means that:
> If `halt_on_error`, then the program is terminating right after this report. We're not winning anything.
> If `halt_on_error`, then the `LIKELY` is reversed. And we're still not winning much, we're not expecting this code to be executed more than `max_errors`, which, I'm assuming, will always be a small number.
>
> My opinion is that it's not making the code faster, and it's making it less clear.
>
> You mentioned performance increases. What tests were you running, and how?
> halt_on_error is true:
> We go through the if once (maybe more times, if we find several errors in several threads).
I don't quite see it like this. Quick check may be called in non-fatal context (e.g. to enable fast path in ACCESS_MEMORY_RANGE). Information about likeliness may inform compiler to e.g. allocate registers in favor of preferred code or align it in a more appropriate way (e.g. to the same cache line).
> You mentioned performance increases. What tests were you running, and how?
Check https://www.mail-archive.com/address-sanitizer@googlegroups.com/msg00403.html . Adding unlikely hint to noreturning if improved code by some 40%.
================
Comment at: lib/asan/asan_internal.h:131
@@ +130,3 @@
+ if (is_error)
+ atomic_fetch_add(&num_reported_errors, 1, memory_order_relaxed);
+ return false;
----------------
filcab wrote:
> You can change the previous `atomic_load` to be an `atomic_fetch_add` of 0 or 1, no?
Well yeah but this would add a risk of overflow.
================
Comment at: lib/asan/asan_report.cc:29
@@ +28,3 @@
+#define QUICK_CHECK_SILENCE \
+ if (QuickCheckSilence(true)) return
+
----------------
kcc wrote:
> Don't define this macro, just use the C++ line as is
Ok.
================
Comment at: lib/asan/asan_report.cc:1014
@@ -966,3 +1013,3 @@
void __asan_report_error(uptr pc, uptr bp, uptr sp, uptr addr, int is_write,
- uptr access_size, u32 exp) {
+ uptr access_size, u32 exp, int fatal) {
ENABLE_FRAME_POINTER;
----------------
kcc wrote:
> I think we should revert the name and the meaning of the flag:
> make it "int recover" instead of "int fatal" to match the flag.
> No?
>
> Also, we need to make the parameter more clear.
> One way is to declare a enum {ABORT_ON_ERROR, RECOVER_ON_ERROR} and pass it as the parameter.
> Another is to
> define two more internal functions
> __asan_report_error_and_abort() &
> __asan_report_error_and_maybe_recover()
> and instead of calling __asan_report_error with a constant in the last parameter call one of these.
>
> At the very least, write /*recover=*/false
Frankly I don't like the idea of adding fatal/recover parameter to public interface at all. Fatality should be controlled by ASAN_OPTIONS and be transparent for user code. Thoughts?
================
Comment at: test/Driver/fsanitize.c:168
@@ -167,3 +167,3 @@
-// CHECK-RECOVER: "-fsanitize-recover={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|vla-bound|alignment|null|vptr|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
+// CHECK-RECOVER: "-fsanitize-recover={{((address|signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|vla-bound|alignment|null|vptr|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
// CHECK-NO-RECOVER-NOT: sanitize-recover
----------------
filcab wrote:
> Why didn't you update the number too? Does the test pass?
Yup, it did. I'll reexamine it.
================
Comment at: test/asan/TestCases/Linux/halt_on_error-torture.cc:1
@@ +1,2 @@
+// Stress test recovery mode with many threads.
+//
----------------
filcab wrote:
> Why is this test linux only?
Mainly because I'm afraid that Mac/BSD maintainers will shout at me when I break their bots)
================
Comment at: test/asan/TestCases/Linux/halt_on_error-torture.cc:29
@@ +28,3 @@
+ // Expect error collisions here
+ // CHECK: AddressSanitizer: use-after-poison
+ volatile int idx = 0;
----------------
kcc wrote:
> don't we want to check that multiple errors were reported?
>
>
> check how the tests set ASAN_OPTIONS nowadays.
> There were some massive changes...
Ok.
================
Comment at: test/asan/TestCases/Linux/halt_on_error-torture.cc:44
@@ +43,3 @@
+
+ pthread_t *tids = (pthread_t *)malloc(nthreads * sizeof(pthread_t));
+
----------------
kcc wrote:
> use C++ new/delete
Ok.
Repository:
rL LLVM
http://reviews.llvm.org/D12318
More information about the llvm-commits
mailing list