Handle relocations to thumb functions when dynamic linking COFF modules

[wallace]

Summary

This adds the necessary logic to support relocations to thumb functions in the COFF dynamic linker.
The jumps to function addresses are mostly blx, which requires the ISA selection bit when jumping to a thumb function.

Note: I'm determining if the relocation requires the ISA bit when creating the relocation entries and not when resolving the relocation. I have to do that because I need the ObjectFile and the actual Symbol, which are available only when creating the entries. It would require a gross refactor if I do it otherwise, but I'm okay with doing it if you think it's better.

[rengolin]

Hi,

Maybe if you added a bit more comments on the code, a better commit message (and some tests :), it'd be easier to understand what you're aiming for. :)

cheers,
--renato

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h

40–41

Is this the only Thumb/COFF relocation?

114

You already do the exact same test inside isThumbRelocation, so it's redundant here.

193

I'm not sure what this does...

269

Nor this...

[compnerd]

This absolutely needs a test, right now its somewhat challenging to follow. I agree with @rengolin that the commit message isn't particularly helpful either.

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h

39

Unnecessary braces.

What other type is there?

40–41

Yes, the way that COFF identifies thumb is via the IMAGE_SCN_MEM_16BIT characteristic.

269

This is a horrible way to handle this. The fact that RE.IsThumbRel is boolean, and therefore gets converted into an implicit 1 or 0 which happens to map to the appropriate ISA selection bit. Can we handle this inside of the Relocation handling perhaps?

[peter.smith]

Not much to add over what has already been said. From an ELF speaker's perspective it looks like you should only identify a symbol as Thumb if it is a function, but I don't know COFF well enough to be sure.

ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h

153

Does Rel mean relative to a thumb symbol or relocation in this context? Looking at the code I think you mean relative. Could the name make this clearer.

My first thought was that IsThumbRel meant this is a Thumb Relocation, not this is a relocation to a Thumb function symbol.

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h

36

Does a data symbol in a section marked as Thumb code always indicate a Thumb address? For example could there be a relocation to literal data in such a section.

I mention it as in ELF bit 0 is set only if the target symbol has type STT_FUNC which I'm guessing is ST_Function in COFF.

[wallace]

Improved the code, added comments, improved the diff description, but test is still missing

[compnerd]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h

153 ↗

