[PATCH] D40147: [MIPS] Handle cross-mode (regular <-> microMIPS) jumps

[Simon Atanasyan via Phabricator via llvm-commits]

atanasyan added inline comments.


================
Comment at: ELF/Arch/Mips.cpp:459
+  switch (Type) {
+  default:
+    return false;
----------------
ruiu wrote:
> Can you write `default` at end?
> Can you write default at end?
Will do.




================
Comment at: ELF/Arch/Mips.cpp:482
+template <class ELFT>
+static void fixupCrossModeJump(uint8_t *Loc, RelType Type, uint64_t *Val) {
+  const endianness E = ELFT::TargetEndianness;
----------------
ruiu wrote:
> This needs a comment so that not only a person who understand the details of MIPS ABI can edit this function.
> This needs a comment so that not only a person who understand the details of MIPS ABI can edit this function.
OK




================
Comment at: ELF/Symbols.cpp:107
+    // set the less-significant bit here.
+    if (Config->EMachine == EM_MIPS && (Sym.StOther & STO_MIPS_MICROMIPS))
+      VA |= 1;
----------------
ruiu wrote:
> I do understand why you need this by reading comment, but doing this in this function doesn't feel right. This function returns an address of a symbol, and the tag whether the target is microMIPS or not is not logically part of its address. Can't you do this when you apply relocations?
> Can't you do this when you apply relocations?
I had such patch. In that case we need:
  - Add the less-significant bit in the `InputSectionBase::relocateAlloc()` or `getRelocTargetVA` before calling the `relocateOne`. We can't do that in the `MIPS::relocateOne` because we do not have any information about target there.
  - Add the less-significant bit in the `Writer::getEntryAddr`. The `Elf_Ehdr::e_entry` field holds just a value so we have to signal a loader that the entry symbol is microMIPS.
  - Add the less-significant bit in the `DynamicSection::writeTo`. By the same reason as above.

This way looks more correct, but introduces more changes in the code. Do you think it's acceptable?



Repository:
  rL LLVM

https://reviews.llvm.org/D40147