Fix data race when concurrent compilation is aborted due to dependency change.

Fix data race when concurrent compilation is aborted due to dependency change.

R=marja@chromium.org
BUG=chromium:419189
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=24361

[marja, 2014-10-01 08:36:40]

Umm, you're assuming that writing a bool is atomic, right? But what if
sizeof(bool) > 1?

[marja, 2014-10-01 08:37:50]

( http://stackoverflow.com/questions/4897844/is-sizeofbool-defined )

[marja, 2014-10-01 08:44:39]

Clarification: I don't think this can malfunction even if writing the bool is
not atomic, since 1) one thread writes it and one thread reads it 2) the bool
only goes from false to true and never back, so even if the reader thread reads
it between the non-atomic write (when the bool contains several bytes: when some
of the bytes have been written and some not), it'll get whatever value, but it
will get the correct value when it reads it the next time.

IOW, it's a bit sloppy but I guess it should work? LGTM?

[Jarin, 2014-10-01 08:56:33]

On 2014/10/01 08:44:39, marja wrote:
> Clarification: I don't think this can malfunction even if writing the bool is
> not atomic, since 1) one thread writes it and one thread reads it 2) the bool
> only goes from false to true and never back, so even if the reader thread
reads
> it between the non-atomic write (when the bool contains several bytes: when
some
> of the bytes have been written and some not), it'll get whatever value, but it
> will get the correct value when it reads it the next time.
> 
> IOW, it's a bit sloppy but I guess it should work? LGTM?

not lgtm.

As discussed offline, there should be no data races on normal variables, i.e.,
variables that are not AtomicWord*. Please use AtomicWord +
Release_Store/Acquire_Load for communication. Otherwise, compiler and hardware
are allowed to do crazy stuff to your code (and TSan will complain).

[Yang, 2014-10-01 09:05:51]

On 2014/10/01 08:56:33, jarin wrote:
> On 2014/10/01 08:44:39, marja wrote:
> > Clarification: I don't think this can malfunction even if writing the bool
is
> > not atomic, since 1) one thread writes it and one thread reads it 2) the
bool
> > only goes from false to true and never back, so even if the reader thread
> reads
> > it between the non-atomic write (when the bool contains several bytes: when
> some
> > of the bytes have been written and some not), it'll get whatever value, but
it
> > will get the correct value when it reads it the next time.
> > 
> > IOW, it's a bit sloppy but I guess it should work? LGTM?
> 
> not lgtm.
> 
> As discussed offline, there should be no data races on normal variables, i.e.,
> variables that are not AtomicWord*. Please use AtomicWord +
> Release_Store/Acquire_Load for communication. Otherwise, compiler and hardware
> are allowed to do crazy stuff to your code (and TSan will complain).

In this case, it won't malfunction. The field is written from the main thread
when the concurrent job is running, and read from the main thread later when the
concurrent job is done running. The DCHECK guarantees that. The original bug
happened because it was stored on the shared flags_ field which the concurrent
thread also reads from.

I changed it to atomic ops nevertheless, though imo unnecessary.

[marja, 2014-10-01 09:08:09]

Whoops, I misunderstood this (didn't notice the DCHECK). If the variable is only
read from the main thread, there's no need for locking! Probably jarin@ too. So
I'd prefer the original patch set.

[Jarin, 2014-10-01 09:13:27]

On 2014/10/01 09:08:09, marja wrote:
> Whoops, I misunderstood this (didn't notice the DCHECK). If the variable is
only
> read from the main thread, there's no need for locking! Probably jarin@ too.
So
> I'd prefer the original patch set.

lgtm.

Ah, then everything is ok. I am sorry for the confusion.

[Yang, 2014-10-01 09:18:00]

On 2014/10/01 09:13:27, jarin wrote:
> On 2014/10/01 09:08:09, marja wrote:
> > Whoops, I misunderstood this (didn't notice the DCHECK). If the variable is
> only
> > read from the main thread, there's no need for locking! Probably jarin@ too.
> So
> > I'd prefer the original patch set.
> 
> lgtm.
> 
> Ah, then everything is ok. I am sorry for the confusion.

My comment was a bit confusing. I changed its wording.

[Yang, 2014-10-01 11:41:28]

Committed patchset #3 (id:40001) manually as 24361 (presubmit successful).