[V4,3/8] opcodes: aarch64: flags to denote subclasses of ldst insns

Message ID 20240701025404.3361349-4-indu.bhagat@oracle.com
State New
Headers
Series Add SCFI support for aarch64 |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Indu Bhagat July 1, 2024, 2:53 a.m. UTC
  [Changes in V4]
 - Specify subclasses only for those iclasses relevent to SCFI:
      ldst_imm9, ldst_pos, ldstpair_indexed, ldstpair_off
[End of changes in V4]

[Changes in V3]
- Use F_LDST_SWAP for lse_atomic ld/st ops.  Use of F_LDST_LOAD or
  F_LDST_STORE was incorrect.
[End of changes in V3]

[New in V2]

The existing iclass information tells us the general shape and purpose
of the instructions.  In some cases, however, we need to further disect
the iclass on the basis of other finer-grain information.  E.g., for the
purpose of SCFI, we need to know whether a given insn with iclass
of ldst_* is a load or a store.

opcodes/
    * aarch64-tbl.h: Use the new F_LDST_* flags.
---
 opcodes/aarch64-tbl.h | 78 +++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 39 deletions(-)
  

Comments

Richard Sandiford July 1, 2024, 6:06 p.m. UTC | #1
Indu Bhagat <indu.bhagat@oracle.com> writes:
> [Changes in V4]
>  - Specify subclasses only for those iclasses relevent to SCFI:
>       ldst_imm9, ldst_pos, ldstpair_indexed, ldstpair_off
> [End of changes in V4]
>
> [Changes in V3]
> - Use F_LDST_SWAP for lse_atomic ld/st ops.  Use of F_LDST_LOAD or
>   F_LDST_STORE was incorrect.
> [End of changes in V3]
>
> [New in V2]
>
> The existing iclass information tells us the general shape and purpose
> of the instructions.  In some cases, however, we need to further disect
> the iclass on the basis of other finer-grain information.  E.g., for the
> purpose of SCFI, we need to know whether a given insn with iclass
> of ldst_* is a load or a store.
>
> opcodes/
>     * aarch64-tbl.h: Use the new F_LDST_* flags.
> ---
>  opcodes/aarch64-tbl.h | 78 +++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
> index 7ef9cea9119..6e523db6277 100644
> --- a/opcodes/aarch64-tbl.h
> +++ b/opcodes/aarch64-tbl.h
> @@ -4123,39 +4123,39 @@ const struct aarch64_opcode aarch64_opcode_table[] =
>    __FP_INSN ("fcsel", 0x1e200c00, 0xff200c00, floatsel, 0, OP4 (Fd, Fn, Fm, COND), QL_FP_COND, F_FPTYPE),
>    FF16_INSN ("fcsel", 0x1ee00c00, 0xff200c00, floatsel, OP4 (Fd, Fn, Fm, COND), QL_FP_COND_H, F_FPTYPE),
>    /* Load/store register (immediate indexed).  */
> -  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, 0),
> -  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, 0),
> -  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDS_SIZE),
> -  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, 0),
> -  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, 0),
> -  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, 0),
> -  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, 0),
> -  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDS_SIZE),
> -  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
> -  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
> -  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, 0),
> +  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, F_LDST_STORE),
> +  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, F_LDST_LOAD),
> +  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
> +  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, F_LDST_STORE),
> +  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, F_LDST_LOAD),
> +  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, F_LDST_STORE),
> +  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, F_LDST_LOAD),
> +  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
> +  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_LDST_STORE | F_GPRSIZE_IN_Q),
> +  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_LDST_LOAD | F_GPRSIZE_IN_Q),
> +  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, F_LDST_LOAD),
>    /* Load/store Allocation Tag instructions.  */
>    MEMTAG_INSN ("stg",  0xd9200800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>    MEMTAG_INSN ("stzg", 0xd9600800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>    MEMTAG_INSN ("st2g", 0xd9a00800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>    MEMTAG_INSN ("stz2g",0xd9e00800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
> -  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
> -  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
> -  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
> -  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
> +  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
> +  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
> +  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
> +  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),

These store an allocation tag, rather than storing the contents of the
registers to regular memory.  I think they should be put in an "other"
category.

>    /* Load/store register (unsigned immediate).  */
> -  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
> -  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
> -  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDS_SIZE),
> -  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
> -  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
> -  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
> -  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
> -  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDS_SIZE),
> -  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
> -  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
> -  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, 0),
> -  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, 0),
> +  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_STORE),

AFAICT, patch 7/8 doesn't distinguish between byte stores and other sizes
of store, even though this matters for CFI purposes.  I think we should
use an "other" category for anything that isn't a full load or store of
a W register, X register, or FPR.

Patch 7/8 shouldn't treat a W, B, H or S load or store as being enough
for CFI purposes.  The width has to be 8 bytes or more.

