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;
MIME-Version: 1.0
References: <20221013154000.3462836-1-chenhuacai@loongson.cn> <8a8fa581-94a9-649d-8c01-f1afd4bc9514@xen0n.name>
In-Reply-To: <8a8fa581-94a9-649d-8c01-f1afd4bc9514@xen0n.name>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Fri, 14 Oct 2022 09:13:39 +0800
Message-ID: <CAAhV-H5U1c_wfWLuxMaHD6c9-k+g-iSqgtcJVwceoL13J7hEiA@mail.gmail.com>
Subject: Re: [PATCH] LoongArch: BPF: Avoid declare variables in switch-case
To:     WANG Xuerui <kernel@xen0n.name>
Cc:     Huacai Chen <chenhuacai@loongson.cn>, loongarch@lists.linux.dev,
        Xuefeng Li <lixuefeng@loongson.cn>,
        Tiezhu Yang <yangtiezhu@loongson.cn>,
        Guo Ren <guoren@kernel.org>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi, Xuerui,

On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <kernel@xen0n.name> wrote:
>
> On 10/13/22 23:40, Huacai Chen wrote:
> > Not all compilers support declare variables in switch-case, so move
> > declarations to the beginning of a function. Otherwise we may get such
> > build errors:
> >
> > arch/loongarch/net/bpf_jit.c: In function =E2=80=98emit_atomic=E2=80=99=
:
> > arch/loongarch/net/bpf_jit.c:362:3: error: a label can only be part of =
a statement and a declaration is not a statement
> >     u8 r0 =3D regmap[BPF_REG_0];
> >     ^~
> > arch/loongarch/net/bpf_jit.c: In function =E2=80=98build_insn=E2=80=99:
> > arch/loongarch/net/bpf_jit.c:727:3: error: a label can only be part of =
a statement and a declaration is not a statement
> >     u8 t7 =3D -1;
> >     ^~
> > arch/loongarch/net/bpf_jit.c:778:3: error: a label can only be part of =
a statement and a declaration is not a statement
> >     int ret;
> >     ^~~
> > arch/loongarch/net/bpf_jit.c:779:3: error: expected expression before =
=E2=80=98u64=E2=80=99
> >     u64 func_addr;
> >     ^~~
> > arch/loongarch/net/bpf_jit.c:780:3: warning: ISO C90 forbids mixed decl=
arations and code [-Wdeclaration-after-statement]
> >     bool func_addr_fixed;
> >     ^~~~
> > arch/loongarch/net/bpf_jit.c:784:11: error: =E2=80=98func_addr=E2=80=99=
 undeclared (first use in this function); did you mean =E2=80=98in_addr=E2=
