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-style Rel64 with a tagged struct and Rel64Tag, renames many relocation constructors to snake_case, centralizes relocation application via Rel64Tag::apply, adds relocator modules (addr, control, data, movs), updates instruction emitters/tests to new constructors, and adds extensive relocation unit tests. (43 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Rel64 (caller)
participant Tag as Rel64Tag
participant Reloc as Reloc Module (addr/control/data/movs)
participant Mem as Memory (byte slice)
participant Instr as Instruction (u32)
Caller->>Tag: Tag::apply(base, value/target, mem, offset)
Tag->>Reloc: dispatch to specific *_reloc(base, value, mem, offset)
Reloc->>Mem: get_bytes_mut(offset)
Reloc->>Instr: read instruction word
Reloc->>Instr: compute delta/immediate and patch bits
Reloc->>Mem: write patched bytes
Reloc-->>Tag: Result<(), Rel64Error>
Tag-->>Caller: propagate Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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
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
4 times, most recently
from
4576689 to
7f449ce
Compare
monoid
force-pushed
the
feat/relocations
branch
2 times, most recently
from
ac0c1a4 to
900c2ad
Compare
monoid
force-pushed
the
feat/relocations
branch
from
900c2ad to
0ab8f2f
Compare
Owner
Author
|
@coderabbitai please, review the recent changes. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harm/src/reloc.rs (1)
338-339: Add boundary regression coverage forABS32andABS16.The dispatcher path here is only exercised with a mid-range
ABS32value, andABS16has no visible success-path coverage. The spec-sensitive edge is the high unsigned boundary (0xffff_ffff/0xffff), so please pin that behavior in tests.✅ Suggested regression tests
#[test] fn test_abs32() { let mut mem = [0; 4]; Rel64Tag::ABS32.apply(0, 0x12345678, &mut mem, 0).unwrap(); assert_eq!(mem, 0x12345678u32.to_le_bytes()); } + + #[test] + fn test_abs32_high_unsigned_boundary() { + let mut mem = [0; 4]; + Rel64Tag::ABS32.apply(0, 0xffff_ffff, &mut mem, 0).unwrap(); + assert_eq!(mem, 0xffff_ffffu32.to_le_bytes()); + } + + #[test] + fn test_abs16_high_unsigned_boundary() { + let mut mem = [0; 2]; + Rel64Tag::ABS16.apply(0, 0xffff, &mut mem, 0).unwrap(); + assert_eq!(mem, 0xffffu16.to_le_bytes()); + }Also applies to: 467-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc.rs` around lines 338 - 339, Add explicit regression tests that exercise the Rel64Tag dispatcher branches for Rel64Tag::ABS32 and Rel64Tag::ABS16 using the high unsigned boundary values (0xffff_ffff for ABS32 and 0xffff for ABS16) so the abs32_reloc and abs16_reloc success paths are covered; locate the dispatcher in reloc.rs (the match arm mapping Rel64Tag::ABS32 => abs32_reloc(...) and Rel64Tag::ABS16 => abs16_reloc(...)) and write tests that construct the corresponding Reloc value/Reloc64Tag, call the relocation path with memory and offset fixtures, and assert the expected success behavior/outcome at those edge values.
🤖 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 338-339: Add explicit regression tests that exercise the Rel64Tag
dispatcher branches for Rel64Tag::ABS32 and Rel64Tag::ABS16 using the high
unsigned boundary values (0xffff_ffff for ABS32 and 0xffff for ABS16) so the
abs32_reloc and abs16_reloc success paths are covered; locate the dispatcher in
reloc.rs (the match arm mapping Rel64Tag::ABS32 => abs32_reloc(...) and
Rel64Tag::ABS16 => abs16_reloc(...)) and write tests that construct the
corresponding Reloc value/Reloc64Tag, call the relocation path with memory and
offset fixtures, and assert the expected success behavior/outcome at those edge
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81772391-dc77-43c6-a854-f13f43957920
⛔ Files ignored due to path filters (1)
aarchmrs-instructions/src/lib.rsis excluded by!aarchmrs-instructions/src/**
📒 Files selected for processing (7)
harm/src/instructions/ldst.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
✅ Files skipped from review due to trivial changes (2)
- harm/src/instructions/ldst.rs
- harm/src/reloc/control.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- harm/src/reloc/data.rs
- harm/src/reloc/claude_tests.rs
- harm/src/reloc/movs.rs
monoid
force-pushed
the
feat/relocations
branch
from
0ab8f2f to
47f395d
Compare
monoid
force-pushed
the
feat/relocations
branch
from
47f395d to
446a864
Compare
monoid
added
harm
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
harm/src/reloc.rs (1)
63-111: Normalize the acronym-based constructor names before this API settles.
ab_s64/pre_l64/pl_t32read like tokenization artifacts, while the rest of the helpers follow the relocation names directly. Since these are public constructors, I'd expose the conventional spellings (abs64,prel64,plt32, etc.) or at least keep them as aliases for ergonomics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc.rs` around lines 63 - 111, The public constructor names like ab_s64, ab_s32, ab_s16, pre_l64, pre_l32, pre_l16, and pl_t32 should be normalized to conventional spellings (abs64, abs32, abs16, prel64, prel32, prel16, plt32); add new pub const fn wrappers with those canonical names that return Self and delegate to the existing implementations (preserving #[inline] and the LabelRef signature), and keep the current odd-named functions for back-compatibility (optionally mark them #[deprecated] with a note pointing to the new name) so callers can migrate without breaking changes.
🤖 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/movs.rs`:
- Around line 159-167: The movw_prel_g3_reloc currently always delegates to
movw_uabs_g3_reloc which only rewrites imm16 and fails for negative deltas;
change movw_prel_g3_reloc to use signed MOV patching: compute delta via
calc_delta(base, value, offset) as now, then if delta is negative call the
signed MOV handler (e.g., movw_sabs_g3_reloc or the module's MOVN/MOVZ-path
function) with the signed delta, otherwise call movw_uabs_g3_reloc for
non‑negative deltas; ensure you pass the correctly-typed delta to the chosen
function and add a small regression test that uses a negative delta for
MOVW_PREL_G3 to prevent regressions.
---
Nitpick comments:
In `@harm/src/reloc.rs`:
- Around line 63-111: The public constructor names like ab_s64, ab_s32, ab_s16,
pre_l64, pre_l32, pre_l16, and pl_t32 should be normalized to conventional
spellings (abs64, abs32, abs16, prel64, prel32, prel16, plt32); add new pub
const fn wrappers with those canonical names that return Self and delegate to
the existing implementations (preserving #[inline] and the LabelRef signature),
and keep the current odd-named functions for back-compatibility (optionally mark
them #[deprecated] with a note pointing to the new name) so callers can migrate
without breaking changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 252d2a64-9cc6-446c-8156-e14c53fe1812
📒 Files selected for processing (7)
harm/src/instructions/dpimm/movewide/movewide_reloc.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
✅ Files skipped from review due to trivial changes (2)
- harm/src/reloc/control.rs
- harm/src/reloc/claude_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- harm/src/instructions/dpimm/movewide/movewide_reloc.rs
- harm/src/reloc/addr.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harm/src/reloc/claude_tests.rs (1)
729-748: Acknowledged: TODO comments document known ADRP overflow behavior.The tests at lines 729-747 assert
BitError::Unsupportedwith TODO comments noting this should ideally beBitError::Overflow. This is acceptable documentation of current behavior, though worth tracking for future improvement.🤖 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 729 - 748, Tests adrp_overflow_positive and adrp_overflow_negative currently assert Err(Rel64Error::InvalidBits(BitError::Unsupported)) even though the expected semantic error should be BitError::Overflow; keep the current assertions for now but make the intent explicit by updating the TODO comment to reference a tracking issue and clarify that adrp_prel_pg_hi21_reloc currently returns BitError::Unsupported (not Overflow) for the tested operands — change the comments in the two tests (adrp_overflow_positive, adrp_overflow_negative) to state the current behavior and include an issue/PR ID to fix adrp_prel_pg_hi21_reloc's error mapping in future.
🤖 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/claude_tests.rs`:
- Around line 729-748: Tests adrp_overflow_positive and adrp_overflow_negative
currently assert Err(Rel64Error::InvalidBits(BitError::Unsupported)) even though
the expected semantic error should be BitError::Overflow; keep the current
assertions for now but make the intent explicit by updating the TODO comment to
reference a tracking issue and clarify that adrp_prel_pg_hi21_reloc currently
returns BitError::Unsupported (not Overflow) for the tested operands — change
the comments in the two tests (adrp_overflow_positive, adrp_overflow_negative)
to state the current behavior and include an issue/PR ID to fix
adrp_prel_pg_hi21_reloc's error mapping in future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c1b3db3-49c6-47fb-b00e-5b9b3a31b9b0
📒 Files selected for processing (7)
harm/src/instructions/dpimm/movewide/movewide_reloc.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
🚧 Files skipped from review as they are similar to previous changes (1)
- harm/src/reloc/control.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harm/src/reloc/addr.rs (1)
149-231: LGTM!The scaled load/store relocations correctly apply alignment validation via the
ScaledOffset*types before encoding the immediate. TheUBitValue<12, N>constructors (per the bits.rs context) verify that the symbol's low 12 bits are properly aligned to the datum size (2/4/8/16 bytes), matching the ELF spec requirement that linkers check alignment.The four functions follow an identical pattern—consider a macro to reduce duplication if this module grows, but the current explicit implementation is clear and maintainable.
🤖 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 149 - 231, These four nearly identical functions (ldst16_abs_lo12_nc_reloc, ldst32_abs_lo12_nc_reloc, ldst64_abs_lo12_nc_reloc, ldst128_abs_lo12_nc_reloc) duplicate the same masking/unpacking logic; replace them with a small macro that parametrizes the ScaledOffset type (ScaledOffset16/32/64/128) and the LDST*_IMM12 constant names (CLEAR_WIDTH, VALUE_WIDTH, IMM12_OFFSET) and emits each function, reusing get_bytes_mut, InstructionCode::unpack/from_u32 and the same mask/insert pattern so the implementation is generated once and less error-prone.
🤖 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/addr.rs`:
- Around line 149-231: These four nearly identical functions
(ldst16_abs_lo12_nc_reloc, ldst32_abs_lo12_nc_reloc, ldst64_abs_lo12_nc_reloc,
ldst128_abs_lo12_nc_reloc) duplicate the same masking/unpacking logic; replace
them with a small macro that parametrizes the ScaledOffset type
(ScaledOffset16/32/64/128) and the LDST*_IMM12 constant names (CLEAR_WIDTH,
VALUE_WIDTH, IMM12_OFFSET) and emits each function, reusing get_bytes_mut,
InstructionCode::unpack/from_u32 and the same mask/insert pattern so the
implementation is generated once and less error-prone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05cf2563-6e59-45ef-81d3-ebc661f98245
📒 Files selected for processing (3)
harm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rs
✅ Files skipped from review due to trivial changes (1)
- harm/src/reloc/claude_tests.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
harm/src/reloc/claude_tests.rs (1)
729-747:⚠️ Potential issue | 🟡 MinorDon’t bake the known-wrong ADRP overflow error into the tests.
These assertions still require
BitError::Unsupported, so a real overflow fix will look like a regression. Either switch them toOverflowwith the implementation change or loosen the match until then.🤖 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 729 - 747, The tests adrp_overflow_positive and adrp_overflow_negative are asserting a specific BitError::Unsupported which will make a future correct overflow fix look like a regression; update the assertions against adr_prel_pg_hi21_reloc to not require Unsupported specifically — either assert Err(Rel64Error::InvalidBits(_)) or match both BitError::Unsupported and BitError::Overflow (or otherwise loosen the pattern) so the tests accept a real overflow error without failing; locate these tests by the function names adrp_overflow_positive, adrp_overflow_negative and the call to adr_prel_pg_hi21_reloc.
🧹 Nitpick comments (1)
harm/src/reloc/claude_tests.rs (1)
2312-2320: Make this a real backwardMOVW_PREL_G3regression test.With
base = 0andsymbol = 0x1000_0000_0000_0000, this is still a forward delta, so the backward/wrapping G3 path remains uncovered. Swapping the operands or renaming the test/comment would make the intent match the case.Based on learnings:
MOVW_PREL_G3intentionally uses the unsigned path, so the backward/wrapping case is the one worth pinning down.🤖 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 2312 - 2320, The test movw_prel_g3_backward currently uses base=0 and symbol=0x1000_0000_0000_0000 which is a forward delta; to exercise the backward/wrapping MOVW_PREL_G3 path swap the operands passed to movw_prel_g3_reloc (use base=0x1000_0000_0000_0000 and symbol=0) and update the expected immediate from 0x1000 to 0x0FFF (and adjust the test comment/name if desired) so the call movw_prel_g3_reloc(...), the MOVZ/MOVN handling in movw_prel_g3_reloc, and the dec_movw(i) assertion reflect the backward/wrapping case.
🤖 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/claude_tests.rs`:
- Around line 729-747: The tests adrp_overflow_positive and
adrp_overflow_negative are asserting a specific BitError::Unsupported which will
make a future correct overflow fix look like a regression; update the assertions
against adr_prel_pg_hi21_reloc to not require Unsupported specifically — either
assert Err(Rel64Error::InvalidBits(_)) or match both BitError::Unsupported and
BitError::Overflow (or otherwise loosen the pattern) so the tests accept a real
overflow error without failing; locate these tests by the function names
adrp_overflow_positive, adrp_overflow_negative and the call to
adr_prel_pg_hi21_reloc.
---
Nitpick comments:
In `@harm/src/reloc/claude_tests.rs`:
- Around line 2312-2320: The test movw_prel_g3_backward currently uses base=0
and symbol=0x1000_0000_0000_0000 which is a forward delta; to exercise the
backward/wrapping MOVW_PREL_G3 path swap the operands passed to
movw_prel_g3_reloc (use base=0x1000_0000_0000_0000 and symbol=0) and update the
expected immediate from 0x1000 to 0x0FFF (and adjust the test comment/name if
desired) so the call movw_prel_g3_reloc(...), the MOVZ/MOVN handling in
movw_prel_g3_reloc, and the dec_movw(i) assertion reflect the backward/wrapping
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 285b4fdb-9ea6-480d-ae32-c980e5e61850
📒 Files selected for processing (3)
harm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
harm/src/reloc/claude_tests.rs (1)
729-747:⚠️ Potential issue | 🟡 MinorDon’t re-freeze the wrong ADRP overflow error.
These two assertions still require
BitError::Unsupported, so a real fix inadr_prel_pg_hi21_relocwill read like a regression. PreferBitError::Overflow, or at least loosen the match until the implementation is corrected.🧪 Minimal test-side change
- Err(Rel64Error::InvalidBits(BitError::Unsupported)) + Err(Rel64Error::InvalidBits(_)) - Err(Rel64Error::InvalidBits(BitError::Unsupported)) + Err(Rel64Error::InvalidBits(_))🤖 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 729 - 747, The tests adrp_overflow_positive and adrp_overflow_negative assert the wrong BitError variant; update their expectations in the assertions that call adr_prel_pg_hi21_reloc to accept the correct overflow error (use Err(Rel64Error::InvalidBits(BitError::Overflow))) or loosen the match to Err(Rel64Error::InvalidBits(_)) until adr_prel_pg_hi21_reloc is fixed; locate the two tests by name (adrp_overflow_positive and adrp_overflow_negative) and replace the BitError::Unsupported match with BitError::Overflow or a wildcard.
🤖 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/claude_tests.rs`:
- Around line 2313-2320: The test movw_prel_g3_backward is misnamed and
exercises the forward (non-wrapping) path because movw_prel_g3_reloc(0x0,
0x1000_0000_0000_0000, ...) yields a positive value-place; either rename the
test to reflect the forward case or swap the first two args to
movw_prel_g3_reloc(0x1000_0000_0000_0000, 0x0, ...) so the value-place is
negative and triggers the wrapping/backward G3 branch, then update the assertion
to match the unsigned G3 encoding produced (check dec_movw(i) for expected
0x0FFF or the correct 16-bit chunk depending on chosen arguments) while keeping
the MOVZ/MOVK context in mind.
---
Duplicate comments:
In `@harm/src/reloc/claude_tests.rs`:
- Around line 729-747: The tests adrp_overflow_positive and
adrp_overflow_negative assert the wrong BitError variant; update their
expectations in the assertions that call adr_prel_pg_hi21_reloc to accept the
correct overflow error (use Err(Rel64Error::InvalidBits(BitError::Overflow))) or
loosen the match to Err(Rel64Error::InvalidBits(_)) until adr_prel_pg_hi21_reloc
is fixed; locate the two tests by name (adrp_overflow_positive and
adrp_overflow_negative) and replace the BitError::Unsupported match with
BitError::Overflow or a wildcard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a5b00b4-0af0-4348-9835-681c44ad6237
📒 Files selected for processing (4)
harm/src/reloc.rsharm/src/reloc/addr.rsharm/src/reloc/claude_tests.rsharm/src/reloc/control.rs
✅ Files skipped from review due to trivial changes (1)
- harm/src/reloc/addr.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- harm/src/reloc/control.rs
There was a problem hiding this comment.
🧹 Nitpick comments (3)
harm/src/reloc/claude_tests.rs (3)
734-746: Typo: "obsure" → "obscure".Minor spelling fix in the TODO comments.
✏️ Suggested fix
- // TODO Unsupported seems to have obsure semantics. + // TODO Unsupported seems to have obscure semantics.And similarly on line 744.
🤖 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 734 - 746, Fix the typo "obsure" → "obscure" in the TODO comment(s) inside the test function adrp_overflow_negative (and the earlier related TODO above it) so the comments read "Unsupported seems to have obscure semantics." Locate the TODO comments near the calls to adr_prel_pg_hi21_reloc and update both occurrences to use "obscure".
1395-1403: Comment doesn't match the actual test values.The comment references
-0x8000and0x7FFF, but the code tests-0x10000and expects0xFFFF. The test logic is correct; only the comment needs updating.✏️ Suggested fix
#[test] fn mov_w_abs_g0s_max_negative() { - // ~i16::MIN = −0x8000~; imm = ~(−0x8000)[15:0] = 0x7FFF + // −0x10000; imm = ~(−0x10000)[15:0] = 0xFFFF let mut mem = insn_buf(MOVZ); movw_sabs_g0_reloc(-0x10000, &mut mem, 0).unwrap();🤖 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 1395 - 1403, Update the incorrect explanatory comment in the test mov_w_abs_g0s_max_negative: it currently mentions −0x8000 and 0x7FFF but the test actually uses -0x10000 and expects 0xFFFF; edit the comment above the test (near mov_w_abs_g0s_max_negative, movw_sabs_g0_reloc, MOVZ, insn_buf, u32_at, movw_opc, dec_movw) to reflect the actual values used (-0x10000 and 0xFFFF) so the comment matches the test logic.
776-776: Typo: "Additionaly" → "Additionally".✏️ Suggested fix
-// Additionaly ADRP tests +// Additionally ADRP tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harm/src/reloc/claude_tests.rs` at line 776, Fix the typo in the comment string "Additionaly ADRP tests" by updating it to "Additionally ADRP tests" (look for the exact comment "Additionaly ADRP tests" in claude_tests.rs and correct the spelling to "Additionally").
🤖 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/claude_tests.rs`:
- Around line 734-746: Fix the typo "obsure" → "obscure" in the TODO comment(s)
inside the test function adrp_overflow_negative (and the earlier related TODO
above it) so the comments read "Unsupported seems to have obscure semantics."
Locate the TODO comments near the calls to adr_prel_pg_hi21_reloc and update
both occurrences to use "obscure".
- Around line 1395-1403: Update the incorrect explanatory comment in the test
mov_w_abs_g0s_max_negative: it currently mentions −0x8000 and 0x7FFF but the
test actually uses -0x10000 and expects 0xFFFF; edit the comment above the test
(near mov_w_abs_g0s_max_negative, movw_sabs_g0_reloc, MOVZ, insn_buf, u32_at,
movw_opc, dec_movw) to reflect the actual values used (-0x10000 and 0xFFFF) so
the comment matches the test logic.
- Line 776: Fix the typo in the comment string "Additionaly ADRP tests" by
updating it to "Additionally ADRP tests" (look for the exact comment
"Additionaly ADRP tests" in claude_tests.rs and correct the spelling to
"Additionally").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4179f31-396a-415c-8c17-063956a34848
📒 Files selected for processing (1)
harm/src/reloc/claude_tests.rs
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