Similarly, for LDP & STP, 7/8 should punt on W and S loads and stores,
since they cannot implement a full save & restore for CFI purposes.

> +  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_LOAD),
> +  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
> +  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, F_LDST_STORE),
> +  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, F_LDST_LOAD),
> +  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, F_LDST_STORE),
> +  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, F_LDST_LOAD),
> +  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
> +  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q | F_LDST_STORE),
> +  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q | F_LDST_LOAD),
> +  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, F_LDST_LOAD),
> +  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, F_LDST_LOAD),

I think PRFM should be marked "other" as well.

>    /* Load/store register (register offset).  */
>    CORE_INSN ("strb", 0x38200800, 0xffe00c00, ldst_regoff, 0, OP2 (Rt, ADDR_REGOFF), QL_LDST_W8, 0),
>    CORE_INSN ("ldrb", 0x38600800, 0xffe00c00, ldst_regoff, 0, OP2 (Rt, ADDR_REGOFF), QL_LDST_W8, 0),
> @@ -4238,19 +4238,19 @@ const struct aarch64_opcode aarch64_opcode_table[] =
>    CORE_INSN ("stnp", 0x2c000000, 0x3fc00000, ldstnapair_offs, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>    CORE_INSN ("ldnp", 0x2c400000, 0x3fc00000, ldstnapair_offs, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>    /* Load/store register pair (offset).  */
> -  CORE_INSN ("stp", 0x29000000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
> -  CORE_INSN ("ldp", 0x29400000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
> -  CORE_INSN ("stp", 0x2d000000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
> -  CORE_INSN ("ldp", 0x2d400000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
> -  {"ldpsw", 0x69400000, 0xffc00000, ldstpair_off, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, 0, 0, 0, VERIFIER (ldpsw)},
> -  MEMTAG_INSN ("stgp", 0x69000000, 0xffc00000, ldstpair_off, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, 0),
> +  CORE_INSN ("stp", 0x29000000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_STORE | F_SF),
> +  CORE_INSN ("ldp", 0x29400000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_LOAD | F_SF),
> +  CORE_INSN ("stp", 0x2d000000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_STORE | 0),
> +  CORE_INSN ("ldp", 0x2d400000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_LOAD),
> +  {"ldpsw", 0x69400000, 0xffc00000, ldstpair_off, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, F_LDST_LOAD, 0, 0, VERIFIER (ldpsw)},

The last one is also "other".

> +  MEMTAG_INSN ("stgp", 0x69000000, 0xffc00000, ldstpair_off, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, F_LDST_STORE),
>    /* Load/store register pair (indexed).  */
> -  CORE_INSN ("stp", 0x28800000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
> -  CORE_INSN ("ldp", 0x28c00000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
> -  CORE_INSN ("stp", 0x2c800000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
> -  CORE_INSN ("ldp", 0x2cc00000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
> -  {"ldpsw", 0x68c00000, 0xfec00000, ldstpair_indexed, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, 0, 0, 0, VERIFIER (ldpsw)},
> -  MEMTAG_INSN ("stgp", 0x68800000, 0xfec00000, ldstpair_indexed, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, 0),
> +  CORE_INSN ("stp", 0x28800000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_STORE | F_SF),
> +  CORE_INSN ("ldp", 0x28c00000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_LOAD | F_SF),
> +  CORE_INSN ("stp", 0x2c800000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_STORE),
> +  CORE_INSN ("ldp", 0x2cc00000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_LOAD),
> +  {"ldpsw", 0x68c00000, 0xfec00000, ldstpair_indexed, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, F_LDST_LOAD, 0, 0, VERIFIER (ldpsw)},

Same here.

Thanks,
Richard

> +  MEMTAG_INSN ("stgp", 0x68800000, 0xfec00000, ldstpair_indexed, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, F_LDST_STORE),
>    /* Load register (literal).  */
>    CORE_INSN ("ldr",   0x18000000, 0xbf000000, loadlit, OP_LDR_LIT,   OP2 (Rt, ADDR_PCREL19),    QL_R_PCREL, F_GPRSIZE_IN_Q),
>    CORE_INSN ("ldr",   0x1c000000, 0x3f000000, loadlit, OP_LDRV_LIT,  OP2 (Ft, ADDR_PCREL19),    QL_FP_PCREL, 0),
  
Indu Bhagat July 11, 2024, 5:45 a.m. UTC | #2
On 7/1/24 11:06, Richard Sandiford wrote:
> Indu Bhagat <indu.bhagat@oracle.com> writes:
>> [Changes in V4]
>>   - Specify subclasses only for those iclasses relevent to SCFI:
>>        ldst_imm9, ldst_pos, ldstpair_indexed, ldstpair_off
>> [End of changes in V4]
>>
>> [Changes in V3]
>> - Use F_LDST_SWAP for lse_atomic ld/st ops.  Use of F_LDST_LOAD or
>>    F_LDST_STORE was incorrect.
>> [End of changes in V3]
>>
>> [New in V2]
>>
>> The existing iclass information tells us the general shape and purpose
>> of the instructions.  In some cases, however, we need to further disect
>> the iclass on the basis of other finer-grain information.  E.g., for the
>> purpose of SCFI, we need to know whether a given insn with iclass
>> of ldst_* is a load or a store.
>>
>> opcodes/
>>      * aarch64-tbl.h: Use the new F_LDST_* flags.
>> ---
>>   opcodes/aarch64-tbl.h | 78 +++++++++++++++++++++----------------------
>>   1 file changed, 39 insertions(+), 39 deletions(-)
>>
>> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
>> index 7ef9cea9119..6e523db6277 100644
>> --- a/opcodes/aarch64-tbl.h
>> +++ b/opcodes/aarch64-tbl.h
>> @@ -4123,39 +4123,39 @@ const struct aarch64_opcode aarch64_opcode_table[] =
>>     __FP_INSN ("fcsel", 0x1e200c00, 0xff200c00, floatsel, 0, OP4 (Fd, Fn, Fm, COND), QL_FP_COND, F_FPTYPE),
>>     FF16_INSN ("fcsel", 0x1ee00c00, 0xff200c00, floatsel, OP4 (Fd, Fn, Fm, COND), QL_FP_COND_H, F_FPTYPE),
>>     /* Load/store register (immediate indexed).  */
>> -  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, 0),
>> -  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, 0),
>> -  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDS_SIZE),
>> -  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, 0),
>> -  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, 0),
>> -  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, 0),
>> -  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, 0),
>> -  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDS_SIZE),
>> -  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>> -  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>> -  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, 0),
>> +  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, F_LDST_STORE),
>> +  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, F_LDST_LOAD),
>> +  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
>> +  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, F_LDST_STORE),
>> +  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, F_LDST_LOAD),
>> +  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, F_LDST_STORE),
>> +  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, F_LDST_LOAD),
>> +  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
>> +  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_LDST_STORE | F_GPRSIZE_IN_Q),
>> +  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_LDST_LOAD | F_GPRSIZE_IN_Q),
>> +  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, F_LDST_LOAD),
>>     /* Load/store Allocation Tag instructions.  */
>>     MEMTAG_INSN ("stg",  0xd9200800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>     MEMTAG_INSN ("stzg", 0xd9600800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>     MEMTAG_INSN ("st2g", 0xd9a00800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>     MEMTAG_INSN ("stz2g",0xd9e00800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>> -  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>> -  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>> -  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>> -  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>> +  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>> +  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>> +  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>> +  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
> 
> These store an allocation tag, rather than storing the contents of the
> registers to regular memory.  I think they should be put in an "other"
> category.
> 

I have changed these to F_SUBCLASS_OTHER. And on the ginsn generation 
side, no memory ginsn op is generated (address calculation op is 
generated if writeback is in effect as usual).  Also added code comments 
  in tc-aarch-ginsn.c to clarify that skipping memory ops is needed 
because the memory op affects the tag only.

>>     /* Load/store register (unsigned immediate).  */
>> -  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>> -  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>> -  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDS_SIZE),
>> -  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>> -  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>> -  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>> -  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>> -  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDS_SIZE),
>> -  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>> -  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>> -  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, 0),
>> -  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, 0),
>> +  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_STORE),
> 
> AFAICT, patch 7/8 doesn't distinguish between byte stores and other sizes
> of store, even though this matters for CFI purposes.  I think we should
> use an "other" category for anything that isn't a full load or store of
> a W register, X register, or FPR.
> 
> Patch 7/8 shouldn't treat a W, B, H or S load or store as being enough
> for CFI purposes.  The width has to be 8 bytes or more.
> 
> Similarly, for LDP & STP, 7/8 should punt on W and S loads and stores,
> since they cannot implement a full save & restore for CFI purposes.
> 

