[PATCH] D75349: [LLD][ELF][ARM] Implement ARM pc-relative relocations for adr and ldr

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the update. I confess that I don't really understand the encoding but we should not waste the efforts making Arch/ARM.cpp complete. I mostly only get some nitpicking.



================
Comment at: lld/ELF/Arch/ARM.cpp:418
+
+// rotr32 - Rotate a 32-bit unsigned value right by a specified # bits.
+static uint32_t rotr32(uint32_t val, uint32_t amt) {
----------------
>From https://llvm.org/docs/CodingStandards.html

> Don’t duplicate function or class name at the beginning of the comment.




================
Comment at: lld/test/ELF/arm-adr-err.s:3
+// RUN: llvm-mc --triple=armv7a-none-eabi --arm-add-build-attributes -filetype=obj -o %t.o %s
+// RUN: not ld.lld -n %t.o -o /dev/null 2>&1 | FileCheck %s
+ .section .os0, "ax", %progbits
----------------
Do we need `-n`?


================
Comment at: lld/test/ELF/arm-adr-long.s:8
+// RUN:               } " > %t.script
+// RUN: ld.lld -n --script %t.script %t.o -o %t
+// RUN: llvm-objdump -d --no-show-raw-insn --triple=armv7a-none-eabi %t | FileCheck %s
----------------
Do we need `-n` ?


================
Comment at: lld/test/ELF/arm-adr-long.s:32
+
+// CHECK: 10000000 <dat1>:
+// CHECK-NEXT: 10000000: andeq   r0, r0, r0
----------------
Beauty is in the eye of the beholder, but does it look better if `CHECK:` lines have sufficient spaces so that the contents align with contents on `CHECK-LINE:`? Additionally, some addresses can be omitted.

```
// CHECK:      10000000 <dat1>:
// CHECK-NEXT:  andeq r0, r0, r0
```

`andeq` is indented to make it clear that the instruction "belongs to" the <dat1> function.


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

https://reviews.llvm.org/D75349