On Fri, Sep 8, 2017 at 1:00 PM, Jiong Wang <jiong.wang@...> wrote:

Hi Y Song,

Thanks for the review and test.

[snip]

I guess, we may need to add something like LD_VAR_ADDR so that
the proper assembly code can be issued. Also we could drop "ll" with
"r1 = tx_port"?

Personally, I prefer drop "ll". The "ll" suffix was there to tell people it is 64bit
constant while the register "r1" is with 64-bit width so I feel it is enough. For
32-bit situation, we need new register set, some thing like "w1 = tx_port".

I just push a patch (similar to your suggestion below but without
assertion) which still has
"ll" suffix for the constant, but no "ll" suffix for symbols. The
reason is that we use "ll"
in the asm printout to differentiate "r = 5"=>"mov r, 5" (32bit imm) and
"r = 5ll" => "ld_imm64 r, 5ll" (64bit imm).

Also, for long long constant, C standard does not allow space between
value and "ll" and
hence "567 ll" is not allowed to represent a number "567ll". Could you
try to disallow
64bit immediate like "567 ll" as well in your patch?

Thanks,

Yonghong

And to generate proper assembly, it looks to me the customized print method for
"uimm64" needs the following code (copied from printOperand):

@@ -90,5 +90,8 @@ void BPFInstPrinter::printImm64Operand(const MCInst *MI, unsigned OpNo,
if (Op.isImm())
O << (uint64_t)Op.getImm();
else
- O << Op;
+ {
+ assert(Op.isExpr() && "Expected an expression");
+ printExpr(Op.getExpr(), O);
+ }
}

Back to your test case bpf-insn-unit.s, you do not need add bpf in the file name
since it is already under BPF directory.

Will change the name. and will fix all reviews below in the next update.

r0 = *(u16 *)skb[2] // BPF_LD | BPF_ABS | BPF_H
r0 = * (u32*)skb[4] // BPF_LD | BPF_ABS | BPF_W

could you add BPF_B test as well?

r0 = * (u16 *)skb[r1] // BPF_LD | BPF_IND | BPF_H
r0 = *(u32 *)skb[r2] // BPF_LD | BPF_IND | BPF_W

BPF_B test?

r1 = 8589934591 ll // BPF_LD | BPF_DW | BPF_IMM
r1 = dummy_map ll // Symbolic version

// ======== BPF_LDX Class ========
r5 = *(u8 *)(r0 + 0) // BPF_LDX | BPF_B
r6 = *(u16 *)(r1 + 8) // BPF_LDX | BPF_H
r7 = *(u32 *)(r2 + 16) // BPF_LDX | BPF_W
r8 = *(u64 *)(r3 - 30) // BPF_LDX | BPF_DW

// ======== TODO: BPF_ST Class =======

There is no insn here, you can remove this line for now.

// ======== BPF_STX Class ========
*(u8 *)(r0 + 0) = r7 // BPF_STX | BPF_B
*(u16 *)(r1 + 8) = r8 // BPF_STX | BPF_H
*(u32 *)(r2 + 16) = r9 // BPF_STX | BPF_W
*(u64 *)(r3 - 30) = r10 // BPF_STX | BPF_DW

lock *(u32 *)(r2 + 16) += r9 // BPF_STX | BPF_W | BPF_XADD
lock *(u64 *)(r3 - 30) += r10 // BPF_STX | BPF_DW | BPF_XADD

// ======== BPF_JMP Class ========
goto Llabel0 // BPF_JA
if r0 == r1 goto Llabel0 // BPF_JEQ
if r1 > r2 goto Llabel0 // BPF_JGT
if r2 >= r3 goto Llabel0 // BPF_JGE
// TODO: BPF_JSET

there is no BPF_JSET, you can remove this.

if r3 != r4 goto Llabel0 // BPF_JNE
if r4 s> r5 goto Llabel0 // BPF_JSGT
if r5 s>= r6 goto Llabel0 // BPF_JSGE
call 1 // BPF_CALL
exit // BPF_EXIT
if r6 < r7 goto Llabel0 // BPF_JLT
if r7 <= r8 goto Llabel0 // BPF_JLE
if r8 s< r9 goto Llabel0 // BPF_JSLT
if r9 s<= r10 goto Llabel0 // BPF_JSLE

Could you add test compare register vs. imm?

// ======== BPF_ALU64 Class ========
r0 += r1 // BPF_ADD
r1 -= r2 // BPF_SUB
r2 *= r3 // BPF_MUL
r3 /= r4 // BPF_DIV
Llabel0:
r4 |= r5 // BPF_OR
r5 &= r6 // BPF_AND
r6 <<= r7 // BPF_LSH
r7 >>= r8 // BPF_RSH
// TODO: BPF_NEG
// TODO: BPF_MOD

There is no BPF_NEG/MOD yet, you can remove these two lines.

r8 ^= r9 // BPF_XOR
r9 = r10 // BPF_MOV
r10 s>>= r0 // BPF_ARSH

Could you add arith operations with register and imm. operands?

bswap16 r1 // BPF_END | BPF_TO_BE
bswap32 r2 // BPF_END | BPF_TO_BE
bswap64 r3 // BPF_END | BPF_TO_BE
// TODO: BPF_END | BPF_TO_LE

There is no BPF_TO_LE yet, you can remove this line.

Could you interleave the CHECK results with insn? This will be

clearer to do manual comparison.