[PATCH] LTO: introduce object file-based on-disk module format.

[Peter Collingbourne]

Added docs.

================
Comment at: include/llvm/Object/IRObjectFile.h:54
@@ -51,1 +53,3 @@
 
+  static ErrorOr<MemoryBufferRef> getBitcodeFromObject(ObjectFile *Obj);
+  static ErrorOr<MemoryBufferRef>
----------------
rafael wrote:
> Some comments saying what these function do would be nice.
> 
> I wonder if findBitcodeIn... might be a more appropriate name. Not sure.
> Some comments saying what these function do would be nice.

Added.

> I wonder if findBitcodeIn... might be a more appropriate name. Not sure.

Ok, I like that name better. Done.

================
Comment at: lib/LTO/LTOModule.cpp:58
@@ +57,3 @@
+bool LTOModule::isBitcodeFile(const void *Mem, size_t Length) {
+  auto BCData = IRObjectFile::getBitcodeFromMemBuffer(
+      MemoryBufferRef(StringRef((const char *)Mem, Length), "<mem>"));
----------------
rafael wrote:
> I don't think we use auto for cases like this.
Changed.

================
Comment at: lib/LTO/LTOModule.cpp:60
@@ -57,1 +59,3 @@
+      MemoryBufferRef(StringRef((const char *)Mem, Length), "<mem>"));
+  return BCData && !BCData->getBuffer().empty();
 }
----------------
rafael wrote:
> Is the empty check needed? It seems better to say that an empty section would be a broken bitcode. That is, this should return true and we would print an error afterwards.
Previously I returned an empty memory buffer from getBitcodeFromMemBuffer if the file was of the wrong type or there was no .llvmbc section present. That was a bad decision in retrospect; that function now returns error codes in those cases, which lets me drop the empty check here and elsewhere.

================
Comment at: lib/LTO/LTOModule.cpp:64
@@ +63,3 @@
+bool LTOModule::isBitcodeFile(const char *Path) {
+  sys::fs::file_magic Type;
+  if (sys::fs::identify_magic(Path, Type))
----------------
rafael wrote:
> It seems tempting to just get the buffer and forward to IRObjectFile::getBitcodeFromMemBuffer. Would avoid the duplicated switch over the type.
Done. Now that I think about it, the previous way wasn't saving us much.

================
Comment at: lib/Object/IRObjectFile.cpp:268
@@ -266,1 +267,3 @@
 
+ErrorOr<MemoryBufferRef> IRObjectFile::getBitcodeFromObject(ObjectFile *Obj) {
+  for (auto Sec : Obj->sections()) {
----------------
rafael wrote:
> Maybe this should take an ObjectFile&?
> 
It can even take a `const ObjectFile &`. Done.

================
Comment at: lib/Object/IRObjectFile.cpp:271
@@ +270,3 @@
+    StringRef SecName;
+    if (std::error_code EC = Sec.getName(SecName)) {
+      return EC;
----------------
rafael wrote:
> You don't need the {
> 
Done.

================
Comment at: lib/Object/IRObjectFile.cpp:276
@@ +275,3 @@
+      StringRef SecContents;
+      if (std::error_code EC = Sec.getContents(SecContents)) {
+        return EC;
----------------
rafael wrote:
> You don't need the {
Done.

================
Comment at: lib/Object/SymbolicFile.cpp:65
@@ +64,3 @@
+
+    auto BCData = IRObjectFile::getBitcodeFromObject(Obj->get());
+    if (!BCData || BCData->getBuffer().empty())
----------------
rafael wrote:
> Explicit type, not auto I think.
Done.

================
Comment at: lib/Object/SymbolicFile.cpp:66
@@ +65,3 @@
+    auto BCData = IRObjectFile::getBitcodeFromObject(Obj->get());
+    if (!BCData || BCData->getBuffer().empty())
+      return std::move(Obj);
----------------
rafael wrote:
> The empty check looks odd.
Removed.

================
Comment at: tools/gold/gold-plugin.cpp:551
@@ -550,3 +550,3 @@
 
-  std::unique_ptr<MemoryBuffer> Buffer = MemoryBuffer::getMemBuffer(
-      StringRef((char *)View, File.filesize), "", false);
+  llvm::ErrorOr<MemoryBufferRef> MBOrErr =
+      object::IRObjectFile::getBitcodeFromMemBuffer(
----------------
rafael wrote:
> You have to handle the case where we don't have a get_view (bfd using the plugin).
> 
> On trunk we create a BufferRef in both branches of the if, so you can probably just move this after the end of the else.
Are you thinking of another function (claim_file_hook?) It looks like the code in this function (getModuleForFile) uses get_view unconditionally. I don't know if this is correct or not, but it seems to have worked up to now.

http://reviews.llvm.org/D4371