[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+    if (!CodeGenOpts.NullPointerIsValid &&
+        getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+      Attrs.addAttribute(llvm::Attribute::NonNull);
----------------
rjmccall wrote:
> rsmith wrote:
> > jdoerfert wrote:
> > > arichardson wrote:
> > > > Isn't the `this` pointer also nonnull in other address spaces?
> > > > 
> > > > In our CHERI fork we use AS200 for the this pointer and would quite like to have the nonnull attribute.
> > > > I can obviously change this line locally when I next merge from upstream, but I would like to avoid diffs and it seems to me like this restriction is unnecessary.
> > > I also think `NullPointerIsValid` is sufficient. 
> > It's my understanding that:
> > * The LLVM `null` value in any address space is the all-zero-bits value.
> > * In address space zero, the `null` value does not correspond to addressable memory, but this is not assumed to hold in other address spaces.
> > * An address-space-zero `null` value that is addressspacecast to a different address space might not be the `null` in the target address space.
> > * The `nonnull` attribute implies that the pointer value is not the `null` value.
> > * A null pointer in the frontend in a non-zero address space corresponds to the value produced by an addressspacecast of an address-space-zero `null` value to the target address space.
> > 
> > That being the case, there is simply no connection between the C and C++ notion of a null pointer and a `null` LLVM pointer value in a non-zero address space in general, so it is not correct to use the `nonnull` attribute in a non-zero address space in general. Only if we know that a C++ null pointer is actually represented by the LLVM `null` value in the corresponding address space can we use the `nonnull` attribute to expose that fact to LLVM. And we do not know that in general.
> I think all of this is correct except that the frontend does know what the bit-pattern of the null pointer is in any particular language-level address space, and it knows what the language-level address space of `this` is.  So we should be able to ask whether the null value in the `this` address space is the all-zero value and use that to decide whether to apply `nonnull`.
Hm, I think the problem is that we don't couple `NullPointerIsValid` with the address space. As I said before. In LLVM-IR, if we don't have the `null-pointer-is-valid` property, then "memory access" implies `dereferenceable` implies `nonnull`. Now, we usually assume non-0 address spaces to have the above property, but that should be decoupled. The query if "null is valid" should take the function and the AS, as it does conceptually in LLVM-core, and then decide if `null-pointer-is-valid` or not. If we had that, @arichardson could define that AS200 does not have valid null pointers. If you do that right now the IR passes will at least deduce `nonnull` based on `derferenceable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993