-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[BPF] Report an warning if certain insn imm operand cannot fit in 32bit #142989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc @jemarch |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
0ba6601 to
17df3a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but what is the conclusion of the upstream discussion? should the error be reproted for Imm > UINT_MAX or for Imm > INT_MAX? I think the latter makes sense, as you suggested upsteream.
Yes, I do prefer Imm > INT_MAX. But I will emit warning instead of error to maintain some kind of backward compatibility. |
474eb57 to
5148d25
Compare
5148d25 to
b51ec73
Compare
|
There are quite a few kernel selftests failing to build because of the -Werror: But all seem to be legit warnings for statements like: |
Right, this one definitely legit warnings. |
b51ec73 to
4da76a2
Compare
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
|
Latest changes lgtm. |
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
In one of upstream thread ([1]), there is a discussion about
the below inline asm code:
if r1 == 0xdeadbeef goto +2;
...
In actual llvm backend, the above 0xdeadbeef will actually do
sign extension to 64bit value and then compare to register r1.
But the code itself does not imply the above semantics. It looks
like the comparision is between r1 and 0xdeadbeef. For example,
let us at a simple C code:
$ cat t1.c
int foo(long a) { return a == 0xdeadbeef ? 2 : 3; }
$ clang --target=bpf -O2 -c t1.c && llvm-objdump -d t1.o
...
w0 = 0x2
r2 = 0xdeadbeef ll
if r1 == r2 goto +0x1
w0 = 0x3
exit
It does try to compare r1 and 0xdeadbeef.
To address the above confusing inline asm issue, llvm backend ([2])
added some range checking for such insns and beyond. For the above
insn asm, the warning like below
warning: immediate out of range, shall fit in int range
will be issued. If -Werror is in the compilation flags, the
error will be issued.
To avoid the above warning/error, the afore-mentioned inline asm
should be rewritten to
if r1 == -559038737 goto +2;
...
Fix a few selftest cases like the above based on insn range checking
requirement in [2].
[1] https://lore.kernel.org/bpf/[email protected]/
[2] llvm/llvm-project#142989
Signed-off-by: Yonghong Song <[email protected]>
Ihor Solodrai reported a case ([1]) where gcc reports an error but clang ignores that error and proceeds to generate incorrect code. More specifically, the problematic code looks like: if r1 == 0xcafefeeddeadbeef goto <label> Here, 0xcafefeeddeadbeef needs to be encoded in a 32-bit imm field of the insns and the 32-bit imm allows sign extenstion to 64-bit imm. Obviously, 0xcafefeeddeadbeef cannot encode properly. The compilation failed for gcc with the following error: Error: immediate out of range, shall fit in 32 bits Given a 64-bit imm value, converting to the proper 32-bit imm value must satisfy the following 64-bit patterns: 00000000 00000000 00000000 00000000 xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx 11111111 11111111 11111111 11111111 1xxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx So if the top 32-bits is 0 or the top 33-bits is 0x1ffffffff, then the 64-bit imm value can be truncated into proper 32-bit imm. Otherwise, a warning message, the same as gcc will be issued. If -Werror is enabled during compilation, the warning will turn into an error. [1] https://lore.kernel.org/bpf/[email protected]/
4da76a2 to
4110710
Compare
…it (llvm#142989) Ihor Solodrai reported a case ([1]) where gcc reports an error but clang ignores that error and proceeds to generate incorrect code. More specifically, the problematic code looks like: if r1 == 0xcafefeeddeadbeef goto <label> Here, 0xcafefeeddeadbeef needs to be encoded in a 32-bit imm field of the insns and the 32-bit imm allows sign extenstion to 64-bit imm. Obviously, 0xcafefeeddeadbeef cannot encode properly. The compilation failed for gcc with the following error: Error: immediate out of range, shall fit in 32 bits Given a 64-bit imm value, converting to the proper 32-bit imm value must satisfy the following 64-bit patterns: 00000000 00000000 00000000 00000000 xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx 11111111 11111111 11111111 11111111 1xxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx So if the top 32-bits is 0 or the top 33-bits is 0x1ffffffff, then the 64-bit imm value can be truncated into proper 32-bit imm. Otherwise, a warning message, the same as gcc, will be issued. If -Werror is enabled during compilation, the warning will turn into an error. [1] https://lore.kernel.org/bpf/[email protected]/
…it (llvm#142989) Ihor Solodrai reported a case ([1]) where gcc reports an error but clang ignores that error and proceeds to generate incorrect code. More specifically, the problematic code looks like: if r1 == 0xcafefeeddeadbeef goto <label> Here, 0xcafefeeddeadbeef needs to be encoded in a 32-bit imm field of the insns and the 32-bit imm allows sign extenstion to 64-bit imm. Obviously, 0xcafefeeddeadbeef cannot encode properly. The compilation failed for gcc with the following error: Error: immediate out of range, shall fit in 32 bits Given a 64-bit imm value, converting to the proper 32-bit imm value must satisfy the following 64-bit patterns: 00000000 00000000 00000000 00000000 xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx 11111111 11111111 11111111 11111111 1xxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx So if the top 32-bits is 0 or the top 33-bits is 0x1ffffffff, then the 64-bit imm value can be truncated into proper 32-bit imm. Otherwise, a warning message, the same as gcc, will be issued. If -Werror is enabled during compilation, the warning will turn into an error. [1] https://lore.kernel.org/bpf/[email protected]/
Ihor Solodrai reported a case ([1]) where gcc reports an error but clang
ignores that error and proceeds to generate incorrect code. More
specifically, the problematic code looks like:
Here, 0xcafefeeddeadbeef needs to be encoded in a 32-bit imm field
of the insns and the 32-bit imm allows sign extenstion to 64-bit imm.
Obviously, 0xcafefeeddeadbeef cannot encode properly.
The compilation failed for gcc with the following error:
Given a 64-bit imm value, converting to the proper 32-bit imm value
must satisfy the following 64-bit patterns:
So if the top 32-bits is 0 or the top 33-bits is 0x1ffffffff, then the 64-bit imm
value can be truncated into proper 32-bit imm. Otherwise, a warning
message, the same as gcc, will be issued. If -Werror is enabled during
compilation, the warning will turn into an error.
[1] https://lore.kernel.org/bpf/[email protected]/