[PATCH] D37335: [MIPS] Initial support of microMIPS code linking

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/Mips.cpp:179-180
+static bool needShuffling(uint32_t Type){
+  return R_MICROMIPS_26_S1 <= Type && Type <= R_MICROMIPS_PC19_S2 &&
+         Type != R_MICROMIPS_PC7_S1 && Type != R_MICROMIPS_PC10_S1;
+}
----------------
So this expression is all R_MICROMIPS_* relocations except ones that have 1 or 2 byte opcodes instead of 4 bytes, right? It might be helpful to add a comment about that because it is not obvious that R_MICROMIPS_26_S1 is the first relocation type and R_MICROMIPS_PC19_S2 the last for R_MICROMIPS_* relocations.


================
Comment at: ELF/Arch/Mips.cpp:190-192
+  uint32_t V = read32<E>(Loc);
+  write16<E>(Loc, V >> 16);
+  write16<E>(Loc + 2, V & 0xffff);
----------------
Is this the same as swapping the first two bytes with the following two bytes? If so, is this different from `unshuffle`?

Can this be something like this?

  uint16_t *A = (uint16_t *)Loc;
  std::swap(A[0], A[1]);


================
Comment at: ELF/Arch/Mips.cpp:201
+
+template <endianness E, uint8_t BSIZE, uint8_t SHIFT>
+static void writeRelocation16(uint8_t *Loc, uint64_t V) {
----------------
BSIZE and SHIFT can be passed as function arguments instead of template arguments.


================
Comment at: ELF/Arch/Mips.cpp:288
+  if (isMicroMips<ELFT>()) {
+    memset(Buf, 0, PltEntrySize);
+    if (isMipsR6<ELFT>()) {
----------------
Do you need this?


================
Comment at: ELF/Arch/Mips.cpp:485
+  case R_MICROMIPS_GOT16:
+    if (Config->Relocatable)
+      writeRelocation<E, 16, 16>(Loc, Val + 0x8000);
----------------
nit: add {}


================
Comment at: ELF/Arch/Mips.cpp:623-624
   }
+  if (needShuffling(Type))
+    unshuffle<E>(Loc);
 }
----------------
Hmm, it looks like you shuffle bytes and then restore the bytes at end of the function. Looks like it's a bit tricky. Is it possible to define a writeRelocation function for R_MICROMIPS_* and shuffle bytes in that function?


Repository:
  rL LLVM

https://reviews.llvm.org/D37335