[PATCH] D30762: [ubsan] Add a nullability sanitizer

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added inline comments.


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:101
+     ``-fsanitize=nullability-assign``, and the argument check with
+     ``-fsanitize=nullability-arg``. While violating nullability rules does
+     not result in undefined behavior, it is often unintentional, so UBSan
----------------
vsk wrote:
> zaks.anna wrote:
> > Have you investigated if -fsanitize=nullability-arg is too noisy? I think we should somehow "recommend" return and assignment check to most users. This is not clear from the help and options provided. The main concern here is that someone would try -fsanitize=nullability, see a ton of warnings they don't care about and think that the whole check is too noisy.
> > 
> > I cannot come up with a great solution. Options that come into mind are:
> >  - Drop "arg" check completely.
> >  - Remove arg from the nullability group.
> >  - Have 2 groups for nullability.
> > 
> > We can also wait for concrete data before making a decision here and address this in subsequent commits.
> The nullability-arg check was not noisy on a small sample (n=4) of Apple-internal projects. I only collected 11 diagnostics in total, all of which seemed actionable. I.e the issues were not isolated to system headers: the project code actually did violate arg nullability. Based on what I've seen so far, I'd like to start off with including the arg check in a "nullability-full" group. If arg check noisiness becomes an issue, then I'd like to create a new "nullability-partial" group with just the return/assignment checks.
> 
> TL;DR: let's rename -fsanitize=nullability to -fsanitize=nullability-full, keep the arg check, and introduce a nullability-partial group later if we need to. Wdyt?
> 
> 
Keeping things as is and introducing a new group if this becomes a problem sounds good to me. You could use "nullability" and "nullability-pedantic". That way you do not need to change the name and get to use "nullability".


https://reviews.llvm.org/D30762