=80=99?
> >            &func_addr, &func_addr_fixed);
> >             ^~~~~~~~~
> >             in_addr
> > arch/loongarch/net/bpf_jit.c:784:11: note: each undeclared identifier i=
s reported only once for each function it appears in
> > arch/loongarch/net/bpf_jit.c:814:3: error: a label can only be part of =
a statement and a declaration is not a statement
> >     u64 imm64 =3D (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
> >     ^~~
> >
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> > ---
> >   arch/loongarch/net/bpf_jit.c | 31 +++++++++++++------------------
> >   1 file changed, 13 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.=
c
> > index 43f0a98efe38..2a9b590f47e6 100644
> > --- a/arch/loongarch/net/bpf_jit.c
> > +++ b/arch/loongarch/net/bpf_jit.c
> > @@ -279,6 +279,7 @@ static void emit_atomic(const struct bpf_insn *insn=
, struct jit_ctx *ctx)
> >       const u8 t1 =3D LOONGARCH_GPR_T1;
> >       const u8 t2 =3D LOONGARCH_GPR_T2;
> >       const u8 t3 =3D LOONGARCH_GPR_T3;
> > +     const u8 r0 =3D regmap[BPF_REG_0];
> >       const u8 src =3D regmap[insn->src_reg];
> >       const u8 dst =3D regmap[insn->dst_reg];
> >       const s16 off =3D insn->off;
> > @@ -359,8 +360,6 @@ static void emit_atomic(const struct bpf_insn *insn=
, struct jit_ctx *ctx)
> >               break;
> >       /* r0 =3D atomic_cmpxchg(dst + off, r0, src); */
> >       case BPF_CMPXCHG:
> > -             u8 r0 =3D regmap[BPF_REG_0];
> > -
> >               move_reg(ctx, t2, r0);
> >               if (isdw) {
> >                       emit_insn(ctx, lld, r0, t1, 0);
> > @@ -390,8 +389,11 @@ static bool is_signed_bpf_cond(u8 cond)
> >
> >   static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ct=
x, bool extra_pass)
> >   {
> > -     const bool is32 =3D BPF_CLASS(insn->code) =3D=3D BPF_ALU ||
> > -                       BPF_CLASS(insn->code) =3D=3D BPF_JMP32;
> > +     u8 t0 =3D -1;
> Here "t0" seems to be a versatile temp value, while the "t1" below is
> the actual GPR $t1. What about renaming "t0" to something like "tmp" to
> reduce confusion? I believe due to things like "t0 =3D LOONGARCH_GPR_ZERO=
"
> the "t0" is 100% not an actual mapping to $t0.
I rename t7 to t0 because there is no t3-t6, t7 looks very strange.
But from emit_cond_jmp() the 3rd and 4th parameters have no difference
so I suppose t0 is just OK, then whether rename it to tmp depends on
Tiezhu's opinion.

> > +     u64 func_addr;
> > +     bool func_addr_fixed;
> > +     int i =3D insn - ctx->prog->insnsi;
> > +     int ret, jmp_offset;
> >       const u8 code =3D insn->code;
> >       const u8 cond =3D BPF_OP(code);
> >       const u8 t1 =3D LOONGARCH_GPR_T1;
> > @@ -400,8 +402,8 @@ static int build_insn(const struct bpf_insn *insn, =
struct jit_ctx *ctx, bool ext
> >       const u8 dst =3D regmap[insn->dst_reg];
> >       const s16 off =3D insn->off;
> >       const s32 imm =3D insn->imm;
> > -     int jmp_offset;
> > -     int i =3D insn - ctx->prog->insnsi;
> > +     const u64 imm64 =3D (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
> > +     const bool is32 =3D BPF_CLASS(insn->code) =3D=3D BPF_ALU || BPF_C=
LASS(insn->code) =3D=3D BPF_JMP32;
> Please consider reducing diff damage and not touching parts not directly
> affected by this change. For example this "is32" declaration and
> initialization was moved although not related to this change.
I think defining variables from simple to complex and grouping them
can make life easier. :)

Huacai
> >
> >       switch (code) {
> >       /* dst =3D src */
> > @@ -724,24 +726,23 @@ static int build_insn(const struct bpf_insn *insn=
, struct jit_ctx *ctx, bool ext
> >       case BPF_JMP32 | BPF_JSGE | BPF_K:
> >       case BPF_JMP32 | BPF_JSLT | BPF_K:
> >       case BPF_JMP32 | BPF_JSLE | BPF_K:
> > -             u8 t7 =3D -1;
> >               jmp_offset =3D bpf2la_offset(i, off, ctx);
> >               if (imm) {
> >                       move_imm(ctx, t1, imm, false);
> > -                     t7 =3D t1;
> > +                     t0 =3D t1;
> >               } else {
> >                       /* If imm is 0, simply use zero register. */
> > -                     t7 =3D LOONGARCH_GPR_ZERO;
> > +                     t0 =3D LOONGARCH_GPR_ZERO;
> >               }
> >               move_reg(ctx, t2, dst);
> >               if (is_signed_bpf_cond(BPF_OP(code))) {
> > -                     emit_sext_32(ctx, t7, is32);
> > +                     emit_sext_32(ctx, t0, is32);
> >                       emit_sext_32(ctx, t2, is32);
> >               } else {
> > -                     emit_zext_32(ctx, t7, is32);
> > +                     emit_zext_32(ctx, t0, is32);
> >                       emit_zext_32(ctx, t2, is32);
> >               }
> > -             if (emit_cond_jmp(ctx, cond, t2, t7, jmp_offset) < 0)
> > +             if (emit_cond_jmp(ctx, cond, t2, t0, jmp_offset) < 0)
> >                       goto toofar;
> >               break;
> >
> > @@ -775,10 +776,6 @@ static int build_insn(const struct bpf_insn *insn,=
 struct jit_ctx *ctx, bool ext
> >
> >       /* function call */
> >       case BPF_JMP | BPF_CALL:
> > -             int ret;
> > -             u64 func_addr;
> > -             bool func_addr_fixed;
> > -
> >               mark_call(ctx);
> >               ret =3D bpf_jit_get_func_addr(ctx->prog, insn, extra_pass=
,
> >                                           &func_addr, &func_addr_fixed)=
;
> > @@ -811,8 +808,6 @@ static int build_insn(const struct bpf_insn *insn, =
struct jit_ctx *ctx, bool ext
> >
> >       /* dst =3D imm64 */
> >       case BPF_LD | BPF_IMM | BPF_DW:
> > -             u64 imm64 =3D (u64)(insn + 1)->imm << 32 | (u32)insn->imm=
;
> > -
> >               move_imm(ctx, dst, imm64, is32);
> >               return 1;
> >
>
> --
> WANG "xen0n" Xuerui
>
> Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
>
>