* [gold][patch] Fix file descriptor leak
@ 2010-11-05 20:48 Cary Coutant
2010-11-05 21:05 ` Ian Lance Taylor
0 siblings, 1 reply; 2+ messages in thread
From: Cary Coutant @ 2010-11-05 20:48 UTC (permalink / raw)
To: Ian Lance Taylor, Binutils
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
PR 10708 is about gold running out of file descriptors. I've found
several places where gold can re-open a file descriptor for reading
without holding a lock on the file; when that happens, the file
descriptor never gets marked as available for reuse. This can cause fd
exhaustion with --no-keep-files-mapped and a low limit on open file
descriptors (ulimit -n).
This patch fixes most of the file descriptor leakages by holding a
Task_lock_obj on the object while we're reading from it. There are
three more cases in the ARM backend -- two are easily fixable, but
Doug wants to restructure the third so that reading the file is
unnecessary, so I'll leave the arm.cc patches to him.
I originally added an assert in fileread.cc,
File_read::find_or_make_view(), to give an internal error if we ever
try to get a view on a file while the file is unlocked (that's how I
ended up finding most of these), but I'm leaving that out at least
until we fix the remaining known problems in arm.cc.
-cary
PR gold/10708
* copy-relocs.cc (Copy_relocs::emit_copy_reloc): Hold a lock on the
object when reading from the file.
* gold.cc (queue_middle_tasks): Hold a lock on the object when doing
second layout pass.
* icf.cc (preprocess_for_unique_sections): Hold a lock on the object
when reading section contents.
(get_section_contents): Likewise.
(icf::find_identical_sections): Likewise.
* mapfile.cc (Mapfile::print_discarded_sections): Hold a lock on the
object when reading from the file.
* plugin.cc (Plugin_manager::layout_deferred_objects): Hold a lock on
the object when doing deferred section layout.
[-- Attachment #2: gold-fd-leak-patch-1.txt --]
[-- Type: text/plain, Size: 6133 bytes --]
Index: copy-relocs.cc
===================================================================
RCS file: /cvs/src/src/gold/copy-relocs.cc,v
retrieving revision 1.11
diff -u -p -r1.11 copy-relocs.cc
--- copy-relocs.cc 25 Aug 2010 08:36:54 -0000 1.11
+++ copy-relocs.cc 5 Nov 2010 01:14:23 -0000
@@ -125,8 +125,17 @@ Copy_relocs<sh_type, size, big_endian>::
bool is_ordinary;
unsigned int shndx = sym->shndx(&is_ordinary);
gold_assert(is_ordinary);
- typename elfcpp::Elf_types<size>::Elf_WXword addralign =
- sym->object()->section_addralign(shndx);
+ typename elfcpp::Elf_types<size>::Elf_WXword addralign;
+
+ {
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from scan_relocs, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Object* obj = sym->object();
+ Task_lock_obj<Object> tl(dummy_task, obj);
+ addralign = obj->section_addralign(shndx);
+ }
typename Sized_symbol<size>::Value_type value = sym->value();
while ((value & (addralign - 1)) != 0)
Index: gold.cc
===================================================================
RCS file: /cvs/src/src/gold/gold.cc,v
retrieving revision 1.85
diff -u -p -r1.85 gold.cc
--- gold.cc 14 Oct 2010 22:10:22 -0000 1.85
+++ gold.cc 5 Nov 2010 01:14:23 -0000
@@ -359,6 +359,7 @@ queue_middle_tasks(const General_options
p != input_objects->relobj_end();
++p)
{
+ Task_lock_obj<Object> tlo(task, *p);
(*p)->layout(symtab, layout, NULL);
}
}
Index: icf.cc
===================================================================
RCS file: /cvs/src/src/gold/icf.cc,v
retrieving revision 1.15
diff -u -p -r1.15 icf.cc
--- icf.cc 28 Sep 2010 17:14:15 -0000 1.15
+++ icf.cc 5 Nov 2010 01:14:23 -0000
@@ -182,6 +182,11 @@ preprocess_for_unique_sections(const std
section_size_type plen;
if (section_contents == NULL)
{
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from queue_middle_tasks, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Task_lock_obj<Object> tl(dummy_task, secn.first);
const unsigned char* contents;
contents = secn.first->section_contents(secn.second,
&plen,
@@ -237,6 +242,11 @@ get_section_contents(bool first_iteratio
if (first_iteration)
{
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from queue_middle_tasks, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Task_lock_obj<Object> tl(dummy_task, secn.first);
contents = secn.first->section_contents(secn.second,
&plen,
false);
@@ -363,6 +373,12 @@ get_section_contents(bool first_iteratio
if (!first_iteration)
continue;
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from queue_middle_tasks, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Task_lock_obj<Object> tl(dummy_task, it_v->first);
+
uint64_t secn_flags = (it_v->first)->section_flags(it_v->second);
// This reloc points to a merge section. Hash the
// contents of this section.
@@ -682,6 +698,12 @@ Icf::find_identical_sections(const Input
p != input_objects->relobj_end();
++p)
{
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from queue_middle_tasks, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Task_lock_obj<Object> tl(dummy_task, *p);
+
for (unsigned int i = 0;i < (*p)->shnum(); ++i)
{
const char* section_name = (*p)->section_name(i).c_str();
Index: mapfile.cc
===================================================================
RCS file: /cvs/src/src/gold/mapfile.cc,v
retrieving revision 1.6
diff -u -p -r1.6 mapfile.cc
--- mapfile.cc 14 Dec 2009 19:53:05 -0000 1.6
+++ mapfile.cc 5 Nov 2010 01:14:23 -0000
@@ -347,6 +347,12 @@ Mapfile::print_discarded_sections(const
++p)
{
Relobj* relobj = *p;
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from Layout_task_runner, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Task_lock_obj<Object> tl(dummy_task, relobj);
+
unsigned int shnum = relobj->shnum();
for (unsigned int i = 0; i < shnum; ++i)
{
Index: plugin.cc
===================================================================
RCS file: /cvs/src/src/gold/plugin.cc,v
retrieving revision 1.40
diff -u -p -r1.40 plugin.cc
--- plugin.cc 14 Oct 2010 22:10:22 -0000 1.40
+++ plugin.cc 5 Nov 2010 01:14:23 -0000
@@ -361,7 +361,14 @@ Plugin_manager::layout_deferred_objects(
for (obj = this->deferred_layout_objects_.begin();
obj != this->deferred_layout_objects_.end();
++obj)
- (*obj)->layout_deferred_sections(this->layout_);
+ {
+ // Lock the object so we can read from it. This is only called
+ // single-threaded from queue_middle_tasks, so it is OK to lock.
+ // Unfortunately we have no way to pass in a Task token.
+ const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+ Task_lock_obj<Object> tl(dummy_task, *obj);
+ (*obj)->layout_deferred_sections(this->layout_);
+ }
}
// Call the cleanup handlers.
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [gold][patch] Fix file descriptor leak
2010-11-05 20:48 [gold][patch] Fix file descriptor leak Cary Coutant
@ 2010-11-05 21:05 ` Ian Lance Taylor
0 siblings, 0 replies; 2+ messages in thread
From: Ian Lance Taylor @ 2010-11-05 21:05 UTC (permalink / raw)
To: Cary Coutant; +Cc: Binutils
Cary Coutant <ccoutant@google.com> writes:
> PR gold/10708
> * copy-relocs.cc (Copy_relocs::emit_copy_reloc): Hold a lock on the
> object when reading from the file.
> * gold.cc (queue_middle_tasks): Hold a lock on the object when doing
> second layout pass.
> * icf.cc (preprocess_for_unique_sections): Hold a lock on the object
> when reading section contents.
> (get_section_contents): Likewise.
> (icf::find_identical_sections): Likewise.
> * mapfile.cc (Mapfile::print_discarded_sections): Hold a lock on the
> object when reading from the file.
> * plugin.cc (Plugin_manager::layout_deferred_objects): Hold a lock on
> the object when doing deferred section layout.
This is OK.
We should add that assert you mentioned as soon as we can.
We should also add some notion to the workqueue as to whether we are
running single-threaded. We can use that to assert if there are ever
two jobs runnable when we think we are single-threaded.
We should then change these dummy_task Task_lock_obj calls to a
different locker which asserts that we are single threaded, does not
require a task, and calls variants of obj->lock and obj->unlock which do
not use a task.
Thanks for tracking this down.
Ian
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2010-11-05 21:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-05 20:48 [gold][patch] Fix file descriptor leak Cary Coutant
2010-11-05 21:05 ` Ian Lance Taylor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).