[PATCH] [UBSAN] Suppress an error report with given type information
Byoungyoung Lee
byoungyoung at chromium.org
Thu Jul 31 19:51:36 PDT 2014
================
Comment at: lib/sanitizer_common/sanitizer_suppressions.cc:25
@@ -25,1 +24,3 @@
+ "signal", "leak", "called_from_lib", "deadlock",
+ "ubsan_vptr"};
----------------
Alexey Samsonov wrote:
> Let the bikeshedding begin. I'd actually vote to exclude tool name from suppression type. Currently suppression names seem to correspond to the error type ("leak", "race", "deadlock"). Let's stick to this way and use smth. like "vptr_check" or "invalid_cast".
done
================
Comment at: lib/sanitizer_common/tests/sanitizer_suppressions_test.cc:73
@@ -72,3 +72,3 @@
// Ensure this test is up-to-date when suppression types are added.
- CHECK_EQ(SuppressionTypeCount, 8);
+ CHECK_EQ(SuppressionTypeCount, 9);
}
----------------
Alexey Samsonov wrote:
> Actually update the test to include new suppression name, as this comment recommends.
Done
================
Comment at: lib/ubsan/ubsan_diag.cc:317
@@ +316,3 @@
+
+void __ubsan::InitializeSuppressions() {
+ SuppressionContext::InitIfNecessary();
----------------
Alexey Samsonov wrote:
> You may now get rid of this function and call SuppressionContext::InitIfNecessary() from __ubsan::InitIfNecessary() directly.
I just wanted to keep the encapsulation as it would require ubsan_init to see SuppressionContext, but let me get rid of this wrapper function.
================
Comment at: lib/ubsan/ubsan_diag.cc:314
@@ +313,3 @@
+
+ ALIGNED(64) static char placeholder[sizeof(SuppressionContext)];
+ suppression_ctx = new(placeholder) SuppressionContext;
----------------
Alexey Samsonov wrote:
> Alexey Samsonov wrote:
> > No, I believe we must first move the code for initializing suppression context into sanitizer_common and make "suppressions" a common runtime flag. I will work on it soon. I've submitted the first patch for that in r214334.
> I think this should be done as of r214344.
When UBSan is running together with LSan, it seems LSan initializes SuppressionContext() before UBSan. I guess this has to be handled in the SuppressionContext(), but right now I simple implemented SuppressionContext::GetOrNull() to avoid the issue as I'm not sure whether my logic is correct.
================
Comment at: lib/ubsan/ubsan_flags.cc:26
@@ -25,2 +25,3 @@
f->print_stacktrace = false;
+ f->suppressions = 0;
----------------
Alexey Samsonov wrote:
> Let's use empty string for consistency with another sanitizers defining this flag.
done
================
Comment at: lib/ubsan/ubsan_flags.cc:33
@@ -31,1 +32,3 @@
+ ParseFlag(options, &f->suppressions, "suppressions",
+ "Suppression format filename to suppress an error report");
}
----------------
Alexey Samsonov wrote:
> I find this flag description confusing.
I think I improved the description, but not sure whether it is clear enough :(
================
Comment at: lib/ubsan/ubsan_handlers_cxx.cc:30
@@ -28,1 +29,3 @@
+extern SuppressionContext *suppression_ctx;
+
----------------
Alexey Samsonov wrote:
> It probably makes more sense to put suppression context into ubsan_diag and implement proper set of methods to access it.
Done, now all suppressions are implemented in ubsan_diag
================
Comment at: lib/ubsan/ubsan_handlers_cxx.cc:43
@@ -39,1 +42,3 @@
+ DynamicTypeInfo DTI = getDynamicTypeInfo((void*)Pointer);
+
----------------
Alexey Samsonov wrote:
> I believe you should put this check before acquiring the SourceLocation above. Otherwise, if two errors happen consecutively and have the same source location, and the first error is suppressed, then you won't report the second one. It would be nice to have a test for it.
Ah! I've added the test but it may not be clean enough. Please comment if you have a better idea.
================
Comment at: lib/ubsan/ubsan_handlers_cxx.cc:47
@@ +46,3 @@
+ Suppression *s;
+ if (suppression_ctx &&
+ suppression_ctx->Match(DTI.getMostDerivedTypeName(), SuppressionUBSanVptr, &s))
----------------
Alexey Samsonov wrote:
> Note that suppression context may not yet be initialized at this point (if InitIfNecessary() hasn't run yet, which is barely possible on Linux, where we use preinit_array, but probably may happen on other platforms).
Can we safely assume that InitIfNecessary() is called if preinit_array is defined? Now InitIfNecessary() is enforced only if preinit_array is not defined. I think we may simply always call InitIfNecessary() as well because this matching function would not be invoked a lot (at least for vptr).
================
Comment at: lib/ubsan/ubsan_handlers_cxx.cc:38
@@ +37,3 @@
+
+ // Suppress an error report if it is specified.
+ if (MatchSuppressions(DTI.getMostDerivedTypeName(), SuppressionVptrCheck))
----------------
Alexey Samsonov wrote:
> // Check if error report is suppressed.
Done.
================
Comment at: lib/ubsan/ubsan_handlers_cxx.cc:37
@@ +36,3 @@
+ DynamicTypeInfo DTI = getDynamicTypeInfo((void*)Pointer);
+ const char *MostDerivedTypeName = DTI.getMostDerivedTypeName();
+
----------------
Alexey Samsonov wrote:
> why not create and pass MangledName around?
As the mangled name was not straight forward to us to blacklist (or suppress here), we actually wanted to bring up the discussion to see whether it's possible to use un-mangled name. But let me fix this as you suggest as it clearly breaks the consistency.
================
Comment at: lib/ubsan/ubsan_init.cc:34
@@ +33,3 @@
+
+ ALIGNED(64) static char placeholder[sizeof(SuppressionContext)];
+ suppression_ctx = new(placeholder) SuppressionContext;
----------------
Alexey Samsonov wrote:
> It's sad to see the duplicated code here. Let me raise a discussion about having common suppression context in sanitizer_common.
Thanks, let me leave the code as it is until then.
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:15
@@ +14,3 @@
+// RUN: (echo "vptr_check:S"; echo "vptr_check:T"; echo "vptr_check:U") > %t.supp
+// RUN: (echo "" ; ASAN_OPTIONS="$ASAN_OPTIONS suppressions=%t.supp" UBSAN_OPTIONS=suppressions=%t.supp %run %t mS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
+// RUN: (echo "" ; ASAN_OPTIONS="$ASAN_OPTIONS suppressions=%t.supp" UBSAN_OPTIONS=suppressions=%t.supp %run %t fS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
----------------
Alexey Samsonov wrote:
> echo ""; remains a mystery for me.
It seems my previous comments were not properly sent to you. These echo commands were prepended to generate non-empty outputs, because FileCheck complains otherwise due to the empty outputs.
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:15
@@ +14,3 @@
+// RUN: (echo "vptr_check:S"; echo "vptr_check:T"; echo "vptr_check:U") > %t.supp
+// RUN: (echo " " ; UBSAN_OPTIONS=suppressions=%t.supp %run %t mS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
+// RUN: (echo " " ; UBSAN_OPTIONS=suppressions=%t.supp %run %t fS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
----------------
Alexey Samsonov wrote:
> Remove stray echo " "
Since running this test generates empty outputs, FileCheck complains about it. I may add one simple printf(), or is there any options for FileCheck to handle empty output cases?
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:152
@@ +151,3 @@
+ p->g();
+ p_tmp = p; // Keep leak sanitizer silent.
+ // CHECK-LOC-SUPPRESS-NOT: [[PTR]]: note: object is of type 'S'
----------------
Alexey Samsonov wrote:
> LSan shouldn't be a problem after r214149 I've submitted recently.
There were strange issues in testing UndefinedBehaviorSanitizer-AddressSanitizer, and let me describe more in the updated patch.
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:15
@@ +14,3 @@
+// RUN: (echo "vptr_check:S"; echo "vptr_check:T"; echo "vptr_check:U") > %t.supp
+// RUN: (echo " " ; ASAN_OPTIONS=suppressions=%t.supp UBSAN_OPTIONS=suppressions=%t.supp %run %t mS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
+// RUN: (echo " " ; ASAN_OPTIONS=suppressions=%t.supp UBSAN_OPTIONS=suppressions=%t.supp %run %t fS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
----------------
Alexey Samsonov wrote:
> remove stray (echo " ")
Oh I misunderstood the meaning of stray :( Fixed now.
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:16
@@ +15,3 @@
+// RUN: (echo " " ; ASAN_OPTIONS=suppressions=%t.supp UBSAN_OPTIONS=suppressions=%t.supp %run %t mS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
+// RUN: (echo " " ; ASAN_OPTIONS=suppressions=%t.supp UBSAN_OPTIONS=suppressions=%t.supp %run %t fS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
+// RUN: (echo " " ; ASAN_OPTIONS=suppressions=%t.supp UBSAN_OPTIONS=suppressions=%t.supp %run %t cS 2>&1) | FileCheck %s --check-prefix=CHECK-SUPPRESS
----------------
Please note that these redundant options (ASAN_OPTIONS and UBSAN_OPTIONS) are given to pass both UBsan tests and UBsan + Asan tests. When UBSan+Asan is running, suppressions flag in UBSAN_OPTIONS is not parsed.
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:24
@@ +23,3 @@
+// RUN: echo "vptr_check:S" > %t.loc-supp
+// RUN: ASAN_OPTIONS=suppressions=%t.loc-supp UBSAN_OPTIONS=suppressions=%t.loc-supp %run %t x- 2>&1 | FileCheck %s --check-prefix=CHECK-LOC-SUPPRESS
+
----------------
This check fails for UBSAN+ASAN while it worked fine when I manually tested. Could you please take a look why this fails?
================
Comment at: test/ubsan/TestCases/TypeCheck/vptr.cpp:24
@@ +23,3 @@
+// RUN: echo "vptr_check:S" > %t.loc-supp
+// RUN: ASAN_OPTIONS=suppressions=%t.loc-supp UBSAN_OPTIONS=suppressions=%t.loc-supp %run %t x- 2>&1 | FileCheck %s --check-prefix=CHECK-LOC-SUPPRESS
+
----------------
Byoungyoung Lee wrote:
> This check fails for UBSAN+ASAN while it worked fine when I manually tested. Could you please take a look why this fails?
Let me also keep debugging this issue in the meantime !
http://reviews.llvm.org/D4702
More information about the llvm-commits
mailing list