I think we can use here same logic as for nonnull.
strlen(X) - X must be noundef => valid pointer.
for libcalls with size arg, we add noundef only if size is known and greater than 0 - so pointers must be noundef (valid ones)
Paths
| Differential D95122
[Libcalls, Attrs] Annotate libcalls with noundef ClosedPublic Authored by xbolva00 on Jan 21 2021, 2:47 AM.
Details Summary I think we can use here same logic as for nonnull. strlen(X) - X must be noundef => valid pointer. for libcalls with size arg, we add noundef only if size is known and greater than 0 - so pointers must be noundef (valid ones)
Diff Detail
Event TimelineThis revision is now accepted and ready to land.Jan 21 2021, 7:07 AM Comment Actions
Thanks This revision now requires changes to proceed.Jan 21 2021, 7:35 AM Comment Actions
I did not look into the changes in this patch yet, but does it mean that from f(i8* nonnull %p) it is allowed to attach noundef? f(i8* nonnull noundef %p) I think this shouldn't be allowed, because the former one isn't UB when %p was null but the latter one raises UB.
I remember that this was to allow free code motion of load/stores if the offset is within bounds. %p = alloca [8 x i32] %q = gep %p, %i ; %i may be partially undef load %q ; If 0 <= %i < 8, %q is dereferenceable even if %q is partially undefined Comment Actions
but we are talking about specific libcalls, so eg. strlen(p), where p is null, is UB. Comment Actions Okay, I think the situation is a bit complex. :/ I think a safer way is to make clang frontend attach noundef to the pointer argument of memcmp/memset/memcpy/.... calls when emitting them. Does it have the same optimization power in practice, or still SimplifyLibCalls allows more optimizations? Comment Actions
The problem is that in theory p can be partially undef which is still dereferenceable. Imagine a pointer that points into somewhere within bounds of alloca [16 x i8] but the lowest 4 bits of the offset is undefined. Comment Actions
My comment should be read in the context of this patch. We only add nonnull when we know there is an access, so the poison of passing null would be accessed causing UB.
Agreed. It seems to make noundef unusable. Comment Actions Or just revert D87994 if it blocks more optimizations than it allows.. We could patch clang, but llvm provides valuetracking’s “isKnownNonZero” which is useful and makes this more powerful. Comment Actions Since this issue only happens when the undef value is involved, another solution is to revert D87994 as suggested & make load/store's pointer noundef & rely on the future that the undef value will be finally removed. Comment Actions
+1 Comment Actions
I don't understand what this has to do with the undef value, or maybe I misinterpret what that means. However, if we revert that patch, I think we do ourselves a favor. xbolva00 mentioned this in D95174: Revert "[LangRef] Clarify the behavior of memory access instructions when pointers/sizes aren't well-defined".Jan 21 2021, 2:42 PM Comment Actions
Proposal for revert:: https://reviews.llvm.org/D95174 Comment Actions Slightly orthogonal to this patch but somewhat related: do we want to make malloc's size noundef as well (as well as many allocator functions' size args)? %size = load i64, i64* %my_ptr_size ; If malloc's size is noundef, !noundef can be attached to this load %ptr = call malloc(i64 %size) If we're moving towards allowing well-defined pointers and access sizes only, this also makes sense. Herald added subscribers: zzheng, aheejin, jgravelle-google and 2 others. · View Herald TranscriptFeb 19 2021, 7:02 PM Closed by commit rG33b0c63775ce: [Libcalls, Attrs] Annotate libcalls with noundef (authored by xbolva00). · Explain Why This revision was automatically updated to reflect the committed changes. xbolva00 added a reverting change: rG94d034fb8658: Revert "[Libcalls, Attrs] Annotate libcalls with noundef".Feb 19 2021, 7:19 PM
Revision Contents
Diff 325137 llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
llvm/test/Analysis/BasicAA/gep-alias.ll
llvm/test/Analysis/TypeBasedAliasAnalysis/memcpyopt.ll
llvm/test/Other/cgscc-libcall-update.ll
llvm/test/Transforms/InstCombine/2010-05-30-memcpy-Struct.ll
llvm/test/Transforms/InstCombine/AMDGPU/memcpy-from-constant.ll
llvm/test/Transforms/InstCombine/ARM/strcmp.ll
llvm/test/Transforms/InstCombine/align-addr.ll
llvm/test/Transforms/InstCombine/fortify-folding.ll
llvm/test/Transforms/InstCombine/getelementptr.ll
llvm/test/Transforms/InstCombine/malloc-free-delete.ll
llvm/test/Transforms/InstCombine/mem-deref-bytes-addrspaces.ll
llvm/test/Transforms/InstCombine/mem-deref-bytes.ll
llvm/test/Transforms/InstCombine/memccpy.ll
llvm/test/Transforms/InstCombine/memchr.ll
llvm/test/Transforms/InstCombine/memcmp-constant-fold.ll
llvm/test/Transforms/InstCombine/memcpy-from-global.ll
llvm/test/Transforms/InstCombine/memcpy-to-load.ll
llvm/test/Transforms/InstCombine/memcpy.ll
llvm/test/Transforms/InstCombine/memcpy_chk-1.ll
llvm/test/Transforms/InstCombine/memmove_chk-1.ll
llvm/test/Transforms/InstCombine/mempcpy.ll
llvm/test/Transforms/InstCombine/memset-1.ll
llvm/test/Transforms/InstCombine/memset_chk-1.ll
llvm/test/Transforms/InstCombine/objsize.ll
llvm/test/Transforms/InstCombine/printf-1.ll
llvm/test/Transforms/InstCombine/puts-1.ll
llvm/test/Transforms/InstCombine/snprintf.ll
llvm/test/Transforms/InstCombine/sprintf-1.ll
llvm/test/Transforms/InstCombine/stpcpy-1.ll
llvm/test/Transforms/InstCombine/stpcpy_chk-1.ll
llvm/test/Transforms/InstCombine/strchr-1.ll
llvm/test/Transforms/InstCombine/strcmp-1.ll
llvm/test/Transforms/InstCombine/strcmp-memcmp.ll
llvm/test/Transforms/InstCombine/strcpy-1.ll
llvm/test/Transforms/InstCombine/strcpy_chk-1.ll
llvm/test/Transforms/InstCombine/strcspn-1.ll
llvm/test/Transforms/InstCombine/strlen-1.ll
llvm/test/Transforms/InstCombine/strlen-2.ll
llvm/test/Transforms/InstCombine/strncat-2.ll
llvm/test/Transforms/InstCombine/strncmp-1.ll
llvm/test/Transforms/InstCombine/strncpy-1.ll
llvm/test/Transforms/InstCombine/strncpy-3.ll
llvm/test/Transforms/InstCombine/strncpy_chk-1.ll
llvm/test/Transforms/InstCombine/strpbrk-1.ll
llvm/test/Transforms/InstCombine/strrchr-1.ll
llvm/test/Transforms/InstCombine/strstr-1.ll
llvm/test/Transforms/LoopUnroll/WebAssembly/basic-unrolling.ll
llvm/test/Transforms/LowerMatrixIntrinsics/multiply-fused-loops.ll
llvm/test/Transforms/LowerMatrixIntrinsics/multiply-fused-multiple-blocks.ll
llvm/test/Transforms/LowerMatrixIntrinsics/multiply-fused.ll
llvm/test/Transforms/MemCpyOpt/lifetime.ll
llvm/test/Transforms/MemCpyOpt/memcpy-to-memset-with-lifetimes.ll
llvm/test/Transforms/MemCpyOpt/pr29105.ll
|