arichardson 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:
> > > 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`.
> > > 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
> >
> > Interesting. How do we get at that? Do we ask the middle-end to
> > constant-fold `addrspacecast i8* null to i8* addrspace(N)` and see if we
> > get a `null` back, or is there a better way?
> >
> > In any case, this patch follows the same pattern we use for return values
> > of reference type, parameters of reference type, and decayed array function
> > parameters with static array bounds, all of which apply `nonnull` only in
> > address space 0. If we want to use a different pattern, to consider whether
> > LLVM's `nonnull` means "not a null pointer" rather than assuming that holds
> > only in address space 0, we should make a holistic change for that
> > throughout CGCall.cpp, rather than touching only the handling of the `this`
> > pointer.
> >
> > It'd also be interesting to consider what we want
> > `__attribute__((nonnull))` to mean in address spaces where the null pointer
> > isn't the zero pointer: should it mean non-zero at the source level /
> > non-null in LLVM IR, or should it mean non-null at the source level (which
> > might be unrepresentable in LLVM IR, but static analysis etc. could still
> > detect it)? We're currently inconsistent on this: static analysis, constant
> > evaluation, and sanitizers treat the attribute as meaning non-null, but IR
> > generation emits the `nonnull` IR attribute, treating it as meaning
> > non-zero instead.
> > Interesting. How do we get at that? Do we ask the middle-end to
> > constant-fold addrspacecast i8* null to i8* addrspace(N) and see if we get
> > a null back, or is there a better way?
>
> In fact, the middle-end has no ability to constant-fold `addrspacecast`,
> because LLVM's address-space support is a giant pile of hacks rather than
> exhibiting any cohesive design principles; or to put it another way, the lack
> of any centralized technical design authority in LLVM means that nobody has
> been able to enforce any rules about, say, targets being able to contribute
> knowledge about address spaces to the middle-end.
>
> But in the frontend, we essentially have our own independent concept of
> address spaces, and the mapping from an AST address space to an IR address
> space can be non-obvious (and, in particular, non-injective), targets can
> perform arbitrary IR for address-space conversions instead of just using
> `addrspacecast`, and so on. This is necessary because we have to actually
> ship a functional compiler, whether with LLVM or despite it. And one of the
> places where it's necessary to work around LLVM is emitting a null pointer
> constant, because of course C requires a null pointer constant to be a
> constant expression, which in LLVM means we have to produce an
> `llvm::Constant*` for it, and we don't get to rely on assumptions like
> `addrspacecast` having appropriate semantics. So instead we just have
> `ASTContext::getTargetNullPointerValue`, which is ultimately fed by the
> TargetInfo, and we expect that to be consistent with the behavior of e.g. the
> address-space-conversion customization point.
>
> I think `__attribute__((nonnull))` clearly needs to be based on the
> language-level null pointer concept rather than having anything to do with a
> zero bit representation, and if that means we can't use it in IR, maybe we
> can improve IR.
> > 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
>
> Interesting. How do we get at that? Do we ask the middle-end to constant-fold
> `addrspacecast i8* null to i8* addrspace(N)` and see if we get a `null` back,
> or is there a better way?
>
We try rather hard to avoid any addrspacecasts in the IR and emit `i8*
addrspace(200) null` for language-level NULL pointers.
A bit more context: In our compiler we support two compilation modes: so-called
"pure-capability" where we use AS 200 for all pointers. This maps to using
capability instructions in the backend (effectively 128-bit fat pointers with a
hidden validity tag). We also support a hybrid compilation mode where we emit
everything in AS0 (64-bit integer pointers) by default and only use AS200 for
annotated language-level pointers.
In both of these address spaces a zero bit-pattern is NULL and not
dereferenceable.
In the "pure-capability" mode our AS200 is effectively the default AS0.
However, we can't use that since we need to use a different AS to select our
instructions in the backend since we extend the MIPS,RISC-V and AArch64
backends and AS0 already maps to integer loads/stores/etc.
So the problem as I see it is that clang needs to be able to tell that a for a
given LLVM IR address space llvm IR `null` (i.e. all zero bits) maps to C/C++
language-level `NULL/nullptr`?
However, I did not see anywhere in the langref that `null` is equivalent to
"all-zero-bits", but I may have missed that part.
All I saw about null was:
> Null pointer constants
> The identifier ‘null’ is recognized as a null pointer constant and must be of
> pointer type.
So unless I missed something it seems like AS<N> could define null as being
all-ones instead, and that could then map to the representation of
language-level NULL on machines where it is not all-zeroes?
In our fork I added a local hack to replace the checks for adding the nonnull
attribute if AS==0 with a `canMarkAsNonNull` helper that checks for
`AS==0||AS==200`. Maybe there should be a helper function that checks whether
language-level NULL maps to null in address space N, but that would probably
require making null-pointer-is-valid a per-AS property?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D17993/new/
https://reviews.llvm.org/D17993
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
