initial implementation of the Sail-generated RISCV disassembler module #2498

moste00 · 2024-10-04T21:34:40Z

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

This PR aims to replace the LLVM-derieved RISCV module with a Sail-derieved RISCV module. The generator tool is being developed here, and for the Sail model of RISCV is here.

Sail is an architecture description language being developed here, it's an imperative language inspired in syntax and semantics by OCaml, with some syntax sugar and innovative features designed specifically for describing computer architectures. See here for a detailed tour and explanation of major features.

The RISCV foundation has adopted the Sail model of RISCV as the "official" definition of the architecture, and therefore it's desirable to generate a C implementation of the any RISCV-related logic from the sail-riscv model, as it will be up-to-date and compliant by construction.

Test plan

The current state of the module doesn't compile, this will be updated as work continues on the module. The initial goal of the work is to be able to invoke cstool and obtain useful results (e.g. the instruction in string form, as a start). Hopefully this goal is not too far.

Closing issues

...

wargio · 2024-10-04T21:36:22Z

arch/RISCV/RISCVModule.c

@@ -2,13 +2,8 @@
 /* RISC-V Backend By Rodrigo Cortes Porto <[email protected]> & 
   Shawn Chang <[email protected]>, HardenedLinux@2018 */

-#ifdef CAPSTONE_HAS_RISCV
+//#ifdef CAPSTONE_HAS_RISCV


This ifdef is required by capstone to slim down the build when you compile only for certain archs.

wargio · 2024-10-04T21:38:19Z

arch/RISCV/riscv_ast.gen.inc

+#include <stdint.h>
+
+#include <stddef.h>
+
+#include <string.h>
+


maybe add at the beginning a multiline comment saying that this SAIL generated from the riscv repository YYYYYY with commit UUUUU

Also the copyright. Will be necessary for the SPDX in the future: #2132

wargio · 2024-10-04T21:43:41Z

arch/RISCV/riscv_decode.gen.inc

+      uint64_t imm_18_13 = (binary_stream & 0x000000007E000000)>>25 ;
+      uint64_t imm_19 = (binary_stream & 0x0000000080000000)>>31 ;
+      tree->ast_node_type = RISCV_JAL ;
+      tree->ast_node.riscv_jal.imm = (imm_19 << 23) | (imm_7_0 << 15) | (imm_8 << 14) | (imm_18_13 << 8) | (imm_12_9 << 4) | (0x0 << 0);


i'm sure the compiler is smart enough to compile out | (0x0 << 0) but this is essentially dead code, maybe add a check in the sail2c code that checks for this pattern and omit the value.

This can also go into a helper function imho. Something like inline void concat_bits(uint8_t *dest, ...);. For which the vargs should be of the form: uint64_t bits, uint64_t offset.

I think the following reads better:

concat_bits((uint8_t*)&tree->ast_node.riscv_jal.imm, imm_19, 23, imm_7_0, 15, imm_8, 14, imm_18_13, 8, imm_12_9, 4);

Also passing bytes as pointer to uint8_t array to support future 192 bit extension.

What are your guys opinions @XVilka @wargio?

not sure if concat_bits is needed since is just an OR. i think as is now is ok.

wargio

Overall is a very good progress, i would suggest tho to maybe split arch/RISCV/riscv_ast2str.gen.inc into multiple files since it too big, maybe split it by RV32 and RV64

@XVilka

XVilka

@thestr4ng3r take a look too, please, when you have time.

XVilka · 2024-10-05T02:03:19Z

arch/RISCV/riscv_ast.gen.inc

+#include <stdint.h>
+
+#include <stddef.h>
+
+#include <string.h>
+


Also the copyright. Will be necessary for the SPDX in the future: #2132

XVilka · 2024-10-05T02:05:08Z

arch/RISCV/riscv_ast.gen.inc

+    uint8_t riscv_zicboz /* bits : 5 */;
+  } ast_node;
+
+} ;


Unnecessary space. Also, I think, enums could be extracted to the top level

XVilka · 2024-10-05T02:05:52Z

arch/RISCV/riscv_decode.gen.inc

