[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

A lot of the test changes here seem to be over-constraining the existing tests. Tests that don't care about `nonnull` / `dereferenceable` `this` pointers should generally not be checking for that. Instead of looking for `nonnull dereferenceable(...)`, I'd prefer to see `{{[^,]*}}` or similar to skip any parameter attributes.

I would also like to see some tests that directly cover the new functionality: specifically, tests that check that the argument to `dereferenceable` is correct, including cases such as classes with virtual bases, virtual function calls, calls through pointers to member functions, and a check that we don't emit the attributes under `-fno-delete-null-pointer-checks`.



================
Comment at: clang/lib/CodeGen/CGCall.cpp:2174
+  }
+
   unsigned ArgNo = 0;
----------------
CJ-Johnson wrote:
> jdoerfert wrote:
> > Even if null pointer is valid we should place dereferenceable.
> > 
> > We also could never place nonnull and let the middle-end make the dereferenceable -> nonnull deduction, though I don't see why we can't just add nonnull here.
> I re-ran ninja check after making this fix and addressed the newly-affected tests. So the patch is fully up to date :)
The LLVM LangRef says that in address space 0, `dereferenceable` implies `nonnull`, so I don't think we can emit `dereferenceable` in `NullPointerIsValid` mode, and we'd need to use `dereferenceable_or_null` instead. (Perhaps the LangRef is wrong, though, and the `null_pointer_is_valid` function attribute overrides that determination.)


================
Comment at: clang/test/CodeGenCXX/array-default-argument.cpp:22-24
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1BC1E1A([[TEMPORARY:.*]])
   // CHECK-EH:  unwind label %[[A_AND_PARTIAL_ARRAY_LPAD:.*]]
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1AD1Ev([[TEMPORARY:.*]])
----------------
This has changed the meaning of the test. Previously we were checking the argument bound in the first call is also passed to the second and third calls; now we're not checking the call arguments at all, because we re-bind `TEMPORARY` in each `CHECK` line.


================
Comment at: clang/test/CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp:126
 //
-// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* %[[RIGHT]])
+// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* nonnull %[[RIGHT]])
 // CHECK: ret
----------------
Why does this call get `nonnull` but not `dereferenceable`? Seems like we should be able to use at least `dereferenceable(sizeof(Right))` here -- but I think we could actually be more aggressive and pass `dereferenceable(sizeof(ChildOverride) - offsetof(ChildOverride, <Right base class>))`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D17993/new/

https://reviews.llvm.org/D17993