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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
