Open
Conversation
Separate relocation tag from label, introducing `Rel64Tag` bare enum, and `Rel64` struct. Applying relocation requires real offsets, not labels.
monoid
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces enum-based Rel64 with a tag+label struct, renames relocation constructors to snake_case, adds relocation application helpers in new modules (addr, control, data, movs), updates call sites/tests to use new constructors, and adds extensive relocation unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Rel64::apply
participant Tag as Rel64Tag
participant Reloc as reloc module (addr/control/data/movs)
participant Mem as Memory (byte slice)
participant Instr as InstructionCode
Caller->>Tag: invoke apply(base, value/label, mem, offset)
Tag->>Reloc: dispatch to specific *_reloc(base, symbol/value, mem, offset)
Reloc->>Mem: get_bytes_mut(offset)
Reloc->>Instr: read instruction/data word(s)
Reloc->>Instr: compute & patch bitfields/bytes
Reloc->>Mem: write back patched bytes
Reloc-->>Caller: return Result<(), Rel64Error>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
harm/src/reloc/movs.rs (1)
19-178: Consider a shared helper for MOV field patching.The unpack/mask/write sequence is duplicated across every MOV relocator here. Pulling that into a small helper that accepts the
imm16chunk plus optional opcode override would make later fixes much less likely to drift between variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc/movs.rs` around lines 19 - 178, The MOV relocators (mov_w_abs_g0_reloc, mov_w_abs_g0nc_reloc, mov_w_abs_g0s_reloc, mov_w_abs_g1_reloc, mov_w_abs_g1nc_reloc, mov_w_abs_g1s_reloc, mov_w_abs_g2_reloc, mov_w_abs_g2nc_reloc, mov_w_abs_g2s_reloc, mov_w_abs_g3_reloc) all duplicate the unpack/mask/write sequence using InstructionCode and MOV_IMM16_OFFSET/ MOV_OPCODE_OFFSET; extract that into a small helper (e.g., patch_mov_instruction(bytes: &mut [u8], imm16: u16, opcode: Option<u32>)) that computes INST_MASK, unpacks InstructionCode, clears the imm/opcode bits, ORs in (imm16 as u32) << MOV_IMM16_OFFSET and, when opcode.is_some(), ORs opcode << MOV_OPCODE_OFFSET, then writes back InstructionCode::from_u32(...).0; update each relocator to compute its imm16/opcode (use MOV_OPCODE_MOVN or MOV_OPCODE_MOVZ where appropriate in mov_*s variants) and call the helper to remove the repeated logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@harm/src/reloc.rs`:
- Around line 262-264: The ABS16/ABS32 cases currently pre-cast Addr to signed
via Addr::cast_signed in the Rel64Tag match, which causes valid unsigned bit
patterns (e.g. 0xffff_ffff or 0xffff) to be rejected; change the Rel64Tag::Abs32
and Rel64Tag::Abs16 arms to pass the original unsigned Addr value into
abs32_reloc and abs16_reloc (i.e., remove the cast_signed usage) so the overflow
checks in harm/src/reloc/data.rs operate on the original unsigned value; keep
Rel64Tag::Abs64 using abs64_reloc as-is.
In `@harm/src/reloc/addr.rs`:
- Around line 82-98: The add_abs_lo_12_nc_reloc function currently computes
delta using calc_offset(base, symbol, offset) which makes the low-12 relocation
depend on P; instead encode the absolute low 12 bits of symbol (symbol & 0xfff).
Replace the calc_offset-based delta with a value derived from symbol: compute
low12 = (symbol as u64) & MASK (cast to u32 when applying) and use that to set
the ADD_IMM12 field (keep the existing masking and shifting logic with
ADD_IMM12_OFFSET and MASK). Keep get_bytes_mut, InstructionCode unpack/pack
usage unchanged and preserve error handling/return type.
In `@harm/src/reloc/claude_tests.rs`:
- Around line 595-613: The tests adrp_overflow_positive and
adrp_overflow_negative assert
Err(Rel64Error::InvalidBits(BitError::Unsupported)) but the comment states the
correct outcome should be an overflow; update these tests to stop codifying the
wrong behavior by either asserting
Err(Rel64Error::InvalidBits(BitError::Overflow)) for both tests or loosening the
match to assert Err(Rel64Error::InvalidBits(_)) so the suite no longer freezes
the current bug in adrp_prel_pg_hi21_reloc and allows proper overflow handling
to be implemented.
---
Nitpick comments:
In `@harm/src/reloc/movs.rs`:
- Around line 19-178: The MOV relocators (mov_w_abs_g0_reloc,
mov_w_abs_g0nc_reloc, mov_w_abs_g0s_reloc, mov_w_abs_g1_reloc,
mov_w_abs_g1nc_reloc, mov_w_abs_g1s_reloc, mov_w_abs_g2_reloc,
mov_w_abs_g2nc_reloc, mov_w_abs_g2s_reloc, mov_w_abs_g3_reloc) all duplicate the
unpack/mask/write sequence using InstructionCode and MOV_IMM16_OFFSET/
MOV_OPCODE_OFFSET; extract that into a small helper (e.g.,
patch_mov_instruction(bytes: &mut [u8], imm16: u16, opcode: Option<u32>)) that
computes INST_MASK, unpacks InstructionCode, clears the imm/opcode bits, ORs in
(imm16 as u32) << MOV_IMM16_OFFSET and, when opcode.is_some(), ORs opcode <<
MOV_OPCODE_OFFSET, then writes back InstructionCode::from_u32(...).0; update
each relocator to compute its imm16/opcode (use MOV_OPCODE_MOVN or
MOV_OPCODE_MOVZ where appropriate in mov_*s variants) and call the helper to
remove the repeated logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4fd0deca-f57e-4c6b-9088-3641ca95ef69
📒 Files selected for processing (11)
harm/src/instructions/control/branch_imm.rsharm/src/instructions/control/testbranch.rsharm/src/instructions/dpimm/movewide/movewide_reloc.rsharm/src/instructions/ldst/ldr.rsharm/src/instructions/ldst/ldrsw.rsharm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rsharm/src/reloc/control.rsharm/src/reloc/data.rsharm/src/reloc/movs.rs
Comment on lines
+595
to
+613
| #[test] | ||
| fn adrp_overflow_positive() { | ||
| let mut mem = [0u8; 4]; | ||
| assert!(matches!( | ||
| adrp_prel_pg_hi21_reloc(0x0, 0x1_0000_0000_0000, &mut mem, 0), | ||
| // TODO Unsupported seems to have obsure semantics. | ||
| Err(Rel64Error::InvalidBits(BitError::Unsupported)) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn adrp_overflow_negative() { | ||
| let mut mem = [0u8; 4]; | ||
| assert!(matches!( | ||
| adrp_prel_pg_hi21_reloc(0x1_0000_0000_0000, 0x0, &mut mem, 0), | ||
| // TODO Unsupported seems to have obsure semantics. | ||
| // Actually it should be BitError::Overflow. | ||
| Err(Rel64Error::InvalidBits(BitError::Unsupported)) | ||
| ),); |
There was a problem hiding this comment.
Don’t freeze the known-wrong ADRP overflow error.
These assertions require BitError::Unsupported, but the inline comment already says the correct outcome should be an overflow. Keeping the exact Unsupported match here will turn a real fix into a test regression. Prefer asserting Overflow or loosening the match so the suite doesn’t codify the current bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@harm/src/reloc/claude_tests.rs` around lines 595 - 613, The tests
adrp_overflow_positive and adrp_overflow_negative assert
Err(Rel64Error::InvalidBits(BitError::Unsupported)) but the comment states the
correct outcome should be an overflow; update these tests to stop codifying the
wrong behavior by either asserting
Err(Rel64Error::InvalidBits(BitError::Overflow)) for both tests or loosening the
match to assert Err(Rel64Error::InvalidBits(_)) so the suite no longer freezes
the current bug in adrp_prel_pg_hi21_reloc and allows proper overflow handling
to be implemented.
The relocation uses "S+A", not "S+A-P".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
harm/src/reloc.rs (1)
262-264:⚠️ Potential issue | 🔴 CriticalKeep ABS16/ABS32 on the original unsigned value.
These arms still reinterpret
Addras signed before narrowing, so valid unsigned bit patterns like0xffff_ffffforAbs32and0xffffforAbs16are rejected beforeharm/src/reloc/data.rscan do the proper width check. Pass the originalvaluethrough unchanged here.Suggested fix
- Rel64Tag::Abs32 => abs32_reloc(value.cast_signed(), memory, offset), - Rel64Tag::Abs16 => abs16_reloc(value.cast_signed(), memory, offset), + Rel64Tag::Abs32 => abs32_reloc(value, memory, offset), + Rel64Tag::Abs16 => abs16_reloc(value, memory, offset),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc.rs` around lines 262 - 264, The Abs32 and Abs16 arms are incorrectly calling value.cast_signed() before narrowing which rejects valid unsigned patterns; change the Rel64Tag::Abs32 and Rel64Tag::Abs16 match arms to pass the original value (without cast_signed()) into abs32_reloc and abs16_reloc respectively so the unsigned bit patterns are preserved; leave Rel64Tag::Abs64 and abs64_reloc unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@harm/src/reloc.rs`:
- Around line 262-264: The Abs32 and Abs16 arms are incorrectly calling
value.cast_signed() before narrowing which rejects valid unsigned patterns;
change the Rel64Tag::Abs32 and Rel64Tag::Abs16 match arms to pass the original
value (without cast_signed()) into abs32_reloc and abs16_reloc respectively so
the unsigned bit patterns are preserved; leave Rel64Tag::Abs64 and abs64_reloc
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a740d91-43dc-4177-85b8-ccd80475424c
📒 Files selected for processing (3)
harm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- harm/src/reloc/claude_tests.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harm/src/reloc.rs (1)
411-417: Address the TODO comment about PRel64 test correctness.The comment on line 412 questions whether the test expectation is correct. Since PRel64 computes
value - (base + offset), the test withbase=0x1000,value=0x123456789abcdef0,offset=0should produce0x123456789abcdef0 - 0x1000which matches the assertion. Consider removing the TODO if verified, or fixing if incorrect.Would you like me to help verify this test expectation or open an issue to track it?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc.rs` around lines 411 - 417, The TODO questioning the PRel64 test expectation should be resolved: confirm that PRel64 computes value - (base + offset) and therefore with base=0x1000, value=0x123456789abcdef0 and offset=0 the expected result is value - 0x1000, so remove the TODO and replace it with a short explanatory comment referencing the PRel64 semantics and the assertion that compares mem to 0x123456789abcdef0u64.wrapping_sub(0x1000). If you want stronger verification, add a brief comment above the assert or a second case verifying non-zero offset behavior (referencing mem and the existing assert) rather than leaving the TODO.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@harm/src/reloc.rs`:
- Around line 411-417: The TODO questioning the PRel64 test expectation should
be resolved: confirm that PRel64 computes value - (base + offset) and therefore
with base=0x1000, value=0x123456789abcdef0 and offset=0 the expected result is
value - 0x1000, so remove the TODO and replace it with a short explanatory
comment referencing the PRel64 semantics and the assertion that compares mem to
0x123456789abcdef0u64.wrapping_sub(0x1000). If you want stronger verification,
add a brief comment above the assert or a second case verifying non-zero offset
behavior (referencing mem and the existing assert) rather than leaving the TODO.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d56074f-3205-40be-8137-4f896f7f294f
📒 Files selected for processing (4)
harm/src/instructions/ldst.rsharm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- harm/src/reloc/claude_tests.rs
monoid
force-pushed
the
feat/relocations
branch
2 times, most recently
from
edee315 to
fdd5502
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
harm/src/reloc.rs (1)
76-94: Consider making these constructorsconst fnfor consistency.The address-related constructors (
adr_prel_lo21,adr_prel_pg_hi21, etc.) use regularfnwhile similar branch/mov constructors useconst fn. Since they all just create struct instances, making them allconst fnwould be consistent and enable compile-time construction.♻️ Example fix for one function
#[inline] - pub fn adr_prel_lo21(label: LabelRef) -> Self { + pub const fn adr_prel_lo21(label: LabelRef) -> Self { Self::new(Rel64Tag::AdrPrelLo21, label) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc.rs` around lines 76 - 94, The four address-related constructors (adr_prel_lo21, adr_prel_pg_hi21, adr_prel_pg_hi21nc, add_abs_lo12nc) should be changed to const fn to match other constructors; update each signature from `pub fn` to `pub const fn` and ensure they still call `Self::new(Rel64Tag::..., label)` so the constructors remain usable in const contexts while referring to the same Rel64Tag variants (AdrPrelLo21, AdrPrelPgHi21, AdrPrelPgHi21Nc, AddAbsLo12Nc).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@harm/src/reloc.rs`:
- Around line 6-22: CI found rustfmt formatting issues in the module-level
documentation at the top of reloc.rs; run `cargo fmt` to automatically fix
formatting and ensure the block comment uses a valid Rust doc delimiter (replace
the non-standard "/**!" opener with a proper "/*!" block-doc or convert to `//!`
line docs) so the header comment in reloc.rs is formatted according to rustfmt.
---
Nitpick comments:
In `@harm/src/reloc.rs`:
- Around line 76-94: The four address-related constructors (adr_prel_lo21,
adr_prel_pg_hi21, adr_prel_pg_hi21nc, add_abs_lo12nc) should be changed to const
fn to match other constructors; update each signature from `pub fn` to `pub
const fn` and ensure they still call `Self::new(Rel64Tag::..., label)` so the
constructors remain usable in const contexts while referring to the same
Rel64Tag variants (AdrPrelLo21, AdrPrelPgHi21, AdrPrelPgHi21Nc, AddAbsLo12Nc).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cdb41de-46b4-4b52-8dc8-247c921aeb01
📒 Files selected for processing (5)
harm/src/instructions/ldst.rsharm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rsharm/src/reloc/movs.rs
monoid
force-pushed
the
feat/relocations
branch
from
fdd5502 to
6579bf4
Compare
monoid
force-pushed
the
feat/relocations
branch
from
6579bf4 to
e549c5d
Compare
Owner
Author
|
@coderabbitai please, review. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
harm/src/reloc/addr.rs (1)
148-226: Consider extracting a macro or helper to reduce duplication.The
ldst16,ldst32,ldst64, andldst128variants follow an identical pattern differing only in theScaledOffset*type and constants. If the pattern needs modification in the future, all four implementations would need consistent updates.That said, this is a minor maintainability concern—the current implementation is correct and clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc/addr.rs` around lines 148 - 226, These four functions (ldst16_abs_lo12_nc_reloc, ldst32_abs_lo12_nc_reloc, ldst64_abs_lo12_nc_reloc, ldst128_abs_lo12_nc_reloc) duplicate the same logic; extract a single helper or macro that takes the ScaledOffset type and the three constants (CLEAR_MASK, VALUE_MASK, and *_OFFSET) as parameters to centralize: implement a generic helper (e.g., apply_ldst_abs_lo12_reloc) that accepts symbol, mem, offset, a closure or trait to construct the ScaledOffset (ScaledOffset16/32/64/128::new), and the CLEAR/VALUE/OFFSET constants, then have each specific function call that helper with the appropriate ScaledOffset constructor and constants to remove the duplicated unpack/clear/set/*bytes assignment logic.harm/src/reloc.rs (1)
17-20: Minor: Trailing whitespace on line 19.Line 19 appears to have trailing whitespace after the
//!doc comment. This may causecargo fmtor linting tools to flag it.-//! +//!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc.rs` around lines 17 - 20, The doc comment block in reloc.rs contains trailing whitespace on the line describing `offset` (the `//!` comment around "the offset in the memory to apply the relocation at."); remove the trailing spaces from that doc comment line so the line ends immediately after the comment text (then run cargo fmt or your editor's trim-trailing-whitespace action to ensure no other doc lines have trailing whitespace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@harm/src/reloc.rs`:
- Around line 328-330: The current use of value.cast_signed() for
Rel64Tag::Abs32 and Rel64Tag::Abs16 reinterprets bit patterns and causes
try_into failures for high unsigned ranges; locate the match arms for
Rel64Tag::Abs32 and Rel64Tag::Abs16 in reloc.rs and replace value.cast_signed()
with a bit-preserving cast (value as i32 for Abs32 and value as i16 for Abs16),
or alternatively change the signatures of abs32_reloc and abs16_reloc to accept
u32/u16 and perform the necessary narrowing internally; ensure you update all
call sites to match the chosen approach so high-bit unsigned inputs are handled
without runtime try_into errors.
---
Nitpick comments:
In `@harm/src/reloc.rs`:
- Around line 17-20: The doc comment block in reloc.rs contains trailing
whitespace on the line describing `offset` (the `//!` comment around "the offset
in the memory to apply the relocation at."); remove the trailing spaces from
that doc comment line so the line ends immediately after the comment text (then
run cargo fmt or your editor's trim-trailing-whitespace action to ensure no
other doc lines have trailing whitespace).
In `@harm/src/reloc/addr.rs`:
- Around line 148-226: These four functions (ldst16_abs_lo12_nc_reloc,
ldst32_abs_lo12_nc_reloc, ldst64_abs_lo12_nc_reloc, ldst128_abs_lo12_nc_reloc)
duplicate the same logic; extract a single helper or macro that takes the
ScaledOffset type and the three constants (CLEAR_MASK, VALUE_MASK, and *_OFFSET)
as parameters to centralize: implement a generic helper (e.g.,
apply_ldst_abs_lo12_reloc) that accepts symbol, mem, offset, a closure or trait
to construct the ScaledOffset (ScaledOffset16/32/64/128::new), and the
CLEAR/VALUE/OFFSET constants, then have each specific function call that helper
with the appropriate ScaledOffset constructor and constants to remove the
duplicated unpack/clear/set/*bytes assignment logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38de3498-5db1-4acc-b6db-93a54261d20c
📒 Files selected for processing (5)
harm/src/instructions/ldst.rsharm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rsharm/src/reloc/movs.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- harm/src/reloc/claude_tests.rs
- harm/src/instructions/ldst.rs
- harm/src/reloc/movs.rs
monoid
force-pushed
the
feat/relocations
branch
3 times, most recently
from
f25a1ac to
4576689
Compare
monoid
force-pushed
the
feat/relocations
branch
from
4576689 to
7f449ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement applying relocation to a program memory.
It separates current memory location and the final memory location for better flexibility. For example, to produce static binary files allocating memory at arbitrary position.
Ref #31.
Summary by CodeRabbit