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

Xtensa Support #2380

Open
wants to merge 135 commits into
base: next
Choose a base branch
from
Open

Xtensa Support #2380

wants to merge 135 commits into from

Conversation

imbillow
Copy link
Contributor

@imbillow imbillow commented Jun 4, 2024

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

...

Test plan

...

Closing issues

...

@@ -12,7 +12,7 @@ class GetOperandRegImm(Patch):
Patch OPERAND.getReg()
to MCOperand_getReg(OPERAND)
Same for isImm()
Same for getImm()|getExpr
Copy link
Collaborator

Choose a reason for hiding this comment

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

So far I excluded support for expressions from Capstone. Because they were used for symbols, which is out of scope for Capstone. Is this different in Xtensa?

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Please keep in mind the LITBASE mechanic Xtensa has.
No other disassembler seems to support it. So it would be nice if CS gives at least an option to take care of it.
See: https://youtu.be/QMO2vMBcx7Y?feature=shared&t=808

@XVilka
Copy link
Contributor

XVilka commented Jun 6, 2024

@imbillow what LLVM version you have used as a base for this PR? Mainstream LLVM 18? Or a fork from developers working on the Xtensa patches?

@imbillow
Copy link
Contributor Author

imbillow commented Jun 6, 2024

@imbillow what LLVM version you have used as a base for this PR? Mainstream LLVM 18? Or a fork from developers working on the Xtensa patches?

I'm using the auto-sync branch of llvm-capstone, I think it's llvm-18, I'm not sure.

commit 5943ec6923d64e63b9645aa230cd9b86bd63a51b (HEAD -> auto-sync, origin/auto-sync, origin/HEAD)
Author: Rot127 <unisono@quyllur.org>
Date:   Tue Jun 4 03:29:20 2024 -0500

    Add Alpha and LoongArch to the CI tests.

@Rot127
Copy link
Collaborator

Rot127 commented Jun 6, 2024

It is LLVM 18. Please quickly diff the Target/Xtensa directories against each other. If there are vast differences, we can consider merging them earlier.

suite/cstest/Makefile Outdated Show resolved Hide resolved
@imbillow
Copy link
Contributor Author

Yeah, also I have to make changes to the fuzzer scripts as well with #2384. So don't put too much effort into it. You can also consider to rebase this one on top of #2384. I won't make changes to the module code there any longer. So you shouldn't get conflicts during rebasing it.

I feel tortured every time I look at your huge PR on chrome because it crashes the browser.

@XVilka
Copy link
Contributor

XVilka commented Jul 22, 2024

Yeah, also I have to make changes to the fuzzer scripts as well with #2384. So don't put too much effort into it. You can also consider to rebase this one on top of #2384. I won't make changes to the module code there any longer. So you shouldn't get conflicts during rebasing it.

I feel tortured every time I look at your huge PR on chrome because it crashes the browser.

You can try using tig or something like that. It would certainly not crash.

@imbillow
Copy link
Contributor Author

Currently compiling with the ninja generator on macos gives an error

cc -DVERSION_MAJOR=1 -DVERSION_MINOR=4 -DVERSION_PATCH=1 -DVERSION_DEVEL=0 -MMD -MP -I include  -std=c11 -Wall -Wextra -pedantic -Wconversion -Wwrite-strings -Wcast-align -Wpointer-arith -Winit-self -Wshadow -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wundef -Wvla -Wdeclaration-after-statement -O2 -DNDEBUG -fPIC  -c -o build/release/shared/src/load.o src/load.c
src/load.c:24:10: fatal error: 'yaml.h' file not found
#include <yaml.h>
         ^~~~~~~~
1 error generated.
make: *** [build/release/shared/src/load.o] Error 1
ninja: build stopped: subcommand failed.

libyaml is also installed.

➤ pkg-config --cflags yaml-0.1
-I/opt/homebrew/Cellar/libyaml/0.2.5/include

@Rot127
Copy link
Collaborator

Rot127 commented Jul 23, 2024

I feel tortured every time I look at your huge PR on chrome because it crashes the browser.

Ouh yeah, sorry. There are a few thousand test files in there. Probably should be separated before merging.

Currently compiling with the ninja generator on macos gives an error

Weird. Guess I didn't understand the find_libary documentation properly. Can you try this on please:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 21577ba42..3b376b18b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -887,7 +887,9 @@ endif()
 
 if(CAPSTONE_BUILD_CSTEST)
     include(ExternalProject)
-    find_library(libyaml yaml REQUIRED)
+    find_library(libyaml
+        NAMES libyaml yaml
+        REQUIRED)
     ExternalProject_Add(cmocka_ext
         PREFIX extern
         URL "https://cmocka.org/files/1.1/cmocka-1.1.7.tar.xz"

@imbillow
Copy link
Contributor Author

@Rot127

That would work but there are new problems.

capstone/suite/cstest/src/cstest.c:12:10: fatal error: 'threads.h' file not found
#include <threads.h>
         ^~~~~~~~~~~
1 error generated.
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -945,7 +945,7 @@ if(CAPSTONE_BUILD_CSTEST)
         DOWNLOAD_EXTRACT_TIMESTAMP true
         CONFIGURE_COMMAND ""
         BUILD_BYPRODUCTS ${LIBCYAML_LIB_DIR}/libcyaml.a
-        BUILD_COMMAND make
+        BUILD_COMMAND PKG_CONFIG=pkg-config make
         BUILD_IN_SOURCE true
         INSTALL_COMMAND ""
     )

I'm considering whether it's feasible to use rust for cstest.

@Rot127
Copy link
Collaborator

Rot127 commented Jul 24, 2024

I'm considering whether it's feasible to use rust for cstest.

I really did not want to make the building steps more complex. Especially because there is not much time.
Also it would put another layer of stuff in between.

And if we start using Rust, it would mean we might as well implement proper bindings for it. But the release is set to September.
Also, if we will implement Rust bindings in the future, we have to write a rs-cstest for them as well. So basically duplicating the work.

That would work but there are new problems.

We can also live with global variables, although it hurts.
But cstest is not multi-threaded anyways (and doesn't need to be, it is fast enough this way)

diff --git a/suite/cstest/src/cstest.c b/suite/cstest/src/cstest.c
index a8793fc62..6c25f5db9 100644
--- a/suite/cstest/src/cstest.c
+++ b/suite/cstest/src/cstest.c
@@ -9,15 +9,14 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <threads.h>
 #include <time.h>
 
 // Pointer to the file list table
 // Must be a thread local, because we cannot pass arguments to `nftw`.
 // So the found test files can only be saved, very annoyingly,
 // to a global/thread-local mutable variables.
-thread_local char ***test_files = NULL;
-thread_local uint32_t file_count = 0;
+char ***test_files = NULL;
+uint32_t file_count = 0;
 
 static void help(const char *self)
 {

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

4 participants