Skip to content

[RISCV] fix: endianness inconsistency between XXX_disassembled.elf and XXX.json #3020

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.

Sign up for GitHub

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

Open
OlivierBBB wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[RISCV] fix: endianness inconsistency between XXX_disassembled.elf and XXX.json #3020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20b7226
feat: process objdump with sed regular expression to fix endianness
OlivierBBB May 5, 2026
5c047a0
Merge branch 'main' into 3019-riscv-endianness-inconsistency
OlivierBBB May 7, 2026
03c9e1a
Merge branch 'main' into 3019-riscv-endianness-inconsistency
OlivierBBB May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion arithmetization/src/test/examples/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ STRIP ?= false
# Ignored for .s and .rs files
ZIG_STRIP ?= true

# Disassembler: prefer llvm-objdump (shows bytes in memory order, matching the JSON format).
# Falls back to the Homebrew keg-only path on macOS, then to riscv64-unknown-elf-objdump.
# Override with: make OBJDUMP=/path/to/objdump
OBJDUMP ?= $(or $(shell command -v llvm-objdump 2>/dev/null),$(shell command -v rust-objdump 2>/dev/null),$(shell test -x /opt/homebrew/opt/llvm/bin/llvm-objdump && echo /opt/homebrew/opt/llvm/bin/llvm-objdump),$(shell command -v riscv64-unknown-elf-objdump 2>/dev/null))
Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

OBJDUMP := can be used if this never changes.

Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 8, 2026

Choose a reason for hiding this comment

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

Empty OBJDUMP causes cryptic build failure

Low Severity

If none of the auto-detected objdump tools (llvm-objdump, rust-objdump, Homebrew path, riscv64-unknown-elf-objdump) are installed, $(or ...) returns an empty string and OBJDUMP is set to empty. The subsequent @$(OBJDUMP) -d --line-numbers -S $(BIN) then expands to @ -d --line-numbers -S <bin>, causing the shell to try to execute -d as a command, producing a confusing error with no indication that the real problem is a missing disassembler.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 03c9e1a. Configure here.


# Paths to go main (to turn bin into json) and zkc main (to execute/debug the json)
GO_MAIN := $(MAKEFILE_DIR)main.go
ZKC_MAIN := $(MAKEFILE_DIR)../../main/riscv/main.zkc
Expand Down Expand Up @@ -68,7 +73,20 @@ compile: $(SRC)
riscv64-unknown-elf-strip -S $(BIN); \
fi
@echo "\nDisassembling $(BIN)"
@riscv64-unknown-elf-objdump -d --line-numbers -S $(BIN) > $(BIN)_disassembled.elf
# the following regular expression maps
#
# <hex-pc>: <byte_1><byte_2><byte_3><byte_4><non hex><etc>
#
# to
#
# <hex-pc>: <byte_1><byte_2><byte_3><byte_4> (<byte_4><byte_3><byte_2><byte_1>)<non hex><etc>
#
# Claude also included the same type of operation for COMPRESSED instructions ... the 2nd -e expression
Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 8, 2026

Choose a reason for hiding this comment

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

Accidentally committed AI tool reference in comment

Low Severity

The comment # Claude also included the same type of operation for COMPRESSED instructions ... the 2nd -e expression is an informal development note referencing the AI tool used to author the code. It's not meaningful documentation for maintainers and has already caused reviewer confusion (per the PR discussion). It should be replaced with a proper explanation of why the second -e expression exists.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 03c9e1a. Configure here.

@$(OBJDUMP) -d --line-numbers -S $(BIN) \
Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

so here the 3 commands above are tried until one of them is succeed? Also, I would add a comment saying that the generated file is modified with sed command for debugging purposes or so, so it is very clear it is not the standard one.

Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

Also, I do not understand the comment about Claude. We do not expect COMPRESSED instructions to appear.

Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

Actually the description of this PR may be a commented in the Makefile, as it helps understanding what happens.

Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

-d --line-numbers -S those flags I believe were specific for riscv64-unknown-elf-objdump, now it seems they are applied in all cases. Does it work with assembly, rust and zig?

Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

Could riscv64-unknown-elf-objdump be used in all cases, with some adjustement using sed to make them consistent otherwise?

| sed -E \
-e 's/^([[:space:]]*[0-9a-f]+:[[:space:]]*)([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})([^0-9a-f])/\1\2\3\4\5 (\5\4\3\2)\6/' \
-e 's/^([[:space:]]*[0-9a-f]+:[[:space:]]*)([0-9a-f]{2})([0-9a-f]{2})([^0-9a-f])/\1\2\3 (\3\2)\4/' \
> $(BIN)_disassembled.elf
Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 7, 2026

