[PATCH] Preliminary support for dynamically loadable coff objects

[David Majnemer]

REPOSITORY
  rL LLVM

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:271-272
@@ +270,4 @@
+    const coff_section *CoffSection = COFFObj->getCOFFSection(Section);
+    bool HasContent = (CoffSection->VirtualSize > 0) 
+      || (CoffSection->SizeOfRawData > 0);
+    bool IsDiscardable = CoffSection->Characteristics &
----------------
This makes sense but is a little subtle.  Both checks are needed because:
- `VirtualSize` may be zero in object files while `SizeOfRawData` is non-zero.
- `SizeOfRawData` may be zero in executables while `VirtualSize` is non-zero.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.h:29
@@ +28,3 @@
+class RuntimeDyldCOFF : public RuntimeDyldImpl {
+
+public:
----------------
This space is superfluous.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:57-58
@@ +56,4 @@
+// symbol in the target address space.
+void RuntimeDyldCOFFX86_64::resolveRelocation(const RelocationEntry &RE,
+  uint64_t Value) {
+  const SectionEntry &Section = Sections[RE.SectionID];
----------------
I thought `Value` was a variable with automatic storage duration, please consider indenting this.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:63
@@ +62,3 @@
+  switch (RE.RelType) {
+
+  case COFF::IMAGE_REL_AMD64_REL32:
----------------
This space is superfluous.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:72
@@ +71,3 @@
+    uint64_t FinalAddress = Section.LoadAddress + RE.Offset;
+    Value -= FinalAddress + 4;
+    uint64_t Result = Value + RE.Addend;
----------------
This seems wrong for `RelType != COFF::IMAGE_REL_AMD64_REL32`.  I think we would want `+ 4` for `IMAGE_REL_AMD64_REL32`, `+ 3` for `IMAGE_REL_AMD64_REL32_1`, etc.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:138
@@ +137,3 @@
+  switch (RelType) {
+
+  case COFF::IMAGE_REL_AMD64_REL32: 
----------------
This space is superfluous.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:146
@@ +145,3 @@
+    uint32_t Delta = (RelType - COFF::IMAGE_REL_AMD64_REL32);
+    uint32_t *Displacement = (uint32_t*)ObjTarget;
+    Addend = *Displacement + Delta;
----------------
Please insert a space between the pointee type and the star token.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:26
@@ +25,3 @@
+class RuntimeDyldCOFFX86_64 : public RuntimeDyldCOFF {
+
+private:
----------------
This space is superfluous.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:37
@@ +36,3 @@
+
+  RuntimeDyldCOFFX86_64(RTDyldMemoryManager *mm) : RuntimeDyldCOFF(mm) {}
+
----------------
Variable names are capitalized in LLVM.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:45-48
@@ +44,6 @@
+
+  relocation_iterator
+    processRelocationRef(unsigned SectionID, relocation_iterator RelI,
+    const ObjectFile &Obj, ObjSectionToIDMap &ObjSectionToID,
+    StubMap &Stubs) override;
+
----------------
This looks a little funny, would you mind reformatting this?  You might want to look into using clang-format, many contributors use it to automate reformatting source code to the LLVM conventions.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:50
@@ +49,3 @@
+
+  unsigned int getStubAlignment() override { return 1; }
+  void registerEHFrames() override;
----------------
You could just go with `unsigned`.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:53-54
@@ +52,4 @@
+  void deregisterEHFrames() override;
+  void finalizeLoad(const ObjectFile &Obj,
+    ObjSectionToIDMap &SectionMap) override;
+};
----------------
Likewise, the way RuntimeDyldELF.h formats this method declaration is more in line with LLVM's style.

================
Comment at: lib/Object/COFFObjectFile.cpp:364-371
@@ -361,3 +363,10 @@
   const coff_section *Sec = toSec(Ref);
-  return Sec->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
+  return (Sec->Characteristics &
+    (COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA
+    | COFF::IMAGE_SCN_MEM_READ
+    | COFF::IMAGE_SCN_MEM_WRITE))
+    ==
+    (COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA
+    | COFF::IMAGE_SCN_MEM_READ
+    | COFF::IMAGE_SCN_MEM_WRITE);
 }
----------------
It might be nicer to store the flags in a variable, perhaps:
  const uint32_t BSSFlags = COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |
                            COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_MEM_WRITE;
  return (Sec->Characteristics & BSSFlags) == BSSFlags;

http://reviews.llvm.org/D7793

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