Skip to content
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 TriCore Architecture #1973

Merged
merged 163 commits into from
May 3, 2023
Merged

Add TriCore Architecture #1973

merged 163 commits into from
May 3, 2023

Conversation

imbillow
Copy link
Contributor

@imbillow imbillow commented Mar 10, 2023

Changes based on https://github.com/TriDis/capstone/tree/tricore, adaptations for new capstone versions and support for current versions of the TriCore instruction set.

The current tc1.6.2 version of instruction set disassembly is almost complete. Other versions actually overlap with tc1.6.2 for the most part, with only a small amount of extra content, which is still under construction.

Then it should be mentioned that basically all the .inc is generated using the modified llvm-tblgen from https://github.com/Rot127/llvm-capstone/tree/auto-sync-tblgen, except for TriCoreGenDisassemblerTables.inc, which modified a small part of DecodeInstruction and DecodeToMCInst

TODO:

  • tc1.1
  • tc1.2
  • tc1.3
  • tc1.3.1
  • tc1.6
  • tc1.6.1
  • tc1.6.2
  • tc1.1 tests
  • tc1.6.2 basic tests
  • others tests

@aquynh
Copy link
Collaborator

aquynh commented Mar 10, 2023 via email

@imbillow
Copy link
Contributor Author

Very nice work! From which llvm is this, please?

Maybe https://github.com/TriDis/llvm-tricore

@aquynh
Copy link
Collaborator

aquynh commented Mar 10, 2023 via email

@imbillow
Copy link
Contributor Author

Yes, and I meant to use this as a starting point only. We could probably maintain TriCore's TableGen and TriCoreDisassembler code ourselves.

@aquynh
Copy link
Collaborator

aquynh commented Mar 10, 2023 via email

@imbillow
Copy link
Contributor Author

I think it's just not as widely used.

@imbillow
Copy link
Contributor Author

ping @XVilka

@XVilka
Copy link
Contributor

XVilka commented Mar 11, 2023

@imbillow @imbillow this arch is indeed not widely used but quite common in the automotive and few other industries. This architecture is still being improved with ISA changes slightly from version to version, latest one being 1.6.2:

The aforementioned LLVM fork isn't complete and abandoned, and Infineon didn't indicate that they plan to implement Tricore support in the mainline LLVM or even some open fork. Thus, I suggest we focus only on the capstone and maintain it separately from the LLVM things.

@XVilka
Copy link
Contributor

XVilka commented Mar 11, 2023

@imbillow note, there are slight differences for some instructions between different ISA versions, we should add the way to specify the particular ISA in the API.

@XVilka
Copy link
Contributor

XVilka commented Mar 13, 2023

@Rot127 could you please also take a look, in case it will interfere with the auto-sync work?

@Rot127
Copy link
Collaborator

Rot127 commented Mar 13, 2023

@XVilka Looks good to me.
Only the architectures which are generated with auto-sync will be adjusted to each other. Since you mentioned that Tricore will not be in the LLVM mainline (and is therefore not a candidate for auto-sync) there's no need to adjust it to the auto-sync design.

@imbillow imbillow force-pushed the tricore branch 2 times, most recently from e4c79bf to 40a81fe Compare March 24, 2023 12:17
@imbillow imbillow marked this pull request as ready for review March 25, 2023 00:26
cs.c Outdated Show resolved Hide resolved
@kabeor
Copy link
Member

kabeor commented Mar 25, 2023

plz check CIFuzz.

Test MC/RISCV/insn-riscv64.s.cs
Test MC/TriCore/ADC_Background_Scan_1_KIT_TC275_LK.s.cs
fail CS_ARCH_TRICORE CS_MODE_TRICORE
Traceback (most recent call last):
  File "./test_corpus.py", line 134, in <module>
    test_file(fname.strip())
  File "./test_corpus.py", line 125, in test_file
    fout.write(unichr(mc_modes[(arch, mode)]))
KeyError: ('CS_ARCH_TRICORE', 'CS_MODE_TRICORE')
2023-03-25 03:08:35,059 - root - ERROR - Building fuzzers failed.
2023-03-25 03:08:35,060 - root - ERROR - Error building fuzzers for (commit: b01e5c9b2261bfad775d21b7c4b62fb5a59e229f, pr_ref: refs/pull/1973/merge).

@Rot127
Copy link
Collaborator

Rot127 commented Mar 25, 2023

Could you please check my messages in Rizins Mattermost? Depending what you say regarding the auto-sync compatibility, I'd like to review this as well.

@imbillow imbillow marked this pull request as draft March 26, 2023 01:33
cstool/cstool.c Outdated Show resolved Hide resolved
docs/TricoreInstructions.ods Outdated Show resolved Hide resolved
@imbillow
Copy link
Contributor Author

There should be special handling for different versions of ISA that hasn't been done yet. Currently, only TC1.6.2 is being referenced. Also, the testing may not be comprehensive enough, and I feel that there are still quite a few bugs present.

@XVilka
Copy link
Contributor

XVilka commented Apr 6, 2023

@imbillow could you please also maintain a very brief TODO/status in the PR description?

@imbillow
Copy link
Contributor Author

imbillow commented Apr 6, 2023

@imbillow could you please also maintain a very brief TODO/status in the PR description?

done

