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 <[email protected]>
---
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;
+ 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;
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;
--
2.31.1
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 <[email protected]>
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.
> + 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.
>
> 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/
Hi, Xuerui,
On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <[email protected]> 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 <[email protected]>
> 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/
>
>
On 10/13/2022 11:40 PM, Huacai Chen wrote:
> Not all compilers support declare variables in switch-case, so move
We know that gcc 13 has no problem, it is better to point out the
compiler version explicitly in the commit message so that the users
can know how to reproduce the build errors.
> declarations to the beginning of a function. Otherwise we may get such
> build errors:
We can resolve the errors by adding a pair of curly braces around
the statements of the case, like this:
switch (...) {
case ...:
{
int ...;
...
break;
}
}
>
> 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;
> ^~~
>
Thanks,
Tiezhu
On 10/14/2022 09:13 AM, Huacai Chen wrote:
> Hi, Xuerui,
>
> On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <[email protected]> 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:
...
>>>
>>> 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.
>
Use "tmp" seems better due to it is a temp value.
>>> + 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.
It looks reasonable, one change per patch is better.
> I think defining variables from simple to complex and grouping them
> can make life easier. :)
>
No strong opinion on this, I am OK either way.
Thanks,
Tiezhu
On Fri, Oct 14, 2022 at 10:18 AM Tiezhu Yang <[email protected]> wrote:
>
>
>
> On 10/14/2022 09:13 AM, Huacai Chen wrote:
> > Hi, Xuerui,
> >
> > On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <[email protected]> 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:
>
> ...
>
> >>>
> >>> 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.
> >
>
> Use "tmp" seems better due to it is a temp value.
OK, then I will use tmp or just tm for alignment.
>
> >>> + 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.
>
> It looks reasonable, one change per patch is better.
>
> > I think defining variables from simple to complex and grouping them
> > can make life easier. :)
> >
>
> No strong opinion on this, I am OK either way.
>
> Thanks,
> Tiezhu
>
>