Translating RISC-V programs directly from ELF files. #1453

lvella · 2024-06-14T14:58:40Z

There are many advantages in using standard assemblers and linkers, like:

maturity;
more complete support of assembly language, which in turn allows for support to more high level languages and compilers;
link-time optimizations;
no need to deal with language edge cases (our assembler and linker deals with a number of rust stuff, but it would need even more to support std).

But it comes with a cost, as we need to lift references to text data back into labels, (because powdr operates on a higher abstraction level), and to do that, we need the ELF file to either be a PIE (Position Independent Executable), or to still have the linkage relocation tables (option --emit-relocs of GNU and LLVM linkers).

(I think.)

leonardoalt

Many but mostly small comments

leonardoalt · 2024-07-05T09:50:29Z

.github/workflows/pr-tests.yml

@@ -155,6 +155,8 @@ jobs:
        key: ${{ runner.os }}-pilcom-node-modules
    - name: Install Rust toolchain 1.77 (with clippy and rustfmt)
      run: rustup toolchain install 1.77-x86_64-unknown-linux-gnu && rustup component add clippy --toolchain 1.77-x86_64-unknown-linux-gnu && rustup component add rustfmt --toolchain 1.77-x86_64-unknown-linux-gnu
+    - name: Install test dependencies
+      run: sudo apt-get install -y binutils-riscv64-unknown-elf lld


why not riscv32?

There is no riscv32 in Ubuntu. You use the 64 for both.

leonardoalt · 2024-07-05T09:51:46Z

riscv/tests/riscv_data/dynamic_relocation/dynamic_relocation.s

+    .section .data
+    .align 4
+    .global data_section
+


was this test manually crafted?

If you call ChatGPT "manually"...

riscv/tests/riscv.rs

riscv/tests/instructions.rs

leonardoalt · 2024-07-05T09:54:19Z

riscv/tests/instruction_tests/sources/rvc.S

+# rvc.S
+#-----------------------------------------------------------------------------
+#
+# Test RVC corner cases.


Where did this test come from?

Mostly from the official testsuite, but I left some comments on the modifications I had to make.

riscv/src/elf.rs

leonardoalt · 2024-07-05T10:51:10Z

riscv/src/elf.rs

+                        // Otherwise, there is something more fishy going on, and we
+                        // panic.
+
+                        // TODO: we should just emit `unimp` for every unknown.


why not done?

It can make the generated code very big and may mask other issues. I reworded the TODO.

Also, there are unanswered questions: should this data be readable from the program itself? What about the valid instructions?

riscv/Cargo.toml

cli-rs/src/main.rs

riscv/Cargo.toml

pacheco · 2024-07-04T17:50:55Z

riscv/src/asm/mod.rs

-                Ok((*r1, *r2, *r3))
-            }
+            [Argument::Register(r1), Argument::Register(r2), Argument::Register(r3)
+            | Argument::RegOffset(None | Some(Expression::Number(0)), r3)] => Ok((*r1, *r2, *r3)),


could you explain why the RegOffset here? isn't there a rrro case? Also, in the rr case, the 0 offset case is not handled, should it be?

See #1453 (comment).

But indeed, for consistency rr should be able to handle the 0 case when decoding the instruction lr.w rd, 0(rs1), but we have never encountered this case in handwritten assembly (the test for this instruction, lrsc.S, omits the 0), and the compiler doesn't place the zero there, either.

I fixed it.

pacheco · 2024-07-04T17:57:30Z

riscv/tests/riscv_data/vec_median/src/main.rs

 #[no_mangle]
