On 09/09/17 00:47, Y Song wrote:

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?

Hi Y Song,

Thanks, patch updated, please review.

Changes since the initial version:

* Addressed all comments on instruction unit tests.
- Renamed test file to "insn-unit.s".
- Removed unnecessary comments.
- Added BPF_B tests for load and store.
- Added BPF_K test for jmp and alu.
(Noticed a seperate issue with unsigned compare then jump. Looks like BPF
backend is alway printing the immediate as signed, I guess we need to
change the operand types in instruction patterns.)
- Adjusted constant load tests.
- Interleaved the expected results with input insns.

* Changed the disassembly output "ll" to "LL" in BPFInstPrinter.cpp.
This is because I noticed MC parser only accept "LL" as long long suffix.
It might be the reason documented here:

https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=19759250