Messages in this thread | | | From | Huacai Chen <> | Date | Fri, 14 Oct 2022 09:13:39 +0800 | Subject | Re: [PATCH] LoongArch: BPF: Avoid declare variables in switch-case |
| |
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 ‘emit_atomic’: > > 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 = regmap[BPF_REG_0]; > > ^~ > > arch/loongarch/net/bpf_jit.c: In function ‘build_insn’: > > 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 = -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 ‘u64’ > > u64 func_addr; > > ^~~ > > arch/loongarch/net/bpf_jit.c:780:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] > > bool func_addr_fixed; > > ^~~~ > > arch/loongarch/net/bpf_jit.c:784:11: error: ‘func_addr’ undeclared (first use in this function); did you mean ‘in_addr’? > > &func_addr, &func_addr_fixed); > > ^~~~~~~~~ > > in_addr > > arch/loongarch/net/bpf_jit.c:784:11: note: each undeclared identifier is 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 = (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 = LOONGARCH_GPR_T1; > > const u8 t2 = LOONGARCH_GPR_T2; > > const u8 t3 = LOONGARCH_GPR_T3; > > + const u8 r0 = regmap[BPF_REG_0]; > > const u8 src = regmap[insn->src_reg]; > > const u8 dst = regmap[insn->dst_reg]; > > const s16 off = insn->off; > > @@ -359,8 +360,6 @@ static void emit_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx) > > break; > > /* r0 = atomic_cmpxchg(dst + off, r0, src); */ > > case BPF_CMPXCHG: > > - u8 r0 = 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 *ctx, bool extra_pass) > > { > > - const bool is32 = BPF_CLASS(insn->code) == BPF_ALU || > > - BPF_CLASS(insn->code) == BPF_JMP32; > > + u8 t0 = -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 = 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 = insn - ctx->prog->insnsi; > > + int ret, jmp_offset; > > const u8 code = insn->code; > > const u8 cond = BPF_OP(code); > > const u8 t1 = 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 = regmap[insn->dst_reg]; > > const s16 off = insn->off; > > const s32 imm = insn->imm; > > - int jmp_offset; > > - int i = insn - ctx->prog->insnsi; > > + const u64 imm64 = (u64)(insn + 1)->imm << 32 | (u32)insn->imm; > > + const bool is32 = BPF_CLASS(insn->code) == BPF_ALU || BPF_CLASS(insn->code) == 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 = 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 = -1; > > jmp_offset = bpf2la_offset(i, off, ctx); > > if (imm) { > > move_imm(ctx, t1, imm, false); > > - t7 = t1; > > + t0 = t1; > > } else { > > /* If imm is 0, simply use zero register. */ > > - t7 = LOONGARCH_GPR_ZERO; > > + t0 = 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 = 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 = imm64 */ > > case BPF_LD | BPF_IMM | BPF_DW: > > - u64 imm64 = (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/ > >
| |