[PATCH] [UBSAN] Suppress an error report with given type information

[Dmitry Vyukov]

On Wed, Jul 30, 2014 at 7:16 AM, Alexey Samsonov <vonosmas at gmail.com> wrote:
> Sergey, Dmitry
>
> Is there a reason that "suppressions" flag is not in sanitizer_common, and we have separate methods for accessing/parsing suppression context in LSan/TSan? Does it make sense to have this code in sanitizer_common instead?

No particular reason. Probably we can merge that code.


> For example, now we work on adding suppressions to UBSan. Note that there is a "supported" mode where we run ASan+LSan+UBSan together. Having separate set of suppressions for LSan and UBSan would be really unconvenient.

If the flag has the same name in both tool, then it must work. Both
tools will parse the same suppressions file.


> ================
> Comment at: lib/sanitizer_common/sanitizer_suppressions.cc:25
> @@ -25,1 +24,3 @@
> +    "signal",     "leak", "called_from_lib", "deadlock",
> +    "ubsan_vptr"};
>
> ----------------
> 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".

Yes, no tool name please.


> ================
> 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);
>  }
> ----------------
> Actually update the test to include new suppression name, as this comment recommends.
>
> ================
> Comment at: lib/ubsan/ubsan_flags.cc:26
> @@ -25,2 +25,3 @@
>    f->print_stacktrace = false;
> +  f->suppressions = 0;
>
> ----------------
> Let's use empty string for consistency with another sanitizers defining this flag.
>
> ================
> 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");
>    }
> ----------------
> I find this flag description confusing.
>
> ================
> Comment at: lib/ubsan/ubsan_handlers_cxx.cc:30
> @@ -28,1 +29,3 @@
>
> +extern SuppressionContext *suppression_ctx;
> +
> ----------------
> It probably makes more sense to put suppression context into ubsan_diag and implement proper set of methods to access it.
>
> ================
> Comment at: lib/ubsan/ubsan_handlers_cxx.cc:43
> @@ -39,1 +42,3 @@
>
> +  DynamicTypeInfo DTI = getDynamicTypeInfo((void*)Pointer);
> +
> ----------------
> 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.
>
> ================
> Comment at: lib/ubsan/ubsan_handlers_cxx.cc:47
> @@ +46,3 @@
> +  Suppression *s;
> +  if (suppression_ctx &&
> +      suppression_ctx->Match(DTI.getMostDerivedTypeName(), SuppressionUBSanVptr, &s))
> ----------------
> 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).
>
> ================
> Comment at: lib/ubsan/ubsan_init.cc:34
> @@ +33,3 @@
> +
> +  ALIGNED(64) static char placeholder[sizeof(SuppressionContext)];
> +  suppression_ctx = new(placeholder) SuppressionContext;
> ----------------
> It's sad to see the duplicated code here. Let me raise a discussion about having common suppression context in sanitizer_common.
>
> http://reviews.llvm.org/D4702
>
>