Conversation
monoid
added
enhancement
📝 WalkthroughWalkthroughThis PR introduces the harm-runtime crate and extends relocation support across ARM64 instruction encoders. The relocation system (Rel64) now supports control flow, memory, and move-wide instruction types with label-based references, enabling multiple instruction variants to generate code with relocation metadata. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 8
🧹 Nitpick comments (6)
harm-runtime/src/lib.rs (1)
1-7: New crate root is minimal and functional.Consider adding a crate-level doc comment (
//!) to describe the purpose ofharm-runtime(dynamic assembler runtime with label/relocation management), which would improve discoverability in generated docs.harm-runtime/Cargo.toml (2)
15-15: Consider usingmemmap2instead ofmemmap.The
memmapcrate (0.7.0) is unmaintained. Thememmap2crate is the actively maintained fork with security fixes and API improvements.🔎 Suggested dependency update
[dependencies] harm = { workspace = true } -memmap = "0.7.0" +memmap2 = "0.9"
17-19: Remove empty sections for cleaner manifest.Empty
[dev-dependencies]and[features]sections can be removed without affecting functionality.🔎 Suggested cleanup
harm = { workspace = true } -memmap = "0.7.0" - -[dev-dependencies] - -[features] +memmap2 = "0.9"harm/src/reloc.rs (1)
50-60: TODO comment indicates incomplete work.The TODO on line 50 mentions
MOVWand someadd/ldst-related relocations. TheMovWAbsG*variants are already implemented below, so consider updating or removing this comment if they fulfill theMOVWrequirement.harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
388-483: Tests provide good coverage of relocation variants.The tests verify that:
- Generated instruction codes match expectations with zero immediates
- Correct
Rel64variants are produced for eachabs_g*functionNote: Test names
test_64_abs_g2s_movkandtest_64_abs_g2nc_movk(lines 451-471) actually testmovz, notmovk. Consider renaming for accuracy.harm/src/instructions/control/branch_imm.rs (1)
41-45: Prefer implementingFrominstead ofInto.Rust convention is to implement
From<T>rather thanInto<T>, asIntois automatically derived fromFrom. TheIntotrait is typically only implemented directly when the target type is from a foreign crate.Proposed fix
-impl Into<BitValue<4>> for BranchCond { - fn into(self) -> BitValue<4> { +impl From<BranchCond> for BitValue<4> { + fn from(cond: BranchCond) -> BitValue<4> { - BitValue::new_u32(self as u8 as u32) + BitValue::new_u32(cond as u8 as u32) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Cargo.tomlharm-runtime/Cargo.tomlharm-runtime/src/labels.rsharm-runtime/src/lib.rsharm-runtime/src/runtime.rsharm/src/instructions.rsharm/src/instructions/control/branch_imm.rsharm/src/instructions/dpimm/movewide.rsharm/src/instructions/dpimm/movewide/movewide_reloc.rsharm/src/instructions/ldst/ldr.rsharm/src/instructions/ldst/ldrsw.rsharm/src/instructions/ldst/macros.rsharm/src/instructions/mov/mov_imm.rsharm/src/reloc.rs
🧰 Additional context used
🧬 Code graph analysis (7)
harm/src/instructions/mov/mov_imm.rs (1)
harm/src/instructions/dpimm/movewide.rs (9)
movn(189-198)movz(214-223)new(57-57)new(74-80)new(90-96)new(106-112)new(121-127)new(137-143)new(153-159)
harm-runtime/src/runtime.rs (2)
harm/src/instructions.rs (1)
instructions(48-50)harm/src/instructions/control/branch_imm.rs (5)
new(275-275)new(285-291)new(302-308)new(317-323)new(333-339)
harm/src/instructions.rs (1)
harm/src/instructions/control/branch_imm.rs (3)
to_code_with_reloc(126-133)to_code_with_reloc(184-192)to_code_with_reloc(253-260)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (3)
harm/src/instructions.rs (1)
instructions(48-50)harm/src/instructions/dpimm/movewide.rs (16)
movk(164-173)movn(189-198)movz(214-223)new(57-57)new(74-80)new(90-96)new(106-112)new(121-127)new(137-143)new(153-159)to_code(176-178)to_code(182-184)to_code(201-203)to_code(207-209)to_code(226-228)to_code(232-234)harm/src/instructions/mov/mov_imm.rs (4)
to_code(88-94)to_code(99-104)to_code(247-253)to_code(258-263)
harm/src/instructions/dpimm/movewide.rs (1)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
new(326-331)
harm/src/instructions/ldst/macros.rs (1)
harm/src/instructions.rs (3)
instructions(48-50)to_code_with_reloc(23-23)to_code_with_reloc(30-32)
harm/src/instructions/control/branch_imm.rs (3)
harm/src/instructions.rs (4)
instructions(48-50)to_code_with_reloc(23-23)to_code_with_reloc(30-32)to_code(19-19)harm/src/instructions/control/branch_reg.rs (5)
to_code(39-41)to_code(48-50)to_code(62-64)new(26-28)reg(31-34)harm-runtime/src/runtime.rs (1)
new(23-25)
🪛 GitHub Actions: Rust
harm/src/reloc.rs
[error] 45-45: cargo fmt -- --check reported formatting issue: 'Call26(LabelRef), // same as Jump26 actually?' should be 'Call26(LabelRef), // same as Jump26 actually?'
harm-runtime/src/runtime.rs
[error] 7-7: cargo fmt -- --check reported formatting issue: import order changed between 'use harm::reloc::{Rel64, LabelId};' and 'use harm::reloc::{LabelId, Rel64};'
🔇 Additional comments (30)
harm/src/instructions/dpimm/movewide.rs (4)
10-11: New module structure for relocation support looks good.The addition of
movewide_relocas apub(crate)module with a wildcard re-export cleanly separates relocation-specific logic while maintaining API ergonomics.Also applies to: 20-20
29-33: Type-level tag pattern is appropriate for this use case.
MovWideTypeTag<X>acts as a phantom type tag enabling the sameMakeMovArgstrait to dispatch to different argument types (immediate vs relocation). TheSealedbound onXcorrectly restricts instantiation to internal types.
69-80: MakeMovArgs implementation correctly updated for MovWideTypeTag indirection.The trait implementation now routes through
MovWideTypeTag<MovImmArgs<RegOrZero32>>while returningUnfallible<MovImmArgs<RegOrZero32>>, maintaining the same effective behavior with added type flexibility.
164-173: Genericmovkfunction signature enables relocation-aware variants.The function is now generic over
Args, usingMovWideTypeTag<Args>as the dispatch mechanism. TheOutcome<Inner = Args>constraint ensures type consistency between the tag and the wrapped result.harm/src/instructions/ldst/ldrsw.rs (1)
160-167: PC-relative relocation correctly specified for LDRSW.The
Rel64::LdPrelLo19relocation type is appropriate for the 19-bit signed PC-relative offset used by LDRSW literal loads.Cargo.toml (1)
9-9: Workspace member added correctly.Note that
harm-runtimeis not added to[workspace.dependencies]. This is fine if other workspace members won't depend on it yet, but consider adding it for consistency when inter-crate dependencies are needed.harm/src/instructions/ldst/ldr.rs (1)
168-183: PC-relative relocations correctly applied to both LDR variants.Both 32-bit and 64-bit LDR literal forms use
Rel64::LdPrelLo19, which is correct—the relocation type encodes the 19-bit PC-relative offset field, independent of the load width.harm/src/instructions/mov/mov_imm.rs (3)
10-10: Import updated to include MovWideTypeTag.The import correctly brings in
MovWideTypeTagalongside the existing move-wide types.
145-147: 32-bit move construction updated to use MovWideTypeTag pathway.The fully-qualified trait method call correctly routes through the new type-tagged implementation while preserving the original logic flow.
Also applies to: 156-158
225-227: 64-bit move construction follows the same pattern consistently.The changes mirror the 32-bit variant, maintaining consistency across the codebase.
Also applies to: 236-238
harm/src/instructions.rs (3)
8-8: Relocation type updated to Rel64.The import change from
ReloctoRel64aligns with the expanded relocation system introduced in this PR.
22-24: RelocatableInstruction trait updated with Rel64 return type.The trait and blanket implementation correctly propagate the new relocation type.
Also applies to: 30-32
44-45: InstructionSeq encode() signatures updated consistently.Both the trait definition and the blanket implementation return
Option<Rel64>, maintaining type consistency throughout the instruction encoding pipeline.Also applies to: 63-66
harm-runtime/Cargo.toml (1)
4-4: Verify Rust edition 2024 compatibility with CI/MSRV.Edition
2024requires Rust 1.85 (released Feb 2025). Update your CI configuration and project's minimum supported Rust version (MSRV) to 1.85 or higher if not already set.harm/src/reloc.rs (1)
1-24: LGTM on type definitions.The
LabelId,Offset,Addr, andLabelReftypes are well-designed for the relocation system. Usingi64forOffsetaligns with the comment about sign-extension to 64 bits.harm/src/instructions/ldst/macros.rs (3)
342-343: Macro signature extended for relocation support.The addition of
$rel:exprparameter cleanly enables relocation-aware PC-offset handling. This is a good design that maintains backward compatibility while adding the new capability.
382-399: New LabelRef trait implementation looks correct.The generated
impl<Rt> $trait_name<Rt, $crate::reloc::LabelRef>follows the same pattern as the existing PC-offset implementations, correctly storing the label reference in theaddrfield.
414-421: RelocatableInstruction implementation is well-structured.The
to_code_with_relocimplementation correctly:
- Generates instruction code with a zero offset placeholder
- Invokes the relocation expression
$rel(self.addr)to produce theRel64variant- Returns both as a tuple with
Some(rel)This aligns with the
RelocatableInstructiontrait signature fromharm/src/instructions.rs.harm-runtime/src/labels.rs (2)
10-15: LabelInfo enum design is appropriate.The
ForwardandOffsetvariants cleanly represent the two states of a label during assembly. The TODO comment about segments indicates future extensibility.
77-83:name_labelallows aliasing a label with multiple names.This is likely intentional, but note that the reverse lookup (name → id) will only remember the most recently inserted name for that id in terms of which name was used. If multiple names point to the same id, that's fine, but there's no way to enumerate all names for a given label.
harm-runtime/src/runtime.rs (2)
36-44:insertmethod implementation is correct.The method properly iterates over encoded instructions, records relocations by position, and appends instructions. The position-based relocation tracking using
self.instructions.len()before push is the correct approach.
46-69: Multipletodo!()placeholders indicate incomplete implementation.The label management methods (
current_label,new_forward_label,new_forward_named_label,assign_forward_label) are stubs. Note thatcurrent_named_labelon lines 53-57 will panic at runtime because it callsnew_forward_named_labelandassign_forward_label, both of which containtodo!().Consider either:
- Marking these methods as unimplemented/unstable in documentation
- Implementing them before merging
- Making them
#[doc(hidden)]or feature-gated if this is intentional WIPharm/src/instructions/dpimm/movewide/movewide_reloc.rs (3)
20-29: AbsG0 and factory function are well-designed.The
#[repr(transparent)]ensures ABI compatibility, and the inline factory function provides a clean API.
141-154:MovRelArgsand relocation traits are well-structured.The
MoveWideReloc64andMoveWideReloc32traits withSealedconstraint provide a clean type-level API. The trait hierarchy (MoveWideReloc32: MoveWideReloc64) correctly reflects that 32-bit relocations are a subset of 64-bit ones.
334-386: RelocatableInstruction implementations are consistent.All six implementations (MovZ/MovK/MovN × 64/32-bit) follow the same pattern:
- Destructure
MovRelArgsto get register and relocation- Generate instruction code with default immediate and appropriate shift
- Extract the
Rel64relocation variant- Return the tuple
This is clean and maintainable.
harm/src/instructions/control/branch_imm.rs (5)
115-134: Branch relocation forLabelRefis correctly implemented.The
MakeBranch<LabelRef>andRelocatableInstructionimplementations properly:
- Store the label reference
- Generate instruction with zero offset placeholder
- Emit
Rel64::Jump26relocation
173-192: Conditional branch relocation is correct.The
CondBr19relocation type is appropriate for the 19-bit conditional branch offset field.
242-261: BranchLink relocation correctly usesCall26.The implementation mirrors
Branch<LabelRef>but usesCall26instead ofJump26, which is the correct distinction forBLvsBinstructions.
269-270:CompareBranchhasSealedforLabelRefbut noRelocatableInstructionimpl.Line 270 adds
impl<Reg> Sealed for CompareBranch<Reg, LabelRef> {}, indicating intent to support label-based compare-and-branch instructions (CBZ/CBNZ). However, there's no correspondingMakeCompareBranch<*, LabelRef>orRelocatableInstructionimplementation.Is this intentional incomplete work, or should these implementations be added? The relocation type would likely be
TstBr14as defined inRel64.
368-386:cbzandcbnzfunction signatures updated correctly.The generic
OffsetDstparameter allows future support forLabelRefonce theMakeCompareBranchimplementations are added.
harm-runtime/src/labels.rs
Outdated
| #[inline] | ||
| pub fn define_named_label(&mut self, name: &str, offset: Offset) -> LabelId { | ||
| if let Some(id) = self.named_labels.get(name).copied() { | ||
| self.labels.insert(id, LabelInfo::Offset(offset)); | ||
| id | ||
| } else { | ||
| let id = self.next_label(); | ||
| self.named_labels.insert(name.to_string(), id); | ||
| self.labels.insert(id, LabelInfo::Offset(offset)); | ||
| id | ||
| } | ||
| } |
There was a problem hiding this comment.
define_named_label may silently overwrite an already-defined label.
Unlike define_label, this method doesn't check if the label already has an Offset before overwriting. If a named label was previously defined with an offset (not just forward-declared), this will silently redefine it, which could mask programming errors.
Proposed fix
#[inline]
pub fn define_named_label(&mut self, name: &str, offset: Offset) -> LabelId {
if let Some(id) = self.named_labels.get(name).copied() {
+ if let Some(LabelInfo::Offset(_)) = self.labels.get(&id) {
+ panic!("Named label {name:?} is already defined");
+ }
self.labels.insert(id, LabelInfo::Offset(offset));
id
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[inline] | |
| pub fn define_named_label(&mut self, name: &str, offset: Offset) -> LabelId { | |
| if let Some(id) = self.named_labels.get(name).copied() { | |
| self.labels.insert(id, LabelInfo::Offset(offset)); | |
| id | |
| } else { | |
| let id = self.next_label(); | |
| self.named_labels.insert(name.to_string(), id); | |
| self.labels.insert(id, LabelInfo::Offset(offset)); | |
| id | |
| } | |
| } | |
| #[inline] | |
| pub fn define_named_label(&mut self, name: &str, offset: Offset) -> LabelId { | |
| if let Some(id) = self.named_labels.get(name).copied() { | |
| if let Some(LabelInfo::Offset(_)) = self.labels.get(&id) { | |
| panic!("Named label {name:?} is already defined"); | |
| } | |
| self.labels.insert(id, LabelInfo::Offset(offset)); | |
| id | |
| } else { | |
| let id = self.next_label(); | |
| self.named_labels.insert(name.to_string(), id); | |
| self.labels.insert(id, LabelInfo::Offset(offset)); | |
| id | |
| } | |
| } |
🤖 Prompt for AI Agents
In harm-runtime/src/labels.rs around lines 64 to 75, define_named_label
currently overwrites an existing label's Offset without checking; update it to
first check the current LabelInfo for the found id and if it's already
LabelInfo::Offset, fail fast (panic or return an Err consistent with the crate's
error handling) instead of silently overwriting; only insert the Offset when the
existing entry is a forward-declared placeholder (or absent), and preserve
existing behavior for creating new ids when the name is not present.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
harm/src/reloc.rs (1)
48-48: Fix formatting: extra space before comment.There's an extra space before the comment marker. This was already flagged in a previous review.
🧹 Nitpick comments (1)
harm/src/reloc.rs (1)
25-61: Consider addingCopyderive and documentation.Since all variants of
Rel64contain onlyCopytypes (Noneis zero-sized, andLabelRefisCopy), adding#[derive(Copy)]would enable more ergonomic usage patterns.Additionally, consider adding documentation comments for the public
Rel64enum and its variants. AArch64 relocation types have specific meanings and use cases that would benefit from brief explanations, especially for variants likeNc(no-check) andS(signed) suffixes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harm-runtime/src/runtime.rsharm/src/reloc.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- harm-runtime/src/runtime.rs
🔇 Additional comments (3)
harm/src/reloc.rs (3)
8-10: LGTM!The
LabelIdnewtype is well-designed with appropriate derives andrepr(transparent).
12-16: LGTM!The type aliases are clear, and the comment explaining why
Offsetisi64provides helpful context.
18-23: LGTM!The
LabelRefstructure appropriately represents label references with offsets.
| Jump26(LabelRef), | ||
| Call26(LabelRef), // same as Jump26 actually? | ||
|
|
||
| // TODO `MOVW` and some `add`/`ldst`-related relocations |
There was a problem hiding this comment.
Clarify or remove the TODO comment.
The TODO mentions "MOVW" relocations, but MovW variants are implemented immediately below (lines 51-60). If additional MOVW or add/ldst-related relocations are still needed, please clarify which ones; otherwise, consider removing or updating this comment to reflect what's been completed.
🤖 Prompt for AI Agents
In harm/src/reloc.rs around line 50, the TODO commenting that "`MOVW` and some
`add`/`ldst`-related relocations" are missing is stale because MovW variants are
implemented directly below; either remove the TODO or update it to be specific:
if there are remaining relocations to implement, list the exact relocation names
(e.g., which add/ldst forms or encodings) and/or reference an issue/PR number
and expected work; otherwise delete the comment so it no longer misleads
readers. Ensure the updated comment clearly states remaining tasks (if any) or
is removed to reflect current implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
58-58: Typos remain unfixed from previous review.The following typos flagged in previous reviews are still present:
- Line 58: "unsinged" should be "unsigned"
- Line 80: "singed" should be "signed"
- Line 91: "unsinged" should be "unsigned"
- Line 113: "singed" should be "signed"
Please correct these spelling errors in the documentation comments.
Also applies to: 80-80, 91-91, 113-113
🧹 Nitpick comments (2)
harm/src/instructions/control/branch_imm.rs (1)
41-45: Consider simplifying the double cast.The conversion
cond as u8 as u32works correctly but can be simplified tocond as u32since Rust allows direct casting fromu8(the repr type) tou32.🔎 Proposed simplification
impl From<BranchCond> for BitValue<4> { fn from(cond: BranchCond) -> Self { - Self::new_u32(cond as u8 as u32) + Self::new_u32(cond as u32) } }harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
159-277: Consider using.expect()instead of.unwrap()for hardcoded shift values.While the hardcoded shift values (0, 16, 32, 48) should never fail, using
.expect("shift value must be valid")would make debugging easier if the assumption is ever violated and would better document the invariant.Example refactor for one implementation
impl MoveWideReloc64 for AbsG0 { #[inline] fn get_shift64(&self) -> Shift64 { - Shift64::new(0).unwrap() + Shift64::new(0).expect("shift value 0 must be valid") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
harm-runtime/Cargo.tomlharm/src/instructions/control/branch_imm.rsharm/src/instructions/dpimm/movewide/movewide_reloc.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- harm-runtime/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (2)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (2)
harm/src/instructions/dpimm/movewide.rs (16)
movk(164-173)movn(189-198)movz(214-223)new(57-57)new(74-80)new(90-96)new(106-112)new(121-127)new(137-143)new(153-159)to_code(176-178)to_code(182-184)to_code(201-203)to_code(207-209)to_code(226-228)to_code(232-234)harm/src/instructions/mov/mov_imm.rs (4)
to_code(88-94)to_code(99-104)to_code(247-253)to_code(258-263)
harm/src/instructions/control/branch_imm.rs (1)
harm/src/instructions.rs (4)
instructions(48-50)to_code_with_reloc(23-23)to_code_with_reloc(30-32)to_code(19-19)
🔇 Additional comments (13)
harm/src/instructions/control/branch_imm.rs (9)
115-134: LGTM! Label-based unconditional branch implementation is correct.The implementation correctly emits a zero offset placeholder in the instruction code and returns a
Jump26relocation record for later resolution.
169-171: LGTM! Good use of the new conversion trait.The updated code properly uses
cond.into()to leverage the newFrom<BranchCond>trait, making the code more idiomatic.
173-193: LGTM! Conditional branch with label reference is correctly implemented.The implementation properly emits a zero offset placeholder and returns a
CondBr19relocation record for conditional branches to labels.
202-207: LGTM! Generic signature properly supports both offset types.The refactored function signature allows
blto work with both immediate offsets and label references while maintaining type safety.
210-213: LGTM! Generic BranchLink structure is well designed.The use of the sealed trait pattern for both
BranchOffsetandLabelRefproperly constrains the API while enabling both immediate and label-based variants.
242-261: LGTM! Branch-and-link with label reference is correctly implemented.The implementation properly uses
Call26relocation type for function call semantics and emits the appropriate zero offset placeholder.
278-340: LGTM! Compare-branch trait implementations are complete and correct.The implementations properly handle both 32-bit and 64-bit register variants with support for direct offsets and
i32conversions.
342-366: LGTM! Instruction encoding for compare-branch is correct.Both 32-bit and 64-bit variants properly dispatch to the correct CBZ/CBNZ instruction encodings based on the
equalfield.
368-386: LGTM! Generic signatures are well designed.The generic signatures for
cbzandcbnzproperly support multiple register and offset types through trait bounds. Note that label-based offsets cannot be used until the missing implementations flagged above (Lines 263-270) are completed.harm/src/instructions/dpimm/movewide/movewide_reloc.rs (4)
130-157: LGTM!The trait hierarchy and Sealed pattern are well-designed. The
MoveWideReloc32: MoveWideReloc64inheritance correctly ensures 32-bit relocations can provide both shift types.
279-319: LGTM!Correctly implements only AbsG0 and AbsG1 variants for 32-bit relocations, since 32-bit addresses only require two 16-bit nibbles.
321-345: LGTM!The
MakeMovArgsimplementations correctly useUnfalliblewrappers since relocation-based instructions cannot fail at construction time (the actual address computation happens at link time).
347-399: LGTM!The
RelocatableInstructionimplementations correctly useMoveImm16::default()as a placeholder since the linker will patch the actual immediate value during relocation. The pattern is consistent across all instruction variants.
| #[test] | ||
| fn test_64_abs_g2s_movk() { | ||
| let label = LabelRef { | ||
| id: LabelId(6), | ||
| addend: 47, | ||
| }; | ||
| let inst = movz(X6, abs_g2_s(label)).to_code_with_reloc(); | ||
| assert_eq!(inst.0, movz(X6, (0, 32)).unwrap().to_code()); | ||
| assert_eq!(inst.1, Some(Rel64::MovWAbsG2S(label))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_64_abs_g2nc_movk() { | ||
| let label = LabelRef { | ||
| id: LabelId(7), | ||
| addend: 48, | ||
| }; | ||
| let inst = movz(X7, abs_g2_nc(label)).to_code_with_reloc(); | ||
| assert_eq!(inst.0, movz(X7, (0, 32)).unwrap().to_code()); | ||
| assert_eq!(inst.1, Some(Rel64::MovWAbsG2Nc(label))); | ||
| } |
There was a problem hiding this comment.
Test names don't match the instructions being tested.
Lines 466 and 477 have naming inconsistencies:
test_64_abs_g2s_movkusesmovzbut the name saysmovktest_64_abs_g2nc_movkusesmovzbut the name saysmovk
These should be renamed to test_64_abs_g2s_movz and test_64_abs_g2nc_movz respectively for clarity.
Proposed fix
#[test]
- fn test_64_abs_g2s_movk() {
+ fn test_64_abs_g2s_movz() {
let label = LabelRef {
id: LabelId(6),
addend: 47,
};
let inst = movz(X6, abs_g2_s(label)).to_code_with_reloc();
assert_eq!(inst.0, movz(X6, (0, 32)).unwrap().to_code());
assert_eq!(inst.1, Some(Rel64::MovWAbsG2S(label)));
}
#[test]
- fn test_64_abs_g2nc_movk() {
+ fn test_64_abs_g2nc_movz() {
let label = LabelRef {
id: LabelId(7),
addend: 48,
};
let inst = movz(X7, abs_g2_nc(label)).to_code_with_reloc();
assert_eq!(inst.0, movz(X7, (0, 32)).unwrap().to_code());
assert_eq!(inst.1, Some(Rel64::MovWAbsG2Nc(label)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_64_abs_g2s_movk() { | |
| let label = LabelRef { | |
| id: LabelId(6), | |
| addend: 47, | |
| }; | |
| let inst = movz(X6, abs_g2_s(label)).to_code_with_reloc(); | |
| assert_eq!(inst.0, movz(X6, (0, 32)).unwrap().to_code()); | |
| assert_eq!(inst.1, Some(Rel64::MovWAbsG2S(label))); | |
| } | |
| #[test] | |
| fn test_64_abs_g2nc_movk() { | |
| let label = LabelRef { | |
| id: LabelId(7), | |
| addend: 48, | |
| }; | |
| let inst = movz(X7, abs_g2_nc(label)).to_code_with_reloc(); | |
| assert_eq!(inst.0, movz(X7, (0, 32)).unwrap().to_code()); | |
| assert_eq!(inst.1, Some(Rel64::MovWAbsG2Nc(label))); | |
| } | |
| #[test] | |
| fn test_64_abs_g2s_movz() { | |
| let label = LabelRef { | |
| id: LabelId(6), | |
| addend: 47, | |
| }; | |
| let inst = movz(X6, abs_g2_s(label)).to_code_with_reloc(); | |
| assert_eq!(inst.0, movz(X6, (0, 32)).unwrap().to_code()); | |
| assert_eq!(inst.1, Some(Rel64::MovWAbsG2S(label))); | |
| } | |
| #[test] | |
| fn test_64_abs_g2nc_movz() { | |
| let label = LabelRef { | |
| id: LabelId(7), | |
| addend: 48, | |
| }; | |
| let inst = movz(X7, abs_g2_nc(label)).to_code_with_reloc(); | |
| assert_eq!(inst.0, movz(X7, (0, 32)).unwrap().to_code()); | |
| assert_eq!(inst.1, Some(Rel64::MovWAbsG2Nc(label))); | |
| } |
🤖 Prompt for AI Agents
In harm/src/instructions/dpimm/movewide/movewide_reloc.rs around lines 465 to
485, the two test function names are inconsistent with the instruction used:
rename test_64_abs_g2s_movk to test_64_abs_g2s_movz and test_64_abs_g2nc_movk to
test_64_abs_g2nc_movz so the function names reflect that they call movz; update
the two fn identifiers only (no logic changes) to keep names and tested
instructions consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/instructions/control/branch_imm.rs
🧰 Additional context used
🧬 Code graph analysis (1)
harm/src/instructions/control/branch_imm.rs (3)
harm/src/instructions.rs (4)
instructions(48-50)to_code_with_reloc(23-23)to_code_with_reloc(30-32)to_code(19-19)harm-types/src/A64/register.rs (4)
from(169-171)from(176-178)from(330-332)from(337-339)harm/src/instructions/control/branch_reg.rs (4)
to_code(39-41)to_code(48-50)to_code(62-64)reg(31-34)
🔇 Additional comments (13)
harm/src/instructions/control/branch_imm.rs (13)
10-10: LGTM! Imports support the new relocation functionality.The added imports (
BitValue,RelocatableInstruction,LabelRef,Rel64) are all necessary for implementing label-based branch instructions and relocations.Also applies to: 14-14, 16-16
41-45: LGTM! Branch condition conversion is correct.The
From<BranchCond> for BitValue<4>implementation correctly converts the 4-bit condition codes toBitValue<4>.
115-134: LGTM! Unconditional branch with label reference is correctly implemented.The implementation properly creates a
Branch<LabelRef>and emits a zero-offset instruction with aJump26relocation, following the standard relocation pattern.
169-171: LGTM! Uses the newInto<BitValue<4>>conversion.The change to use
cond.into()is cleaner and leverages the newly implementedFrom<BranchCond> for BitValue<4>trait.
173-193: LGTM! Conditional branch with label reference is correctly implemented.The implementation properly creates conditional branches with label references and uses
CondBr19relocation, which is appropriate for the 19-bit conditional branch offset field.
202-213: LGTM! Branch-and-link generics follow the established pattern.Making
BranchLinkgeneric overOffsetand updating theblfunction signature is consistent with the approach used forBranchandb.
215-261: LGTM! Branch-and-link implementations are complete and correct.All
BranchLinkimplementations follow established patterns:
- Direct offset construction
- Feature-gated
i32convenience conversionLabelRefsupport withCall26relocation (correct for branch-and-link instructions)
263-270: LGTM! Generic structure enables label reference support.Making
CompareBranchgeneric overOffsetenables the necessary flexibility for both immediate offsets and label references.
278-324: LGTM! 64-bit compare-branch implementations are complete.The
MakeCompareBranchimplementations forRegOrZero64correctly handle:
- Direct
CompareBranchOffsetconstruction- Fallible
i32conversion- Label reference support
This addresses the past review comment about incomplete implementation for label references.
326-370: LGTM! 32-bit compare-branch implementations mirror the 64-bit variants.The implementations for
RegOrZero32follow the same correct pattern as the 64-bit variants, providing complete coverage for compare-branch operations with label references.
372-383: LGTM! Raw instruction implementations correctly emit CBZ/CBNZ.The
RawInstructionimplementations properly select between CBZ (equal) and CBNZ (not equal) variants for both 64-bit and 32-bit registers.Also applies to: 403-414
434-452: LGTM! Generic signatures enable flexible compare-branch API.The generic signatures for
cbzandcbnzenable the functions to work with various register types (32/64-bit) and offset types (immediate, i32, LabelRef) while maintaining type safety.
705-755: Test coverage is comprehensive, but may need updates if relocation type changes.The tests thoroughly cover label-based relocations for all four CBZ/CBNZ variants (64/32-bit, zero/nonzero). They correctly verify that:
- Instructions generate machine code matching the zero-offset variant
- Relocations are properly emitted
Note: If the relocation type for CBZ/CBNZ changes from
TstBr14to a 19-bit type (per the previous comment), these test assertions will need to be updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
401-542: Consider adding test coverage for G0 Nc/S variants (optional).The test suite provides good coverage of the relocation types and instructions. For completeness, you could add tests for
abs_g0_ncandabs_g0_svariants (both 32-bit and 64-bit paths), though these are less common in practice.Example test additions
#[test] fn test_64_abs_g0nc_movk() { let label = LabelRef { id: LabelId(9), addend: 50, }; let inst = movk(X9, abs_g0_nc(label)).to_code_with_reloc(); assert_eq!(inst.0, movk(X9, (0, 0)).unwrap().to_code()); assert_eq!(inst.1, Some(Rel64::MovWAbsG0Nc(label))); } #[test] fn test_64_abs_g0s_movn() { let label = LabelRef { id: LabelId(10), addend: 51, }; let inst = movn(X10, abs_g0_s(label)).to_code_with_reloc(); assert_eq!(inst.0, movn(X10, (0, 0)).unwrap().to_code()); assert_eq!(inst.1, Some(Rel64::MovWAbsG0S(label))); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harm/src/instructions/control/branch_imm.rsharm/src/instructions/dpimm/movewide/movewide_reloc.rs
🔇 Additional comments (14)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (7)
20-128: LGTM! Clean relocation type definitions.The AbsG* struct definitions and their constructor functions follow a consistent, idiomatic pattern. The documentation accurately describes the bit ranges and overflow semantics for each nibble (G0 through G3), and all the typos from previous reviews have been properly addressed.
130-157: LGTM! Well-designed trait hierarchy.The trait design cleanly separates 32-bit and 64-bit relocation paths using the
MoveWideReloc32extension ofMoveWideReloc64. TheSealedtrait pattern provides good encapsulation, andMovRelArgseffectively bundles the destination register with the relocation descriptor.
159-277: LGTM! Correct shift and relocation calculations.All ten
MoveWideReloc64implementations correctly calculate the bit shifts (0, 16, 32, 48) corresponding to the four 16-bit nibbles of a 64-bit address. The relocation enum variants are consistently mapped, and the use of.unwrap()on compile-time constant shifts is safe.
279-319: LGTM! Correct 32-bit relocation subset.The
MoveWideReloc32implementations correctly cover only the G0 and G1 nibbles (bits [31:0]), which is appropriate for 32-bit addressing. G2 and G3 variants correctly do not implement this trait since they reference bits beyond the 32-bit address space.
321-345: LGTM! Type-safe argument construction.The
MakeMovArgsimplementations provide clean type-level dispatch for 32-bit vs 64-bit register paths. The use ofUnfallibleappropriately signals that these conversions cannot fail, and the trait bounds ensure compile-time correctness.
347-399: LGTM! Consistent relocatable instruction encoding.All six
RelocatableInstructionimplementations follow a consistent, correct pattern: they generate instruction code with a placeholder immediate and the appropriate shift, then return the code paired with the relocation metadata. The separation of 64-bit and 32-bit paths via trait bounds ensures type safety.
477-485: Clarification: test name is actually correct.The past review comment suggested renaming
test_64_abs_g2nc_movktotest_64_abs_g2nc_movz, but this was incorrect. Line 482 usesmovk(X7, abs_g2_nc(label)), so the current function name accurately reflects the instruction being tested.harm/src/instructions/control/branch_imm.rs (7)
41-45: LGTM!The conversion from
BranchCondtoBitValue<4>is correct. All enum values fit within 4 bits, and the cast chain is safe.
115-134: LGTM!The label-based relocation support for unconditional branch is correctly implemented:
Jump26is the appropriate relocation type for the 26-bit immediate field- Zero offset placeholder is correctly used in the instruction encoding
173-193: LGTM!The conditional branch relocation implementation is correct:
CondBr19is the appropriate relocation type for conditional branches with 19-bit immediate fields- The
cond.into()conversion uses the newly addedFromimplementation
242-261: LGTM!The branch-with-link relocation implementation is correct:
Call26is the appropriate relocation type for BL instructions- The generic structure allows both offset-based and label-based calls
434-452: LGTM!The generic function signatures for
cbzandcbnzare well-designed:
- Type inference allows callers to use either immediate offsets or label references
- The trait bounds ensure type safety while providing flexibility
705-755: LGTM!Comprehensive test coverage for label-based CBZ/CBNZ relocations:
- All register width variants (32-bit and 64-bit) are tested
- Tests verify both instruction encoding and relocation generation
- Different label IDs and addends are used to ensure uniqueness
385-432: Code is correct; no separate compare-branch relocation type exists.The
Rel64enum contains only a single 19-bit relocation type (CondBr19) for all instructions with 19-bit immediate fields (conditional branches, compare-and-branch, etc.). The test-and-branch instructions use the distinctTstBr14type. Since CBZ/CBNZ and B.cond share identical 19-bit encoding in ARM64, reusingCondBr19for both is the intended design.
Implement typetag pattern for `MovImmArgs` and `MovRelArgs`.
monoid
force-pushed
the
harm/relocations
branch
from
446c581 to
8d57860
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
harm/src/reloc.rs (1)
50-60: TODO comment is stale —MovWvariants are implemented.The comment on line 50 mentions "
MOVWand someadd/ldst-related relocations" as TODO, butMovWAbsG0throughMovWAbsG3(and theirNc/Svariants) are already implemented directly below. Consider updating the TODO to specify only the remainingadd/ldstrelocations, or remove it if all intended work is complete.harm-runtime/src/labels.rs (2)
49-62: Replacetodo!()withpanic!()for consistent error handling.Line 56 uses
todo!()for the already-defined label case, which will panic with a less informative message. For consistency with the panic on line 60 and clearer runtime diagnostics, usepanic!()directly.Proposed fix
LabelInfo::Offset(_) => { - todo!("Label {id:?} is already defined"); + panic!("Label {id:?} is already defined"); }
64-75:define_named_labelmay silently overwrite an already-defined label.Unlike
define_label, this method doesn't check if the label already has anOffsetbefore overwriting. If a named label was previously defined (not just forward-declared), this will silently redefine it, potentially masking programming errors.Proposed fix
pub fn define_named_label(&mut self, name: &str, offset: Offset) -> LabelId { if let Some(id) = self.named_labels.get(name).copied() { + if let Some(LabelInfo::Offset(_)) = self.labels.get(&id) { + panic!("Named label {name:?} is already defined"); + } self.labels.insert(id, LabelInfo::Offset(offset)); id } else {
🧹 Nitpick comments (6)
harm-runtime/src/labels.rs (1)
77-83:name_labelmay overwrite an existing name mapping without warning.If the same name is already associated with a different
LabelId, this will silently replace the mapping. Consider checking for existing name conflicts to prevent accidental aliasing bugs.Proposed fix
pub fn name_label(&mut self, id: LabelId, name: &str) { if self.labels.contains_key(&id) { + if let Some(&existing_id) = self.named_labels.get(name) { + if existing_id != id { + panic!("Name {name:?} is already assigned to label {existing_id:?}"); + } + } self.named_labels.insert(name.to_string(), id); } else { panic!("Label {id:?} is not registered"); } }harm-runtime/src/runtime.rs (1)
27-34: Consider pre-allocatingrelocationsinwith_capacity.
with_capacitypre-allocatesinstructionsbut notrelocations. If the caller expects many relocations, consider accepting a second capacity hint or usingHashMap::with_capacitywith a reasonable fraction ofcap.harm/src/instructions/dpimm/movewide.rs (2)
10-20: Consider explicit re-exports instead of wildcard.The wildcard re-export
pub use self::movewide_reloc::*;makes it unclear which items are being exposed publicly. This can make it harder for users to understand the API surface and may lead to unintended exports if the module is extended later.🔎 Alternative approach with explicit re-exports
-pub use self::movewide_reloc::*; +pub use self::movewide_reloc::{ + abs_g0, abs_g0_nc, abs_g0_s, abs_g1, abs_g1_nc, abs_g1_s, + abs_g2, abs_g2_nc, abs_g2_s, abs_g3, MovRelArgs +};This makes the public API explicit and easier to understand.
29-33: Expand documentation for the type-level tag pattern.The documentation briefly states that
MovWideTypeTagis "never constructed," but doesn't explain the design rationale or how it enables the relocation functionality. Users encountering this type in error messages or documentation may be confused about its purpose.🔎 Suggested documentation improvement
-/// This is a technical type used for type constraints. It is never constructed, but only used in trait definitions -/// and constraints. +/// A type-level tag used to enable type-safe dispatch for move-wide instruction arguments. +/// +/// This zero-sized type is never instantiated at runtime but serves as a marker in trait bounds +/// to allow the same public API (`movk`, `movn`, `movz`) to work with both immediate values +/// (`MovImmArgs`) and relocatable label references (`MovRelArgs`). The generic parameter `X` +/// determines which argument type the instruction will accept. +/// +/// Users typically don't interact with this type directly; it's used internally by the +/// move-wide instruction constructors. pub struct MovWideTypeTag<X>(PhantomData<X>);harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
401-542: Good test coverage with room for optional expansion.The tests cover the primary relocation scenarios for both 64-bit and 32-bit variants. Each test correctly verifies that the emitted instruction matches a zero-immediate variant and that the appropriate relocation is attached.
Consider adding tests for the currently untested combinations:
- 64-bit:
abs_g0_ncandabs_g0_svariants with MovZ/MovK/MovN- 64-bit: MovK with AbsG0, MovN with AbsG2
- This would improve coverage of the matrix of instruction types × relocation types
Example test structure
#[test] fn test_64_abs_g0nc_movk() { let label = LabelRef { id: LabelId(9), addend: 50, }; let inst = movk(X9, abs_g0_nc(label)).to_code_with_reloc(); assert_eq!(inst.0, movk(X9, (0, 0)).unwrap().to_code()); assert_eq!(inst.1, Some(Rel64::MovWAbsG0Nc(label))); }harm/src/instructions/control/branch_imm.rs (1)
705-755: Good test coverage for compare-and-branch; consider adding tests for B and BL.The tests comprehensively cover CBZ/CBNZ with label references for both 32-bit and 64-bit variants. Each test correctly verifies that the code matches a zero-offset instruction and the appropriate
CondBr19relocation is attached.Consider adding tests for the other label-based branch instructions introduced in this PR:
Branch<LabelRef>(unconditional branch with Jump26 relocation)Branch<(BranchCond, LabelRef)>(conditional branch with CondBr19)BranchLink<LabelRef>(branch-with-link using Call26)Example test structure
#[test] fn test_b_label() { let label = LabelRef { id: LabelId(10), addend: 0, }; let inst = b::<_, LabelRef>(label); let (code, reloc) = inst.to_code_with_reloc(); assert_eq!(code, b(BranchOffset::default()).to_code()); assert_eq!(reloc, Some(Rel64::Jump26(label))); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.tomlharm-runtime/Cargo.tomlharm-runtime/src/labels.rsharm-runtime/src/lib.rsharm-runtime/src/runtime.rsharm/src/instructions.rsharm/src/instructions/control/branch_imm.rsharm/src/instructions/control/testbranch.rsharm/src/instructions/dpimm/movewide.rsharm/src/instructions/dpimm/movewide/movewide_reloc.rsharm/src/instructions/ldst/ldr.rsharm/src/instructions/ldst/ldrsw.rsharm/src/instructions/ldst/macros.rsharm/src/instructions/mov/mov_imm.rsharm/src/reloc.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- harm-runtime/Cargo.toml
- harm/src/instructions/ldst/ldrsw.rs
- Cargo.toml
- harm-runtime/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (7)
harm/src/instructions/mov/mov_imm.rs (1)
harm/src/instructions/dpimm/movewide.rs (9)
movn(189-198)movz(214-223)new(57-57)new(74-80)new(90-96)new(106-112)new(121-127)new(137-143)new(153-159)
harm-runtime/src/runtime.rs (2)
harm/src/instructions.rs (1)
instructions(48-50)harm/src/instructions/control/branch_imm.rs (7)
new(275-275)new(285-291)new(302-308)new(317-323)new(333-339)new(348-354)new(363-369)
harm/src/instructions.rs (2)
harm/src/instructions/control/testbranch.rs (2)
to_code_with_reloc(123-133)to_code_with_reloc(138-148)harm/src/instructions/control/branch_imm.rs (5)
to_code_with_reloc(126-133)to_code_with_reloc(184-192)to_code_with_reloc(253-260)to_code_with_reloc(387-400)to_code_with_reloc(418-431)
harm/src/instructions/ldst/macros.rs (2)
harm/src/instructions.rs (3)
instructions(48-50)to_code_with_reloc(23-23)to_code_with_reloc(30-32)harm/src/instructions/control/testbranch.rs (4)
to_code(93-103)to_code(108-118)to_code_with_reloc(123-133)to_code_with_reloc(138-148)
harm/src/instructions/control/testbranch.rs (1)
harm/src/instructions.rs (4)
instructions(48-50)to_code_with_reloc(23-23)to_code_with_reloc(30-32)to_code(19-19)
harm/src/instructions/dpimm/movewide.rs (1)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (2)
new(326-331)new(339-344)
harm/src/instructions/dpimm/movewide/movewide_reloc.rs (1)
harm/src/instructions/dpimm/movewide.rs (16)
movk(164-173)movn(189-198)movz(214-223)new(57-57)new(74-80)new(90-96)new(106-112)new(121-127)new(137-143)new(153-159)to_code(176-178)to_code(182-184)to_code(201-203)to_code(207-209)to_code(226-228)to_code(232-234)
🔇 Additional comments (24)
harm/src/reloc.rs (1)
1-24: LGTM!The foundational types (
LabelId,Offset,Addr,LabelRef) are well-designed for relocation handling. Using#[repr(transparent)]onLabelIdis appropriate for potential FFI compatibility, and the type aliases provide clear semantics.harm/src/instructions/ldst/ldr.rs (1)
168-183: LGTM!The PC-relative literal LDR rules now correctly specify
Rel64::LdPrelLo19as the relocation type, consistent with the AArch64 relocation model for 19-bit PC-relative load offsets. Both 32-bit and 64-bit variants appropriately use the same relocation type.harm/src/instructions/mov/mov_imm.rs (3)
10-11: LGTM!The import of
MovWideTypeTagalongsideMakeMovArgsandMovImmArgssets up the type-level dispatch needed for the generic move-wide argument construction path.
144-158: LGTM!The refactored 32-bit path correctly uses the
MovWideTypeTag<MovImmArgs<_>>indirection throughMakeMovArgs::new, enabling the unified construction path that supports both immediate and relocation-based variants. The.map()chain properly transforms the outcome intoMovNOrZImm.
224-238: LGTM!The 64-bit path mirrors the 32-bit implementation correctly, using the same
MovWideTypeTag<MovImmArgs<_>>pattern for consistency across register widths.harm/src/instructions/ldst/macros.rs (1)
342-424: LGTM!The macro extension cleanly adds
LabelRefsupport for PC-relative loads:
- The new
$rel:exprparameter allows callers to specify the relocation type.- The
LabelReftrait implementation (lines 385-399) mirrors the existingPc + LdStPcOffsetpath.- The
RelocatableInstructionimpl (lines 414-421) correctly emits a zero offset in the opcode and returns the appropriateRel64relocation constructed via$rel(self.addr).This pattern is consistent with other relocation-aware instructions like
TestBranch.harm/src/instructions/control/testbranch.rs (3)
32-33: LGTM!The
Sealedimpls andMakeTestBranchimpls forLabelRefoffset variants are correctly added for both 64-bit and 32-bit register widths. The implementation pattern is consistent with the existingTestBranchOffsetvariants.Also applies to: 65-89
121-149: LGTM!The
RelocatableInstructionimplementations correctly:
- Generate the base opcode using a default (zero) offset via the existing
RawInstructionimpl.- Return
Rel64::TstBr14with the label reference for the linker to patch.This is the correct pattern for producing relocatable machine code.
315-337: LGTM!Good test coverage for the relocation path. The tests verify that:
- The opcode matches the non-relocated version with a default offset.
- The relocation correctly captures the
LabelRefinRel64::TstBr14.Consider adding 32-bit variant tests (
test_tbz_32_reloc,test_tbnz_32_reloc) for completeness, though the implementation is symmetric.harm/src/instructions.rs (2)
8-8: LGTM!The
Rel64type is correctly propagated through the instruction encoding traits:
RelocatableInstruction::to_code_with_relocsignature updated.- Blanket impl for
RawInstructioncorrectly returnsNonefor non-relocatable instructions.This is a clean API evolution to support the expanded relocation model.
Also applies to: 22-33
44-66: LGTM!The
InstructionSeqtrait and its blanket implementation correctly propagateRel64through the encoding pipeline. Theinstructions()andbytes()helper methods properly discard relocations when only the raw instruction stream is needed.harm/src/instructions/dpimm/movewide.rs (3)
69-160: LGTM: Type-level indirection implemented consistently.The refactoring from direct
MovImmArgsimplementations toMovWideTypeTag<MovImmArgs>is applied consistently across all register and shift variants. This indirection enables supporting both immediate and relocation-based arguments through the same trait, which aligns with the PR's objective of adding relocation support.
189-223: LGTM: Consistent pattern applied to movn and movz.The same type-level indirection pattern is applied consistently to
movnandmovz, maintaining API symmetry withmovk. The existing tests at lines 323-363 demonstrate that the changes maintain backward compatibility for common usage patterns.
164-173: Themovkfunction's new genericArgsparameter maintains full backward compatibility. All existing callers work without modification, as the compiler correctly infersArgsfrom the register type. The comprehensive test suite (50+ test cases) confirms type inference succeeds for all patterns: simple integers, tuples with shifts, and fallible forms. No cryptic error messages are introduced for correctly-formed calls.harm/src/instructions/dpimm/movewide/movewide_reloc.rs (5)
20-139: LGTM: Well-structured relocation type hierarchy.The AbsGx types provide a type-safe way to express which 16-bit nibble of an address a move-wide instruction targets. The
#[doc(hidden)]on types with public constructors is an effective pattern to keep the type namespace clean while allowing usage. Documentation clearly maps each type to its corresponding bit range in the 64-bit address space.
141-154: LGTM: Clean trait design for relocation metadata.The trait hierarchy appropriately models the relationship between 32-bit and 64-bit relocations.
MoveWideReloc32extendingMoveWideReloc64allows code reuse while maintaining type safety, since 32-bit instructions can use the lower address groups (G0, G1) that also exist in 64-bit.
159-277: LGTM: Correct shift and relocation mapping.All shift calculations correctly map to their respective 16-bit nibbles (0, 16, 32, 48 for G0-G3). The
unwrap()calls onShift64::new()are safe since the shift values are compile-time constants that fit within the valid range. Each relocation type correctly corresponds to its address group and checking variant.
347-399: LGTM: Relocatable instruction emission follows standard pattern.The implementations correctly emit instructions with zero immediates and attach the appropriate relocation records. The linker will later patch the immediate fields with the computed label offsets. Reusing the existing
movk,movn, andmovzfunctions from the parent module maintains consistency and avoids duplication.
279-319: The exclusion of AbsG2/G3 from MoveWideReloc32 is correct.AbsG2 and AbsG3 address bits [47:32] and [63:48] respectively, which lie beyond the 32-bit address space [31:0]. Implementing MoveWideReloc32 for these types would be architecturally unsound since 32-bit move-wide instructions cannot reference bits beyond [31:0]. The test suite confirms this design: all 32-bit register tests (W1–W4) only use AbsG0 and AbsG1 variants, and no attempts are made to use 32-bit registers with AbsG2 or AbsG3.
harm/src/instructions/control/branch_imm.rs (5)
41-45: LGTM: Straightforward enum-to-bits conversion.The
Fromimplementation correctly convertsBranchCondto a 4-bit value. The cast chainas u8 as u32is safe since all enum variants fit in 4 bits (0x0-0xF). This implementation enables usingcond.into()in the instruction encoding functions at lines 169 and 188.
115-134: LGTM: Correct relocation for unconditional branch to label.The implementation correctly uses
Rel64::Jump26for unconditional branch instructions, which matches the 26-bit immediate field in the B instruction encoding. The zero placeholder offset will be patched by the linker during relocation resolution.
173-193: LGTM: Correct relocation for conditional branch to label.The implementation correctly uses
Rel64::CondBr19for conditional branch instructions, matching the 19-bit immediate field in the B.cond encoding. This aligns with the ARM64 ELF relocation specification for conditional branches, as confirmed by the previous review.
202-261: LGTM: Branch-with-link correctly uses Call26 relocation.The
BranchLink<LabelRef>implementation appropriately usesRel64::Call26for function call relocations, distinguishing it from theJump26used for unconditional branches. This distinction is important for tooling that analyzes call graphs or performs call-specific optimizations.
385-432: LGTM: Compare-and-branch correctly uses CondBr19 relocation.The
RelocatableInstructionimplementations forCompareBranchwith labels correctly useRel64::CondBr19for both 32-bit and 64-bit variants. This matches the 19-bit immediate field in CBZ/CBNZ instructions, as confirmed by previous reviews and the ARM64 specification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
harm-runtime/src/labels.rs (2)
11-11: Clarify or address the TODO comment.The TODO mentions "segment" but doesn't explain what needs to be implemented. This could refer to tracking which segment (e.g.,
.text,.data) a label belongs to, but the intent is unclear.Would you like me to:
- Open an issue to track this work with more details, or
- Help implement segment tracking if you can clarify the requirements?
8-13: Consider adding documentation for the public API.The
LabelInfoenum and its variants lack documentation. Adding doc comments would improve developer experience by clarifying:
- What each variant represents
- When to use
ForwardvsOffset- The overall purpose of the enum in the label resolution system
Example documentation structure
+/// Information about a label's resolution state. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum LabelInfo { + /// Label has been referenced but not yet defined (forward reference). Forward, // TODO segment + /// Label has been defined at the given offset. Offset(Offset), }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harm-runtime/src/labels.rsharm-runtime/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- harm-runtime/src/lib.rs
🔇 Additional comments (2)
harm-runtime/src/labels.rs (2)
1-5: LGTM!Standard copyright header with BSD 3-clause license is present and properly formatted.
6-6: LGTM!Import correctly references the
Offsettype from the harm relocation module.
1 participant
Implement some instructions referencing labels and returning relocations.
ldr/ldrswmovXRefs #31.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.