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

[Rafael Ávila de Espíndola]

It feels like these new format support should be documented somewhere. BitCodeFormat.rst maybe?

Also, an entry in the release notes would be nice.

================
Comment at: include/llvm/Object/IRObjectFile.h:54
@@ -51,1 +53,3 @@
 
+  static ErrorOr<MemoryBufferRef> getBitcodeFromObject(ObjectFile *Obj);
+  static ErrorOr<MemoryBufferRef>
----------------
Some comments saying what these function do would be nice.

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

================
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>"));
----------------
I don't think we use auto for cases like this.

================
Comment at: lib/LTO/LTOModule.cpp:60
@@ -57,1 +59,3 @@
+      MemoryBufferRef(StringRef((const char *)Mem, Length), "<mem>"));
+  return BCData && !BCData->getBuffer().empty();
 }
----------------
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.

================
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))
----------------
It seems tempting to just get the buffer and forward to IRObjectFile::getBitcodeFromMemBuffer. Would avoid the duplicated switch over the type.

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


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


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

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

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

================
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(
----------------
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.

http://reviews.llvm.org/D4371