Gold-plugin: Broaden scope of get/release_input_file to scope of Module.

[jvoung]

Summary

Move calls to get_input_file and release_input_file out of
getModuleForFile(). Otherwise release_input_file may end up
unmapping a view of the file while the view is still being
used by the Module (on 32-bit hosts).

Fix for PR22482.

[• rafael]

When you say "binutils", you mean the bfd ld?

I think this is a bug in bfd ld. release_input_file is not supposed to handle views. See

https://sourceware.org/bugzilla/show_bug.cgi?id=17896#c9

[jvoung]

I've been testing with gold instead bfd ld.

The call chain is roughly this:

#0 gold::File_read::View::~View (this=0x86891e8) at ../../../src/binutils/gold/fileread.cc:141
#1 0x0821dafa in gold::File_read::clear_views (this=0x86734c8, mode=gold::File_read::CLEAR_VIEWS_NORMAL) at ../../../src/binutils/gold/fileread.cc:844
#2 0x0821ed7e in gold::File_read::release (this=0x86734c8) at ../../../src/binutils/gold/fileread.cc:300
#3 0x0821eef0 in gold::File_read::unlock (this=0x86734c8, task=0x8673260) at ../../../src/binutils/gold/fileread.cc:332
#4 0x0810dc97 in gold::Object::unlock (this=0x8679140, t=0x8673260) at ../../../src/binutils/gold/object.h:431
#5 0x083d9e49 in gold::Plugin_manager::release_input_file (this=0x8663ba8, handle=0) at ../../../src/binutils/gold/plugin.cc:801
#6 0x083d5f30 in gold::release_input_file (handle=0x0) at ../../../src/binutils/gold/plugin.cc:1521
#7 0xf7fca621 in allSymbolsReadHook(llvm::raw_fd_ostream*) () from LLVMgold.so

Where line 141 at the top of the stack is:

File_read::View::~View() {
//...

case DATA_MMAPPED:
  if (::munmap(const_cast<unsigned char*>(this->data_), this->size_) != 0)

//...
}

So are you saying that clear_views() shouldn't be called?

[jvoung]

Add test using --no-map-whole-files. Also, only pass the filesize.

[• rafael]

LGTM.
Thanks!

llvm/trunk/test/tools/gold/no-map-whole-file.ll

+; RUN: llvm-as -o %t.bc %s
+; RUN: ld -plugin %llvmshlibdir/LLVMgold.so -plugin-opt=emit-llvm \
+; RUN:    --no-map-whole-files -r -o %t2.bc %t.bc
+; RUN: llvm-dis < %t2.bc -o - | FileCheck %s
+
+; CHECK: main
+define i32 @main() {
+  ret i32 0
+}

llvm/trunk/tools/gold/gold-plugin.cpp

 static void freeSymName(ld_plugin_symbol &Sym) {
   free(Sym.name);
   free(Sym.comdat_key);
   Sym.name = nullptr;
   Sym.comdat_key = nullptr;
 }
 
 static std::unique_ptr<Module>
-getModuleForFile(LLVMContext &Context, claimed_file &F, raw_fd_ostream *ApiFile,
+getModuleForFile(LLVMContext &Context, claimed_file &F,
+                 off_t Filesize, raw_fd_ostream *ApiFile,
                  StringSet<> &Internalize, StringSet<> &Maybe) {
-  ld_plugin_input_file File;
-  if (get_input_file(F.handle, &File) != LDPS_OK)
-    message(LDPL_FATAL, "Failed to get file information");
 
   if (get_symbols(F.handle, F.syms.size(), &F.syms[0]) != LDPS_OK)
     message(LDPL_FATAL, "Failed to get symbol information");
 
   const void *View;
   if (get_view(F.handle, &View) != LDPS_OK)
     message(LDPL_FATAL, "Failed to get a view of file");
 
-  MemoryBufferRef BufferRef(StringRef((const char *)View, File.filesize), "");
+  MemoryBufferRef BufferRef(StringRef((const char *)View, Filesize), "");
   ErrorOr<std::unique_ptr<object::IRObjectFile>> ObjOrErr =
       object::IRObjectFile::create(BufferRef, Context);
 
   if (std::error_code EC = ObjOrErr.getError())
     message(LDPL_FATAL, "Could not read bitcode from file : %s",
             EC.message().c_str());
 
-  if (release_input_file(F.handle) != LDPS_OK)
-    message(LDPL_FATAL, "Failed to release file information");
-
   object::IRObjectFile &Obj = **ObjOrErr;
 
   Module &M = Obj.getModule();
 
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
   DenseSet<GlobalValue *> Drop;
static ld_plugin_status allSymbolsReadHook(raw_fd_ostream *ApiFile) {
   std::unique_ptr<Module> Combined(new Module("ld-temp.o", Context));
   Linker L(Combined.get());
 
   std::string DefaultTriple = sys::getDefaultTargetTriple();
 
   StringSet<> Internalize;
   StringSet<> Maybe;
   for (claimed_file &F : Modules) {
+    ld_plugin_input_file File;
+    if (get_input_file(F.handle, &File) != LDPS_OK)
+      message(LDPL_FATAL, "Failed to get file information");
     std::unique_ptr<Module> M =
-        getModuleForFile(Context, F, ApiFile, Internalize, Maybe);
+        getModuleForFile(Context, F, File.filesize, ApiFile,
+                         Internalize, Maybe);
     if (!options::triple.empty())
       M->setTargetTriple(options::triple.c_str());
     else if (M->getTargetTriple().empty()) {
       M->setTargetTriple(DefaultTriple);
     }
 
     if (L.linkInModule(M.get()))
       message(LDPL_FATAL, "Failed to link module");
+    if (release_input_file(F.handle) != LDPS_OK)
+      message(LDPL_FATAL, "Failed to release file information");
   }
 
   for (const auto &Name : Internalize) {
     GlobalValue *GV = Combined->getNamedValue(Name.first());
     if (GV)
       internalize(*GV);
   }