Updating MDRawMiscInfo to support verions 3 and 4 of the MINIDUMP_MISC_INFO_N structure

Updating MDRawMiscInfo to support verions 3 and 4 of the MINIDUMP_MISC_INFO_N
structure.  Added the necessary code for swapping and string conversion from
UTF-16.  Found and fixed a bug in MinidumpAssertion::Read where the max string
length passed to UTF16codeunits was in bytes instead of UTF-16 chars. 

Tested with a minidump containing a version 3 structure to validate the string
conversion routines.  Interestingly enough the time_zone names does not appear
to be abbreviation as the documentation was suggesting but full names, e.g.
Eastern Standard Time:

MDRawMiscInfo
  size_of_info                 = 232
  flags1                       = 0xf7
  process_id                   = 0x54c4
  process_create_time          = 0x51a9323c
  process_user_time            = 0x1
  process_kernel_time          = 0x0
  processor_max_mhz            = 3100
  processor_current_mhz        = 1891
  processor_mhz_limit          = 3100
  processor_max_idle_state     = 0x1
  processor_current_idle_state = 0x1

The new fileds follow:
  process_integrity_level      = 0x1000
  process_execute_flags        = 0x4d
  protected_process            = 0
  time_zone_id                 = 2
  time_zone.bias               = 300
  time_zone.standard_name      = Eastern Standard Time
  time_zone.daylight_name      = Eastern Daylight Time
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1204

[Ivan Penkov]

Please, review.

[Mark Mentovai]

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:662: uint16_t wYear;
All of the fields in structures in this file have been named according to Google
coding style naming conventions. These should be adjusted too: year, month,
day_of_week, etc.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:673: int32_t      bias;
It was not obvious to me, so I had to look in MS documentation to find that
“bias” is what they’re calling the offset to UTC in minutes. Can we have a
comment that says so?

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:674: uint16_t    
standard_name[32];  /* UTF-16 */
Also, the meaning of the fields isn’t totally clear. “bias” is the current bias
value, the standard_* fields specify when a transition to standard happens and
what the bias should be then, etc.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:675: MDSystemTime standard_date;
For example, this is in local time, but that’s not obvious, and a comment is
warranted.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:704: *
MD_MISCINFO_FLAGS1_PROCESSOR_POWER_INFO. */
The old comment said that these fields weren’t present in the unnumbered
structure. No reason to get rid of that.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3555: {  // Swap MDTimeZoneInformation
What’s with the new {scope}?

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3559: {  // Swap MDSystemTime
Here too, and throughout this function.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3589: // Use UTF16ToUTF8 to convert to string.
Is this a TODO? Do you want to just do it? I think that in most of the classes
here, where a string shows up in-line in a stream like this, Read just reads it
in directly and does any conversions necessary.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3647: // TODO(ivanpe): Print version 4 fields
Then you can implement this.

[Ivan Penkov]

Done.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:662: uint16_t wYear;
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> All of the fields in structures in this file have been named according to
Google
> coding style naming conventions. These should be adjusted too: year, month,
> day_of_week, etc.

Good catch.  Done.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:673: int32_t      bias;
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> It was not obvious to me, so I had to look in MS documentation to find that
> “bias” is what they’re calling the offset to UTC in minutes. Can we have a
> comment that says so?

Good idea.  Done.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:674: uint16_t    
standard_name[32];  /* UTF-16 */
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> Also, the meaning of the fields isn’t totally clear. “bias” is the current
bias
> value, the standard_* fields specify when a transition to standard happens and
> what the bias should be then, etc.

Done.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:675: MDSystemTime standard_date;
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> For example, this is in local time, but that’s not obvious, and a comment is
> warranted.

Done.

https://breakpad.appspot.com/617002/diff/2001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:704: *
MD_MISCINFO_FLAGS1_PROCESSOR_POWER_INFO. */
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> The old comment said that these fields weren’t present in the unnumbered
> structure. No reason to get rid of that.

Done.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3555: {  // Swap MDTimeZoneInformation
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> What’s with the new {scope}?

True, not needed.  Sometimes it helps with the grouping of operations and
comments.  I added Swap functions for the different structures that are being
swapped by these operations and got rid of the scoping stuff.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3559: {  // Swap MDSystemTime
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> Here too, and throughout this function.

Done.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3589: // Use UTF16ToUTF8 to convert to string.
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> Is this a TODO? Do you want to just do it? I think that in most of the classes
> here, where a string shows up in-line in a stream like this, Read just reads
it
> in directly and does any conversions necessary.

Yes, you are right.  I added code to convert the UTF-16 strings to UTF-8.  While
doing so I refactored a couple of functions and noticed a bug in
MinidumpAssertion::Read where the max string length passed to UTF16codeunits was
in bytes instead of in UTF-16 chars.  Fixed.

https://breakpad.appspot.com/617002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3647: // TODO(ivanpe): Print version 4 fields
On 2013/07/19 21:36:30, Mark Mentovai wrote:
> Then you can implement this.

Done.

[Siggi]

lgtm with a nit - for what it's worth.

Are there really no tests in breakpad for these classes?

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:767: #define MD_MISCINFO_SIZE 24
nit: perhaps these can be defined to offsetof incantations, which would allow
for asserting these values against the macro.
Alternatively these values could be asserted against offsetof incantations in
some .cc file?

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:735: * may not be set.  Use flags1
or sizeOfInfo to determine whether these
size_of_info (shows up a couple more times, one of them was preexisting.)

[Ivan Penkov]

Done.

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:735: * may not be set.  Use flags1
or sizeOfInfo to determine whether these
On 2013/07/22 15:30:29, Mark Mentovai wrote:
> size_of_info (shows up a couple more times, one of them was preexisting.)

Done.

https://breakpad.appspot.com/617002/diff/7001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:767: #define MD_MISCINFO_SIZE 24
On 2013/07/20 15:22:02, Siggi wrote:
> nit: perhaps these can be defined to offsetof incantations, which would allow
> for asserting these values against the macro.
> Alternatively these values could be asserted against offsetof incantations in
> some .cc file?

Done.

[Mark Mentovai]

LGTM

[Ted Mielczarek]

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:679: uint16_t standard_name[32]; 
/* UTF-16 */
Mention that these are full time zone names in practice, if that differs from
the documentation.

Also, are these strings NULL terminated? You should mention that.

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:689: * Pacific Daylight Time.  This
string can be empty. */
Same here.

[Ivan Penkov]

Done.

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:679: uint16_t standard_name[32]; 
/* UTF-16 */
On 2013/08/02 16:18:40, Ted Mielczarek wrote:
> Mention that these are full time zone names in practice, if that differs from
> the documentation.
> 
> Also, are these strings NULL terminated? You should mention that.

Done.

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:689: * Pacific Daylight Time.  This
string can be empty. */
On 2013/08/02 16:18:40, Ted Mielczarek wrote:
> Same here.

Done.

https://breakpad.appspot.com/617002/diff/12001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:689: * Pacific Daylight Time.  This
string can be empty. */
On 2013/08/02 16:18:40, Ted Mielczarek wrote:
> Same here.

Done.