-fn main() {


should all programs be pub fn main? if so poseidon_gl_via_coprocessor and evm should also be changed

This is not needed, and I don't know why I changed it. Reverted.

pacheco · 2024-07-04T21:00:19Z

riscv/src/code_gen.rs

    fn rro(&self) -> Result<(Register, Register, u32), Self::Error>;
-    fn rrro(&self) -> Result<(Register, Register, Register, u32), Self::Error>;


why isn't rrro there anymore?

rrro stands for "register, register, register, offset", but there is no RISC-V instruction that encodes this much information (there is not enough space in the 32 bits). It existed because the assembly syntax for these instructions allowed for writing amoadd.d rd, rs2, 0(rs1), but there is no place to put that 0 in the actual encoding (if I try to assemble with any other value I get an error).

So, at first I replaced rrro() with rrr(), and changed it to accept both None and 0 offset. Then I figured out that the order of the registers expected by these instructions was different from other instructions using rrr(), so I created rrr2() (ok, the name could be better) to say that this is an instruction that expects 3 registers, but in a different order from rrr().

i see, i think name is fine, can you add a small comment?

There are comments on both rrr() and rrr2() explaining the difference between them.

pacheco · 2024-07-04T21:19:17Z

riscv/src/lib.rs


    fs::write(powdr_asm_file_name.clone(), &powdr_asm).unwrap();
    log::info!("Wrote {}", powdr_asm_file_name.to_str().unwrap());

    Some((powdr_asm_file_name, powdr_asm))
 }

-/// Compiles a riscv asm file all the way down to PIL and generates
-/// fixed and witness columns.
+#[allow(clippy::print_stderr)]


i think this is not needed?

The comment was outdated, but I have no idea why where this line came from. I am removing it.

pacheco · 2024-07-05T11:50:35Z

riscv/tests/instruction_tests/README.md

@@ -9,65 +9,61 @@ kind of arithmetic over `.text` label and addresses, nor alignment or spacing
 directives in `.text` sections. Most unsupported instructions are related to
 this limitation.

+One consequence of this limitation is that all the supported uses of `auipc` and
+`lui`, when referring to a `.text` label, is when they can be fused with the
+next instruction to make use of the loaded address immediately (e.g. `addi` or


this phrase is a bit confusing, is it saying that these instructions are only supported when they can be merged with a following instruction?

I'll try to write it better. But using auipc and lui to load a .text address is only supported if it can be fused with the next instruction. It works fine if they are used to load a data address.

pacheco · 2024-07-05T11:53:25Z

riscv/tests/instruction_tests/README.md

- amoswap_w
- amoxor_w
+- `amoand_w`
+- `momax_w`


amomax_w?

pacheco · 2024-07-05T11:59:14Z

riscv/tests/instruction_tests/sources/rvc.S

+  #define RVC_TEST_CASE(n, r, v, code...) \
+    TEST_CASE (n, r, v, .option push; .option rvc; code; .align 2; .option pop)
+
+  // Powdr note: there was a test case from the original riscv-tests here,


does the // comment notation work here?

Yes, because it worked.

pacheco · 2024-07-05T12:00:11Z

riscv/tests/instruction_tests/sources/rvc.S

+      2:j fail; \
+      1:)
+
+// Powdr note: tests 36 and 37 were removed, as they did arithmetic on text addresses, which we don't support.


does it make sense to comment them out instead of removing? also, 38 and 39 are not present here either

38 and 39 were already missing. Maybe they are RISC-V 64 specific?

I restored the removed tests, but commented out.

pacheco · 2024-07-05T12:04:07Z

riscv/tests/instructions.rs

