[PATCH] Preliminary support for dynamically loadable coff objects

[David Majnemer]

REPOSITORY
  rL LLVM

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:296-304
@@ -281,6 +296,9 @@
   const ObjectFile *Obj = Section.getObject();
   if (auto *ELFObj = dyn_cast<object::ELFObjectFileBase>(Obj))
     return ELFObj->getSectionType(Section) == ELF::SHT_NOBITS;
+  if (auto *COFFObj = dyn_cast<object::COFFObjectFile>(Obj))
+    return COFFObj->getCOFFSection(Section)->Characteristics &
+            COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
 
   auto *MachO = cast<MachOObjectFile>(Obj);
   unsigned SectionType = MachO->getSectionType(Section);
----------------
Your change is correct, it's a shame we can't just ask `SectionRef::isBSS`...

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.cpp:187
@@ +186,3 @@
+  switch (RelType) {
+  case COFF::RelocationTypeAMD64::IMAGE_REL_AMD64_ADDR32NB: {
+    uint32_t *TargetAddress = (uint32_t *)Target;
----------------
Any reason you chose to include the `RelocationTypeAMD64` scope qualifier?

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.cpp:231-233
@@ +230,5 @@
+  // Assert for now that any fixup be resolvable within the object scope.
+  if (TargetOffset == UnknownAddressOrSize) {
+    llvm_unreachable("External symbol reference?");
+  }
+
----------------
Curly braces here aren't needed.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.cpp:236-237
@@ +235,4 @@
+  switch (RelType) {
+  case COFF::RelocationTypeAMD64::IMAGE_REL_AMD64_ADDR64:
+  case COFF::RelocationTypeAMD64::IMAGE_REL_AMD64_ADDR32NB:
+  {
----------------
Could you remove the extra scope qualifiers? I think the enumeration value is pretty specific on its own.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.cpp:261-262
@@ +260,4 @@
+  // Look for and record the EH frame sections.
+  ObjSectionToIDMap::iterator i, e;
+  for (i = SectionMap.begin(), e = SectionMap.end(); i != e; ++i) {
+    const SectionRef &Section = i->first;
----------------
This might be a little simpler as a range-based for loop.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.cpp:265
@@ +264,3 @@
+    StringRef Name;
+    Section.getName(Name);
+    // Note unwind info is split across .pdata and .xdata, so this
----------------
Is it not possible for this to fail in practice?

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.h:29
@@ +28,3 @@
+// Helper for extensive error checking in debug builds.
+std::error_code Check1(std::error_code Err) {
+  if (Err) {
----------------
Any reason why you chose to make this different from ELF? They use `Check`.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.h:46-61
@@ +45,18 @@
+
+  unsigned getMaxStubSize() override {
+    if (Arch == Triple::aarch64 || Arch == Triple::aarch64_be)
+      return 20; // movz; movk; movk; movk; br
+    if (Arch == Triple::arm || Arch == Triple::thumb)
+      return 8; // 32-bit instruction and 32-bit address
+    else if (Arch == Triple::mipsel || Arch == Triple::mips)
+      return 16;
+    else if (Arch == Triple::ppc64 || Arch == Triple::ppc64le)
+      return 44;
+    else if (Arch == Triple::x86_64)
+      return 6; // 2-byte jmp instruction + 32-bit relative address
+    else if (Arch == Triple::systemz)
+      return 16;
+    else
+      return 0;
+  }
+
----------------
Something tells me you aren't trying to support SystemZ ;)

I'd remove support for everything but the ARM and x86 variants.

================
Comment at: lib/Object/COFFObjectFile.cpp:193-195
@@ -192,3 +192,5 @@
     Result = SymbolRef::ST_File;
-  } else if (SectionNumber == COFF::IMAGE_SYM_DEBUG) {
+  } else if (SectionNumber == COFF::IMAGE_SYM_DEBUG ||
+             Symb.isSectionDefinition()) {
+    // TODO: perhaps we need a new symbol type ST_Section.
     Result = SymbolRef::ST_Debug;
----------------
Good catch :)

================
Comment at: lib/Object/COFFObjectFile.cpp:376
@@ -366,3 +375,3 @@
   const coff_section *Sec = toSec(Ref);
-  return Sec->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
+  return Sec->SizeOfRawData == 0;
 }
----------------
What's the rationale for this change?

http://reviews.llvm.org/D7793

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/