[PATCH] D35640: [lld] [COFF] Align import address chunks to the pointer size

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Chunks.cpp:186
 static void applyArm64Ldr(uint8_t *Off, uint64_t Imm) {
   int Size = read32le(Off) >> 30;
+  if ((Imm & ((1 << Size) - 1)) != 0)
----------------
mstorsjo wrote:
> ruiu wrote:
> > It is better to use uint32_t so that you don't need to think about sign-extended shift.
> Sure - do you want that change separately in a different change, or squashed into this one?
It's probably better to do in a separate patch, even though I don't care if you do that as passed-by change with this patch.


================
Comment at: COFF/Chunks.cpp:187
   int Size = read32le(Off) >> 30;
+  if ((Imm & ((1 << Size) - 1)) != 0)
+    fatal("Misaligned ldr/str offset");
----------------
mstorsjo wrote:
> ruiu wrote:
> > Use llvm::isPowerOf2_32.
> Doesn't that check a different thing? This tests if the lowest `Size` bits of `Imm` are nonzero, while `llvm::isPowerOf2_32` checks if the value is a power of two?
Ah, that's right.


================
Comment at: COFF/Chunks.cpp:188
+  if ((Imm & ((1 << Size) - 1)) != 0)
+    fatal("Misaligned ldr/str offset");
   Imm >>= Size;
----------------
ruiu wrote:
> Error messages should start with lowercase letter.
Please fix.


https://reviews.llvm.org/D35640