Choose a reason for hiding this comment

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

Byte reversal breaks with llvm-objdump output format

High Severity

The sed byte-reversal is designed for GNU objdump's big-endian display format (e.g., 08800137 → (37018008)), but OBJDUMP now prefers llvm-objdump, which the comment at line 30 explicitly says "shows bytes in memory order." If llvm-objdump outputs 37018008, the sed reversal produces (08800137) — the opposite of what's in the JSON, defeating the stated purpose of aligning endianness with the JSON file. The transformation is tool-dependent but applied unconditionally regardless of which objdump was selected.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5c047a0. Configure here.

@echo "\nGenerating $(JSON) from $(BIN)"
@go run $(GO_MAIN) $(BIN) "$(IN_BYTES)" $(PROGRAM_OFFSET) $(IN_BYTES_OFFSET) $(ENTRY_POINT) > $(JSON)

Expand Down
24 changes: 13 additions & 11 deletions arithmetization/src/test/examples/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ The executable, the json and the disassembled elf file all live in the `<ext>/bi
- `rustc (>= rustc 1.88.0)` with `riscv64imac-unknown-none-elf` target — for Rust programs
- `go (>= 1.26.1)` — to convert ELF to JSON
- `go-corset, zkc (>= 1.2.12)` — to execute/debug the JSON
- one of `llvm-objdump`, `rust-objdump` (from `cargo-binutils`), or `riscv64-unknown-elf-objdump` to disassemble the ELF

## Usage

Expand Down Expand Up @@ -86,17 +87,18 @@ zkc-test <name>.zig ZIG_STRIP=false

## Options

| Variable | Default | Description |
|------------------|-----------------------------------------------------------------------------------------|----------------------------------------------------------------------------|
| `SRC` | `asm/src/<TEST>`, `zig/src/<TEST>`, or `rust/src/<TEST>` depending on extension | Path to the source file, can be overridden |
| `BIN` | `asm/bin/<NAME>`, `zig/zig-out/bin/<NAME>`, or `rust/bin/<NAME>` depending on extension | Path to the output ELF binary, can be overridden |
| `JSON` | same directory as `BIN`, with `.json` extension | Path to the output JSON file, can be overridden |
| `STRIP` | `false` | Strip debug symbols from the ELF after compilation |
| `ZIG_STRIP` | `true` | Strip when compiling Zig (reduces binary size), ignored for `.s` and `.rs` |
| `IN_BYTES` | `""` | Input bytes written to memory at `IN_BYTES_OFFSET` before execution |
| `PROGRAM_OFFSET` | `0` | Memory offset where the program is loaded (up to 128 MB) |
| `IN_BYTES_OFFSET` | `0x8000000` | Memory offset where input bytes are written (up to 1 GB) |
| `ENTRY_POINT` | `0` | Entry point offset |
| Variable | Default | Description |
|-------------------|-----------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------|
| `SRC` | `asm/src/<TEST>`, `zig/src/<TEST>`, or `rust/src/<TEST>` depending on extension | Path to the source file, can be overridden |
| `BIN` | `asm/bin/<NAME>`, `zig/zig-out/bin/<NAME>`, or `rust/bin/<NAME>` depending on extension | Path to the output ELF binary, can be overridden |
| `JSON` | same directory as `BIN`, with `.json` extension | Path to the output JSON file, can be overridden |
| `OBJDUMP` | auto-detected | Disassembler; tried in order: `llvm-objdump`, `rust-objdump`, Homebrew `llvm-objdump`, `riscv64-unknown-elf-objdump` |
| `STRIP` | `false` | Strip debug symbols from the ELF after compilation |
| `ZIG_STRIP` | `true` | Strip when compiling Zig (reduces binary size), ignored for `.s` and `.rs` |
| `IN_BYTES` | `""` | Input bytes written to memory at `IN_BYTES_OFFSET` before execution |
| `PROGRAM_OFFSET` | `0` | Memory offset where the program is loaded (up to 128 MB) |
| `IN_BYTES_OFFSET` | `0x8000000` | Memory offset where input bytes are written (up to 1 GB) |
Copy link
Copy Markdown
Contributor

@lorenzogentile404 lorenzogentile404 May 6, 2026

Choose a reason for hiding this comment

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

To make clear we did not forget a 0, we may write 0x08000000.

| `ENTRY_POINT` | `0` | Entry point offset |

## Target ISA

Expand Down
Loading