Added ctrl-\ command that dumps isolate stacks to dart binary.

Added ctrl-\ command that dumps isolate stacks to dart binary.

Committed: https://code.google.com/p/dart/source/detail?r=21589

[siva, 2013-04-04 00:47:15]

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc#n...
runtime/bin/vmstats_impl.cc:343: return dup;
Please see comment in other CL about moving this as strndup
to platform/c99_support_win.h

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc#n...
runtime/bin/vmstats_impl.cc:364: itr != instance_->isolate_table_.end(); ++itr)
{
This map could potentially be changing while this function
is iterating through it right.

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc#n...
runtime/bin/vmstats_impl.cc:374: SeekNamedValue("name", &scanner);
How come you are not using the JSONReader class which
has a Seek(const char* name) method to seek out name/value
pairs. Would be more efficient than repeated scanning
starting from the first again.

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl_linu...
File runtime/bin/vmstats_impl_linux.cc (right):

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl_linu...
runtime/bin/vmstats_impl_linux.cc:16: }
You should not be handling any other signals right so it should be an
UNREACHABLE or Fatal...

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl_linu...
runtime/bin/vmstats_impl_linux.cc:21: if (signal(SIGQUIT, sig_handler) ==
SIG_ERR) {
You should be using the sigaction call here and not signal.
(from man page: The behavior of signal() varies across
UNIX versions, and has also varied historically across
different versions of Linux.   Avoid  its  use:
use sigaction(2) instead.).

sigaction always you to control the behavior of signal
delivery when you are inside a signal handler (that is
nested invocation of the signal handler).

[Tom Ball, 2013-04-04 20:05:53]

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc#n...
runtime/bin/vmstats_impl.cc:343: return dup;
On 2013/04/04 00:47:15, siva wrote:
> Please see comment in other CL about moving this as strndup
> to platform/c99_support_win.h

Done.

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc#n...
runtime/bin/vmstats_impl.cc:364: itr != instance_->isolate_table_.end(); ++itr)
{
On 2013/04/04 00:47:15, siva wrote:
> This map could potentially be changing while this function
> is iterating through it right.

Right -- I added a MutexLocker.

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl.cc#n...
runtime/bin/vmstats_impl.cc:374: SeekNamedValue("name", &scanner);
On 2013/04/04 00:47:15, siva wrote:
> How come you are not using the JSONReader class which
> has a Seek(const char* name) method to seek out name/value
> pairs. Would be more efficient than repeated scanning
> starting from the first again.

Its Seek is limited to top-level name-value pairs, so any data inside an array
or object is skipped. I debated enhancing JSONReader, but it would have doubled
its size for this one small client.

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl_linu...
File runtime/bin/vmstats_impl_linux.cc (right):

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl_linu...
runtime/bin/vmstats_impl_linux.cc:16: }
On 2013/04/04 00:47:15, siva wrote:
> You should not be handling any other signals right so it should be an
> UNREACHABLE or Fatal...

Done.

https://codereview.chromium.org/13603002/diff/1/runtime/bin/vmstats_impl_linu...
runtime/bin/vmstats_impl_linux.cc:21: if (signal(SIGQUIT, sig_handler) ==
SIG_ERR) {
On 2013/04/04 00:47:15, siva wrote:
> You should be using the sigaction call here and not signal.
> (from man page: The behavior of signal() varies across
> UNIX versions, and has also varied historically across
> different versions of Linux.   Avoid  its  use:
> use sigaction(2) instead.).
> 
> sigaction always you to control the behavior of signal
> delivery when you are inside a signal handler (that is
> nested invocation of the signal handler).

Replaced with sigaction, except for Windows because it only has signal
(http://www.suacommunity.com/dictionary/sigaction-entry.php).

[siva, 2013-04-05 15:55:09]

https://codereview.chromium.org/13603002/diff/7001/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/13603002/diff/7001/runtime/bin/vmstats_impl.c...
runtime/bin/vmstats_impl.cc:354: MonitorLocker ml(instance_monitor_);
This could lead to potential deadlocks if the signal is handled by a thread
running an Isolate which is already holding this lock (say an Isolate is on it's
way out and it is trying to remove itself from the list and the signal handler
runs on that thread).

https://codereview.chromium.org/13603002/diff/7001/runtime/bin/vmstats_impl.c...
runtime/bin/vmstats_impl.cc:380: char* trace =
VmStatusService::GetVmStatus(buffer);
I assume this schedules an interrupt on the isolate and waits for the
stack_trace to be collected. I was wondering how this would work if the signal
was handled by the thread which is currently running the Isolate, wouldn't it
lead to a dead lock.

The code it would execute I presume is:
char* Isolate::DoStacktraceInterrupt(Dart_IsolateInterruptCallback cb) {
  ASSERT(stacktrace_ == NULL);
  SetVmStatsCallback(cb);
  if (status_sync == NULL) {
    status_sync = new Monitor();
  }
  ScheduleInterrupts(Isolate::kVmStatusInterrupt);
  {
    MonitorLocker ml(status_sync);
    if (stacktrace_ == NULL) {  // It may already be available.
      ml.Wait();
    }
  }
  SetVmStatsCallback(NULL);
  ASSERT(stacktrace_ != NULL);
  // result is freed by VmStats::WebServer().
  char* result = stacktrace_;
  stacktrace_ = NULL;
  return result;
}

[Tom Ball, 2013-04-09 22:45:25]

Addressed Siva's feedback. In particular, a thread is spawned for each
signal-initiated stack dump, so that the VmStats::instance_monitor_ can't get in
a deadly embrace (since signal handling code is executed on the current thread,
that thread may already have locked the monitor).

[Tom Ball, 2013-04-11 23:40:08]

PTAL, as I've addressed all of Siva's comments.

[siva, 2013-04-16 00:28:06]

lgtm

[siva, 2013-04-16 00:28:07]

lgtm

[Tom Ball, 2013-04-16 23:11:41]

Committed patchset #10 manually as r21589 (presubmit successful).