(On Diff #73337)

I think that we care more if the target is a thumb function. IsTargetThumbFunc would be more descriptive, and happens to mirror the .thumb_func directive for the GNU assembler syntax.

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h

25 ↗

(On Diff #73337)

IsThumbFunc is shorter but just as descriptive IMO.

[wallace]

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h

25 ↗

(On Diff #73337)

will do, thanks!

[wallace]

shortened long function names

[wallace]

added a test for the mov32t relocation

[wallace]

finished adding tests

[wallace]

polished the test

[wallace]

ping

[peter.smith]

I've got one comment about the #TODO in the test. I don't know much about COFF and I'm not a contributor to this area so I think you'll need to get approval from someone like compnerd.

test/ExecutionEngine/RuntimeDyld/ARM/COFF_Thumb.s

129 ↗

(On Diff #73374)

What is the reason for the # TODO? What will it take for them to be done?

[compnerd]

test/ExecutionEngine/RuntimeDyld/ARM/COFF_Thumb.s

124 ↗

(On Diff #73374)

Would be nice to give this a proper label. Eventually, it would be nice to be able to run this test against MCJIT.

129 ↗

(On Diff #73374)

Theres a single relocation that is emitted as per the PE/COFF specification. We need to be able to index through the relocation to process that component of the address.

142 ↗

(On Diff #73374)

Again, a label for this would be nice.

[wallace]

add labels, but I'm pretty sure you won't like them
I have literary problems...

[wallace]

.

ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h

public:
   };
 
   /// True if this is a PCRel relocation (MachO specific).
   bool IsPCRel;
 
   /// The size of this relocation (MachO specific).
   unsigned Size;
 
+  // True if this is a thumb relocation to a function symbol (Windows specific).
+  bool IsThumbRel;
+
   RelocationEntry(unsigned id, uint64_t offset, uint32_t type, int64_t addend)
       : SectionID(id), Offset(offset), RelType(type), Addend(addend),
-        SymOffset(0), IsPCRel(false), Size(0) {}
+        SymOffset(0), IsPCRel(false), Size(0), IsThumbRel(false) {}
 
   RelocationEntry(unsigned id, uint64_t offset, uint32_t type, int64_t addend,
                   uint64_t symoffset)
       : SectionID(id), Offset(offset), RelType(type), Addend(addend),
-        SymOffset(symoffset), IsPCRel(false), Size(0) {}
+        SymOffset(symoffset), IsPCRel(false), Size(0), IsThumbRel(false) {}
 
   RelocationEntry(unsigned id, uint64_t offset, uint32_t type, int64_t addend,
                   bool IsPCRel, unsigned Size)
       : SectionID(id), Offset(offset), RelType(type), Addend(addend),
-        SymOffset(0), IsPCRel(IsPCRel), Size(Size) {}
+        SymOffset(0), IsPCRel(IsPCRel), Size(Size), IsThumbRel(false) {}
 
   RelocationEntry(unsigned id, uint64_t offset, uint32_t type, int64_t addend,
                   unsigned SectionA, uint64_t SectionAOffset, unsigned SectionB,
                   uint64_t SectionBOffset, bool IsPCRel, unsigned Size)
       : SectionID(id), Offset(offset), RelType(type),
         Addend(SectionAOffset - SectionBOffset + addend), IsPCRel(IsPCRel),
-        Size(Size) {
+        Size(Size), IsThumbRel(false) {
+    Sections.SectionA = SectionA;
+    Sections.SectionB = SectionB;
+  }
+
+  RelocationEntry(unsigned id, uint64_t offset, uint32_t type, int64_t addend,
+                  unsigned SectionA, uint64_t SectionAOffset, unsigned SectionB,
+                  uint64_t SectionBOffset, bool IsPCRel, unsigned Size,
+                  bool IsThumbRel)
+      : SectionID(id), Offset(offset), RelType(type),
+        Addend(SectionAOffset - SectionBOffset + addend), IsPCRel(IsPCRel),
+        Size(Size), IsThumbRel(IsThumbRel) {
     Sections.SectionA = SectionA;
     Sections.SectionB = SectionB;
   }
 };
 
 class RelocationValueRef {
 public:
   unsigned SectionID;

ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h

 #include "llvm/Object/COFF.h"
 #include "llvm/Support/COFF.h"
 #include "../RuntimeDyldCOFF.h"
 
 #define DEBUG_TYPE "dyld"
 
 namespace llvm {
 
+static bool isThumbRelocation(symbol_iterator Symbol, const ObjectFile &Obj,
+                              section_iterator Section) {
+  Expected<SymbolRef::Type> SymTypeOrErr = Symbol->getType();
+  if (!SymTypeOrErr) {
+    std::string Buf;
+    raw_string_ostream OS(Buf);
+    logAllUnhandledErrors(SymTypeOrErr.takeError(), OS, "");
+    OS.flush();
+    report_fatal_error(Buf);
+  }
+
+  if (*SymTypeOrErr != SymbolRef::ST_Data &&
+      *SymTypeOrErr != SymbolRef::ST_Function) {
+    return false;
+  }
+  return cast<COFFObjectFile>(Obj).getCOFFSection(*Section)->Characteristics &
+         COFF::IMAGE_SCN_MEM_16BIT;
+}
+
 class RuntimeDyldCOFFThumb : public RuntimeDyldCOFF {
 public:
   RuntimeDyldCOFFThumb(RuntimeDyld::MemoryManager &MM,
                        JITSymbolResolver &Resolver)
       : RuntimeDyldCOFF(MM, Resolver) {}
 
   unsigned getMaxStubSize() override {
     return 16; // 8-byte load instructions, 4-byte jump, 4-byte padding
if (Section == Obj.section_end()) {
       addRelocationForSymbol(RE, TargetName);
     } else {
       if (auto TargetSectionIDOrErr =
           findOrEmitSection(Obj, *Section, Section->isText(), ObjSectionToID))
         TargetSectionID = *TargetSectionIDOrErr;
       else
         return TargetSectionIDOrErr.takeError();
 
+      Expected<SymbolRef::Type> SymTypeOrErr = Symbol->getType();
+      if (!SymTypeOrErr) {
+        std::string Buf;
+        raw_string_ostream OS(Buf);
+        logAllUnhandledErrors(SymTypeOrErr.takeError(), OS, "");
+        OS.flush();
+        report_fatal_error(Buf);
+      }
+
+      bool IsThumbRel = isThumbRelocation(Symbol, Obj, Section);
+
       switch (RelType) {
       default: llvm_unreachable("unsupported relocation type");
       case COFF::IMAGE_REL_ARM_ABSOLUTE:
         // This relocation is ignored.
         break;
-      case COFF::IMAGE_REL_ARM_ADDR32:
+      case COFF::IMAGE_REL_ARM_ADDR32: {
+        RelocationEntry RE = RelocationEntry(
+            SectionID, Offset, RelType, Addend, TargetSectionID,
+            getSymbolOffset(*Symbol), 0, 0, false, 0, IsThumbRel);
+        addRelocationForSection(RE, TargetSectionID);
+        break;
+      }
       case COFF::IMAGE_REL_ARM_ADDR32NB: {
         RelocationEntry RE =
             RelocationEntry(SectionID, Offset, RelType, Addend, TargetSectionID,
                             getSymbolOffset(*Symbol), 0, 0, false, 0);
         addRelocationForSection(RE, TargetSectionID);
         break;
       }
       case COFF::IMAGE_REL_ARM_SECTION: {
         RelocationEntry RE =
             RelocationEntry(TargetSectionID, Offset, RelType, 0);
         addRelocationForSection(RE, TargetSectionID);
         break;
       }
       case COFF::IMAGE_REL_ARM_SECREL: {
         RelocationEntry RE = RelocationEntry(SectionID, Offset, RelType,
                                              getSymbolOffset(*Symbol) + Addend);
         addRelocationForSection(RE, TargetSectionID);
         break;
       }
       case COFF::IMAGE_REL_ARM_MOV32T: {
-        RelocationEntry RE =
-            RelocationEntry(SectionID, Offset, RelType, Addend, TargetSectionID,
-                            getSymbolOffset(*Symbol), 0, 0, false, 0);
+        RelocationEntry RE = RelocationEntry(
+            SectionID, Offset, RelType, Addend, TargetSectionID,
+            getSymbolOffset(*Symbol), 0, 0, false, 0, IsThumbRel);
         addRelocationForSection(RE, TargetSectionID);
         break;
       }
       case COFF::IMAGE_REL_ARM_BRANCH20T:
       case COFF::IMAGE_REL_ARM_BRANCH24T:
       case COFF::IMAGE_REL_ARM_BLX23T: {
         RelocationEntry RE =
             RelocationEntry(SectionID, Offset, RelType,
case COFF::IMAGE_REL_ARM_ABSOLUTE:
       // This relocation is ignored.
       break;
     case COFF::IMAGE_REL_ARM_ADDR32: {
       // The target's 32-bit VA.
       uint64_t Result =
           RE.Sections.SectionA == static_cast<uint32_t>(-1)
               ? Value
               : Sections[RE.Sections.SectionA].getLoadAddressWithOffset(RE.Addend);
+      Result |= RE.IsThumbRel;
       assert(static_cast<int32_t>(Result) <= INT32_MAX &&
              "relocation overflow");
       assert(static_cast<int32_t>(Result) >= INT32_MIN &&
              "relocation underflow");
       DEBUG(dbgs() << "\t\tOffset: " << RE.Offset
                    << " RelType: IMAGE_REL_ARM_ADDR32"
                    << " TargetSection: " << RE.Sections.SectionA
                    << " Value: " << format("0x%08" PRIx32, Result) << '\n');
case COFF::IMAGE_REL_ARM_MOV32T: {
 
       auto EncodeImmediate = [](uint8_t *Bytes, uint16_t Immediate)  {
         Bytes[0] |= ((Immediate & 0xf000) >> 12);
         Bytes[1] |= ((Immediate & 0x0800) >> 11);
         Bytes[2] |= ((Immediate & 0x00ff) >>  0);
         Bytes[3] |= (((Immediate & 0x0700) >>  8) << 4);
       };
 
-      EncodeImmediate(&Target[0], static_cast<uint32_t>(Result) >> 00);
+      EncodeImmediate(&Target[0],
+                      (static_cast<uint32_t>(Result) >> 00) | RE.IsThumbRel);
       EncodeImmediate(&Target[4], static_cast<uint32_t>(Result) >> 16);
 
       break;
     }
     case COFF::IMAGE_REL_ARM_BRANCH20T: {
       // The most significant 20-bits of the signed 21-bit relative displacement
       uint64_t Value =
           RE.Addend - (Sections[RE.SectionID].getLoadAddress() + RE.Offset) - 4;