Skip to content

Commit

Permalink
Fix PC-relative data accesses with LDR
Browse files Browse the repository at this point in the history
We need to dereference twice, because we embed the address in a way that
adds a layer of indirection.
  • Loading branch information
langston-barrett committed Jun 8, 2020
1 parent ddb2f44 commit 3cedd6a
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion renovate-aarch32/src/Renovate/Arch/AArch32/ISA.hs
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,7 @@ armConcretizeAddresses _mem insnAddr i0 tgt =
-- We will fix that by translating into a three instruction sequence:
--
-- > ldr rt, [pc, #8]
-- > ldr [rt], [rt, #0]
-- > b +8
-- > .word <address that the data is really at>
--
Expand All @@ -562,7 +563,17 @@ armConcretizeAddresses _mem insnAddr i0 tgt =
i1 = ARMInstruction $ DA.Instruction DA.LDR_l_A1 (DA.Annotated () p DA.:< DA.Annotated () rt DA.:< DA.Annotated () u DA.:< DA.Annotated () w DA.:< DA.Annotated () cond DA.:< DA.Annotated () (DA.Bv12 0) DA.:< DA.Nil)
i2 = toAnnotatedARM A32Repr $ DA.Instruction DA.B_A1 (DA.Bv4 unconditional DA.:< DA.Bv24 (0 `DB.shiftR` 2) DA.:< DA.Nil)
i3 = ARMBytes (LBS.toStrict (BB.toLazyByteString (BB.word32LE w32)))
in i1 DLN.:| [ i2, i3 ]
-- Operands marked with ??? were blindly chosen by disassembling a similar instruction.
i4 = ARMInstruction $ DA.Instruction DA.LDR_i_A1_off $
(DA.Annotated () (DA.Bv1 1) -- ???
DA.:< DA.Annotated () rt -- target register?
DA.:< DA.Annotated () rt -- source register?
DA.:< DA.Annotated () (DA.Bv1 1) -- ???
DA.:< DA.Annotated () (DA.Bv1 0) -- ???
DA.:< DA.Annotated () (DA.Bv4 14) -- ???
DA.:< DA.Annotated () (DA.Bv12 0) -- offset
DA.:< DA.Nil)
in i1 DLN.:| [ i2, i3, i4 ]
_ -> ARMInstruction (DA.Instruction (coerce opc) (FC.fmapFC toUnitAnnotation operands)) DLN.:| []
_ -> ARMInstruction (DA.Instruction (coerce opc) (FC.fmapFC toUnitAnnotation operands)) DLN.:| []
(R.NoTarget, ThumbInstruction {}) ->
Expand Down

4 comments on commit 3cedd6a

@travitch
Copy link
Contributor

Choose a reason for hiding this comment

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

Question and comment here.

Question: The original load from [pc, #8] was trying to load that .word after the branch. Since there is an extra instruction inserted, is that not now 12 bytes away?

Comment: the DA.Bv4 14 is the condition field - in particular 14 is flagging unconditional execution. Can we give a name to that value for clarity?

@langston-barrett
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: The original load from [pc, #8] was trying to load that .word after the branch. Since there is an extra instruction inserted, is that not now 12 bytes away?

Good question! I made the additional load come after the .word inserted into the instruction stream specifically to not mess with that calculation. So I think it has no effect.

Comment: the DA.Bv4 14 is the condition field - in particular 14 is flagging unconditional execution. Can we give a name to that value for clarity?

Yes, I'll do that.

@travitch
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, can we have the assembly in the comment reflect that too? I see it now, but was initially confused.

@langston-barrett
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, can we have the assembly in the comment reflect that too? I see it now, but was initially confused.

Oh, will do. My bad! Misleading comments are worse than no comments.

Please sign in to comment.