+      tree->ast_node.rtype.op = RISCV_SLT;
+      return ;
+  }
+  if ((binary_stream & 0x000000000000007F == 0x33) && ((binary_stream & 0x0000000000007000)>>12 == 0x3) && ((binary_stream & 0x00000000FE000000)>>25 == 0x00)) { 


Please split these too long lines in the generator into two.

Rot127

For the prototype/first implementation the decode is fine like this. But before we can merge it into next we need to optimize two things:

Size

      uint64_t rd = (binary_stream & 0x0000000000000F80)>>7 ;
      uint64_t rs1 = (binary_stream & 0x00000000000F8000)>>15 ;
      uint64_t rs2 = (binary_stream & 0x0000000001F00000)>>20 ;
      tree->ast_node_type = RISCV_RTYPE ;
      tree->ast_node.rtype.rs2 = rs2;
      tree->ast_node.rtype.rs1 = rs1;
      tree->ast_node.rtype.rd = rd;

These specific lines are repeated 10 times in the decoder.
I assume there are other decoding patterns happening just as often. In the final version we should not have any duplicated code in here.

Runtime complexity

I greped for ^ if and found 505 if cases in the decode function. This means for an illegal instructions it does at least 505 comparisons (assuming the compiler doesn't optimize something out). Which is something more than ~O(n * 10) (n = number of bits).
But we should reach in worst case O(n * 1) and O(log(n)) on average before we merge it to next.

The current structure is fine. Also because you have the RzIL task as well. So no worries.

What is important though, is that the decoded details (operand details) are stable. No matter how the architecture of this decoder is. Because on once you finished RzIL we would not want to refactor the whole RzIL work, just because we optimized the Capstone decoder :)

That said, good job! Looks like a lot of work! Well done!

Rot127 · 2024-10-05T04:59:43Z

arch/RISCV/RISCVDetails.c

+    } else if ((first_byte >> 6) & 0x1 == 0x0) {
+        insn->size = 8;
+    } else {
+        return false;


Please fprintf a warning here that instructions >64bit are not supported yet.

Or do it below where you check the result of this function. Either way, just please inform the user about it.

Rot127 · 2024-10-05T05:00:54Z

arch/RISCV/RISCVDetails.h

@@ -0,0 +1,3 @@
+#include "capstone.h"


Suggested change

#include "capstone.h"

#include <capstone/capstone.h>

Rot127 · 2024-10-05T05:01:56Z

arch/RISCV/RISCVDisassembly.h

@@ -0,0 +1,5 @@
+#include "../../include/capstone/capstone.h"


Suggested change

#include "../../include/capstone/capstone.h"

#include <capstone/capstone.h>

Rot127 · 2024-10-05T05:08:05Z

arch/RISCV/riscv_ast.gen.inc

+        RISCV_AMOMAXU
+      } op;
+
+      uint8_t aq /* bits : 1 */;


These /* bits : 1 */ comments, what do they mean?

aq encodes bit 1 of instruction.

aq is one bit wide.

Please make this more clear. E.g. for the first meaning you could replace it with insn_bits[1:1]. And for the second meaning: bit_width : 1.

Guess it means the last, though being more descriptive doesn't hurt. This one has a low priority though.

Rot127 · 2024-10-05T05:23:57Z

arch/RISCV/riscv_decode.gen.inc

