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

jit: Fail to successfully execute the JIT assembler #168

Closed
jserv opened this issue Jul 19, 2023 · 11 comments · Fixed by #250
Closed

jit: Fail to successfully execute the JIT assembler #168

jserv opened this issue Jul 19, 2023 · 11 comments · Fixed by #250

Comments

@jserv
Copy link
Contributor

jserv commented Jul 19, 2023

xkon was tested with interpreter. However, JIT compiler fails to execute it successfully.
The way to reproduce:

$ build/rvemu build/jit-bf.elf

Its output:

Execute 5 times with JIT precompile using xkon.

+0x2a = <.L0B>
+0x6e = <.L0E>
+0x118 = <.L1B>
+0x15c = <.L1E>
Address OPcode  ------- Instruction --------------------------------------------
...
??;(f0?Iaddi sp,sp,-32	# c.addi sp,-32
Segmentation fault (core dumped)

@jserv
Copy link
Contributor Author

jserv commented Jul 20, 2023

QEMU (w/ TCG) fails to run jit-bf.elf either.

$ qemu-riscv32 jit-bf.elf
...
Execute 5 times with JIT precompile using xkon.
+0x2c = <.L0B>
+0x70 = <.L0E>
+0x11a = <.L1B>
+0x15e = <.L1E>
Segmentation fault (core dumped)

@qwe661234
Copy link
Collaborator

After applying the PR #175 patch, this testcase sometimes passes and sometimes fails due to an issue with the assert function in the file src/map.c. The same issue also happens in interpreter-only mode.

  • error message
rv32emu: src/map.c:242: void rb_remove(map_t, map_node_t *): Assertion `nodep && nodep->node == node' failed.
Aborted (core dumped)

@jserv
Copy link
Contributor Author

jserv commented Jul 23, 2023

rv32emu: src/map.c:242: void rb_remove(map_t, map_node_t *): Assertion `nodep && nodep->node == node' failed.
Aborted (core dumped)

Can you use rr to debug the recording?

@EagleTw
Copy link
Collaborator

EagleTw commented Aug 23, 2023

@qwe661234 can you provide the reproduction steps?

@qwe661234
Copy link
Collaborator

@qwe661234 can you provide the reproduction steps?

The repoduction way is

./build/rv32emu ./build/jit-bf.elf 

@jserv
Copy link
Contributor Author

jserv commented Aug 24, 2023

The repoduction way is

build/rv32emu build/jit-bf.elf 

This issue is more easily reproducible on x86-64 machines. However, when using the Aarch64 architecture, I haven't encountered this issue on both macOS and GNU/Linux. I am unsure if there is a hidden alignment issue or not.

@EagleTw
Copy link
Collaborator

EagleTw commented Aug 24, 2023

I executed 30 times in Aarch64, and all executed properly.
I cannot reproduce the assertion since I have no x64 machines.

rv32emu: src/map.c:242: void rb_remove(map_t, map_node_t *): Assertion `nodep && nodep->node == node' failed.
Aborted (core dumped)

The assertion failure above means that we cannot find the to-be-removed target node in the map.
If I have the environment, I would set breakpoint in front of the to-be-removed target and use find() to confirm this.

@jserv
Copy link
Contributor Author

jserv commented Aug 24, 2023

I executed 30 times in Aarch64, and all executed properly. I cannot reproduce the assertion since I have no x64 machines.

Alternatively, check the tutorial Running x86_64-based containers on Mac computers with an Apple silicon processor.

@visitorckw
Copy link
Collaborator

I quickly reviewed the relevant code and thanks to @ypaskell 's mention, "The assertion failure above means that we cannot find the to-be-removed target node in the map". It led me to think that perhaps the issue in the rb_remove function, when searching for the node to delete, might be due to using node->data for comparison instead of node->key.

I conducted some simple tests on x86_64 Linux, and it appears that this correction can potentially resolve the problem. However, I haven't thoroughly reviewed other code related to the map. If I haven't misunderstood, I can make code adjustments later after my class and then send a pull request to correct this error.

@EagleTw
Copy link
Collaborator

EagleTw commented Oct 16, 2023

Hi @visitorckw,

Thank you for your careful inspection.
I believe this is the root cause.
Can you test it out and submit a PR?

It's better to also modify the unit test to ensure this behavior is tested.

@visitorckw
Copy link
Collaborator

Thanks for your prompt response.
I'll conduct more testing, adjust the unit tests, and submit the PR later.

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 a pull request may close this issue.

4 participants