[PATCH] D153407: [lld][ARM] Add support for 16-bit thumb group relocations

Wed Jun 21 21:46:32 PDT 2023

MaskRay accepted this revision.
MaskRay added a comment.

In D153407#4437758 <https://reviews.llvm.org/D153407#4437758>, @peter.smith wrote:

> One tiny nit aside, looks good to me. Please leave some time for MaskRay to take a look.
>
> For reference: the R_ARM_THM_ALU_ABS_G* relocations are defined in the Static Thumb16 Relocations table https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#5615static-thumb16-relocations .

Thanks. Looks good to me as well. It would be good to place the link to the summary/commit message.

================
Comment at: lld/ELF/Arch/ARM.cpp:678
+    val >>= 8;
+    [[fallthrough]];
+  case R_ARM_THM_ALU_ABS_G2_NC:
----------------
This may be an instance that duplicating `write16(loc, (read16(loc) &~ 0x00ff) | (val & 0xff));` is better than a chain of ` [[fallthrough]];`...

If the case code is close enough, we should not worry much that we made a mistake in the formula.

================
Comment at: lld/test/ELF/arm-thumb-alu.s:42
+// CHECK: adds    r0, #0x33
+// CHECK: movs    r0, #0x33
+// CHECK: movs    r0, #0x33
----------------
Add `-NEXT` for all these continuation lines. This will make FileCheck report a better error when something goes wrong.

================
Comment at: lld/test/ELF/arm-thumb-alu.s:51
+R_ARM_THM_ALU_ABS_G1_NC:
+ adds r0, :lower8_15:sym1
+ movs r0, :lower8_15:sym1
----------------
2-space indentation is more common.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153407/new/

https://reviews.llvm.org/D153407