+
+    }
+    if (op != 0xFFFFFFFFFFFFFFFF) { 
+        uint64_t rd = (binary_stream & 0x0000000000000F80)>>7 ;


Please implement a helper function like inline uint64_t get_bit_field(uint8_t *bytes, size_t start, size_t n); for those. It can be in utils.h

Mind that the bytes are passed as a pointer! So we can go beyond 64bit in the future. The array size of bytes can be assumed by the requested bits. If start > 64 we know the array should be at >8bytes.

These helper functions are important because they allow bit extraction of >64bit instructions. The currently generated code has a maximum of 64bit supported.

Rot127 · 2024-10-05T05:29:41Z

arch/RISCV/riscv_decode.gen.inc

+      uint64_t imm_18_13 = (binary_stream & 0x000000007E000000)>>25 ;
+      uint64_t imm_19 = (binary_stream & 0x0000000080000000)>>31 ;
+      tree->ast_node_type = RISCV_JAL ;
+      tree->ast_node.riscv_jal.imm = (imm_19 << 23) | (imm_7_0 << 15) | (imm_8 << 14) | (imm_18_13 << 8) | (imm_12_9 << 4) | (0x0 << 0);


This can also go into a helper function imho. Something like inline void concat_bits(uint8_t *dest, ...);. For which the vargs should be of the form: uint64_t bits, uint64_t offset.

I think the following reads better:

concat_bits((uint8_t*)&tree->ast_node.riscv_jal.imm, imm_19, 23, imm_7_0, 15, imm_8, 14, imm_18_13, 8, imm_12_9, 4);

Also passing bytes as pointer to uint8_t array to support future 192 bit extension.

What are your guys opinions @XVilka @wargio?

Rot127 · 2024-10-05T05:32:04Z

arch/RISCV/riscv_decode.gen.inc

+      tree->ast_node.riscv_jal.rd = rd;
+      return ;
+  }
+  if ((binary_stream & 0x000000000000007F == 0x67) && ((binary_stream & 0x0000000000007000)>>12 == 0x0)) { 


The binary_stream & 0x000000000000007F == 0x67 also in a helper please. E.g.
inline bool test_bits64(uint64_t bytes, size_t start, size_t n, uint64_t expected)

XVilka · 2024-10-23T15:31:59Z

@moste00 please update the PR with your latest state of the generated code

wargio · 2024-10-25T02:33:31Z

include/capstone/riscv.h

-
-  	RISCV_INS_ENDING,
-} riscv_insn;
+#include "riscv_insn.gen.inc"


this is not ok, but we can write a script to update this.

Rot127

Very much like what I see! Awesome job!
Especially the changes to riscv_decode.gen.inc :)

Please focus on just making it work. You can ignore my comments for now. They are just there so we don't forget about it.

Because I could only take a shallow look, I'll check again in the next days.

Rot127 · 2024-10-26T12:03:21Z

arch/RISCV/RISCVDetails.h

@@ -1,3 +1,3 @@
-#include "capstone.h"
+#include "../include/capstone/capstone.h"


Suggested change

#include "../include/capstone/capstone.h"

#include <capstone/capstone.h>

Rot127 · 2024-10-26T12:04:06Z

arch/RISCV/RISCVDisassembler.c

