sail-riscv icon indicating copy to clipboard operation
sail-riscv copied to clipboard

Published 20 hours ago • verified publisher icon riscv

fix disassembly problems

Open KotorinMinami opened this issue 10 months ago • 4 comments

Fix Issue #21 by adding function to deal with the sign number

KotorinMinami avatar Apr 23 '24 02:04 KotorinMinami

Test Results

712 tests  ±0   712 :white_check_mark: ±0   0s :stopwatch: ±0s   6 suites ±0     0 :zzz: ±0    1 files   ±0     0 :x: ±0 

Results for commit f6e1f25b. ± Comparison against base commit e1242d85.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar May 08 '24 20:05 github-actions[bot]

Reopening the PR: Due to certain Git-related actions, I had to close the previous PR and reopen it. @Timmmm I'm glad you can review it and provide feedback. I also think it might be a good idea to submit it to the upstream repository, "sail," if possible. If you're willing, I would appreciate discussing how to submit the application to the upstream repository, including keeping it in sync with relevant modules in the upstream repository. In fact, from the beginning, I intended to implement this function by writing a .ml file. However, upon realizing the need to submit it upstream, I decided to engage with you first. Finally, if the closing and reopening of the PR has caused any unnecessary inconvenience for you, I sincerely apologize.

KotorinMinami avatar May 09 '24 20:05 KotorinMinami

No problem at all! This looks ready to merge to me now. @billmcspadden-riscv do you want to do the honours or would I be ok to merge non-risky PRs like this?

@KotorinMinami I think it probably makes sense to add this file to Sail. No need to write any OCaml; you can just add the file next to the existing hex_bits.sail. Maybe extend it to 64 bits to match that file.

Timmmm avatar May 09 '24 20:05 Timmmm

I'd like to let @Alasdair look over this first given it's delving into the depths of forward and backwards mapping match functions

jrtc27 avatar May 09 '24 20:05 jrtc27

@jrtc27 any chance you could take another look at this? To refresh your memory, you pointed out that the disassembly direction of the mapping accepted too-large literals, but it turned out that:

  1. It was incorrect for the unsigned mapping too.
  2. Alasdair has since fixed the issue in the Sail compiler.
  3. The Sail compiler doesn't let you use the disassembler anyway.

Thanks!

Timmmm avatar May 21 '24 13:05 Timmmm

Sure, that's fine. I would like to see this move into the Sail runtime itself though in the long term alongside the unsigned functions, as it's much better equipped to do this efficiently and correctly.

jrtc27 avatar May 21 '24 16:05 jrtc27