Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in rv_s_insn
From: Pu Lehui
Date: Tue Aug 08 2023 - 22:32:46 EST
On 2023/8/9 0:08, Luke Nelson wrote:
static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
{
- u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
+ u32 imm11_5 = (imm11_0 >> 5) & 0x7f, imm4_0 = imm11_0 & 0x1f;
Hi Luke,
keep u8 and add 0x7f explicit mask should work. I ran the repro case and it can silence the warning.
return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
(imm4_0 << 7) | opcode;
Hi Luke,
Thank for more detailed explanation.
That does fix the warning, but I think explicitly declaring imm11_5
as u32 is more clear here than the current code which relies on
implicit promotion of imm11_5 from u8 to signed int in the expression
(imm11_5 << 25).
Because of the promotion to signed int, (imm11_5 << 25) is technically
signed overflow and undefined behavior whenever the shift changes
the value in the sign bit. In practice, this isn't an issue; both
because the kernel is compiled with -fno-strict-overflow, but also
because GCC documentation explicitly states that "GCC does not use
the latitude given in C99 and C11 only to treat certain aspects of
signed '<<' as undefined" [1].
Though it may not be an issue in practice, since I'm touching this
line anyways to fix the warning, I think it makes sense to update
the type of imm11_5 to be u32 at the same time.
Agree. But this inconsistency looks weird, i.e. imm11_5 change to u32
while rs2, rs1 and funct3 still u8. Anyway, our primary goal is to
silence the sparse warning, and the current patch looks good to me.
Let's go ahead.
Feel free to add:
Reviewed-by: Pu Lehui <pulehui@xxxxxxxxxx>
Nit: maybe use "riscv, bpf" for the subject will look nice for the riscv-bpf git log tree.
Sure, I can send out a new revision with an updated subject line.
- Luke
[1]: https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html