Improve Linux ARM CPU information reporting.

Improve Linux ARM CPU information reporting.

This patch fixes the code in minidump_writer.cc responsible for
writing the CPU-specific information when running on ARM-based
Linux systems. The format of /proc/cpuinfo differs significantly
from x86 and x86_64.

Note that this adds a new 'arm_cpu_info' struct to the
MDCPUInformation union type to hold a few ARM-specific CPU
information bits.

[Ted Mielczarek]

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1106: bool
WriteCPUInformation(MDRawSystemInfo* sys_info) {
It would be really nice to be able to write some unit tests for this, given the
scope of the changes here. As-is that's probably tricky (unless there's a clever
way to mock sys_open/sys_read in a unit test), but perhaps a slight refactoring
would make it possible? I'd at least like to have some sanity checks on an
x86-format cpuinfo and arm SMP-format and non-SMP-format cpuinfo files.

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1136: #error "Uknnown CPU
arch"
"Unknown".

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1171: const char* column =
(const char *)memchr(line, ':', line_len);
Does memchr compile as a GCC builtin, or should we be worried about this being a
library call?

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1214:
my_read_decimal_ptr(&val, value);
This seems really fiddly. There really isn't any sane way to know the format in
advance?

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1233: // which is enclosed
in parentheses, as in:
Comments should be third-person, no "We".

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1242: if
(FieldMatches(field, field_len, "Hardware")) {
These should probably be "else if" chains, no?

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1264: // make sure we got
everything we wanted
Fix the "we" here while you're here?

https://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1308:
sys_info->processor_revision = info->arm_cpu_info.variant;
If you're repurposing these fields for arm, do you really need the entries in
arm_cpu_info? (Also, now I'm curious as to what Windows 8 on ARM uses. Does it
still use CEDumps, or did they extend the Minidump format?)

https://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidum...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidum...
src/google_breakpad/common/minidump_format.h:555: u_int32_t architecture;       
       /* 5 = ARMv5, 6 = ARMv6, 7 = ARMv7 */
The values for this probably deserve an enum in this file.

https://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidum...
src/google_breakpad/common/minidump_format.h:556: u_int32_t implementer;        
       /* 0x41 = ARM */
Are there other known values for this, or is it sort of arbitrary? If so, I'd
make an enum for this as well. If not, what you have is fine.

https://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidum...
src/google_breakpad/common/minidump_format.h:559: u_int32_t hardware_revision;  
       /* SoC revision number */
These are sort of confusing. Is there an ARM spec you can reference in a comment
that would explain them better?

https://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidum...
src/google_breakpad/common/minidump_format.h:561: MDRVA     features_rva;       
       /* Features from /proc/cpuinfo */
You're just including the cpu feature string wholesale?

https://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidum...
src/google_breakpad/common/minidump_format.h:562: } arm_cpu_info;
I'm a little worried about the size of the info you're sticking in here.
x86_cpu_info is 24 bytes, and other_cpu_info is 16 bytes, so the size of this
union ought to be 24 bytes (I'm not sure how it works out with alignment).
Currently this enum is defined to be compatible with CPU_INFORMATION from the
Microsoft SDK headers. If we make it slightly larger I'm concerned that we'll
break compatibility with Microsoft tools.

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

https://breakpad.appspot.com/427002/diff/1/src/processor/minidump.cc#newcode3089
src/processor/minidump.cc:3089: // due to the system_info_.cpu union.
This is gross.

[digit]

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1106: bool
WriteCPUInformation(MDRawSystemInfo* sys_info) {
I'll try to refactor this with a function that takes a mock LineReader as input.

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1136: #error "Uknnown CPU
arch"
thanks

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1171: const char* column =
(const char *)memchr(line, ':', line_len);
even if it does, my_memchr should be a macro to memchr and this line should use
it :)

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1204: // format string to
sscanf() and printf().
ow, this comment is really obsolete.

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1214:
my_read_decimal_ptr(&val, value);
In the general case no. but $KERNEL/arch/arm/kernel/setup.c uses two formats for
"CPU Part" depending on a CPUID feature that seems to have changed a long time
ago, we could scrap that and assume that CPUs are more recent than an ARM7) and
remove the 0x/0X detection code above.

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1233: // which is enclosed
in parentheses, as in:
Yes, I'll fix them.

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1242: if
(FieldMatches(field, field_len, "Hardware")) {
yes.

http://breakpad.appspot.com/427002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1308:
sys_info->processor_revision = info->arm_cpu_info.variant;
I didn't find information on the web last time I looked at it. I hope it's clear
that this patch is just a first draft. I have strictly no preferred way to store
this information.

http://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidump...
File src/google_breakpad/common/minidump_format.h (right):

http://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidump...
src/google_breakpad/common/minidump_format.h:556: u_int32_t implementer;        
       /* 0x41 = ARM */
0x41 is a very common value, but there are others (see the patch for
MinidumpSystemInfo::GetCPUVendor). An enum sounds fine.

http://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidump...
src/google_breakpad/common/minidump_format.h:559: u_int32_t hardware_revision;  
       /* SoC revision number */
On 2012/08/06 14:43:35, Ted Mielczarek wrote:
> These are sort of confusing. Is there an ARM spec you can reference in a
comment
> that would explain them better?

These essentially are the ARM CPUID fields exploded into several components. I
think it's probably better to simply store the CPUID 32-bit value in there
though.

The current structure is just a first proposal. I experimented with several
changes, didn't really find anything that was really satisfying. Suggestions for
improvements welcomed :-)

http://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidump...
src/google_breakpad/common/minidump_format.h:561: MDRVA     features_rva;       
       /* Features from /proc/cpuinfo */
On 2012/08/06 14:43:35, Ted Mielczarek wrote:
> You're just including the cpu feature string wholesale?

Yes, I first experimented with enums for individal bit-flags, but this added a
lot of parsing with very little benefits in terms of reporting. Moreover, there
are always new tags introduced periodically.

http://breakpad.appspot.com/427002/diff/1/src/google_breakpad/common/minidump...
src/google_breakpad/common/minidump_format.h:562: } arm_cpu_info;
I was not aware of this. This will have to be fixed.

http://breakpad.appspot.com/427002/diff/1/src/processor/minidump.cc
File src/processor/minidump.cc (right):

http://breakpad.appspot.com/427002/diff/1/src/processor/minidump.cc#newcode3089
src/processor/minidump.cc:3089: // due to the system_info_.cpu union.
On 2012/08/06 14:43:35, Ted Mielczarek wrote:
> This is gross.

Done.

[Ted Mielczarek]

Did you intend to pick this patch back up and just forget about it?

[digit]

On Tue, Mar 12, 2013 at 7:09 PM, <ted.mielczarek@gmail.com> wrote:

> Did you intend to pick this patch back up and just forget about it?
>
> Yes, I got caught by more important matters. Thanks for the recall, I
think I can work something up this week.
Is there more information about Windows/ARM these days that we could use
btw?


> https://breakpad.appspot.com/**427002/<https://breakpad.appspot.com/427002/>
>

[Ted Mielczarek]

On 2013/03/13 13:14:21, digit wrote:
> Is there more information about Windows/ARM these days that we could use
> btw?

I looked into this last year:
https://groups.google.com/forum/?fromgroups=#!topic/google-breakpad-dev/0K3Dl...

The CONTEXT structures are all in the Windows 8 SDK, but I can't find any
information that they're doing anything more interesting than what's currently
in the SDK (128 bits for non-x86 CPU features). It would be nice to get my hands
on a Windows RT minidump and see what it actually contains, but I wouldn't know
where to start with acquiring one.