[gold][patch] Fix file descriptor leak

[gold][patch] Fix file descriptor leak

[Cary Coutant]

[-- 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.

Re: [gold][patch] Fix file descriptor leak

[Ian Lance Taylor]

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