* gold: fix race in FileRead::~View.
@ 2010-12-14 6:58 Ralf Wildenhues
2010-12-14 15:26 ` Ian Lance Taylor
0 siblings, 1 reply; 2+ messages in thread
From: Ralf Wildenhues @ 2010-12-14 6:58 UTC (permalink / raw)
To: binutils
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());
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: gold: fix race in FileRead::~View.
2010-12-14 6:58 gold: fix race in FileRead::~View Ralf Wildenhues
@ 2010-12-14 15:26 ` Ian Lance Taylor
0 siblings, 0 replies; 2+ messages in thread
From: Ian Lance Taylor @ 2010-12-14 15:26 UTC (permalink / raw)
To: Ralf Wildenhues; +Cc: binutils
Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:
> BTW, is there a coding style recommendation on leading white space
> (a TAB for eight spaces, or expanded)?
Not really. I prefer a tab, but many Google programmers use editor
modes to expand all tabs to spaces, and I didn't try to fight it.
> 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.
I would usually write File_read::View::~View in the ChangeLog entry.
> 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;
Only do the lock and stats update if necessary. Write
if (!parameters->options_valid() || parameters->options().stats())
{
file_counts_initialize_lock.initialize();
Hold_optional_lock hl(file_counts_lock);
File_read::current_mapped_bytes -= this->size_;
}
as in File_read::release.
This is OK with that change.
Thanks for noticing this and thanks for the patch.
Ian
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2010-12-14 15:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-14 6:58 gold: fix race in FileRead::~View Ralf Wildenhues
2010-12-14 15:26 ` 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).