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

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:98
+     pointer as a function parameter which is annotated with ``_Nonnull``,
+     or assigning null to a lvalue marked ``_Nonnull``. You can enable
+     just the return value check with ``-fsanitize=nullability-return``,
----------------
s/a/an/


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:99
+     or assigning null to a lvalue marked ``_Nonnull``. You can enable
+     just the return value check with ``-fsanitize=nullability-return``,
+     the assignment check with ``-fsanitize=nullability-assign``, and the
----------------
Can you rephrase this without using 2nd person?


================
Comment at: lib/CodeGen/CGCall.cpp:2926
+void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV,
+                                           SourceLocation EndLoc) {
+  if (!CurCodeDecl)
----------------
This function could use some comments.


================
Comment at: lib/CodeGen/CGCall.cpp:3301
+    CheckKind = SanitizerKind::NonnullAttribute;
+  } else {
+    if (!PVD)
----------------
comment what this early return means?


================
Comment at: lib/CodeGen/CGDecl.cpp:676
+void CodeGenFunction::EmitNullabilityCheck(LValue Dst, llvm::Value *Src,
+                                           SourceLocation Loc) {
+  if (!SanOpts.has(SanitizerKind::NullabilityAssign))
----------------
Comment what the check is doing?


================
Comment at: lib/CodeGen/CGDecl.cpp:1911
+    if (auto Nullability = Ty->getNullability(getContext())) {
+      if (Nullability && *Nullability == NullabilityKind::NonNull) {
+        SanitizerScope SanScope(this);
----------------
Is this a typo, or do you. really want `Nullability` here? Asking because everywhere else there is a check for `! Nullability && *Nullability == NullabilityKind::NonNull`.
Ans should this condition be factored out?


================
Comment at: lib/CodeGen/CodeGenFunction.h:1766
 
+  /// \brief Emit a test that checks if the return value \p RV is nonnull.
+  void EmitReturnValueCheck(llvm::Value *RV, SourceLocation EndLoc);
----------------
The \brief is redundant. We run doxygen with autobrief.


================
Comment at: lib/CodeGen/CodeGenFunction.h:3474
 
+  /// \brief Emit a test that checks if \p Src is nonnull if \p Dst is
+  /// annotated as nonnull.
----------------
ditto


https://reviews.llvm.org/D30762