-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add operands access support for TriCore #2034
Conversation
779f910
to
a4bca6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.clang-format
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
arch/TriCore/TriCoreMapping.c
Outdated
#include "TriCoreGenCSMappingInsnOp.inc" | ||
}; | ||
|
||
static inline cs_op_type TriCore_get_op_type(MCInst *MI, unsigned OpNum) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw that you already did this partially. Some of them are possibly still useful. Also see the comment below regarding Mapping.c
write_count = detail->regs_write_count; | ||
|
||
// implicit registers | ||
memcpy(regs_read, detail->regs_read, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, was in a rush. Meant it the other way around.
tricore_op_mem mem; // base/disp value for MEM operand | ||
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 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cs_simple_types.h
Outdated
@@ -19,160 +19,162 @@ typedef enum { | |||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these lines formatted with the .clang-format
of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Yes, I'm still reviewing the code, will merge soon. |
@imbillow as Capstone tends to merge without squashing commits, please squash relevant commits semantically into multiple with descriptive commit messages. |
@imbillow it's |
Everything looks great, thanks! |
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 |
@peace-maker Nah, we just forgot about it. This must be fixed before the |
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.
Some additional work has been done:
cs_simple_types.h
,MCInst.{c,h}
,MCInstrDesc.{c,h}
,utils.{c,h}
)CMake
-Wall
-DCMAKE_COMPILE_WARNING_AS_ERROR=ON
.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