Currently, the checks in dw2gencfi.c issue a warning if CFI offsets are 
not a multiple of 8.  So if user says --scfi=experimental for
     stp     s8, s9, [sp, 96]
they get an error:
     Error: register save offset not a multiple of 8

I was sort of relying on the above for some cases, while leaving the 
users on their own for other cases. E.g, No error is issued if the user 
does a save/restore of S register like so:
     str    s8, [sp, 96]

Hmm, I could punt on W and S by not creating ginsn mem ops for them 
altogether.  Should the user be warned though ? How about something like :
    Warning: ignored probable save/restore op of less than 8-byte

As this will be diagnosed by the ginsn creation code, the warning will 
be issued when a load / store:
   - of callee-saved register,
   - of width < 8 bytes,
   - and with addr_reg involving REG_SP or REG_FP is seen

To avoid the high number of warnings, we could warn once (for the first 
sub 8-byte applicable load/store). Do you think the warning is useful ?

>> +  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_LOAD),
>> +  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
>> +  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, F_LDST_STORE),
>> +  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, F_LDST_LOAD),
>> +  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, F_LDST_STORE),
>> +  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, F_LDST_LOAD),
>> +  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
>> +  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q | F_LDST_STORE),
>> +  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q | F_LDST_LOAD),
>> +  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, F_LDST_LOAD),
>> +  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, F_LDST_LOAD),
> 
> I think PRFM should be marked "other" as well.
> 

