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

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

rsmith added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2165
+    assert(getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0 &&
+           "Expected `this` pointer without address space attribute.");
+
----------------
jdoerfert wrote:
> I'm unsure why this assertion has to hold and more importantly why we need it. @arsenm do you?
Even if the `this` pointer is always an address-space-0 pointer for now, it would be more consistent with how we handle `Attribute::NonNull` elsewhere to skip adding the `NonNull` attribute for non-address-space-0 pointers rather than asserting here.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2174
+  }
+
   unsigned ArgNo = 0;
----------------
jdoerfert wrote:
> rsmith wrote:
> > 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.)
> > (Perhaps the LangRef is wrong, though, and the null_pointer_is_valid function attribute overrides that determination.)
> 
> This is the case. IMHO, dereferenceable has to be correct here, regardless of the mode. You could access the object in the function entry, which we would use to deduce dereferenceable, and if the `NullPointerIsValid` is not translated to the function attribute also to `nonnull`. 
OK, if the LangRef is wrong about this, then I agree we should emit `dereferenceable` unconditionally. Thanks for clarifying.


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

https://reviews.llvm.org/D17993