Add thread-safety to creation of MemCopy and modulo functions.

Add thread-safety to creation of MemCopy and modulo functions.


BUG=
TEST=release test-api MultipleIsolatesOnIndividualThreads on Windows X64 build.

Committed: http://code.google.com/p/v8/source/detail?r=7437

[Vitaly Repeshko, 2011-03-30 10:17:19]

Drive by comments.


Thanks,
Vitaly

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode215
src/platform-posix.cc:215: void OS::MemCopy(void* dest, const void* src, size_t
size) {
Why does this have to be in platform files? It'd be nice to avoid repeating it.

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode220
src/platform-posix.cc:220: memcopy_function = CreateMemCopyFunction();
You need a memory barrier between function creation and storing it to the global
pointer to make double-checked locking safe. Use MemoryBarrier or Release_Store
from atomicops.

[William Hesse, 2011-03-30 11:26:44]

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode215
src/platform-posix.cc:215: void OS::MemCopy(void* dest, const void* src, size_t
size) {
On 2011/03/30 10:17:19, Vitaly Repeshko wrote:
> Why does this have to be in platform files? It'd be nice to avoid repeating
it.

It depends on whether it is an inline function or not.  Since the global statics
need to be declared in a single compilation unit, and v8utils does not have a
.cc file, and MemCopy seems like a platform-specific replacement for the OS
memcpy function, this seemed like the right place for it.

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode220
src/platform-posix.cc:220: memcopy_function = CreateMemCopyFunction();
On 2011/03/30 10:17:19, Vitaly Repeshko wrote:
> You need a memory barrier between function creation and storing it to the
global
> pointer to make double-checked locking safe. Use MemoryBarrier or
Release_Store
> from atomicops.

Done.

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode220
src/platform-posix.cc:220: memcopy_function = CreateMemCopyFunction();
We end the function CreateMemCopyFunction() with a CPU::FlushICache, but that
does nothing, so I have also added a memory barrier there, just before returning
the pointer. 
On 2011/03/30 10:17:19, Vitaly Repeshko wrote:
> You need a memory barrier between function creation and storing it to the
global
> pointer to make double-checked locking safe. Use MemoryBarrier or
Release_Store
> from atomicops.

[Vitaly Repeshko, 2011-03-30 12:25:01]

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode215
src/platform-posix.cc:215: void OS::MemCopy(void* dest, const void* src, size_t
size) {
On 2011/03/30 11:26:44, William Hesse wrote:
> On 2011/03/30 10:17:19, Vitaly Repeshko wrote:
> > Why does this have to be in platform files? It'd be nice to avoid repeating
> it.
> 
> It depends on whether it is an inline function or not.  Since the global
statics
> need to be declared in a single compilation unit, and v8utils does not have a
> .cc file, and MemCopy seems like a platform-specific replacement for the OS
> memcpy function, this seemed like the right place for it.

I think creating v8utils.cc is a better solution. Otherwise, we have to write
this code in all platforms that support ia32. Am I missing something?

http://codereview.chromium.org/6777007/diff/1/src/platform-posix.cc#newcode220
src/platform-posix.cc:220: memcopy_function = CreateMemCopyFunction();
On 2011/03/30 11:26:44, William Hesse wrote:
> We end the function CreateMemCopyFunction() with a CPU::FlushICache, but that
> does nothing, so I have also added a memory barrier there, just before
returning
> the pointer. 
> On 2011/03/30 10:17:19, Vitaly Repeshko wrote:
> > You need a memory barrier between function creation and storing it to the
> global
> > pointer to make double-checked locking safe. Use MemoryBarrier or
> Release_Store
> > from atomicops.
> 

Well, strictly speaking we need both CPU and compiler memory barriers here.
Having one at the end of CreateMemCopyFunction() means only having the CPU
barrier. The compiler is free to put something in the global pointer before
storing the return value there. (Even though a sane compiler won't do that in
this case.) To prevent it from doing this we need a memory barrier here:
MemCopyFunction tmp = CreateMemCopyFunction();
MemoryBarrier();
memcopy_function = tmp;
--or--
Release_Store(&memcopy_function, CreateMemCopyFunction());

[fschneider, 2011-03-30 12:38:19]

Lgtm.