Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6\))
Subject: Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in
 rv_s_insn
From:   Luke Nelson <lukenels@cs.washington.edu>
In-Reply-To: <b85bcf1d-9467-4df6-da11-8f0b24165ada@huawei.com>
Date:   Tue, 8 Aug 2023 09:08:38 -0700
Cc:     bpf@vger.kernel.org, kernel test robot <lkp@intel.com>,
        Xi Wang <xi.wang@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>,
        Yonghong Song <yonghong.song@linux.dev>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <CC58CCF3-1994-4C50-B8FC-E1520ED743BF@cs.washington.edu>
References: <20230727024931.17156-1-luke.r.nels@gmail.com>
 <b85bcf1d-9467-4df6-da11-8f0b24165ada@huawei.com>
To:     Pu Lehui <pulehui@huawei.com>
Precedence: bulk


>>  static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, =
u8 opcode)
>>  {
>> - u8 imm11_5 =3D imm11_0 >> 5, imm4_0 =3D imm11_0 & 0x1f;
>> + u32 imm11_5 =3D (imm11_0 >> 5) & 0x7f, imm4_0 =3D imm11_0 & 0x1f;
>=20
> Hi Luke,
>=20
> keep u8 and add 0x7f explicit mask should work. I ran the repro case =
and it can silence the warning.
>=20
>>=20
>>   return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) =
|
>>   (imm4_0 << 7) | opcode;

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.

> 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