Page MenuHomeFreeBSD

arm64/disassem.c: Fix typo sxts to sxts and amount for TYPE_02
ClosedPublic

Authored by koliagogsadze_gmail.com on Mar 29 2023, 7:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 18, 2:19 PM
Unknown Object (File)
Jan 5 2024, 5:46 PM
Unknown Object (File)
Dec 25 2023, 5:13 PM
Unknown Object (File)
Dec 23 2023, 1:54 AM
Unknown Object (File)
Dec 12 2023, 11:50 AM
Unknown Object (File)
Sep 6 2023, 2:47 AM
Unknown Object (File)
Aug 26 2023, 1:53 PM
Unknown Object (File)
Aug 15 2023, 2:22 PM
Subscribers

Details

Summary

Fixed typo sxts to sxtx according to Arm64 documentation.

The current implementation is wrong, since it takes an <amount>
only by 31-30 bits and if user write extend with zero amount,
it will write <size> but must be #0.

Arm64 documentation says:
Is the index shift amount optional only when <extend> is not LSL.
Where it is permitted to be optional, it defaults to #0. It is encoded
in S.

So according to the documentation we have to take into account <S>
and if <amount> is zero, we will assign 0 otherwise calculate amount
from op(31:30).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/arm64/arm64/disassem.c
506–513

or we can use liket this since amount is already initialized

if (scale != 0)
        /* Calculate amount, it's op(31:30) */
        amount = (insn >> ARM_INSN_SIZE_OFFSET) &
	         ARM_INSN_SIZE_MASK;
This revision is now accepted and ready to land.Apr 14 2023, 5:59 PM

Hi, sorry for the delay. I was looking at the change again before merging, and now it doesn't seem quite right to me.

https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDR--register---Load-Register--register--?lang=en#sa_amount_1

It seems like in this code we will only ever set amount to 1 or 0. But in the link above it seems like the only legal values are: #0, #2, and #3, depending on the <size> parameter of the instruction.

Should it be something like this?

if (scale == 0)
    amount = 0;
else {
    if (((insn >> ARM_INSN_SIZE_OFFSET) & ARM_INSN_SIZE_MASK) != 0)
        /* <size> == 64-bit */
        amount = 3;
    else
        /* <size> == 32-bit */
        amount = 2;
}

Hi, sorry for the delay. I was looking at the change again before merging, and now it doesn't seem quite right to me.

https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDR--register---Load-Register--register--?lang=en#sa_amount_1

It seems like in this code we will only ever set amount to 1 or 0. But in the link above it seems like the only legal values are: #0, #2, and #3, depending on the <size> parameter of the instruction.

Should it be something like this?

if (scale == 0)
    amount = 0;
else {
    if (((insn >> ARM_INSN_SIZE_OFFSET) & ARM_INSN_SIZE_MASK) != 0)
        /* <size> == 64-bit */
        amount = 3;
    else
        /* <size> == 32-bit */
        amount = 2;
}

No, we extract bit range from 2 bits (31 and 30 bit), so amount will not only 1 or 0 in current implementation.

ARM_INSN_SIZE_MASK is 0b11
ARM_INSN_SIZE_OFFSET is 30

yes, if we look at ldr instruction there is <size>:
32-bit (size == 10)
64-bit (size == 11)

We can have #2 for 32 bit and #3 for 64 bit or #0 if scale is 0.

However, cases like strb and ldrb have always #0, that's why we use <size>:

https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDRB--immediate---Load-Register-Byte--immediate--

for example, we can write <extend> with <amount> - #0 or just <extend>:

strb    w0, [x1, x2, sxtx #0]
strb    w0, [x1, w2, uxtw]

if you write something else, you will get expected compiler error message:

error: expected 'lsl' or 'sxtx' with optional shift of #0
 strb w0, [x1, x2, sxtx #1]

cases like strh or ldrh can be #1 or #0:
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/STRH--register---Store-Register-Halfword--register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDRH--register---Load-Register-Halfword--register--
https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDRSH--register---Load-Register-Signed-Halfword--register--

strh    w0, [x1, w2, uxtw #1]
strh    w0, [x1, x2, lsl #1]
strh    w0, [sp, w2, sxtw]
strh    w0, [sp, x2, sxtx #0]

This change was also tested in the branch https://github.com/freebsd/freebsd-src/compare/main...toor1245:freebsd-src:arm64-disasm-str-test