[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
----------------
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.


================
Comment at: lib/CodeGen/CGDecl.cpp:1907
+
+  // Skip the return value nullability check if the nullability preconditions
+  // are broken.
----------------
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.


https://reviews.llvm.org/D30762