+
+    } else if (insn->size == 4) {
+        struct ast instruction;
+        decode(&instruction, code[3] << 24 | code[2] << 16 | code[1] << 8 | code[0]);


For these byte extractions we have readBytes32 and others in utils.c. Please use those.

Rot127 · 2024-10-26T12:09:38Z

arch/RISCV/RISCVDisassembler.c

+        while (*curr != ' ') {
+            mnemonic_len++;
+            curr++;
+        }


Please use strchr for this. You can get the length by subtracting the pointers.

Rot127 · 2024-10-26T12:09:56Z

arch/RISCV/RISCVDisassembler.c

+        while (*curr) {
+            operand_len++;
+            curr++;
+        }


Rot127 · 2024-10-26T12:10:32Z

arch/RISCV/RISCVDisassembler.c

+    } else {
+


Suggested change

} else {

Rot127 · 2024-10-26T12:11:37Z

arch/RISCV/RISCVModule.c

@@ -2,26 +2,38 @@
 /* RISC-V Backend By Rodrigo Cortes Porto <[email protected]> & 


Set your copyright. Please use SPDX header style:

# Copyright © 2022 Rot127 <[email protected]> # SPDX-License-Identifier: BSD-3

…bad output)

…tringfication (printer_t)

XVilka · 2024-11-10T05:00:09Z

arch/RISCV/riscv_helpers_ast2str.h

+DEF_HEX_BITS(31)
+DEF_HEX_BITS(32)
+
+void hex_bits_signed(uint64_t bitvec, char **s, size_t *len, uint8_t bvlen_bits,


Either make these functions inline, or do not put into header file

XVilka · 2024-11-10T05:02:00Z

include/capstone/riscv_insn.gen.inc

@@ -0,0 +1,984 @@
+/*=======================================================================*/
+/*This code was generated by the tool riscv_disasm_from_sail (see
+ * https://github.com/moste00/riscv_disasm_from_sail)*/


Should rename the project, maybe since it's capstone auto-sync, something like capstone-autosync-sail?

Once the repository transferred to rizinorg, also update the URL

cc @wargio

XVilka · 2024-11-10T05:02:23Z

include/capstone/riscv_insn.gen.inc

+/*SPDX-License-Identifier: BSD-3-Clause*/
+/*=======================================================================*/
+
+#ifndef __Riscv_insn_gen_inc__


Please use capital letters for include guards

Also above in the other files please.

XVilka · 2024-11-10T05:02:34Z

include/capstone/riscv_insn.gen.inc

+#ifndef __Riscv_insn_gen_inc__
+#define __Riscv_insn_gen_inc__
+#include <stdint.h>
+


Unnecessary empty line between includes

XVilka · 2024-11-10T05:03:04Z

include/capstone/riscv_insn.gen.inc

+#include <string.h>
+
+enum riscv_insn {
+  //--------------------- RISCV_REV8---------------------


Maybe think about converting these into Doxygen, if it's ever possible?

XVilka · 2024-11-10T05:03:47Z

arch/RISCV/riscv_decode_compressed.gen.inc

+
+void decode_compressed(struct ast *tree, uint64_t binary_stream) {
+  // ---------------------------C_NOP-------------------------------
+  {


Unnecessary additional {} braces

Oh those are necessary, because many of the If conditions and the associated context use variables with the same identifier. This wasn't a problem in the original Sail because every rule match forms a different scope, but it's a problem now in C.

The single easiest solution is adding those braces to start a new scope for each rule. I guess I could also solve the problem by making a new function decode_<instruction_case> for every rule to achieve the same effect, but that would complicate the generator a bit.

XVilka · 2024-11-10T05:04:34Z

arch/RISCV/riscv_helpers_ast2str.h

+  *ps = " , ";                                                                 \
+  *plen = 3
+
+static inline void hex_bits(uint64_t bitvec, char **s, size_t *len,


Add Doxygen comments for these helper functions. Also, maybe move to the capstone utils instead? cc @Rot127

~~Yes, please move it to `utils.c~~

~~Actually, you can just use sprintf(). We depend on libc anyways.~~

Sorry, ignore it. See my comment in the final review message.

Rot127

For all the string operations: It is better to use SStream everywhere. And don't do direct operations on char *. This is what it was implement for anyways. Is better tested and is convenient to use.

Edit: Sorry, pressed the "review" button by accident. Will add some more comments.

Rot127 · 2024-11-10T10:32:59Z

CMakeLists.txt

        arch/RISCV/RISCVDisassembler.h
-        arch/RISCV/RISCVInstPrinter.h
-        arch/RISCV/RISCVMapping.h
+        arch/RISCV/riscv_helpers_ast2str.h


We should rename them before merging to CamelCase

Rot127 · 2024-11-10T10:33:49Z

arch/RISCV/RISCVDetails.c

@@ -0,0 +1,36 @@
+#include "RISCVDetails.h"


Rot127 · 2024-11-10T10:35:20Z

arch/RISCV/RISCVDisassembler.h

Add include guard and your copyright please.

Rot127 · 2024-11-10T10:36:15Z

arch/RISCV/RISCVModule.c

@@ -4,29 +4,28 @@



Add your copyright.

Rot127 · 2024-11-10T10:37:36Z

arch/RISCV/RISCVPrinter.h

Copyright please

Rot127 · 2024-11-10T10:38:53Z

arch/RISCV/riscv_ast.gen.inc

+ * version 0b9c639f19da48734cbf14b61f6ad200b7c70de9.*/
+/*DO NOT MODIFY THIS CODE MANUALLY.*/
+/* */
+/*SPDX-License-Identifier: BSD-3-Clause*/


Your copyright.

Rot127 · 2024-11-10T10:50:08Z

arch/RISCV/riscv_decode.gen.inc

+    if (((binary_stream & 0x000000000000007F) == 0x67) &&
+        (((binary_stream & 0x0000000000007000) >> 12) == 0x0)) {


These if cases are ok for the prototype. Especially, since they more comprehensible. But for the final version we have to reduce runtime complexity again to something like O(log n).

Rot127 · 2024-11-10T10:55:24Z

arch/RISCV/riscv_helpers_ast2str.h

+  *ps = " , ";                                                                 \
+  *plen = 3
+
+static inline void hex_bits(uint64_t bitvec, char **s, size_t *len,


~~Yes, please move it to `utils.c~~

Rot127 · 2024-11-10T10:59:39Z

arch/RISCV/riscv_helpers_ast2str.h

+  *ps = " , ";                                                                 \
+  *plen = 3
+
+static inline void hex_bits(uint64_t bitvec, char **s, size_t *len,


~~Actually, you can just use sprintf(). We depend on libc anyways.~~

Rot127 · 2024-11-10T11:05:11Z

arch/RISCV/riscv_helpers_ast2str.h

+  *ps = " , ";                                                                 \
+  *plen = 3
+
+static inline void hex_bits(uint64_t bitvec, char **s, size_t *len,


Sorry, ignore it. See my comment in the final review message.

Rot127

Well done! This is a really nice basis to develop further.
I just pushed a new experimental branch (based on newest next). Please rebase your PR on top of it.

Rot127 · 2024-11-10T11:07:35Z

arch/RISCV/riscv_helpers_rvconf.h

+
+typedef struct riscv_conf {
+  Void2Bool sys_enable_fdext;
+  Void2Bool sys_enable_zfinx;


Doxygen please. Also for the Void2Bool callback.

Rot127 · 2024-11-10T11:07:47Z

arch/RISCV/riscv_helpers_rvconf.h

Rot127 · 2024-11-10T11:09:29Z

include/capstone/riscv_insn.gen.inc

+/*SPDX-License-Identifier: BSD-3-Clause*/
+/*=======================================================================*/
+
+#ifndef __Riscv_insn_gen_inc__


Also above in the other files please.

…s in riscv.h, changed name to reflect other archs conventio

github-actions bot added RISCV Arch LLVM-generated-files labels Oct 4, 2024

wargio reviewed Oct 4, 2024

View reviewed changes

XVilka suggested changes Oct 5, 2024

View reviewed changes

Rot127 reviewed Oct 5, 2024

View reviewed changes

Rot127 mentioned this pull request Oct 5, 2024

SAIL based module generator #2392

Open

wargio reviewed Oct 25, 2024

View reviewed changes

Rot127 reviewed Oct 26, 2024

View reviewed changes

moste00 added 3 commits November 7, 2024 23:38

initial implementation of the Sail-generated RISCV disassembler module

e451a37

fixed code till it compiled, cstool runs successfully (although with …

2861a7b

…bad output)

added license and added compressed decoding files, still didn't fix s…

be61c4d

…tringfication (printer_t)

moste00 force-pushed the riscv_disassembly_using_sail branch from 08d68d1 to be61c4d Compare November 7, 2024 21:59

XVilka mentioned this pull request Nov 10, 2024

[ASUpdater] Error while Generating Mapping tables for RISCV capstone-engine/llvm-capstone#46

Open

XVilka suggested changes Nov 10, 2024

View reviewed changes

Rot127 reviewed Nov 10, 2024

View reviewed changes

Rot127 requested changes Nov 10, 2024

View reviewed changes

fixed printer (hack), added compressed instruction, no longer include…

eff6b64

…s in riscv.h, changed name to reflect other archs conventio

	#include "../../include/capstone/capstone.h"
	#include <capstone/capstone.h>

		@@ -1,3 +1,3 @@
		#include "capstone.h"
		#include "../include/capstone/capstone.h"

		@@ -2,26 +2,38 @@
		/* RISC-V Backend By Rodrigo Cortes Porto <[email protected]> &

		if (((binary_stream & 0x000000000000007F) == 0x67) &&
		(((binary_stream & 0x0000000000007000) >> 12) == 0x0)) {

initial implementation of the Sail-generated RISCV disassembler module #2498

Are you sure you want to change the base?

initial implementation of the Sail-generated RISCV disassembler module #2498

Conversation

moste00 commented Oct 4, 2024

wargio Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rot127 Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wargio left a comment • edited Loading

Choose a reason for hiding this comment

XVilka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rot127 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rot127 Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rot127 Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVilka commented Oct 23, 2024

Choose a reason for hiding this comment

Rot127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moste00 Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rot127 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rot127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wargio Oct 4, 2024 •

edited

Loading

Rot127 Oct 5, 2024 •

edited

Loading

wargio left a comment •

edited

Loading

Rot127 left a comment •

edited

Loading

Rot127 Oct 5, 2024 •

edited

Loading

Rot127 Oct 5, 2024 •

edited

Loading

moste00 Nov 10, 2024 •

edited

Loading

Rot127 left a comment •

edited

Loading