Surfacing the process create time in google_breakpad::ProcessState

Surfacing the process create time in google_breakpad::ProcessState
and updating minidump_stackwalk to show process uptime.

I tested this with a minidump from Chrome and I got a result that
is inline with what the Windows debugger is showing for that dump:

minidump_stackwalk output:
--------------------------
Process uptime: 601 seconds

WinDBG output:
--------------
Process Uptime: 0 days 0:10:01.000

I didn't update the machine readable output of minidump_stackwalk
on purpose in order to avoid breaking someone that uses it.
It can be added later to the machine output if needed.

R=mark@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1406

[Mark Mentovai]

I’m having a hard time with breakpad.appspot.com. Much of it isn’t working
properly, including the ability to enter inline comments. It’s also very slow to
load.

minidump_processor.h:736: There is already a misc_info() accessor. It shouldn’t
be necessary to make misc_info_ protected. In fact, all of the changes to this
file seem unnecessary. Is this just for testing? If so, I’d suggest finding a
better way to make the tests work. We shouldn’t have to pervert things like
access control so much to make tests work.

minidump_processor.cc:141: The process creation time is one of the least
important things in the list, relative to everything else. The list now has some
sort of ordering to it, where things basically follow the order of utility in a
minidump file. Let’s put has_process_create_time either after threads or at the
end.

[ivanpe]

On 2014/11/25 04:29:08, Mark Mentovai wrote:
> I’m having a hard time with http://breakpad.appspot.com. Much of it isn’t
working
> properly, including the ability to enter inline comments. It’s also very slow
to
> load.
> 
> minidump_processor.h:736: There is already a misc_info() accessor. It
shouldn’t
> be necessary to make misc_info_ protected. In fact, all of the changes to this
> file seem unnecessary. Is this just for testing? If so, I’d suggest finding a
> better way to make the tests work. We shouldn’t have to pervert things like
> access control so much to make tests work.

It was required for testing only.  Making the test class friend is another
option.  PTAL.

> 
> minidump_processor.cc:141: The process creation time is one of the least
> important things in the list, relative to everything else. The list now has
some
> sort of ordering to it, where things basically follow the order of utility in
a
> minidump file. Let’s put has_process_create_time either after threads or at
the
> end.

Done.

[Mark Mentovai]

LGTM.

And breakpad.appspot.com seems to be working correctly again.

[Ted Mielczarek]

Note that we've switched Socorro away from using |minidump_stackwalk| directly
in favor of a custom binary, like we should have from the start. Don't worry
about the machine readable output anymore, we'll just leave it be for now, and
maybe remove it at a later date.

https://breakpad.appspot.com/7754002/diff/40001/src/google_breakpad/processor...
File src/google_breakpad/processor/process_state.h (right):

https://breakpad.appspot.com/7754002/diff/40001/src/google_breakpad/processor...
src/google_breakpad/processor/process_state.h:130: uint32_t
process_create_time_;
I find it interesting that the misc info has this in a 32-bit value, when 64-bit
Windows has switched to a 64-bit time_t. I suppose if they ever change that
we'll have to re-plumb all this anyway, so it's not a big deal.

[mmandlis]

Such a micro-nit that it's even embarrassing to comment on)))

https://breakpad.appspot.com/7754002/diff/40001/src/processor/minidump_stackw...
File src/processor/minidump_stackwalk.cc (right):

https://breakpad.appspot.com/7754002/diff/40001/src/processor/minidump_stackw...
src/processor/minidump_stackwalk.cc:692: // avaiable in the dump.
nit: available (typo)

[ivanpe]

On 2014/11/25 14:13:49, Ted Mielczarek wrote:
> Note that we've switched Socorro away from using |minidump_stackwalk| directly
> in favor of a custom binary, like we should have from the start. Don't worry
> about the machine readable output anymore, we'll just leave it be for now, and
> maybe remove it at a later date.
> 
>
https://breakpad.appspot.com/7754002/diff/40001/src/google_breakpad/processor...
> File src/google_breakpad/processor/process_state.h (right):
> 
>
https://breakpad.appspot.com/7754002/diff/40001/src/google_breakpad/processor...
> src/google_breakpad/processor/process_state.h:130: uint32_t
> process_create_time_;
> I find it interesting that the misc info has this in a 32-bit value, when
64-bit
> Windows has switched to a 64-bit time_t. I suppose if they ever change that
> we'll have to re-plumb all this anyway, so it's not a big deal.

Thanks, Ted.  It is good to know that we don't need to worry about the machine
format anymore.  Regarding the use of the 32-bit value, I was just preserving
the type from the structure where this came from:

  /* The next 3 fields are only valid if flags1 contains
   * MD_MISCINFO_FLAGS1_PROCESS_TIMES. */
  uint32_t process_create_time;  /* time_t process started */
  uint32_t process_user_time;    /* seconds of user CPU time */
  uint32_t process_kernel_time;  /* seconds of kernel CPU time */

[ivanpe]

On 2014/11/25 19:22:37, mmandlis wrote:
> Such a micro-nit that it's even embarrassing to comment on)))
> 
>
https://breakpad.appspot.com/7754002/diff/40001/src/processor/minidump_stackw...
> File src/processor/minidump_stackwalk.cc (right):
> 
>
https://breakpad.appspot.com/7754002/diff/40001/src/processor/minidump_stackw...
> src/processor/minidump_stackwalk.cc:692: // avaiable in the dump.
> nit: available (typo)

Done.

[ivanpe]

Committed patchset #4 manually as r1406 (presubmit successful).