Done.

>>     /* Load/store register (register offset).  */
>>     CORE_INSN ("strb", 0x38200800, 0xffe00c00, ldst_regoff, 0, OP2 (Rt, ADDR_REGOFF), QL_LDST_W8, 0),
>>     CORE_INSN ("ldrb", 0x38600800, 0xffe00c00, ldst_regoff, 0, OP2 (Rt, ADDR_REGOFF), QL_LDST_W8, 0),
>> @@ -4238,19 +4238,19 @@ const struct aarch64_opcode aarch64_opcode_table[] =
>>     CORE_INSN ("stnp", 0x2c000000, 0x3fc00000, ldstnapair_offs, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>>     CORE_INSN ("ldnp", 0x2c400000, 0x3fc00000, ldstnapair_offs, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>>     /* Load/store register pair (offset).  */
>> -  CORE_INSN ("stp", 0x29000000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
>> -  CORE_INSN ("ldp", 0x29400000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
>> -  CORE_INSN ("stp", 0x2d000000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>> -  CORE_INSN ("ldp", 0x2d400000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>> -  {"ldpsw", 0x69400000, 0xffc00000, ldstpair_off, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, 0, 0, 0, VERIFIER (ldpsw)},
>> -  MEMTAG_INSN ("stgp", 0x69000000, 0xffc00000, ldstpair_off, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, 0),
>> +  CORE_INSN ("stp", 0x29000000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_STORE | F_SF),
>> +  CORE_INSN ("ldp", 0x29400000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_LOAD | F_SF),
>> +  CORE_INSN ("stp", 0x2d000000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_STORE | 0),
>> +  CORE_INSN ("ldp", 0x2d400000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_LOAD),
>> +  {"ldpsw", 0x69400000, 0xffc00000, ldstpair_off, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, F_LDST_LOAD, 0, 0, VERIFIER (ldpsw)},
> 
> The last one is also "other".
> 
>> +  MEMTAG_INSN ("stgp", 0x69000000, 0xffc00000, ldstpair_off, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, F_LDST_STORE),
>>     /* Load/store register pair (indexed).  */
>> -  CORE_INSN ("stp", 0x28800000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
>> -  CORE_INSN ("ldp", 0x28c00000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
>> -  CORE_INSN ("stp", 0x2c800000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>> -  CORE_INSN ("ldp", 0x2cc00000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
>> -  {"ldpsw", 0x68c00000, 0xfec00000, ldstpair_indexed, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, 0, 0, 0, VERIFIER (ldpsw)},
>> -  MEMTAG_INSN ("stgp", 0x68800000, 0xfec00000, ldstpair_indexed, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, 0),
>> +  CORE_INSN ("stp", 0x28800000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_STORE | F_SF),
>> +  CORE_INSN ("ldp", 0x28c00000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_LOAD | F_SF),
>> +  CORE_INSN ("stp", 0x2c800000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_STORE),
>> +  CORE_INSN ("ldp", 0x2cc00000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_LOAD),
>> +  {"ldpsw", 0x68c00000, 0xfec00000, ldstpair_indexed, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, F_LDST_LOAD, 0, 0, VERIFIER (ldpsw)},
> 
> Same here.
> 
> Thanks,
> Richard
> 
>> +  MEMTAG_INSN ("stgp", 0x68800000, 0xfec00000, ldstpair_indexed, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, F_LDST_STORE),
>>     /* Load register (literal).  */
>>     CORE_INSN ("ldr",   0x18000000, 0xbf000000, loadlit, OP_LDR_LIT,   OP2 (Rt, ADDR_PCREL19),    QL_R_PCREL, F_GPRSIZE_IN_Q),
>>     CORE_INSN ("ldr",   0x1c000000, 0x3f000000, loadlit, OP_LDRV_LIT,  OP2 (Ft, ADDR_PCREL19),    QL_FP_PCREL, 0),
  
Indu Bhagat July 12, 2024, 1:59 p.m. UTC | #3
On 7/10/24 22:45, Indu Bhagat wrote:
> On 7/1/24 11:06, Richard Sandiford wrote:
>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>> [Changes in V4]
>>>   - Specify subclasses only for those iclasses relevent to SCFI:
>>>        ldst_imm9, ldst_pos, ldstpair_indexed, ldstpair_off
>>> [End of changes in V4]
>>>
>>> [Changes in V3]
>>> - Use F_LDST_SWAP for lse_atomic ld/st ops.  Use of F_LDST_LOAD or
>>>    F_LDST_STORE was incorrect.
>>> [End of changes in V3]
>>>
>>> [New in V2]
>>>
>>> The existing iclass information tells us the general shape and purpose
>>> of the instructions.  In some cases, however, we need to further disect
>>> the iclass on the basis of other finer-grain information.  E.g., for the
>>> purpose of SCFI, we need to know whether a given insn with iclass
>>> of ldst_* is a load or a store.
>>>
>>> opcodes/
>>>      * aarch64-tbl.h: Use the new F_LDST_* flags.
>>> ---
>>>   opcodes/aarch64-tbl.h | 78 +++++++++++++++++++++----------------------
>>>   1 file changed, 39 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
>>> index 7ef9cea9119..6e523db6277 100644
>>> --- a/opcodes/aarch64-tbl.h
>>> +++ b/opcodes/aarch64-tbl.h
>>> @@ -4123,39 +4123,39 @@ const struct aarch64_opcode 
>>> aarch64_opcode_table[] =
>>>     __FP_INSN ("fcsel", 0x1e200c00, 0xff200c00, floatsel, 0, OP4 (Fd, 
>>> Fn, Fm, COND), QL_FP_COND, F_FPTYPE),
>>>     FF16_INSN ("fcsel", 0x1ee00c00, 0xff200c00, floatsel, OP4 (Fd, 
>>> Fn, Fm, COND), QL_FP_COND_H, F_FPTYPE),
>>>     /* Load/store register (immediate indexed).  */
>>> -  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W8, 0),
>>> -  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W8, 0),
>>> -  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R8, F_LDS_SIZE),
>>> -  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>> ADDR_SIMM9), QL_LDST_FP, 0),
>>> -  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>> ADDR_SIMM9), QL_LDST_FP, 0),
>>> -  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W16, 0),
>>> -  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W16, 0),
>>> -  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R16, F_LDS_SIZE),
>>> -  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>>> -  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>>> -  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_X32, 0),
>>> +  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W8, F_LDST_STORE),
>>> +  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W8, F_LDST_LOAD),
>>> +  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
>>> +  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>> ADDR_SIMM9), QL_LDST_FP, F_LDST_STORE),
>>> +  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>> ADDR_SIMM9), QL_LDST_FP, F_LDST_LOAD),
>>> +  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W16, F_LDST_STORE),
>>> +  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_W16, F_LDST_LOAD),
>>> +  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
>>> +  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R, F_LDST_STORE | F_GPRSIZE_IN_Q),
>>> +  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_R, F_LDST_LOAD | F_GPRSIZE_IN_Q),
>>> +  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>> ADDR_SIMM9), QL_LDST_X32, F_LDST_LOAD),
>>>     /* Load/store Allocation Tag instructions.  */
>>>     MEMTAG_INSN ("stg",  0xd9200800, 0xffe00c00, ldst_unscaled, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>     MEMTAG_INSN ("stzg", 0xd9600800, 0xffe00c00, ldst_unscaled, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>     MEMTAG_INSN ("st2g", 0xd9a00800, 0xffe00c00, ldst_unscaled, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>     MEMTAG_INSN ("stz2g",0xd9e00800, 0xffe00c00, ldst_unscaled, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>> -  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>> -  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>> -  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>> -  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>> +  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>> +  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>> +  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>> +  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 
>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>
>> These store an allocation tag, rather than storing the contents of the
>> registers to regular memory.  I think they should be put in an "other"
>> category.
>>
> 
> I have changed these to F_SUBCLASS_OTHER. And on the ginsn generation 
> side, no memory ginsn op is generated (address calculation op is 
> generated if writeback is in effect as usual).  Also added code comments 
>   in tc-aarch-ginsn.c to clarify that skipping memory ops is needed 
> because the memory op affects the tag only.
> 
>>>     /* Load/store register (unsigned immediate).  */
>>> -  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>>> -  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>>> -  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, 
>>> OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDS_SIZE),
>>> -  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, 
>>> OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>>> -  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, 
>>> OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>>> -  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>>> -  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>>> -  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, 
>>> OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDS_SIZE),
>>> -  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>>> -  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>>> -  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, 
>>> OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, 0),
>>> -  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, 
>>> OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, 0),
>>> +  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, 
>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_STORE),
>>
>> AFAICT, patch 7/8 doesn't distinguish between byte stores and other sizes
>> of store, even though this matters for CFI purposes.  I think we should
>> use an "other" category for anything that isn't a full load or store of
>> a W register, X register, or FPR.
>>
>> Patch 7/8 shouldn't treat a W, B, H or S load or store as being enough
>> for CFI purposes.  The width has to be 8 bytes or more.
>>
>> Similarly, for LDP & STP, 7/8 should punt on W and S loads and stores,
>> since they cannot implement a full save & restore for CFI purposes.
>>
> 
> Currently, the checks in dw2gencfi.c issue a warning if CFI offsets are 
> not a multiple of 8.  So if user says --scfi=experimental for
>      stp     s8, s9, [sp, 96]
> they get an error:
>      Error: register save offset not a multiple of 8
> 
> I was sort of relying on the above for some cases, while leaving the 
> users on their own for other cases. E.g, No error is issued if the user 
> does a save/restore of S register like so:
>      str    s8, [sp, 96]
> 
> Hmm, I could punt on W and S by not creating ginsn mem ops for them 
> altogether.  Should the user be warned though ? How about something like :
>     Warning: ignored probable save/restore op of less than 8-byte
> 
> As this will be diagnosed by the ginsn creation code, the warning will 
> be issued when a load / store:
>    - of callee-saved register,
>    - of width < 8 bytes,
>    - and with addr_reg involving REG_SP or REG_FP is seen
> 
> To avoid the high number of warnings, we could warn once (for the first 
> sub 8-byte applicable load/store). Do you think the warning is useful ?
> 

