Linux profiler: check whether signal handler is called in the VM thread.

Linux profiler: check whether signal handler is called in the VM thread.

I have several Chromium's core files having SIGPROF signal handler called in the context of an arbitrary thread, causing a crash. This change introduces checking of current thread in the signal handler.

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

[Dean McNamee, 2009-09-02 10:32:44]

Why is it called on another thread?

[Søren Thygesen Gjesse, 2009-09-02 11:22:53]

From what I can see this will only work when V8 is only ever used from one
thread, as is the case for the Chromium renderer process. However if different
threads are using V8 then this will only get samples from one of these.

[Mikhail Naganov, 2009-09-03 11:51:19]

I've implemented a check for the case of multiple VM threads.

[Mads Ager (chromium), 2009-09-03 12:08:53]

http://codereview.chromium.org/171115/diff/3001/4001
File src/platform-linux.cc (right):

http://codereview.chromium.org/171115/diff/3001/4001#newcode619
Line 619: // it means that the VM thread is running, and trying to sample it
will
Could you update the comment to state that only the stack traversal is unsafe in
this case?  The rest of the sampling takes place as normal.

http://codereview.chromium.org/171115/diff/3001/4001#newcode626
Line 626: || ThreadManager::CurrentId() != 0) return true;
Could you move the '||' to the previous line to match (most) of the rest of the
code?

I think the first thread will get thread_id 0, so this check will not be
precise.  We should probably add a ThreadManager::HasThreadId method instead
that returns Thread::HasThreadLocal(thread_id_key).

Thinking about this some more, there is no guarantee that the current thread is
the thread using V8 just because it has a v8 thread id.  It could be doing stuff
that does not involve v8 when it gets the interrupt.

[Mikhail Naganov, 2009-09-04 08:44:19]

OK, I learned the implementation of threading in V8 and updated my code. I also
wrote a test to be confident.

What I changed in v8threads is that thread ids are now 1-based, instead of
0-based. As NULL is defined as 0, having a thread id value 0 in TLS is
indistinguishable from not having this value stored.

Mads, Soeren, can you take another look, please?

http://codereview.chromium.org/171115/diff/3001/4001
File src/platform-linux.cc (right):

http://codereview.chromium.org/171115/diff/3001/4001#newcode619
Line 619: // it means that the VM thread is running, and trying to sample it
will
On 2009/09/03 12:08:53, Mads Ager wrote:
> Could you update the comment to state that only the stack traversal is unsafe
in
> this case?  The rest of the sampling takes place as normal.

Done.

http://codereview.chromium.org/171115/diff/3001/4001#newcode626
Line 626: || ThreadManager::CurrentId() != 0) return true;
On 2009/09/03 12:08:53, Mads Ager wrote:
> Could you move the '||' to the previous line to match (most) of the rest of
the
> code?
> 
> I think the first thread will get thread_id 0, so this check will not be
> precise.  We should probably add a ThreadManager::HasThreadId method instead
> that returns Thread::HasThreadLocal(thread_id_key).
> 
> Thinking about this some more, there is no guarantee that the current thread
is
> the thread using V8 just because it has a v8 thread id.  It could be doing
stuff
> that does not involve v8 when it gets the interrupt.

Done.

[Mads Ager (chromium), 2009-09-04 09:00:00]

LGTM

[Søren Thygesen Gjesse, 2009-09-04 09:29:57]

LGTM

http://codereview.chromium.org/171115/diff/6001/7002
File src/v8threads.cc (right):

http://codereview.chromium.org/171115/diff/6001/7002#newcode244
Line 244: int ThreadManager::last_id_ = 0;
Please add a comment about that a thread cannot have id 0.

http://codereview.chromium.org/171115/diff/6001/7002#newcode331
Line 331: int thread_id = ++last_id_;
Maybe assert thread_id > 0.

[Mikhail Naganov, 2009-09-04 11:09:22]

Thanks!

http://codereview.chromium.org/171115/diff/6001/7002
File src/v8threads.cc (right):

http://codereview.chromium.org/171115/diff/6001/7002#newcode244
Line 244: int ThreadManager::last_id_ = 0;
On 2009/09/04 09:29:57, Søren Gjesse wrote:
> Please add a comment about that a thread cannot have id 0.

Done.

http://codereview.chromium.org/171115/diff/6001/7002#newcode331
Line 331: int thread_id = ++last_id_;
On 2009/09/04 09:29:57, Søren Gjesse wrote:
> Maybe assert thread_id > 0.

Done.