-    fn run_instruction_test(assembly: &str, name: &str) {
+    fn run_instruction_test(path: &Path, name: &str) {
+        // Test from ELF path:
+        verify_riscv_asm_file(path, &Runtime::base(), false);


nitpick as this is only used here i think: have elf in the function name somehow?

But this also executes the test from the assembly path.

ah right, i got confused by the asm in the name, but asm here is from the input not output

Co-authored-by: Leo <leo@powdrlabs.com>

riscv/src/elf.rs

pacheco · 2024-07-05T12:53:01Z

riscv/src/code_gen.rs


    /// The name of the function that should be called to start the program.
-    fn start_function(&self) -> &str;
+    fn start_function(&self) -> impl AsRef<str>;
 }

 /// Translates a RISC-V program to POWDR ASM.


the comment about "will call the methods just once" can be removed now i think

It is still true...

pacheco · 2024-07-05T13:15:42Z

riscv/src/elf.rs

+        let section_data = &file_buffer[p.p_offset as usize..(p.p_offset + p.p_filesz) as usize];
+
+        // Test if executable
+        if p.p_flags & 1 == 1 {


p.is_executable() then you can remove the comment

There is no such method. But I improved it by using the named constant.

https://github.com/m4b/goblin/blob/617775898fdfd0b843b2e937c80e455cf6f2a637/src/elf/program_header.rs#L150
isn't this it?

Ah, sorry. rust-analyzer didn't find it, so I didn't think it existed. But I'll try again.

pacheco · 2024-07-05T14:00:02Z

riscv/src/elf.rs

+
+/// Find all the references to text addresses in the instructions and add them
+/// to the set.
+fn search_text_addrs(


not a big deal, but i wonder if all these were part of the InstructionLifter it wouldn't make the code a bit simpler to follow? e.g., maybe then you don't need the TwoOrOneMapper trait or the ReadOrWrite enum

The reason to have the TwoOrOneMapper trait is to be able to write the algorithm in try_map_two_by_two() independently of types, so it is was easier to write and reason about.

I don't see how to get rid of ReadOrWrite enum without replacing with something very similar. The lifting algorithm has to run twice, once just collecting the references, and another making use of them.

pacheco · 2024-07-05T14:20:35Z

riscv/src/elf.rs

+    }
+
+    /// Get the symbol or a default label formed from the address value.
+    fn get_as_string(&self, addr: u32) -> String {


is the method needed? get(addr).into() is fine no?

Co-authored-by: Leo <leo@powdrlabs.com>

…pport

Co-authored-by: Leo <leo@powdrlabs.com>

leonardoalt · 2024-07-08T13:47:45Z

CI failing

…pport

The latests version our dependencies depends on.

lvella

a

riscv/tests/instruction_tests/README.md

leonardoalt · 2024-07-08T20:56:10Z

riscv/src/lib.rs

-) -> (Option<PathBuf>, BTreeMap<String, PathBuf>) {
+pub struct CompilationResult {
+    pub executable: Option<PathBuf>,
+    assemblies: BTreeMap<String, PathBuf>,


why just make one pub and the other not?

because there is no method to get the pub one

leonardoalt · 2024-07-08T20:58:32Z

cli-rs/src/main.rs

@@ -359,9 +359,14 @@ fn compile_riscv_asm<F: FieldElement>(
        None => powdr_riscv::Runtime::base(),
    };

-    powdr_riscv::compile_riscv_asm::<F>(
+    powdr_riscv::compile_riscv_asm_bundle::<F>(


is elf still the default compilation path here?

There is no ELF path in this function, this only from assembly.

Co-authored-by: Leo <leo@powdrlabs.com>

leonardoalt · 2024-07-08T22:07:01Z

The other thing that's lost are labels, which I assume are useful for the profiler? @pacheco

For example, for the trivial test we get via asm:

    main:
    trivial_dash_be3334ad2e9c9ee9___dot_Lfunc_begin0:
    .debug loc 338 7 17;
    tmp1 <== jump_dyn(x1);

and via elf:

    main:
    tmp1 <== jump_dyn(x1);

lvella and others added 30 commits May 22, 2024 13:14

Attempt to make vec_median a binary.

bc8e5c1

Missing moved file.

2544ec3

works

e750285

Starting implementation of instructions lifting.

6050592

Refactoring instruction processor to be more generic.

424115f

Implementing more general jalr.

d362de7

Merge branch 'jalr-instruction' into HEAD

6261fa8

Lifting sets of instructions into high level (pseudo)instructions.

2a0920a

Instruction lifting possibly done.

6879d74

Loading data and lifting references to text labels.

5866a4b

Relocation apparently working.

69130e1

Merge remote-tracking branch 'origin/main' into elf-support

d87c3dc

Merge branch 'main' into elf-support

c3c8842

Refactoring powdr asm generator to make it source agnostic.

d92987e

Make Label generic.

3c71255

Setting the stack pointer.

bc53657

Using a non-conflicting start function name.

708dfee

Merge branch 'main' into split-code-gen

27872cd

Moving global declarations to asm_translate.

856a128

Doing RISC-V initializations on runtime lib, with linker support.

adfc41d

Modify RISC-V compilation to build binaries instead of libraries.

d04c23f

Correctly finding all the assembly files.

7ce3446

Linker settings for the ELF file to be usable.

890f529

Relocation table test.

ce334a1

Removing unrelated test.

42af5f7

Fixing lint.

9f509a8

Updating comments.

bad34de

Bumping version because the runtime changes are incompatible.

5c703da

(I think.)

Fixing lint.

531e8ee

Addressing reviews.

d138007

Better wording.

7691764

lvella requested a review from chriseth July 4, 2024 16:09

leonardoalt reviewed Jul 5, 2024

View reviewed changes

pacheco reviewed Jul 5, 2024

View reviewed changes

Update riscv/src/elf.rs

d20a613

Co-authored-by: Leo <leo@powdrlabs.com>

pacheco reviewed Jul 5, 2024

View reviewed changes

lvella and others added 5 commits July 5, 2024 17:06

Grammar fix.

ce69fea

Co-authored-by: Leo <leo@powdrlabs.com>

Update riscv/src/elf.rs

7c134f7

Co-authored-by: Leo <leo@powdrlabs.com>

Addressing lots of reviews.

750d569

Merge branch 'elf-support' of github.com:powdr-labs/powdr into elf-su…

6be0212

…pport

Update riscv/src/elf.rs

0c46213

Co-authored-by: Leo <leo@powdrlabs.com>

lvella added 5 commits July 8, 2024 14:51

More review addressing.

799aa23

Merge branch 'elf-support' of github.com:powdr-labs/powdr into elf-su…

c2743ed

…pport

Translating from assembly by default.

d753b58

Using is_executable.

d36cf75

Placing private below pub.

f2b8c34

lvella requested review from pacheco and leonardoalt July 8, 2024 14:22

lvella added 2 commits July 8, 2024 15:46

Reverting itertools to 0.12.

cc0b5ed

The latests version our dependencies depends on.

Merge branch 'main' into elf-support

e3cce8f

lvella commented Jul 8, 2024

View reviewed changes

leonardoalt reviewed Jul 8, 2024

View reviewed changes

Update riscv/tests/instruction_tests/README.md

42cf0cc

Co-authored-by: Leo <leo@powdrlabs.com>

leonardoalt approved these changes Jul 8, 2024

View reviewed changes

leonardoalt added this pull request to the merge queue Jul 8, 2024

Merged via the queue into main with commit b06fa28 Jul 8, 2024
6 checks passed

leonardoalt deleted the elf-support branch July 8, 2024 22:49

		fn rro(&self) -> Result<(Register, Register, u32), Self::Error>;
		fn rrro(&self) -> Result<(Register, Register, Register, u32), Self::Error>;

Translating RISC-V programs directly from ELF files. #1453

Translating RISC-V programs directly from ELF files. #1453

Conversation

lvella commented Jun 14, 2024 • edited Loading

leonardoalt 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

lvella Jul 5, 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

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

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

lvella Jul 8, 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

Choose a reason for hiding this comment

leonardoalt commented Jul 8, 2024

lvella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvella Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt commented Jul 8, 2024

lvella commented Jun 14, 2024 •

edited

Loading

lvella Jul 5, 2024 •

edited

Loading

lvella Jul 8, 2024 •

edited

Loading

lvella Jul 8, 2024 •

edited

Loading