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

[Vedant Kumar via Phabricator via cfe-commits]

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

Thanks for your comments, and sorry for jumping the gun earlier with an updated diff. I'll attach a fixed-up diff shortly.



================
Comment at: lib/CodeGen/CGDecl.cpp:1911
+    if (auto Nullability = Ty->getNullability(getContext())) {
+      if (Nullability && *Nullability == NullabilityKind::NonNull) {
+        SanitizerScope SanScope(this);
----------------
jroelofs wrote:
> aprantl wrote:
> > 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?
> Could just stick `if (auto Nullability =` in there, and remove the `Nullability &&`.
'Nullability' should not have been checked for truthiness twice. By factoring out, do you mean moving the check's logic out of EmitParmDecl and into a helper? If so, I'm not sure if that would make the code cleaner, but I'm open to it.


================
Comment at: test/CodeGenObjC/ubsan-null-retval.m:14
+  // CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* [[ARG]], null, !nosanitize
+  // CHECK-NEXT: br i1 [[ICMP]], label %[[CONT:.+]], label %[[HANDLE:[^,]+]]
+  // CHECK: [[HANDLE]]:
----------------
jroelofs wrote:
> The:
> 
> ```
> alloca
> store
> load
> ```
> 
> part of this looks like it's just making extra work for mem2reg. Is the alloca actually necessary?
> 
> 
Clang likes its allocas :). This is typical -O0 behavior: IIUC it simplifies IRGen. E.g for expressions which take the address of an argument, because there is guaranteed storage for the arg.


https://reviews.llvm.org/D30762