[PATCH] Preliminary support for dynamically loadable coff objects

[Andy Ayers]

Will have an updated patch out shortly....


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 &
----------------
majnemer wrote:
> 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.
I'll add a comment here.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.h:29
@@ +28,3 @@
+class RuntimeDyldCOFF : public RuntimeDyldImpl {
+
+public:
----------------
majnemer wrote:
> This space is superfluous.
Am going to run clang-format as suggested to clear these up.

================
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;
----------------
majnemer wrote:
> 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.
There was a "Delta" in processRelocationRef that was supposed to take care of this, but I had the sign wrong there... will move it up here since it probably fits better here than there.

Note the _N variants subtract additional N, not add it. Eg REL32_1 means there's a reloc that begins 5 bytes from the end of the instruction.


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

================
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;
+
----------------
majnemer wrote:
> 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.
Yep, clang-format it is.

================
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);
 }
----------------
majnemer wrote:
> 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;
Will do.

http://reviews.llvm.org/D7793

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