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

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1911
+    if (auto Nullability = Ty->getNullability(getContext())) {
+      if (Nullability && *Nullability == NullabilityKind::NonNull) {
+        SanitizerScope SanScope(this);
----------------
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 &&`.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:829
+    if (auto Nullability = FnRetTy->getNullability(getContext()))
+      if (Nullability && *Nullability == NullabilityKind::NonNull)
+        if (!(SanOpts.has(SanitizerKind::ReturnsNonnullAttribute) &&
----------------
Hasn't the `if (auto Nullability =` already checked the truthiness of the var, making the `Nullability &&` part of the second check redundant?


================
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]]:
----------------
The:

```
alloca
store
load
```

part of this looks like it's just making extra work for mem2reg. Is the alloca actually necessary?




https://reviews.llvm.org/D30762