I am inclining towards working on this as a follow-up patch (not 
included in V5).  Punting on S and W registers should be easy, but I 
would like to test it out to be sure I am not misssing anything.

Further, I am tending towards not adding a new warning for this in the 
current infrastructure at all (mostly because there will be false 
positives which dilutes the eficacy of the warning).  IIUC, the usage of 
W and S registers to save/restore callee-saved registers is not 
ABI-conformant anyway.

Please let me know your opinion on defering this as a follow-up patch.

Thanks
  
Indu Bhagat July 13, 2024, 7:34 a.m. UTC | #4
On 7/12/24 6:59 AM, Indu Bhagat wrote:
> On 7/10/24 22:45, Indu Bhagat wrote:
>> On 7/1/24 11:06, Richard Sandiford wrote:
>>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>>> [Changes in V4]
>>>>   - Specify subclasses only for those iclasses relevent to SCFI:
>>>>        ldst_imm9, ldst_pos, ldstpair_indexed, ldstpair_off
>>>> [End of changes in V4]
>>>>
>>>> [Changes in V3]
>>>> - Use F_LDST_SWAP for lse_atomic ld/st ops.  Use of F_LDST_LOAD or
>>>>    F_LDST_STORE was incorrect.
>>>> [End of changes in V3]
>>>>
>>>> [New in V2]
>>>>
>>>> The existing iclass information tells us the general shape and purpose
>>>> of the instructions.  In some cases, however, we need to further disect
>>>> the iclass on the basis of other finer-grain information.  E.g., for 
>>>> the
>>>> purpose of SCFI, we need to know whether a given insn with iclass
>>>> of ldst_* is a load or a store.
>>>>
>>>> opcodes/
>>>>      * aarch64-tbl.h: Use the new F_LDST_* flags.
>>>> ---
>>>>   opcodes/aarch64-tbl.h | 78 
>>>> +++++++++++++++++++++----------------------
>>>>   1 file changed, 39 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
>>>> index 7ef9cea9119..6e523db6277 100644
>>>> --- a/opcodes/aarch64-tbl.h
>>>> +++ b/opcodes/aarch64-tbl.h
>>>> @@ -4123,39 +4123,39 @@ const struct aarch64_opcode 
>>>> aarch64_opcode_table[] =
>>>>     __FP_INSN ("fcsel", 0x1e200c00, 0xff200c00, floatsel, 0, OP4 
>>>> (Fd, Fn, Fm, COND), QL_FP_COND, F_FPTYPE),
>>>>     FF16_INSN ("fcsel", 0x1ee00c00, 0xff200c00, floatsel, OP4 (Fd, 
>>>> Fn, Fm, COND), QL_FP_COND_H, F_FPTYPE),
>>>>     /* Load/store register (immediate indexed).  */
>>>> -  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_X32, 0),
>>>> +  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, F_LDST_STORE),
>>>> +  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, F_LDST_LOAD),
>>>> +  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
>>>> +  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, F_LDST_STORE),
>>>> +  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, F_LDST_LOAD),
>>>> +  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, F_LDST_STORE),
>>>> +  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, F_LDST_LOAD),
>>>> +  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
>>>> +  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_LDST_STORE | F_GPRSIZE_IN_Q),
>>>> +  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_LDST_LOAD | F_GPRSIZE_IN_Q),
>>>> +  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_X32, F_LDST_LOAD),
>>>>     /* Load/store Allocation Tag instructions.  */
>>>>     MEMTAG_INSN ("stg",  0xd9200800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>>     MEMTAG_INSN ("stzg", 0xd9600800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>>     MEMTAG_INSN ("st2g", 0xd9a00800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>>     MEMTAG_INSN ("stz2g",0xd9e00800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> +  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>> +  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>> +  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>> +  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>
>>> These store an allocation tag, rather than storing the contents of the
>>> registers to regular memory.  I think they should be put in an "other"
>>> category.
>>>
>>
>> I have changed these to F_SUBCLASS_OTHER. And on the ginsn generation 
>> side, no memory ginsn op is generated (address calculation op is 
>> generated if writeback is in effect as usual).  Also added code 
>> comments   in tc-aarch-ginsn.c to clarify that skipping memory ops is 
>> needed because the memory op affects the tag only.
>>
>>>>     /* Load/store register (unsigned immediate).  */
>>>> -  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, 
>>>> OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, 
>>>> OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, 
>>>> OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, 
>>>> OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, 
>>>> OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, 0),
>>>> -  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, 
>>>> OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, 0),
>>>> +  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_STORE),
>>>
>>> AFAICT, patch 7/8 doesn't distinguish between byte stores and other 
>>> sizes
>>> of store, even though this matters for CFI purposes.  I think we should
>>> use an "other" category for anything that isn't a full load or store of
>>> a W register, X register, or FPR.
>>>
>>> Patch 7/8 shouldn't treat a W, B, H or S load or store as being enough
>>> for CFI purposes.  The width has to be 8 bytes or more.
>>>
>>> Similarly, for LDP & STP, 7/8 should punt on W and S loads and stores,
>>> since they cannot implement a full save & restore for CFI purposes.
>>>
>>
>> Currently, the checks in dw2gencfi.c issue a warning if CFI offsets 
>> are not a multiple of 8.  So if user says --scfi=experimental for
>>      stp     s8, s9, [sp, 96]
>> they get an error:
>>      Error: register save offset not a multiple of 8
>>
>> I was sort of relying on the above for some cases, while leaving the 
>> users on their own for other cases. E.g, No error is issued if the 
>> user does a save/restore of S register like so:
>>      str    s8, [sp, 96]
>>
>> Hmm, I could punt on W and S by not creating ginsn mem ops for them 
>> altogether.  Should the user be warned though ? How about something 
>> like :
>>     Warning: ignored probable save/restore op of less than 8-byte
>>
>> As this will be diagnosed by the ginsn creation code, the warning will 
>> be issued when a load / store:
>>    - of callee-saved register,
>>    - of width < 8 bytes,
>>    - and with addr_reg involving REG_SP or REG_FP is seen
>>
>> To avoid the high number of warnings, we could warn once (for the 
>> first sub 8-byte applicable load/store). Do you think the warning is 
>> useful ?
>>
> 
> I am inclining towards working on this as a follow-up patch (not 
> included in V5).  Punting on S and W registers should be easy, but I 
> would like to test it out to be sure I am not misssing anything.
> 
> Further, I am tending towards not adding a new warning for this in the 
> current infrastructure at all (mostly because there will be false 
> positives which dilutes the eficacy of the warning).  IIUC, the usage of 
> W and S registers to save/restore callee-saved registers is not 
> ABI-conformant anyway.
> 
> Please let me know your opinion on defering this as a follow-up patch.
> 

This is hopefully addressed in V5 that I will be sending soon.

No warning yet, but we will now punt on S and W registers.

Thanks
Indu
  

Patch

diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
index 7ef9cea9119..6e523db6277 100644
--- a/opcodes/aarch64-tbl.h
+++ b/opcodes/aarch64-tbl.h
@@ -4123,39 +4123,39 @@  const struct aarch64_opcode aarch64_opcode_table[] =
   __FP_INSN ("fcsel", 0x1e200c00, 0xff200c00, floatsel, 0, OP4 (Fd, Fn, Fm, COND), QL_FP_COND, F_FPTYPE),
   FF16_INSN ("fcsel", 0x1ee00c00, 0xff200c00, floatsel, OP4 (Fd, Fn, Fm, COND), QL_FP_COND_H, F_FPTYPE),
   /* Load/store register (immediate indexed).  */
-  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, 0),
-  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, 0),
-  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDS_SIZE),
-  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, 0),
-  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, 0),
-  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, 0),
-  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, 0),
-  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDS_SIZE),
-  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
-  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
-  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, 0),
+  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, F_LDST_STORE),
+  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W8, F_LDST_LOAD),
+  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
+  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, F_LDST_STORE),
+  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, ADDR_SIMM9), QL_LDST_FP, F_LDST_LOAD),
+  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, F_LDST_STORE),
+  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_W16, F_LDST_LOAD),
+  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
+  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_LDST_STORE | F_GPRSIZE_IN_Q),
+  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_LDST_LOAD | F_GPRSIZE_IN_Q),
+  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, F_LDST_LOAD),
   /* Load/store Allocation Tag instructions.  */
   MEMTAG_INSN ("stg",  0xd9200800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
   MEMTAG_INSN ("stzg", 0xd9600800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
   MEMTAG_INSN ("st2g", 0xd9a00800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
   MEMTAG_INSN ("stz2g",0xd9e00800, 0xffe00c00, ldst_unscaled, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
-  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
-  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
-  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
-  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
+  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
+  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
+  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
+  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
   /* Load/store register (unsigned immediate).  */
-  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
-  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
-  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDS_SIZE),
-  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
-  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
-  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
-  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
-  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDS_SIZE),
-  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
-  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
-  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, 0),
-  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, 0),
+  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_STORE),
+  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_LOAD),
+  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
+  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, F_LDST_STORE),
+  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, F_LDST_LOAD),
+  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, F_LDST_STORE),
+  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, F_LDST_LOAD),
+  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
+  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q | F_LDST_STORE),
+  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q | F_LDST_LOAD),
+  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, F_LDST_LOAD),
+  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, F_LDST_LOAD),
   /* Load/store register (register offset).  */
   CORE_INSN ("strb", 0x38200800, 0xffe00c00, ldst_regoff, 0, OP2 (Rt, ADDR_REGOFF), QL_LDST_W8, 0),
   CORE_INSN ("ldrb", 0x38600800, 0xffe00c00, ldst_regoff, 0, OP2 (Rt, ADDR_REGOFF), QL_LDST_W8, 0),
