arm64/disassem.c: Add support insts of shifted register with rsv option

[koliagogsadze_gmail.com]

Summary

Added disassembly support for the following shifted register instructions:

• adds
• subs
• sub
• neg
• negs
• cmp
• cmn

Test Plan

https://github.com/freebsd/freebsd-src/compare/main...toor1245:freebsd-src:arm64-shifted-registers-rsv-tests

[koliagogsadze_gmail.com]

refs to review:
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/ADDS--shifted-register---Add--shifted-register---setting-flags-
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/CMN--shifted-register---Compare-Negative--shifted-register---an-alias-of-ADDS--shifted-register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/CMP--shifted-register---Compare--shifted-register---an-alias-of-SUBS--shifted-register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/NEG--shifted-register---Negate--shifted-register---an-alias-of-SUB--shifted-register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/NEGS--Negate--setting-flags--an-alias-of-SUBS--shifted-register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/SUB--shifted-register---Subtract--shifted-register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/SUBS--shifted-register---Subtract--shifted-register---setting-flags-

[mhorne]

The wording used by the docs is confusing, but it is actually saying that disassemblers should output the aliases when possible.

For example, we should print cmn x2, #1 rather than adds xzr, x2, #1.

This is the behaviour of GNU and LLVM objdump utilities.

[mhorne]

sys/arm64/arm64/disassem.c

156–158

I believe it will be sufficient to add the alias like this, since we walk the list front-to-back, it will be matched first.

[koliagogsadze_gmail.com]

In D40006#912681, @mhorne wrote:

The wording used by the docs is confusing, but it is actually saying that disassemblers should output the aliases when possible.

For example, we should print cmn x2, #1 rather than adds xzr, x2, #1.

This is the behaviour of GNU and LLVM objdump utilities.

understood, thanks!

[koliagogsadze_gmail.com]

sys/arm64/arm64/disassem.c

156–158

Unfortunatelly, we can't do like this, because in case of TYPE_01 we read RD and RN registers as "Mandatory tokens" via arm64_disasm_read_token function:

ret = arm64_disasm_read_token(i_ptr, insn, "RD", &rd);
ret |= arm64_disasm_read_token(i_ptr, insn, "RN", &rn);

if we just use constant (e.g. 11111) instead of RD(5) or RN(5) we will get error message:

printf("ERROR: "
    "Missing mandatory token for op %s type %d\n",
    i_ptr->name, i_ptr->type);

I was considering introducing OP_ flags, I think it's possible to do it in this type, but we have to properly handle conditions with current logic, cases like:
we have to take into account OP_ flags that indicates RD or RN is 11111.

Distinguish when we only have RD or only RN with RM, and remember that RM can be missing like this

OP <RD>, <RN>, #<imm>{, <shift [0, 12]>} SF32/64

I think adding new patterns will complicate the TYPE_1 logic and make it harder to read/maintain, so the second approach is to introduce a new type TYPE_04

What do you think?

[koliagogsadze_gmail.com]

sys/arm64/arm64/disassem.c

156–158

second approach patch:

0001-arm64-disassem.c-add-aliases-of-shifted-register-ins.patch4 KBDownload

[mhorne]

sys/arm64/arm64/disassem.c

156–158

Okay, that is indeed a problem. I like your second solution, it is clean enough.

I do not think that the "mandatory token" checks are very important, because the only way they can be triggered is when a disassembly mask is incorrectly defined. In other words, once the author (you) has correctly defined the entry in arm64_i[], these error messages will never be emitted.

So, it might be possible to remove these checks, and make all of rd, rm, and rn optional for TYPE_01. If it works and is not too messy I would prefer that to adding a whole new TYPE, but maybe there is something I am overlooking.

[koliagogsadze_gmail.com]

sys/arm64/arm64/disassem.c

156–158

I removed these checks in TYPE_01. We should check that we have rd and rn, if they are present, then we will use these patterns:

OP <RD>, <RN>, <RM>{, <shift [LSL, LSR, ASR]> #<imm>} SF32/64
OP <RD>, <RN>, #<imm>{, <shift [0, 12]>} SF32/64

if we have either only RD or only RN, we will use:

OP <RD>, <RM> {, <shift> #<imm> }
OP <RN>, <RM> {, <shift> #<imm> }

We do not change the RM logic, because it is the same and it is last register in the patterns:

OP <RD>, <RN>, <RM>{, <shift [LSL, LSR, ASR]> #<imm>} SF32/64
OP <RD>, <RM> {, <shift> #<imm> }
OP <RN>, <RM> {, <shift> #<imm> }

patch:

0001-arm64-shift-reg-with-rsv-type_01.patch4 KBDownload

in this case we added few checks and removed validation on "mandatory token", so this looks like a minimal change to me and does not complicate the logic too much, now I think that it's better suited than adding a new type, what do you think?

[mhorne]

sys/arm64/arm64/disassem.c

156–158

Yes, the result is excellent! Please go ahead and update this review with the new approach.

[koliagogsadze_gmail.com]

Added new patterns to TYPE_01

[mhorne]

Looks good, one thing to fix.

sys/arm64/arm64/disassem.c

416

Consider changing these to bool type. Maybe it needs to be a separate change to handle arm64_disasm_read_token() return value. Either way it is not critical.

494

Since these are effectively boolean values, we should check this way.

497

[koliagogsadze_gmail.com]

Updated type of rm_absent, rd_absent, rn_absent to bool.

[mhorne]

Thanks. I will commit this and D39820 today or tomorrow.