[AArch64] SME instructions with indexed operands do not have correct disassembly information #2285
Comments
|
Looking into this a bit further, it seems to originate from the In capstone's LLVM, Which would explain the above issues with I think it also explains the issue with Looking at other SME instructions in |
|
I'm not 100% sure on how to implement a fix for this, but for the regex above; all AArch64 memory operands will be preceeded by Additionally, said regex should check that there is no As far as I can tell, other AArch64 instructions do not seem to be affected, only SME/SME2. |
|
Great, thank you for the update! |
|
@FinnWilkinson I am currently at this. Thanks for spotting the faulty regex. It is fixed now. Regarding the representation of SME operands. Something like: typedef struct {
aarch64_sme_op_type type; ///< AArch64_SME_OP_TILE, AArch64_SME_OP_TILE_VEC
aarch64_reg tile; ///< Matrix tile register
aarch64_reg slice_reg; ///< slice index reg
// ...
} aarch64_op_sme_matrix;
typedef struct {
// Some type field
aarch64_reg pred_reg;
aarch64_reg vec_select;
int32_t index; ///< Potentially also a union of other ops which can be used here.
} aarch64_op_sme_pred;
typedef struct {
// Some type field
union {
aarch64_op_sme_matrix matrix;
aarch64_op_sme_pred pred;
}
} aarch64_sme_op;I am a little hesitant to add more operands. But the SME ops don't really fit in the previous ones. So I'd really appreciate your comment on this, since you seem to work with this AArch64 extension a lot.
Yeah, SME/SME2 is really not that well defined in LLVM, unfortunately. So they need all kind of hacky solutions. |
|
This is now the resulting output: |
|
Hi @Rot127 , sorry for the delayed reply I've only just seen the newest comments. This generallt all looks good to me, but I have some concerns about predicates in typedef struct aarch64_op_index {
aarch64_reg base;
int32_t imm_offset;
} aarch64_op_index;This could also be used by NEON, SVE, and predicate registers and remove the need for Additionally, just to ensure functional correctness, what do the following instructions get disassembled as?
|
The thing is that these indices differ quite a lot (the matrix indices can have immediate ranges. And additionally the terminology differs between the members of the indices structs (
I'd like to have the terminology of the struct fields relatively close to the ISA, if possible Generally though I would follow your advice. I only interact with AArch64 in the Capstone context. And because you seem to use it heavily in practice, I would follow your design in the end.
As long as we don't change it after the
Thanks for the test cases! As it looks like the alias versions are still miss some operands. If you have more test cases, please let me know. Those operands are pretty complex. DetailsWith ALIAS operand set With REAL operand set |
|
I think the current implementation of SME operands (non predicates) works well, so I'd be opposed to changing from this. How about having a new For the test case outputs:
I'll compile some more test cases now and put them here, but I need to formulate the hex first. |
This would be great. Thank you!
Ok, sounds good to me. To summarize:
|
|
Will typedef struct aarch64_op_pred {
aarch64_reg pred;
aarch64_reg vec_select;
int32_t im_offset;
} aarch64_op_pred;? And for ...
operands[0].type: SME
operands[0].sme.type: 2
operands[0].sme.tile: za0.s
operands[0].sme.slice_reg: w12
operands[0].sme.slice_offset: 0
operands[0].sme.is_vertical: false
...seems good to me. |
Ah, well. Yes. Let's just do it as you said. |
Yes, lets! Here are some more complex assembly tests that would be good to validate as working. At the moment this is about as complex as SVE and SME gets (hex may be the wrong way round):
|
|
Change is done. Also fixed, that the predicate regs were not added to the Details
Sorry, late night working. Got confused with my own implementation. The real operand set can always be accessed, either by adding the The reason is: to retrieve both detail sets, an instruction must be disassembled twice.
Fix this later this week. The new test cases Output is below. Note the register lists. I didn't add a new operand type for those. Same for the Details |
I think this looks great now. Thanks!
Understood!
Given you can have strided vector register lists as well as consecutive (although I'm struggling to find an example currently), I think it would be beneficial to print them individually for both clarity and for using Capstone as a tool inside other projects. Rather than a new structure though, could we not print them as seperate operands? The SME2 spec also states that although
I agree with not printing As an aside, should |
|
I've also just noticed that |
|
The Real: Alias: Register lists are now as you suggested. Here the output of all instructions: DetailsI'll add all the instructions from here as tests now ans start fuzzing afterwards. If you still find something, please let me know. Thanks for helping out btw. It is difficult to get everything right in detail if you have to handle 3+ architectures with extensions. It's easy to miss things if one doesn't work with them all the time. |
|
All the above look great, thanks very mcuh for all your hard work on this! I'm looking forward to it being merged into If I spot anything further I'll be sure to let you know / open new issues. |
* Run clang-format * Remove arm.h header from AArch64 files * Update all AArch64 module files to LLVM-18. * Add check if the differs save file is up-to-date with the current files. * Add new generator for MC test trnaslation. * Fix warnings * Update generated AsmWriter files * Remove unused variable * Change MCPhysReg type to int16_t as LLVM 18 dictates. With LLVM 18 the MCPhysReg value's type is changed to int16_t. If we update modules to LLVM 18, they will generate compiler warnings that uint16_t* should not be casted to int16_t*. This makes changing the all tables to int16_t necessary, because the alternative is to duplicate all MCPhysReg related code. Which is even worse. * Assign enum values to raw_struct member * Add printAdrAdrpLabel def * Add header to regression test files. * Write files to build dir and ignore more parsing errors. * Fix parsing of MC test files. * Reset parser after every block * Add write and patch header step. * Add and update MC tests for AArch64 * Fix clang-tidy warnings * Don't warn about padding issues. They break automatically initialized structs we can not change easily. * Fix: Incorrect access of LLVM instruction descriptions. * Initialize DecoderComplete flag * Add more mapping and flag details * Add function to get MCInstDesc from table * Fix incorrect memory operand access types. * Fix test where memory was not written, ut only read. * Attempt to fix Windows build * Fix 2268 The enum values were different and hence lead to different decoding. * Refactor SME operands. - Splits SME operands in Matrix and Predicate operands. - Fixes general problems of incorrect detections with the vector select/index operands of predicate registers. - Simplifies code. * Fix up typo in WRITE * Print actual path to struct fields * Add Registers of SME operands to the reg-read list * Add tests for SME operands. * Use Capstone reg enum for comparison * Fix tests: 'Vector arra...' to 'operands[x].vas' * Add the developer fuzz option. * Fix Python bindings for SME operands * Fix variable shadowing. * Fix clang-tidy warnings * Add missing break. * Fix varg usage * Brackets for case * Handle AArch64_OP_GROUP_AdrAdrpLabel * Fix endian issue with fuzzing start bytes * Move previous sme.pred to it's own operand type. * Fix calculation for imm ranges * Print list member flag * Fix up operand strings for cstest * Do only a shallow clone of the cmocka stable branch * Fix: Don't categorize ZT0 as a SME matrix operand. * Remove unused code. * Add flag to distinguish Vn and Qn registers. * Add all registers to detail struct, even if emitted in the asm text * Fix: Increment op count after each list member is added. * Remove implicit write to NZCV for MSR Imm instructions. * Handle several alias operands. * Add details for zero alias with za0.h * Add SME tile to write list if written * Add write access flags to operands which are zeroed. * Add SME tests of #2285 * Fix tests with latest syntax changes. * Fix segfault if memory operand is only a label without register. * Fix python bindings * Attempt to fix clang-tidy warning for some configurations. * Add missing test file (accidentially blocked by gitignore.) * Print clang-tidy version before linting. * Update differ save file * Formatting * Use clang-tidy-15 as if possible. * Remove search patterns for MC tests, since they need to be reworked anyways. * Enum to upper case change * Add information to read the OSS fuzz result. * Fix special case of SVE2 operands. Apparently ZT0 registers can an index attached, get which is BOUND to it. We have no "index for reg" field. So it is simply saved as an immediate. * Handle LLVM expressions without asserts. * Ensure choices are always saved. * OP_GROUP enums can't be all upper case because they contain type information. * Fix compatibility header patching * Update saved_choices.json * Allow mode == None in test_corpus
Working on the latest
nextbranch, disassembling a range of SME instructions yields incorrect disassembly information, often with additional memory operands being defined.It seems to be an issue with the pattern matching of
[...]. A similar issue occurred when implementing PR #1907 and was fixed in PR #1925. I'm not sure how different the backend is now after the auto-sync update, but perhaps a similar fix can be implemented from this?Listed below are some examples:
Issue - Operand 2
p3.s's index is identified as an additional memory operand.Issue - Operand
p4is mistaken for a memory operand andx10as its register offset. Also, operands[0] should not have an access specifier, and operands[2] should not have a vector indexIssue - Same as above.
The text was updated successfully, but these errors were encountered: