Update platform-solaris to support runtime profiling

Update platform-solaris to support runtime profiling

[Vitaly Repeshko, 2011-03-10 22:08:26]

LGTM. I'll fix the comment and land.


Thanks,
Vitaly

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

http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc#newcode683
src/platform-solaris.cc:683: /*
Instead of commented code, we should say that there doesn't seem to be a way do
deliver a signal to a particular thread.

[Vitaly Repeshko, 2011-03-10 22:14:27]

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

http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc#newcode683
src/platform-solaris.cc:683: /*
On 2011/03/10 22:08:26, Vitaly Repeshko wrote:
> Instead of commented code, we should say that there doesn't seem to be a way
do
> deliver a signal to a particular thread.

Wait a second. There's thr_kill which seems to do right thing. Do you have a
Solaris system to try it on? You'll need to give it a return value from
thr_self.

[ry, 2011-03-10 22:17:15]

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

http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc#newcode683
src/platform-solaris.cc:683: /*
yes. will update - going to use pthread_kill

[Vitaly Repeshko, 2011-03-10 22:20:18]

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

http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc#newcode683
src/platform-solaris.cc:683: /*
On 2011/03/10 22:17:15, ry wrote:
> yes. will update - going to use pthread_kill

pthread_kill didn't work on linux because you can't send it to a dead thread and
there's no easy way to get notified of thread death. We had to use lower level
calls there.

[ry, 2011-03-10 22:24:04]

On 2011/03/10 22:20:18, Vitaly Repeshko wrote:
> http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc
> File src/platform-solaris.cc (right):
> 
>
http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc#newcode683
> src/platform-solaris.cc:683: /*
> On 2011/03/10 22:17:15, ry wrote:
> > yes. will update - going to use pthread_kill
> 
> pthread_kill didn't work on linux because you can't send it to a dead thread
and
> there's no easy way to get notified of thread death. We had to use lower level
> calls there.

Is there a test that tests this?

[Vitaly Repeshko, 2011-03-10 22:27:02]

On 2011/03/10 22:24:04, ry wrote:
> On 2011/03/10 22:20:18, Vitaly Repeshko wrote:
> > http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc
> > File src/platform-solaris.cc (right):
> > 
> >
>
http://codereview.chromium.org/6674002/diff/1/src/platform-solaris.cc#newcode683
> > src/platform-solaris.cc:683: /*
> > On 2011/03/10 22:17:15, ry wrote:
> > > yes. will update - going to use pthread_kill
> > 
> > pthread_kill didn't work on linux because you can't send it to a dead thread
> and
> > there's no easy way to get notified of thread death. We had to use lower
level
> > calls there.
> 
> Is there a test that tests this?

Yes, IIRC our cctest suite caught a crash in one of the threading tests with
pthread_kill.

[ry, 2011-03-10 23:27:28]

> Yes, IIRC our cctest suite caught a crash in one of the 
> threading tests with pthread_kill.

cctest passes 100% for both release and debug with patch set 2.

[Vitaly Repeshko, 2011-03-11 00:08:27]

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc
File src/platform-solaris.cc (right):

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc#newc...
src/platform-solaris.cc:683: pthread_kill(vm_tgid_, SIGPROF);
Does work with vm_tid_ here?

[Vyacheslav Egorov (Chromium), 2011-03-11 00:30:47]

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc
File src/platform-solaris.cc (right):

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc#newc...
src/platform-solaris.cc:683: pthread_kill(vm_tgid_, SIGPROF);
This looks really suspicious. vm_tgid_ is not a pthread_t/thread id at all. It's
a PID. 

Can we check pthread_kill return result? I have a feeling that it just returns
EINVAL.

[ry, 2011-03-11 01:02:00]

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc
File src/platform-solaris.cc (right):

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc#newc...
src/platform-solaris.cc:683: pthread_kill(vm_tgid_, SIGPROF);
On 2011/03/11 00:08:27, Vitaly Repeshko wrote:
> Does work with vm_tid_ here?

Oops. With that I get crashes

[Vitaly Repeshko, 2011-03-11 01:08:46]

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc
File src/platform-solaris.cc (right):

http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc#newc...
src/platform-solaris.cc:683: pthread_kill(vm_tgid_, SIGPROF);
On 2011/03/11 01:02:00, ry wrote:
> On 2011/03/11 00:08:27, Vitaly Repeshko wrote:
> > Does work with vm_tid_ here?
> 
> Oops. With that I get crashes

So if it crashes because of a stale thread handle, we have to use something
lower level. Please try using thr_* stuff.

[ry, 2011-03-11 01:14:50]

On 2011/03/11 01:08:46, Vitaly Repeshko wrote:
> http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc
> File src/platform-solaris.cc (right):
> 
>
http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc#newc...
> src/platform-solaris.cc:683: pthread_kill(vm_tgid_, SIGPROF);
> On 2011/03/11 01:02:00, ry wrote:
> > On 2011/03/11 00:08:27, Vitaly Repeshko wrote:
> > > Does work with vm_tid_ here?
> > 
> > Oops. With that I get crashes
> 
> So if it crashes because of a stale thread handle, we have to use something
> lower level. Please try using thr_* stuff.

the thr_kill is the same function - still get crashes. It's uncertain if there
is a different way to do this in Solaris...

[Vitaly Repeshko, 2011-03-11 01:32:09]

On 2011/03/11 01:14:50, ry wrote:
> On 2011/03/11 01:08:46, Vitaly Repeshko wrote:
> > http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc
> > File src/platform-solaris.cc (right):
> > 
> >
>
http://codereview.chromium.org/6674002/diff/5001/src/platform-solaris.cc#newc...
> > src/platform-solaris.cc:683: pthread_kill(vm_tgid_, SIGPROF);
> > On 2011/03/11 01:02:00, ry wrote:
> > > On 2011/03/11 00:08:27, Vitaly Repeshko wrote:
> > > > Does work with vm_tid_ here?
> > > 
> > > Oops. With that I get crashes
> > 
> > So if it crashes because of a stale thread handle, we have to use something
> > lower level. Please try using thr_* stuff.
> 
> the thr_kill is the same function - still get crashes. It's uncertain if there
> is a different way to do this in Solaris...

OK, let's give up for now. In the worst case we'll just get no statistical
profiling ticks (so running with --prof won't produce profiles), but the
Crankshaft infrastructure won't be affected because it doesn't depend on
signals. I'll land the first patch set with the comment updated.

-- Vitaly

[Vitaly Repeshko, 2011-03-11 01:45:22]

Landed in r7140. Closing code review.