Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in rv_s_insn

[Pu Lehui <pulehui@huawei.com>]

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@huawei.com>

>> 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