[PATCH] D90529: Allow nonnull attribute to accept poison

Juneyoung Lee via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 2 13:02:08 PST 2020


aqjune added a comment.

In D90529#2368303 <https://reviews.llvm.org/D90529#2368303>, @jdoerfert wrote:

> Fair. Though, I think we want to produce poison for one set of attributes for which the name "value attribute" was not well chosen.
> So far, the things I think should produce poison not UB are:
>
> (pure) value attributes:
>
> - nonnull
> - align
> - [used_bits] (not existing yet)
>
> (context) value attributes:
>
> - dereferenceable
> - dereferenceable_or_null
> - [object_size] (as proposed on the list)
>
> WDYT?

In case of `dereferenceable`, my opinion is still slightly different: it represents the property of the memory at the point.
If a memory block is freed, a same pointer value won't be dereferenceable after the deallocation.

Other than the conceptual reason, my practical concern about `dereferenceable` is that `dereferenceable` is hard to use unless it is with `noundef`.
Since loading poison is UB, the attribute can't still give a guarantee that loading the pointer is well defined.
Furthermore, `noundef` is hard to infer from the context. For example,

  store i32 0, i32* %p
  call void @f(i32* %p) ; can we infer %p's noundef & dereferenceable(4) from store?

`store i32 0, i32* %p` does not guarantee that `%p` is noundef because it can be partially undef but still dereferenceable (D87994 <https://reviews.llvm.org/D87994> is a relevant LangRef patch).

The example above was propagating the attribute from caller to callee, but the inverse direction (callee -> caller) is the same: If `@f` contains a store, it guarantees that the pointer is dereferenceable but not noundef.

In order to regain optimization power, we need a way to attach noundef more aggressively.
D81678 <https://reviews.llvm.org/D81678> will attach noundef to arguments, and additionally I think it is valid for clang to attach `!noundef` when lowering non-char typed lvalues to load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90529



More information about the llvm-commits mailing list