@@ -4238,19 +4238,19 @@  const struct aarch64_opcode aarch64_opcode_table[] =
   CORE_INSN ("stnp", 0x2c000000, 0x3fc00000, ldstnapair_offs, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
   CORE_INSN ("ldnp", 0x2c400000, 0x3fc00000, ldstnapair_offs, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
   /* Load/store register pair (offset).  */
-  CORE_INSN ("stp", 0x29000000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
-  CORE_INSN ("ldp", 0x29400000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
-  CORE_INSN ("stp", 0x2d000000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
-  CORE_INSN ("ldp", 0x2d400000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
-  {"ldpsw", 0x69400000, 0xffc00000, ldstpair_off, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, 0, 0, 0, VERIFIER (ldpsw)},
-  MEMTAG_INSN ("stgp", 0x69000000, 0xffc00000, ldstpair_off, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, 0),
+  CORE_INSN ("stp", 0x29000000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_STORE | F_SF),
+  CORE_INSN ("ldp", 0x29400000, 0x7ec00000, ldstpair_off, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_LOAD | F_SF),
+  CORE_INSN ("stp", 0x2d000000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_STORE | 0),
+  CORE_INSN ("ldp", 0x2d400000, 0x3fc00000, ldstpair_off, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_LOAD),
+  {"ldpsw", 0x69400000, 0xffc00000, ldstpair_off, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, F_LDST_LOAD, 0, 0, VERIFIER (ldpsw)},
+  MEMTAG_INSN ("stgp", 0x69000000, 0xffc00000, ldstpair_off, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, F_LDST_STORE),
   /* Load/store register pair (indexed).  */
-  CORE_INSN ("stp", 0x28800000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
-  CORE_INSN ("ldp", 0x28c00000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_SF),
-  CORE_INSN ("stp", 0x2c800000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
-  CORE_INSN ("ldp", 0x2cc00000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, 0),
-  {"ldpsw", 0x68c00000, 0xfec00000, ldstpair_indexed, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, 0, 0, 0, VERIFIER (ldpsw)},
-  MEMTAG_INSN ("stgp", 0x68800000, 0xfec00000, ldstpair_indexed, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, 0),
+  CORE_INSN ("stp", 0x28800000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_STORE | F_SF),
+  CORE_INSN ("ldp", 0x28c00000, 0x7ec00000, ldstpair_indexed, 0, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_R, F_LDST_LOAD | F_SF),
+  CORE_INSN ("stp", 0x2c800000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_STORE),
+  CORE_INSN ("ldp", 0x2cc00000, 0x3ec00000, ldstpair_indexed, 0, OP3 (Ft, Ft2, ADDR_SIMM7), QL_LDST_PAIR_FP, F_LDST_LOAD),
+  {"ldpsw", 0x68c00000, 0xfec00000, ldstpair_indexed, 0, CORE, OP3 (Rt, Rt2, ADDR_SIMM7), QL_LDST_PAIR_X32, F_LDST_LOAD, 0, 0, VERIFIER (ldpsw)},
+  MEMTAG_INSN ("stgp", 0x68800000, 0xfec00000, ldstpair_indexed, OP3 (Rt, Rt2, ADDR_SIMM11), QL_STGP, F_LDST_STORE),
   /* Load register (literal).  */
   CORE_INSN ("ldr",   0x18000000, 0xbf000000, loadlit, OP_LDR_LIT,   OP2 (Rt, ADDR_PCREL19),    QL_R_PCREL, F_GPRSIZE_IN_Q),
   CORE_INSN ("ldr",   0x1c000000, 0x3f000000, loadlit, OP_LDRV_LIT,  OP2 (Ft, ADDR_PCREL19),    QL_FP_PCREL, 0),