Add operands access support for TriCore by b1llow · Pull Request #2034 · capstone-engine/capstone

b1llow · 2023-05-27T23:18:37Z

Some additional work has been done:

Add operands access supoort for TriCore
Checked in some necessary changes from the https://github.com/Rot127/capstone/tree/auto-sync branch (cs_simple_types.h, MCInst.{c,h}, MCInstrDesc.{c,h}, utils.{c,h})
Fixed some compilation warnings under CMake -Wall -DCMAKE_COMPILE_WARNING_AS_ERROR=ON
Updated the .clang-format file (modified from the Linux Kernel repository)， but this .clang-format configuration is not completely identical to the existing code style.

NOTE: MCInst.writeback should be removed, but I've kept it for compatibility with the current version

cstool -d tc162 8fff8381
 0  8f ff 83 81  xor    %d8, %d15, 0x3f
        ID: 390 (xor.t)
        op_count: 3
                operands[0].type: REG = d8
                        .access: WRITE
                operands[1].type: REG = d15
                        .access: READ
                operands[2].type: IMM = 0x3f
                        .access: READ
        Registers read: d15
        Registers modified: d8

cstool -d tc162 a900c005
 0  a9 00 c0 05  st.da  [%p0+c]0, %p0
        ID: 345 (st.da)
        op_count: 2
                operands[0].type: MEM
                        .mem.base: REG = p0
                        .mem.disp: 0x0
                        .access: WRITE
                operands[1].type: REG = p0
                        .access: READ
        Registers read: p0
        Registers modified: p0

XVilka · 2023-05-28T04:39:03Z

@Rot127 @kabeor please take a look

Rot127

Very nice! Thanks a lot for making the effort to look into the auto-sync PRs!
The many requested changes are mostly because more stuff was already done.

@kabeor What is the progress with version v5?
Because #1949 and #2013 implement and refactor almost all necessary things for auto-sync. Having them merged soon will at least at a proper reference point for the work of others.

Rot127 · 2023-05-28T12:18:08Z

@kabeor Formatting is in general inconsistent in CS because the previous .clang-forma did not work properly. We should consider to format all files before the next release (or the release afterwards) with this.

Rot127 · 2023-05-28T12:46:12Z

+#include "TriCoreGenCSMappingInsnOp.inc"
+};
+
+static inline cs_op_type TriCore_get_op_type(MCInst *MI, unsigned OpNum)


The following commits implement generic functions and macros for accessing the mapping tables, detail operands and check for flags (doing_mem, detail is set):

3cade5a, b8cf7ff, 0352ed9, 0352ed9, 066168d, c13e7c2

Would recommend to pull those in as well.

Just saw that you already did this partially. Some of them are possibly still useful. Also see the comment below regarding Mapping.c

Rot127 · 2023-05-28T13:01:32Z

+	write_count = detail->regs_write_count;
+
+	// implicit registers
+	memcpy(regs_read, detail->regs_read,


Could you add a assert(sizeof(cs_regs) <= sizeof(insn->detail->regs_read)); here. Also below.
Just in case someone plays with the arrays sizes in the future.

But this assertion is not True

/// Type of array to keep the list of registers typedef uint16_t cs_regs[64];

uint16_t regs_read [MAX_IMPL_R_REGS=20]; ///< list of implicit registers read by this insn

Sorry, was in a rush. Meant it the other way around.

Rot127 · 2023-05-28T13:30:44Z

+		unsigned int reg; ///< register value for REG operand
+		int32_t imm; ///< immediate value for IMM operand
+		tricore_op_mem mem; ///< base/disp value for MEM operand
 	};


Took me until here to understand how you fill memory operands :D
Doing it via fill_mem() is fine.

Nonetheless, please set the TRICORE_OP_MEM = CS_OP_MEM and update the operand type of the base register and disponent with TRICORE_OP_MEM (in fill_tricore_imm() via tricore->operands[tricore->op_count].type |= TRICORE_OP_MEM).

If we extend TriCore's td files in the future and define complex memory operands there (memory operands which have the base register and disponent as sub-operands defined), it is easier to modify the code here.

Rot127 · 2023-05-28T21:09:48Z

 	// If you change this numbering, you must change the values in
 	// ValueTypes.td as well!
 	CS_DATA_TYPE_Other = 1, // This is a non-standard value
