[PATCH] D75799: [JumpThreading] Don't create PHI nodes with "is.constant" values
Bill Wendling via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Mar 7 16:45:08 PST 2020
void added a comment.
In D75799#1911082 <https://reviews.llvm.org/D75799#1911082>, @nikic wrote:
> This looks somewhat dubious to me. This seems like an InstCombine-style fold that is performed in JumpThreading -- but why? Won't it just get folded either earlier or later? As the test case is over-reduced, it's hard to tell where this would make a difference in practice.
No. In the original testcase (from net/ipv4/tcp.c in Linux with ASAN enabled), there are two calls to `copy_to_user` and `copy_from_user` near where the testcase is located. They each call `check_copy_size`:
static inline __attribute__((unused)) __attribute__((no_instrument_function))
__attribute__((always_inline)) bool
check_copy_size(const void *addr, size_t bytes, bool is_source)
{
int sz = __builtin_object_size(addr, 0);
if (__builtin_expect(!!(sz >= 0 && sz < bytes), 0)) {
if (!__builtin_constant_p(bytes))
copy_overflow(sz, bytes);
else if (is_source)
__bad_copy_from();
else
__bad_copy_to();
return false;
}
check_object_size(addr, bytes, is_source);
return true;
}
static inline __attribute__((unused)) __attribute__((no_instrument_function))
__attribute__((always_inline)) unsigned long __attribute__((warn_unused_result))
copy_from_user(void *to, const void *from, unsigned long n)
{
if (__builtin_expect(!!(check_copy_size(to, n, false)), 1))
n = _copy_from_user(to, from, n);
return n;
}
static inline __attribute__((unused)) __attribute__((no_instrument_function))
__attribute__((always_inline)) unsigned long __attribute__((warn_unused_result))
copy_to_user(void *to, const void *from, unsigned long n)
{
if (__builtin_expect(!!(check_copy_size(from, n, true)), 1))
n = _copy_to_user(to, from, n);
return n;
}
The `__bad_copy_*` functions don't actually exist, so we get a linking error if those code paths aren't removed. The GVN patch combines the `llvm.is.constant()` check that's used by both of the `copy_*_user` functions. It *should* resolve to `false`, which would cause it to execute `copy_overflow` and DCE the paths with the `__bad_*` calls. All of that seems okay and while I'm not particularly sure that GVN should care about `is.constant` calls, it's probably innocuous.
So once we get to jump threading, there's a call to `llvm.is.constant` that's the conditional of two `br` instructions (one for each `copy_*_user` call). While it's threading the blocks in this area. See `%5` below:
define i32 @do_tcp_getsockopt(%struct.sock* %sk, i32 %level, i32 %optname, i8* %optval, i32* %optlen) {
...
if.end18: ; preds = %if.then11, %if.end7
%len.0 = phi i32 [ 24, %if.then11 ], [ %conv, %if.end7 ]
%conv19 = sext i32 %len.0 to i64
%cmp3.i.i70 = icmp ugt i32 %len.0, 24
%5 = call i1 @llvm.is.constant.i64(i64 %conv19) #4
br i1 %cmp3.i.i70, label %if.then.i.i71, label %if.end12.i.i74
if.then.i.i71: ; preds = %if.end18
br i1 %5, label %if.else.i.i73, label %if.then7.i.i72
if.then7.i.i72: ; preds = %if.then.i.i71
call void (i8*, ...) @__warn_printk(i8* getelementptr inbounds ([38 x i8], [38 x i8]* @.str, i64 0, i64 0), i32 24, i64 %conv19) #9
call void asm sideeffect "...", "i,i,i,i,~{dirflag},~{fpsr},~{flags}"(i8* getelementptr inbounds ([30 x i8], [30 x i8]* @.str.1, i64 0, i64 0), i32 127, i32 2305, i64 12) #4
br label %copy_from_user.exit
if.else.i.i73: ; preds = %if.then.i.i71
call void @__bad_copy_to() #9
br label %copy_from_user.exit
...
if.then.i.i: ; preds = %zerocopy_rcv_out
br i1 %5, label %if.else.i.i, label %if.then7.i.i
if.then7.i.i: ; preds = %if.then.i.i
call void (i8*, ...) @__warn_printk(i8* getelementptr inbounds ([38 x i8], [38 x i8]* @.str, i64 0, i64 0), i32 24, i64 %conv19) #9
call void asm sideeffect "...", "i,i,i,i,~{dirflag},~{fpsr},~{flags}"(i8* getelementptr inbounds ([30 x i8], [30 x i8]* @.str.1, i64 0, i64 0), i32 127, i32 2305, i64 12) #4
br label %copy_to_user.exit
if.else.i.i: ; preds = %if.then.i.i
call void @__bad_copy_from() #9
br label %copy_to_user.exit
This is converted to this code after jump threading. See `%5` and `%8`:
define i32 @do_tcp_getsockopt(%struct.sock* %sk, i32 %level, i32 %optname, i8* %optval, i32* %optlen) {
...
if.end18: ; preds = %if.then11, %if.end7
%len.0 = phi i32 [ 24, %if.then11 ], [ %conv, %if.end7 ]
%conv19 = sext i32 %len.0 to i64
%cmp3.i.i70 = icmp ugt i32 %len.0, 24
%5 = call i1 @llvm.is.constant.i64(i64 %conv19) #4
br i1 %cmp3.i.i70, label %if.then.i.i71, label %if.end12.i.i74
if.then.i.i71: ; preds = %if.end18
br i1 %5, label %if.else.i.i73, label %if.then7.i.i72
if.then7.i.i72: ; preds = %if.then.i.i71
call void (i8*, ...) @__warn_printk(i8* getelementptr inbounds ([38 x i8], [38 x i8]* @.str, i64 0, i64 0), i32 24, i64 %conv19) #9
call void asm sideeffect "...", "i,i,i,i,~{dirflag},~{fpsr},~{flags}"(i8* getelementptr inbounds ([30 x i8], [30 x i8]* @.str.1, i64 0, i64 0), i32 127, i32 2305, i64 12) #4
br label %copy_from_user.exit
if.end12.i.i74: ; preds = %if.end18
%6 = phi i1 [ %5, %if.end18 ]
%cmp3.i.i7011 = phi i1 [ %cmp3.i.i70, %if.end18 ]
%conv196 = phi i64 [ %conv19, %if.end18 ]
%len.05 = phi i32 [ %len.0, %if.end18 ]
br i1 %6, label %if.then.i77, label %if.then.i.i.i75
if.then.i.i.i75: ; preds = %if.end12.i.i74
call void @__check_object_size(i8* nonnull %0, i64 %conv196, i1 zeroext false)
br label %if.then.i77
if.then.i77: ; preds = %if.then11, %if.then.i.i.i75, %if.end12.i.i74
%len.0516 = phi i32 [ %len.05, %if.then.i.i.i75 ], [ %len.05, %if.end12.i.i74 ], [ 24, %if.then11 ]
%cmp3.i.i701115 = phi i1 [ %cmp3.i.i7011, %if.then.i.i.i75 ], [ %cmp3.i.i7011, %if.end12.i.i74 ], [ false, %if.then11 ]
%7 = phi i1 [ %6, %if.then.i.i.i75 ], [ %6, %if.end12.i.i74 ], [ true, %if.then11 ]
%conv197 = phi i64 [ %conv196, %if.then.i.i.i75 ], [ %conv196, %if.end12.i.i74 ], [ 24, %if.then11 ]
%call2.i76 = call i64 @_copy_from_user(i8* nonnull %0, i8* %optval, i64 %conv197)
br label %copy_from_user.exit
copy_from_user.exit: ; preds = %if.then.i77, %if.else.i.i73, %if.then7.i.i72
%8 = phi i1 [ %7, %if.then.i77 ], [ %5, %if.then7.i.i72 ], [ %5, %if.else.i.i73 ]
...
if.then.i.i: ; preds = %zerocopy_rcv_out
br i1 %8, label %if.else.i.i, label %if.then7.i.i
if.then7.i.i: ; preds = %if.then.i.i
call void (i8*, ...) @__warn_printk(i8* getelementptr inbounds ([38 x i8], [38 x i8]* @.str, i64 0, i64 0), i32 24, i64 %conv198)
call void asm sideeffect "...", "i,i,i,i,~{dirflag},~{fpsr},~{flags}"(i8* getelementptr inbounds ([30 x i8], [30 x i8]* @.str.1, i64 0, i64 0), i32 127, i32 2305, i64 12) #2
br label %copy_to_user.exit
if.else.i.i: ; preds = %if.then.i.i
call void @__bad_copy_from()
br label %copy_to_user.exit
So it's unnecessarily complicated and the `__bad_copy_from` isn't being DCE'd as it should be.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75799/new/
https://reviews.llvm.org/D75799
More information about the llvm-commits
mailing list