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

RzIL v810 support #4096

Merged
merged 5 commits into from
Jan 14, 2024
Merged

RzIL v810 support #4096

merged 5 commits into from
Jan 14, 2024

Conversation

imbillow
Copy link
Contributor

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

...

Test plan

...

Closing issues

...

XVilka

This comment was marked as resolved.

@XVilka XVilka mentioned this pull request Jan 11, 2024
38 tasks
@XVilka
Copy link
Member

XVilka commented Jan 11, 2024

As for tracetesting, I wasn't able to find any good open source V810 emulator. If there is none, we can skip this test. Maybe can craft more convoluted "emulateme" test instead?

UPDATE: There is MAME though: https://github.com/mamedev/mame/tree/master/src/devices/cpu/v810

@XVilka
Copy link
Member

XVilka commented Jan 11, 2024

We also lack any ELF and analysis tests for this architecture. Might be a good idea also to check if DWARF works too.

Comment on lines +454 to +474
"gpr Reserved_8 .32 164 0\n"
"gpr Reserved_9 .32 168 0\n"
"gpr Reserved_10 .32 172 0\n"
"gpr Reserved_11 .32 176 0\n"
"gpr Reserved_12 .32 180 0\n"
"gpr Reserved_13 .32 184 0\n"
"gpr Reserved_14 .32 188 0\n"
"gpr Reserved_15 .32 192 0\n"
"gpr Reserved_16 .32 196 0\n"
"gpr Reserved_17 .32 200 0\n"
"gpr Reserved_18 .32 204 0\n"
"gpr Reserved_19 .32 208 0\n"
"gpr Reserved_20 .32 212 0\n"
"gpr Reserved_21 .32 216 0\n"
"gpr Reserved_22 .32 220 0\n"
"gpr Reserved_23 .32 224 0\n"
Copy link
Member

Choose a reason for hiding this comment

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

why these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since several instructions need to read and write SR registers with index, these reversed registers need to be at the location.

@imbillow

This comment was marked as resolved.

@XVilka

This comment was marked as resolved.

@imbillow

This comment was marked as resolved.

@XVilka

This comment was marked as resolved.

@imbillow

This comment was marked as resolved.

@imbillow
Copy link
Contributor Author

the v810 binary: rizinorg/rizin-testbins#138

@imbillow imbillow requested review from XVilka and wargio January 13, 2024 17:49
Copy link
Member

@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.

The RzIL part looks fine. Could you please extend also ELF and DWARF loaders to load debug information for these ELF files?
I wonder if you can also compile that float_ex1.c with and without DWARF to check the debuginfo information loading and the analysis precision.

test/db/rzil/v810 Outdated Show resolved Hide resolved
@imbillow
Copy link
Contributor Author

The RzIL part looks fine. Could you please extend also ELF and DWARF loaders to load debug information for these ELF files?

I wonder if you can also compile that float_ex1.c with and without DWARF to check the debuginfo information loading and the analysis precision.

Is it going in this PR?

@XVilka
Copy link
Member

XVilka commented Jan 14, 2024

@imbillow ELF architecture detection should be just a couple of lines and can be done in the same PR; see: 0440ab4

The rest of my suggestions are indeed better to be done in a separate PR.

@imbillow
Copy link
Contributor Author

@imbillow ELF architecture detection should be just a couple of lines and can be done in the same PR; see: 0440ab4

The rest of my suggestions are indeed better to be done in a separate PR.

This v810-gcc 4.9.4 looks buggy and can't generate DWARF.

./v810-gcc/bin/v810-gcc -gdwarf-4 -std=c99 emulateme.c -o emulateme.v810
emulateme.c: In function 'decrypt':
emulateme.c:26:1: internal compiler error: in create_cie_data, at dwarf2cfi.c:2880
 }
 ^

emulateme.c:26:1: internal compiler error: Segmentation fault
v810-gcc: internal compiler error: Segmentation fault (program cc1)
fish: Job 1, './v810-gcc/bin/v810-gcc -gdwa...' terminated by signal SIGSEGV (Address boundary error)

@XVilka
Copy link
Member

XVilka commented Jan 14, 2024

@imbillow ELF architecture detection should be just a couple of lines and can be done in the same PR; see: 0440ab4
The rest of my suggestions are indeed better to be done in a separate PR.

This v810-gcc 4.9.4 looks buggy and can't generate DWARF.

./v810-gcc/bin/v810-gcc -gdwarf-4 -std=c99 emulateme.c -o emulateme.v810
emulateme.c: In function 'decrypt':
emulateme.c:26:1: internal compiler error: in create_cie_data, at dwarf2cfi.c:2880
 }
 ^

emulateme.c:26:1: internal compiler error: Segmentation fault
v810-gcc: internal compiler error: Segmentation fault (program cc1)
fish: Job 1, './v810-gcc/bin/v810-gcc -gdwa...' terminated by signal SIGSEGV (Address boundary error)

Ok, ignore my suggestion about DWARF and just add ELF detection and analysis test (functions/variables/etc) in a separate PR as we agreed.

@XVilka XVilka merged commit 0263478 into dev Jan 14, 2024
46 of 47 checks passed
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.

3 participants