This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
gold: fix race in FileRead::~View.
- From: Ralf Wildenhues <Ralf dot Wildenhues at gmx dot de>
- To: binutils at sourceware dot org
- Date: Tue, 14 Dec 2010 07:55:43 +0100
- Subject: 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());