This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

gold: fix race in FileRead::~View.


When the first Relocate_task is started, it seems that the modification
of File_read::current_mapped_bytes in ~View should be protected by a
lock, cf. this helgrind output with --threads --thread-count=4:

==23563== Thread #2 was created
==23563==    at 0x5CFC6CE: clone (clone.S:77)
==23563==    by 0x4E37172: pthread_create@@GLIBC_2.2.5 (createthread.c:75)
==23563==    by 0x4C2CA4A: pthread_create_WRK (hg_intercepts.c:257)
==23563==    by 0x4C2CB5E: pthread_create@* (hg_intercepts.c:288)
==23563==    by 0x68BC71: gold::Workqueue_thread::Workqueue_thread(gold::Workqueue_threader_threadpool*, int) (workqueue-threads.cc:87)
==23563==    by 0x68C139: gold::Workqueue_threader_threadpool::set_thread_count(int) (workqueue-threads.cc:168)
==23563==    by 0x68B38C: gold::Workqueue::set_thread_count(int) (workqueue.cc:507)
==23563==    by 0x526589: gold::queue_initial_tasks(gold::General_options const&, gold::Dirsearch&, gold::Command_line const&, gold::Workqueue*, gold::Input_objects*, gold::Symbol_table*, gold::Layout*, gold::Mapfile*) (gold.cc:179)
==23563==    by 0x40533B: main (main.cc:244)
==23563== 
==23563== Possible data race during write of size 8 at 0xa42108 by thread #2
==23563==    at 0x52182C: gold::File_read::View::~View() (fileread.cc:73)
==23563==    by 0x523558: gold::File_read::clear_views(gold::File_read::Clear_views_mode) (fileread.cc:714)
==23563==    by 0x521FFE: gold::File_read::release() (fileread.cc:215)
==23563==    by 0x6085ED: gold::Object::release() (object.h:306)
==23563==    by 0x60C265: gold::Relocate_task::run(gold::Workqueue*) (reloc.cc:239)
==23563==    by 0x68AD00: gold::Workqueue::find_and_run_task(int) (workqueue.cc:319)
==23563==    by 0x68B33D: gold::Workqueue::process(int) (workqueue.cc:495)
==23563==    by 0x68C25F: gold::Workqueue_threader_threadpool::process(int) (workqueue-internal.h:92)
==23563==    by 0x68BD58: gold::Workqueue_thread::thread_body(void*) (workqueue-threads.cc:117)
==23563==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==23563==    by 0x4E369C9: start_thread (pthread_create.c:300)
==23563==    by 0x5CFC70C: clone (clone.S:112)


The patch below seems to avoid the race.  OK to commit?
Alternatively, would you prefer a File_read::remove_view helper
function?

BTW, is there a coding style recommendation on leading white space
(a TAB for eight spaces, or expanded)?

Thanks,
Ralf

gold: fix race in FileRead::~View.

gold/ChangeLog:
2010-12-12  Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	* fileread.cc (file_counts_lock, file_counts_initialize_lock)
	(total_mapped_bytes, current_mapped_bytes, maximum_mapped_bytes):
	Move definition before File_read::View member definitions.
	(~View): Initialize and hold lock before updating
	current_mapped_bytes.

diff --git a/gold/fileread.cc b/gold/fileread.cc
index a16738a..b8be176 100644
--- a/gold/fileread.cc
+++ b/gold/fileread.cc
@@ -57,6 +57,17 @@ readv(int, const iovec*, int)
 namespace gold
 {
 
+// Class File_read.
+
+// A lock for the File_read static variables.
+static Lock* file_counts_lock = NULL;
+static Initialize_lock file_counts_initialize_lock(&file_counts_lock);
+
+// The File_read static variables.
+unsigned long long File_read::total_mapped_bytes;
+unsigned long long File_read::current_mapped_bytes;
+unsigned long long File_read::maximum_mapped_bytes;
+
 // Class File_read::View.
 
 File_read::View::~View()
@@ -68,10 +79,14 @@ File_read::View::~View()
       delete[] this->data_;
       break;
     case DATA_MMAPPED:
-      if (::munmap(const_cast<unsigned char*>(this->data_), this->size_) != 0)
-        gold_warning(_("munmap failed: %s"), strerror(errno));
-      File_read::current_mapped_bytes -= this->size_;
-      break;
+      {
+	if (::munmap(const_cast<unsigned char*>(this->data_), this->size_) != 0)
+	  gold_warning(_("munmap failed: %s"), strerror(errno));
+	file_counts_initialize_lock.initialize();
+	Hold_optional_lock hl(file_counts_lock);
+	File_read::current_mapped_bytes -= this->size_;
+	break;
+      }
     case DATA_NOT_OWNED:
       break;
     default:
@@ -100,15 +115,6 @@ File_read::View::is_locked()
 
 // Class File_read.
 
-// A lock for the File_read static variables.
-static Lock* file_counts_lock = NULL;
-static Initialize_lock file_counts_initialize_lock(&file_counts_lock);
-
-// The File_read static variables.
-unsigned long long File_read::total_mapped_bytes;
-unsigned long long File_read::current_mapped_bytes;
-unsigned long long File_read::maximum_mapped_bytes;
-
 File_read::~File_read()
 {
   gold_assert(this->token_.is_writable());


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]