This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jvoung on Feb 10 2015, 11:54 AM.

Details

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.

Diff Detail

Event Timeline

jvoung updated this revision to Diff 19698.Feb 10 2015, 11:54 AM
jvoung retitled this revision from to Gold-plugin: Broaden scope of get/release_input_file to scope of Module..
jvoung updated this object.
jvoung edited the test plan for this revision. (Show Details)
jvoung added reviewers: rafael, nlewycky.
jvoung added a subscriber: Unknown Object (MLST).
rafael edited edge metadata.Feb 10 2015, 12:37 PM

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

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 updated this revision to Diff 19718.Feb 10 2015, 4:42 PM
jvoung edited edge metadata.

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

rafael accepted this revision.Feb 10 2015, 5:47 PM
rafael edited edge metadata.

LGTM.
Thanks!

This revision is now accepted and ready to land.Feb 10 2015, 5:47 PM
This revision was automatically updated to reflect the committed changes.