[llvm] acf3279 - For non-null pointer checks, do not descend through out-of-bounds GEPs

[Philip Reames via llvm-commits]

This is a correct fix, but it seems a bit conservative.

For any address space for which null is not a valid object (e.g. most of 
them), the existing code was correct.

If I'm not missing something, would you mind making this change 
conditional on the appropriate address space property check?

Philip

On 4/9/21 6:09 AM, Momchil Velikov via llvm-commits wrote:
> Author: Momchil Velikov
> Date: 2021-04-09T14:09:23+01:00
> New Revision: acf3279a037ff9c8591f551e92b8e7a8c27b61a4
>
> URL: https://github.com/llvm/llvm-project/commit/acf3279a037ff9c8591f551e92b8e7a8c27b61a4
> DIFF: https://github.com/llvm/llvm-project/commit/acf3279a037ff9c8591f551e92b8e7a8c27b61a4.diff
>
> LOG: For non-null pointer checks, do not descend through out-of-bounds GEPs
>
> In LazyValueInfoImpl::isNonNullAtEndOfBlock we populate a set of
> pointers, known to be non-null at the end of a block (e.g. because we
> did a load through them). We then infer that any pointer, based on an
> element of this set is non-null as well ("based" here meaning a
> non-null pointer is the underlying object). This is incorrect, even if
> the base pointer was non-null, the value of a GEP, that lacks the
> inbounds` attribute, may be null.
>
> This issue appeared as miscompilation of the following test case:
>
> int puts(const char *);
>
> typedef struct iter {
>    int *val;
> } iter_t;
>
> static long distance(iter_t first, iter_t last) {
>    long r = 0;
>    for (; first.val != last.val; first.val++)
>      ++r;
>    return r;
> }
>
> int main() {
>    int arr[2] = {0};
>    iter_t i, j;
>    i.val = arr;
>    j.val = arr + 1;
>    if (distance(i, j) >= 2)
>      puts("failed");
>    else
>      puts("passed");
> }
>
> This fixes PR49662.
>
> Differential Revision: https://reviews.llvm.org/D99642
>
> Added:
>      llvm/test/Transforms/JumpThreading/nonnull-gep-out-of-bounds.ll
>
> Modified:
>      llvm/lib/Analysis/LazyValueInfo.cpp
>
> Removed:
>      
>
>
> ################################################################################
> diff  --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
> index 75fef34eaf4a3..6bf03e884c935 100644
> --- a/llvm/lib/Analysis/LazyValueInfo.cpp
> +++ b/llvm/lib/Analysis/LazyValueInfo.cpp
> @@ -658,7 +658,7 @@ bool LazyValueInfoImpl::isNonNullAtEndOfBlock(Value *Val, BasicBlock *BB) {
>                              Val->getType()->getPointerAddressSpace()))
>       return false;
>   
> -  Val = getUnderlyingObject(Val);
> +  Val = Val->stripInBoundsOffsets();
>     return TheCache.isNonNullAtEndOfBlock(Val, BB, [](BasicBlock *BB) {
>       NonNullPointerSet NonNullPointers;
>       for (Instruction &I : *BB)
>
> diff  --git a/llvm/test/Transforms/JumpThreading/nonnull-gep-out-of-bounds.ll b/llvm/test/Transforms/JumpThreading/nonnull-gep-out-of-bounds.ll
> new file mode 100644
> index 0000000000000..37737bb0dcd57
> --- /dev/null
> +++ b/llvm/test/Transforms/JumpThreading/nonnull-gep-out-of-bounds.ll
> @@ -0,0 +1,18 @@
> +; RUN: opt -jump-threading -S %s -o - | FileCheck %s
> +
> +define i32 @f(i64* %a, i64 %i) {
> +entry:
> +  store i64 0, i64* %a, align 8
> +  %p = getelementptr i64, i64* %a, i64 %i
> +  %c = icmp eq i64* %p, null
> +  ; `%a` is non-null at the end of the block, because we store through it.
> +  ; However, `%p` is derived from `%a` via a GEP that is not `inbounds`, therefore we cannot judge `%p` is non-null as well
> +  ; and must retain the `icmp` instruction.
> +  ; CHECK: %c = icmp eq i64* %p, null
> +  br i1 %c, label %if.else, label %if.then
> +if.then:
> +  ret i32 0
> +
> +if.else:
> +  ret i32 1
> +}
>
>
>          
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits