[PATCH] D29192: [AArch64] Fix incorrect relocation encodings for ILP32

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've checked that the relocation name is correct. I'm a bit suspicious of allowing the fixup_aarch64_ldst_imm12_scale8 in LP32, with a 32-bit sized GOT I think that this is likely to be a programming mistake that we should probably error on. I'm not hugely familiar with lp32 programming so there may be a case for it that I'm missing?



================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:253
+          Ctx.reportError(Fixup.getLoc(),
+                          "Bad LP64 reloc: fixup_aarch64_ldst_imm12_scale4 "
+                          "VK_GOT IsNC");
----------------
I think it is more a bad fixup rather than Bad LP64 relocation. The other error messages are of the form "invalid fixup for ..."
Possible suggestion "invalid fixup for got relative load/store instruction on LP64". 

I don't have a particularly strong opinion here though.



================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:266
       if (SymLoc == AArch64MCExpr::VK_GOT && IsNC)
-        return R_CLS(LD64_GOT_LO12_NC);
+        return IsILP32 ? ELF::R_AARCH64_P32_LD32_GOT_LO12_NC
+                       : ELF::R_AARCH64_LD64_GOT_LO12_NC;
----------------
Is there a case where this fixup is valid in ILP32 given the size of the .got is 32-bits? I think that from your test cases you would expect it for // CHECK: ldr x24, [x23, :got_lo12:sym]  
This should probably be an error so that we only accept ldr w24, [x23, :got_lo12:sym].

  


================
Comment at: test/MC/AArch64/arm32-elf-relocs.s:226
 
-#   ldr x24, [x23, :gottprel_lo12:sym]
-#   ldr d22, [x21, #:gottprel_lo12:sym]
+   ldr x24, [x23, :gottprel_lo12:sym]
+   ldr d22, [x21, :gottprel_lo12:sym]
----------------
The removal of the # has uncommented out these instructions, but I don't see any extra CHECKs. If they are superfluous I recommend removing them.


Repository:
  rL LLVM

https://reviews.llvm.org/D29192