[PATCH] D124677: [ConstantFold] Don't convert getelementptr to ptrtoint+inttoptr

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D124677#3483890 <https://reviews.llvm.org/D124677#3483890>, @aqjune wrote:

> I agree that introducing ptrtoint + inttoptr here doesn't sound like a good idea because both it is bad for alias analysis and its correctness is not clear.
>
>> For the case where V == ptrtoint Ptr, this folds to inttoptr 0, which further folds to null. In that case provenance becomes incorrect. This has been observed as a real-world miscompile with rustc.
>
> If LLVM is using the definition of null pointer in C, inttoptr 0 must be `null`, implying that folding `gep p, -(ptrtoint p)` to `null` must be the problematic one.
>
>   C17, 6.3.2.3.p3. An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant.

I don't think this follows: This is talking about "integer constant expressions", which are a front-end concern. It means that the front-end is required to match for `(void*)0` and emit that as `ptr null` rather than `inttoptr (i64 0 to ptr)`. At least from that wording, doing `int x = 0; (void*)x`does not result in a null pointer (though possibly other wording implies that?)

I don't think having `ptrtoint 1` have universal provenance and `ptrtoint 0` have nullary provenance can lead to consistent semantics. It renders many transforms that are "obviously correct" illegal, such as:

  define ptr @src(ptr %p, i64 %idx) {
    %p2 = getelementptr i8, ptr %p, i64 %idx
    ret ptr %p2
  }
  define ptr @tgt(ptr %p, i64 %idx) {
    %p.int = ptrtoint ptr %p to i64
    %p.add = add i64 %p.int, %idx
    %p2 = inttoptr i64 %p.add to ptr
    ret ptr %p2
  }

While this transform is very undesirable, it should be correct because it only increases provenance. However, due to the special `ptrtoint 0` handling this is incorrect for the special case where `p.int == -idx`.

In D124677#3483900 <https://reviews.llvm.org/D124677#3483900>, @aqjune wrote:

>> This has been observed as a real-world miscompile with rustc.
>
> Could you share a link to the bug report please?

https://github.com/rust-lang/rust/pull/96538


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

https://reviews.llvm.org/D124677