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

i#3544 RV64: Added an encoder and some fixes and improvments to the decoder #6095

Merged
merged 36 commits into from
Jun 22, 2023

Conversation

ksco
Copy link
Contributor

@ksco ksco commented May 30, 2023

This patch added the following changes:

  1. Added an encoder
  2. Fixed various disassemble issues
  3. Optimized disassembled immediate format
  4. Added unit tests for the encoder and decoder

Issue: #3544

@ksco ksco requested a review from abhinav92003 May 30, 2023 11:06
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/riscv64/codec.c Show resolved Hide resolved
@ksco
Copy link
Contributor Author

ksco commented Jun 15, 2023

I wrote a simple test case in the above commits to check if things work ok, now I can use ctest -L RISCV64 to run the api.ir test, but the test itself doesn't work (Segfaulted due to uninitialized our_environ variable used by standalone_init).

I noticed that the RISC-V version of api.ir binary doesn't contain the _init symbol, so some initialization routines are not being called before main.

$ # RISC-V
$ riscv64-linux-gnu-objdump -d ./suite/tests/bin/api.ir | grep "<_init>"
$
$ # x86_64
$ objdump -d ./suite/tests/bin/api.ir | grep "<_init>"
0000000000003000 <_init>:
$

I'm guessing it's a CMake mistake, but I'm not familiar with CMake, do you know what's possibly wrong? @abhinav92003 @derekbruening, thanks.

@derekbruening
Copy link
Contributor

I wrote a simple test case in the above commits to check if things work ok, now I can use ctest -L RISCV64 to run the api.ir test, but the test itself doesn't work (Segfaulted due to uninitialized our_environ variable used by standalone_init).

I noticed that the RISC-V version of api.ir binary doesn't contain the _init symbol, so some initialization routines are not being called before main.

$ # RISC-V
$ riscv64-linux-gnu-objdump -d ./suite/tests/bin/api.ir | grep "<_init>"
$
$ # x86_64
$ objdump -d ./suite/tests/bin/api.ir | grep "<_init>"
0000000000003000 <_init>:
$

I'm guessing it's a CMake mistake, but I'm not familiar with CMake, do you know what's possibly wrong? @abhinav92003 @derekbruening, thanks.

If there's no init what is the entry point then? Is there no libc setup code? Missing cross toolchain pieces? I would build a simple C app with a main directly and if it works I would examine its entry point and startup code and then start diffing the build commands between it and api.ir.

@ksco
Copy link
Contributor Author

ksco commented Jun 15, 2023

Thank you! According to what you said, I noticed RISC-V gcc does not generate _init/_fini at all, I don't why, maybe they're too old and have been replaced by init_array/fini_array? Anyway, after the above commit, it seems working.

@ksco
Copy link
Contributor Author

ksco commented Jun 17, 2023

I've added unit tests for the encoder and decoder and also enabled them in CI. Please review again, thanks!

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Thank you for adding tests! Just a few comments/suggestions.

core/ir/disassemble_shared.c Outdated Show resolved Hide resolved
core/ir/opnd_api.h Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
core/ir/opnd_api.h Outdated Show resolved Hide resolved
suite/tests/api/ir_riscv64.c Outdated Show resolved Hide resolved
suite/tests/api/ir_riscv64.c Show resolved Hide resolved
suite/tests/api/ir_riscv64.c Outdated Show resolved Hide resolved
suite/tests/api/ir_riscv64.c Outdated Show resolved Hide resolved
suite/tests/api/ir_riscv64.c Outdated Show resolved Hide resolved
@ksco ksco requested a review from derekbruening June 21, 2023 06:35
Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Thank you for making the requested changes, it looks good.

suite/tests/api/ir_riscv64.c Outdated Show resolved Hide resolved
@ksco ksco enabled auto-merge June 22, 2023 02:15
@ksco ksco dismissed abhinav92003’s stale review June 22, 2023 02:58

Alrealdy reviewed.

@ksco ksco added this pull request to the merge queue Jun 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 22, 2023
@derekbruening derekbruening merged commit 744dd67 into DynamoRIO:master Jun 22, 2023
@ksco ksco deleted the encoder branch June 23, 2023 17:53
@ksco
Copy link
Contributor Author

ksco commented Jul 13, 2023

@derekbruening Hi, I successfully ran a simple hello world on RISC-V, but the size of the patch is relatively large. Is it better to split it into small PRs and submit them separately?

This is probably not the right place to ask this kind of question, please let me know if there are other recommended ways.

@derekbruening
Copy link
Contributor

@derekbruening Hi, I successfully ran a simple hello world on RISC-V, but the size of the patch is relatively large. Is it better to split it into small PRs and submit them separately?

This is probably not the right place to ask this kind of question, please let me know if there are other recommended ways.

Smaller is always better: easier and faster to review; easier to isolate and revert if something goes wrong.

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

Successfully merging this pull request may close these issues.

3 participants