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

[Vedant Kumar via Phabricator via cfe-commits]

vsk marked 9 inline comments as done.
vsk added a comment.

The plan is to start off with -fsanitize=nullability, and then create a nullability-pedantic group later if it's really necessary. I think I've addressed all of the inline comments, and will upload a new diff shortly.



================
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
----------------
zaks.anna wrote:
> 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".
Sgtm.


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:141
   -  ``-fsanitize=undefined``: All of the checks listed above other than
-     ``unsigned-integer-overflow``.
+     ``unsigned-integer-overflow`` and ``nullability``.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of
----------------
filcab wrote:
> ```... and the ``nullability`` group.```
I define the group after the 'undefined' group, so I used a slightly different spelling.


================
Comment at: lib/CodeGen/CGDecl.cpp:1907
+
+  // Skip the return value nullability check if the nullability preconditions
+  // are broken.
----------------
zaks.anna wrote:
> I would add a comment explaining why this is needed, similar to what you included in the commit message: 
> "One point of note is that the nullability-return check is only allowed
> to kick in if all arguments to the function satisfy their nullability
> preconditions. This makes it necessary to emit some null checks in the
> function body itself."
> 
> Maybe even rename CanCheckRetValNullability into RetValNullabilityPrecondition. I like this more than "CanCheck" because it's just a precondition value. You check if it is satisfied (evaluates to true or false) later when it's used in a branch.
I added the comment and did the rename. I like 'RetValNullabilityPrecondition'.


================
Comment at: test/CodeGenObjC/ubsan-nullability.m:114
+  // CHECK: [[NONULL]]:
+  // CHECK: ret i32*
+}
----------------
filcab wrote:
> `CHECK-NEXT`?
I've promoted all the existing 'CHECK' lines to 'CHECK-NEXT's where possible.


https://reviews.llvm.org/D30762