@imbillow imbillow marked this pull request as ready for review April 9, 2023 19:21
@imbillow
Copy link
Contributor Author

imbillow commented Apr 9, 2023

Now our disassembled mnemonic and op_str are pretty good, but inst_detail is not quite as good

@XVilka
Copy link
Contributor

XVilka commented Apr 10, 2023

@kabeor could you please take a look at this to give the early feedback, since majority of the code was written, also to allow the CI jobs to run?

@XVilka
Copy link
Contributor

XVilka commented Apr 10, 2023

@imbillow see also:

Traceback (most recent call last):
  File "./test_corpus.py", line 149, in <module>
    test_file(fname.strip())
  File "./test_corpus.py", line 64, in test_file
    "CS_MODE_TRICORE_110": CS_MODE_TRICORE_110,
NameError: global name 'CS_MODE_TRICORE_110' is not defined
2023-04-10 14:22:54,188 - root - ERROR - Building fuzzers failed.

@imbillow
Copy link
Contributor Author

Because I'm not very familiar with Python 2, so I converted the test_corpus.py file to Python 3 syntax, resulting in the new test_corpus3.py. Their functionalities should be essentially the same.

However, the cifuzz GitHub action relies on google/oss-fuzz to complete the fuzz test, so it won't use test_corpus3.py. But I think it should still be useful, so I submitted it anyway.

@XVilka
Copy link
Contributor

XVilka commented Apr 11, 2023

@kabeor what do you think about such script conversion?

Copy link
Contributor

@XVilka XVilka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @aquynh @kabeor all your feedback was addressed. Your call now.

@Rot127 Rot127 mentioned this pull request Apr 26, 2023
5 tasks
include/capstone/tricore.h Outdated Show resolved Hide resolved
@kabeor
Copy link
Member

kabeor commented Apr 27, 2023

Then we can merge

bindings/python/capstone/tricore.py Show resolved Hide resolved
bindings/python/capstone/tricore_const.py Outdated Show resolved Hide resolved
suite/test_corpus3.py Outdated Show resolved Hide resolved
imbillow and others added 3 commits May 1, 2023 22:52
Co-authored-by: Rot127 <45763064+Rot127@users.noreply.github.com>
@imbillow
Copy link
Contributor Author

imbillow commented May 1, 2023

@kabeor Hi, the problem you mentioned has been fixed, please review again.

@Rot127
Copy link
Collaborator

Rot127 commented May 2, 2023

@kabeor @aquynh Is it ready to be merged?

@kabeor
Copy link
Member

kabeor commented May 2, 2023

@imbillow @Rot127 It's really great, can you solve the conflict so I can merge?

@XVilka
Copy link
Contributor

XVilka commented May 3, 2023

@kabeor looks like conflict was resolved 😁

@kabeor
Copy link
Member

kabeor commented May 3, 2023

Thanks again for this great work! Merged.

@kabeor kabeor merged commit 98f1155 into capstone-engine:next May 3, 2023
@XVilka
Copy link
Contributor

XVilka commented May 4, 2023

@aquynh @kabeor what do you think about tagging 5.0-rc3 with that (and release 5.0 once it's stable enough)? I wonder if it make sense to release 5.0 before auto-sync is merged, and focus 5.1 on auto-sync work?
cc @imbillow @Rot127

Maybe addressing these two first though: #2010 and #2009

@aquynh
Copy link
Collaborator

aquynh commented May 4, 2023

this is what i am planning to do.

anything else do you think should be merged before rc3 tag?

@aquynh
Copy link
Collaborator

aquynh commented May 4, 2023

@aquynh @kabeor what do you think about tagging 5.0-rc3 with that (and release 5.0 once it's stable enough)? I wonder if it make sense to release 5.0 before auto-sync is merged, and focus 5.1 on auto-sync work? cc @imbillow @Rot127

Maybe addressing these two first though: #2010 and #2009

yes, please make pull req for the above issues before rc3 tag.

my main concern now is that the Pypi package is still properly generated for all platforms.

@Rot127
Copy link
Collaborator

Rot127 commented May 4, 2023

I wonder if it make sense to release 5.0 before auto-sync is merged, and focus 5.1 on auto-sync work?

@aquynh Would agree with releasing v5.0 before auto-sync and v5.1 for updated ARM, PPC and whatever else is done at this point.

@aquynh
Copy link
Collaborator

aquynh commented May 4, 2023

I wonder if it make sense to release 5.0 before auto-sync is merged, and focus 5.1 on auto-sync work?

@aquynh Would agree with releasing v5.0 before auto-sync and v5.1 for updated ARM, PPC and whatever else is done at this point.

yes the auto-sync is a big change, so I want to release 5.0 before merging that.

@@ -708,6 +726,8 @@ def __gen_detail(self):
(self.operands) = bpf.get_arch_info(self._raw.detail.contents.arch.bpf)
elif arch == CS_ARCH_RISCV:
(self.operands) = riscv.get_arch_info(self._raw.detail.contents.arch.riscv)
elif arch == CS_ARCH_TRICORE:
(self.operands) = riscv.get_arch_info(self._raw.detail.contents.arch.tricore)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be tricore.get_arch_info instead of riscv.get_arch_info?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peace-maker Could you please send a new PR to fix these errors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants