Improve ARM CPU info reporting.

Improve ARM CPU info reporting.

This patch improves several things for Linux/ARM:

- Better detection of the number of CPUs on the target
  device. The content of /proc/cpuinfo only matches the
  number of "online" CPUs, which varies over time with
  recent Android devices.

- Reconstruct the CPUID and ELF hwcaps values from
  /proc/cpuinfo, this is useful to better identify
  target devices in minidumps.

- Make minidump_dump display the new information
  in useful ways.

- Write a small helper class to parse /proc/cpuinfo
  and also use it for x86/64.

- Write a small helper class to parse sysfds cpu lists.

- Add a my_memchr() implementation.

- Add unit tests.

Tested on a Nexus S (1 CPU), Galaxy Nexus (2 CPUs)
and a Nexus 4 (4 CPUs).

[digit]

for the record, this is intended to be a better version of
http://breakpad.appspot.com/427002/

[Mark Mentovai]

Nice work. Ted may have comments too. I didn’t review the cpuinfo parsing
carefully because I wasn’t in the mood to connect a phone and look at its
/proc/cpuinfo. Lame, I know.

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

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:69: #include
"client/linux/minidump_writer/proc_cpuinfo_reader.h"
Sort.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1216: #if defined(__i386) ||
defined(__x86_64)
We typically use __i386__ and __x86_64__ (yes, I see you’re moving this from
elsewhere, but let’s use our normal canonical form since you’re moving it).

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1239: #error "Unknown CPU
arch"
Pointless now that this is already within a block that looks for the two CPUs
it’s looking for.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1318: } cpu_id_entries[] = {
const this table?

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1330: }
cpu_features_entries[] = {
and this?

I guess you can const the x86 ones too.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1379: const int fd2 =
sys_open("/sys/devices/system/cpu/possible", O_RDONLY, 0);
80 columns.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1379: const int fd2 =
sys_open("/sys/devices/system/cpu/possible", O_RDONLY, 0);
Why not reuse the same variable for fd?

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1390: sys_close(fd1);
You already closed fd1 above.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1429: result = (result &
((1U << entry->bit_length)-1)) << entry->bit_lshift;
80 columns here and on line 1476.

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

http://breakpad.appspot.com/540003/diff/1/src/processor/minidump_processor.cc...
src/processor/minidump_processor.cc:478: GetARMCpuInfo(raw_system_info,
&info->cpu_info);
Since this is the only call site for this function, can you move that function
right above this one? I think that’s how this file usually handles these helper
functions.

[Lei Zhang (chromium)]

Do we have unit tests for the /proc/cpuinfo parsing code? I remember we had some
off by 1 errors before, and we did not notice them until later when the crash
reports contained funny values.

[digit]

Humm, Patch Set 1 is missing several source files, including the /proc/cpuinfo
parser and its unit tests. I'll update this. If you have example test cases to
add, let me know and I'll add them.

[Lei Zhang (chromium)]

On 2013/03/21 21:32:38, digit wrote:
> Humm, Patch Set 1 is missing several source files, including the /proc/cpuinfo
> parser and its unit tests. I'll update this. If you have example test cases to
> add, let me know and I'll add them.

I have nothing in particular. Just wanted to make sure the parse gets exercised.

[digit]

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

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:69: #include
"client/linux/minidump_writer/proc_cpuinfo_reader.h"
On 2013/03/21 19:13:09, Mark Mentovai wrote:
> Sort.

Done.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1216: #if defined(__i386) ||
defined(__x86_64)
Sure, I've also fixed the other __i386 in this file.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1239: #error "Unknown CPU
arch"
On 2013/03/21 19:13:09, Mark Mentovai wrote:
> Pointless now that this is already within a block that looks for the two CPUs
> it’s looking for.

Done.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1318: } cpu_id_entries[] = {
The one for x86 weren't const, so I tried to follow the same style :) I changed
them all.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1379: const int fd2 =
sys_open("/sys/devices/system/cpu/possible", O_RDONLY, 0);
I've changed it. I just started using two fds, then later refactored the code as
it is above. I've simplified it further as you suggest.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1390: sys_close(fd1);
On 2013/03/21 19:13:09, Mark Mentovai wrote:
> You already closed fd1 above.

Done.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1390: sys_close(fd1);
Good catch. Done.

http://breakpad.appspot.com/540003/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1429: result = (result &
((1U << entry->bit_length)-1)) << entry->bit_lshift;
On 2013/03/21 19:13:09, Mark Mentovai wrote:
> 80 columns here and on line 1476.

Done.

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

http://breakpad.appspot.com/540003/diff/1/src/processor/minidump_processor.cc...
src/processor/minidump_processor.cc:478: GetARMCpuInfo(raw_system_info,
&info->cpu_info);
ok, I've done this, but I'm used to put all anonymous functions in a top-level
namespace, as suggested by several Chrome reviewers.

[digit]

ping?

[Ted Mielczarek]

Sorry, forgot about this. I only have one objection, the default CPU info in
minidump_writer.cc. Otherwise just some minor comments.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/cpu_set_unittest.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/cpu_set_unittest.cc:28: // OF THIS SOFTWARE,
EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Thanks for writing unit tests for this code!

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1307: #elif
defined(__ARM_EABI__)
Is this valid for OABI as well? (I don't actually know, and I don't have any
hardware to test that theory on.) The old code would at least build on all
__arm__.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1357: // returned when this
happens.
Ick. I would rather that we just returned completely bogus data here rather than
incorrect defaults. That seems way more confusing.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1386:
sys_info->number_of_processors = static_cast<uint8_t>(cpu_count);
A nice followup here would be to store the present and possible values as well,
although we'd have to find a place for that.

It would be nice to get that info on x86 as well, although probably less
important.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/proc_cpuinfo_reader.h (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader.h:85: val += 1;
Any reason not to just write ++val?

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader.h:109: return value_;
Maybe assert(value_)?

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader.h:115: *length =
value_len_;
Similarly.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc:1: // Copyright
(c) 2013, Google Inc.
Thanks for writing these tests, the cpuinfo parsing has always made me wary due
to its lack of tests.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc:59: }
This could stand to get moved to a shared header somewhere.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc:103: };
This too.

http://breakpad.appspot.com/540003/diff/11001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

http://breakpad.appspot.com/540003/diff/11001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:556: uint32_t elf_hwcaps;    /*
linux specific, 0 otherwise */
Do you think this is ever likely to overflow 32 bits? (The kernel seems to just
use "unsigned int", so maybe not. You've got 8 bytes to spare in this struct
anyway, so maybe it's not terribly concerning right now.

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
File src/processor/minidump_processor.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:312: std::string* cpu_info) {
Should probably assert raw_info && cpu_info here.

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:324: { 0x69, "Intel/Marvell" },
Do these two lists of constants want to be enums in minidump_format.h?

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:378: uint32_t vendor_id = (cpuid >> 24) &
0xff;
Same with these masks, do these want to be macros or something in
minidump_format.h?

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:474: info->cpu = cpu_string;
I think info->cpu should still be "arm". For every other architecture we have a
static string for this value, and provide additional info in cpu_info.

[digit]

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1307: #elif
defined(__ARM_EABI__)
I really don't know, we can probably assume it is. I'm not aware of any device
in use today that supports OABI, my understanding is that it was dropped a long
time ago from any Linux/ARM based project, though I could be wrong.

I've changed it to use __arm__ instead.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1357: // returned when this
happens.
That's correct. I really thought ARMv5 devices were out of the market, but it
looks like their are still a few. See
http://forum.xda-developers.com/showthread.php?t=1596800

I've changed this to more random values.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1386:
sys_info->number_of_processors = static_cast<uint8_t>(cpu_count);
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> A nice followup here would be to store the present and possible values as
well,
> although we'd have to find a place for that.
> 
> It would be nice to get that info on x86 as well, although probably less
> important.

I thought about that, but didn't want to touch the non-CPU specific structure
for now, since I'm unsure its layout isn't fixed by some convention inherited
from Windows minidumps. If you have some ideas, please let me know, but I'd
prefer to add that in another patch.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/proc_cpuinfo_reader.h (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader.h:85: val += 1;
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> Any reason not to just write ++val?

Done.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader.h:109: return value_;
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> Maybe assert(value_)?

Done.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader.h:115: *length =
value_len_;
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> Similarly.

Done.

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc:59: }
Done, see src/common/tests/auto_testfile.h, the file name is modeled after
auto_tempdir.h, but I wonder if we'd better rename it auto_test_file.h instead?

http://breakpad.appspot.com/540003/diff/11001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/proc_cpuinfo_reader_unittest.cc:103: };
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> This too.

Done.

http://breakpad.appspot.com/540003/diff/11001/src/google_breakpad/common/mini...
File src/google_breakpad/common/minidump_format.h (right):

http://breakpad.appspot.com/540003/diff/11001/src/google_breakpad/common/mini...
src/google_breakpad/common/minidump_format.h:556: uint32_t elf_hwcaps;    /*
linux specific, 0 otherwise */
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> Do you think this is ever likely to overflow 32 bits? (The kernel seems to
just
> use "unsigned int", so maybe not. You've got 8 bytes to spare in this struct
> anyway, so maybe it's not terribly concerning right now.

I don't think it's likely. The 32-bits are pretty much hard-coded in the binary
format of /proc/self/auxv for now. And there is still a lot of room to grow.

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
File src/processor/minidump_processor.cc (right):

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:324: { 0x69, "Intel/Marvell" },
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> Do these two lists of constants want to be enums in minidump_format.h?

I don't think it's worth it. These tables are likely to evolve quickly over time
when new devices / SoC are released, and there is little reason to put them in a
header file that is strictly specific to the minidump file format itself.

http://breakpad.appspot.com/540003/diff/11001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:378: uint32_t vendor_id = (cpuid >> 24) &
0xff;
On 2013/03/26 14:53:41, Ted Mielczarek wrote:
> Same with these masks, do these want to be macros or something in
> minidump_format.h?

For the same reason, I don't think it'd be very useful.

[Ted Mielczarek]

LGTM

I'm not really concerned about the naming of auto_testfile.h.

[Ted Mielczarek]

Doesn't look like you landed this. FYI.

[digit]

Sorry, that's right. I've rebased to a more recent upstream in Patch Set 5.

[digit]

Commited as 1160