-	CS_DATA_TYPE_i1 = 2,	// This is a 1 bit integer value


Are these lines formatted with the .clang-format of this PR?

Then please add the following options (if there is no objection from others):

For comment alignment

AlignTrailingComments: Kind: Always OverEmptyLines: 2

For escaped newlines (aligning them breaks for me. I guess because of the tabs?).

AlignEscapedNewlines: DontAlign

XVilka · 2023-05-29T02:26:24Z

Do you think this should go into 5.0 release as well? @imbillow @kabeor

kabeor · 2023-05-29T02:40:02Z

@XVilka Yes, I'm still reviewing the code, will merge soon.

XVilka · 2023-05-30T02:55:10Z

@imbillow as Capstone tends to merge without squashing commits, please squash relevant commits semantically into multiple with descriptive commit messages.

XVilka · 2023-05-30T03:34:59Z

@imbillow it's support not supoort by the way, in both PR title and commit message

kabeor · 2023-05-30T13:17:27Z

Everything looks great, thanks!

peace-maker · 2023-05-30T18:26:39Z

Since the comment doesn't show up here - this broke accessing the instruction details in the python bindings.

I'm not sure if you're aware and that's the normal workflow - ignore if this is just bad timing on my side looking at the code :D

Rot127 · 2023-05-30T18:56:14Z

@peace-maker Nah, we just forgot about it. This must be fixed before the v5 release. If you find time, it would be great if you could open a PR.

capstone-engine#2034 changed the `regs_read` array size and added a new `writeback` element to the cs_detail struct. Those changes weren't reflected in the Python bindings causing details to be missing.

b1llow marked this pull request as ready for review May 27, 2023 23:47

b1llow mentioned this pull request May 27, 2023

Capstone@next TriCore rizinorg/rizin#3463

Merged

5 tasks

b1llow force-pushed the tc-reg-rw branch 2 times, most recently from 779f910 to a4bca6c Compare May 28, 2023 03:54

XVilka approved these changes May 28, 2023

View reviewed changes

Rot127 requested changes May 28, 2023

View reviewed changes

b1llow force-pushed the tc-reg-rw branch from 8c62e58 to db59326 Compare May 28, 2023 20:31

Rot127 reviewed May 28, 2023

View reviewed changes

b1llow force-pushed the tc-reg-rw branch from 305372b to 3905d4d Compare May 30, 2023 02:49

Rot127 and others added 3 commits May 30, 2023 11:08

Pull auto-sync's changes from capstone-engine@2ab11ad

b0e7417

Fix compile warnings under CMake -Wall

f40f097

Add .clang-format and format

9d6c489

b1llow force-pushed the tc-reg-rw branch from 3905d4d to a09cc37 Compare May 30, 2023 03:13

XVilka approved these changes May 30, 2023

View reviewed changes

b1llow changed the title ~~Add operands access supoort for TriCore~~ Add operands access support for TriCore May 30, 2023

Add operands access support for TriCore

6c3a899

b1llow force-pushed the tc-reg-rw branch from a09cc37 to 6c3a899 Compare May 30, 2023 04:28

Rot127 approved these changes May 30, 2023

View reviewed changes

kabeor merged commit 7729902 into capstone-engine:next May 30, 2023

Rot127 mentioned this pull request May 30, 2023

Modernize Capstone testing #1984

Closed

peace-maker mentioned this pull request Jun 5, 2023

Fix Python bindings after changes to cs_detail #2041

Merged

b1llow deleted the tc-reg-rw branch November 20, 2023 03:10

Conversation

b1llow commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XVilka commented May 28, 2023

Uh oh!

Rot127 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rot127 May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XVilka commented May 29, 2023

Uh oh!

kabeor commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XVilka commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XVilka commented May 30, 2023

Uh oh!

kabeor commented May 30, 2023

Uh oh!

peace-maker commented May 30, 2023

Uh oh!

Rot127 commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

b1llow commented May 27, 2023 •

edited

Loading

Rot127 May 29, 2023 •

edited

Loading

kabeor commented May 29, 2023 •

edited

Loading

XVilka commented May 30, 2023 •

edited

Loading