Add ARM support

kernel_collector.cc

Index: kernel_collector.cc
diff --git a/kernel_collector.cc b/kernel_collector.cc
index 652e7ff3a0d5875ca5e9d92f63161d03328d4000..43af81c8ba5d9e29fabd719966cd0e04b63df9c8 100644
--- a/kernel_collector.cc
+++ b/kernel_collector.cc
@@ -19,11 +19,24 @@ static const int kMaxHumanStringLength = 40;
 static const char kPreservedDumpPath[] = "/sys/kernel/debug/preserved/kcrash";
 const uid_t kRootUid = 0;
 // Time in seconds from the final kernel log message for a call stack
-// to count towards the signature of the kcrash.
+// to count towards the not be gesignature of the kcrash.

[Michael Krebs, 2011/03/29 02:20:18]

Was this an intentional comment change?

[sjg, 2011/04/01 20:04:15]

Nope. It seems to be gone now so must have been a typo.

 static const int kSignatureTimestampWindow = 2;
 // Kernel log timestamp regular expression.
 static const std::string kTimestampRegex("^<.*>\\[\\s*(\\d+\\.\\d+)\\]");
 
+/*
+ * For ARM we see:
+ *   "<5>[   39.458982] PC is at write_breakme+0xd0/0x1b4"
+ * For x86:
+ *   "<0>[   37.474699] EIP: [<790ed488>] write_breakme+0x80/0x108 \
+ *    SS:ESP 0068:e9dd3efc
+ */
+static const std::string pcRegex[KernelCollector::archCount] = {

[Michael Krebs, 2011/03/29 02:20:18]

I believe using std::string is not normally allowed for globals (according to
"http://www.corp.google.com/eng/doc/cppguide.xml#Static_and_Global_Variables"),
but considering kTimestampRegex is already one, I guess this is okay since it's
consistent.

+  " PC is at ([^\\+ ]+).*",
+  " EIP: \\[<.*>\\] ([^\\+ ]+).*",  // X86 uses EIP for the program counter
+  " Unsupported architecture"

[Michael Krebs, 2011/03/29 02:20:18]

If it's an unknown architecture, it might be better to give some sort of
error/warning instead of trying to match on "Unsupported architecture" as a
regex (see my comment below).

[sjg, 2011/04/01 20:04:15]

Have added a warning in the constructor and a test to catch it.

+};
+
 KernelCollector::KernelCollector()
     : is_enabled_(false),
       preserved_dump_path_(kPreservedDumpPath) {
@@ -90,11 +103,19 @@ void KernelCollector::ProcessStackTrace(
     unsigned *hash,
     float *last_stack_timestamp) {
   pcrecpp::RE line_re("(.+)", pcrecpp::MULTILINE());
-  pcrecpp::RE stack_trace_start_re(kTimestampRegex + " Call Trace:$");
+  pcrecpp::RE stack_trace_start_re(kTimestampRegex +
+        " (Call Trace|Backtrace):$");
   // Match lines such as the following and grab out "error_code".
-  // <4>[ 6066.849504]  [<7937bcee>] error_code+0x66/0x6c
+  // <4>[ 6066.849504]  [<7937bcee>] ? error_code+0x66/0x6c
+  // The ? may or may not be present
+
+  // For ARM:
+  // <4>[ 3498.731164] [<c0057220>] (__bug+0x20/0x2c) from [<c018062c>]
+  // (write_breakme+0xdc/0x1bc)
   pcrecpp::RE stack_entry_re(kTimestampRegex +
-                             "  \\[<.*>\\]([\\s\\?]+)([^\\+ ]+)");
+    "\\s+\\[<[[:xdigit:]]+>\\]"      // Matches "  [<7937bcee>]"
+    "([\\s\\?(]+)"                   // Matches " ? ("
+    "([^\\+ )]+)");                  // Matches until \ + space )
   std::string line;
   std::string hashable;
 
@@ -137,14 +158,28 @@ void KernelCollector::ProcessStackTrace(
   }
 }
 
+enum KernelCollector::ArchKind KernelCollector::GetArch(void)
+{
+#if defined(COMPILER_GCC) && defined(ARCH_CPU_ARM_FAMILY)
+  return archArm;
+#elif defined(COMPILER_GCC) && defined(ARCH_CPU_X86_FAMILY)
+  return archX86;
+#else
+  return archUnknown;
+#endif
+}
+
 bool KernelCollector::FindCrashingFunction(
-    pcrecpp::StringPiece kernel_dump,
-    bool print_diagnostics,
-    float stack_trace_timestamp,
-    std::string *crashing_function) {
-  pcrecpp::RE eip_re(kTimestampRegex + " EIP: \\[<.*>\\] ([^\\+ ]+).*",
-                     pcrecpp::MULTILINE());
+  pcrecpp::StringPiece kernel_dump,
+  bool print_diagnostics,
+  float stack_trace_timestamp,
+  std::string *crashing_function) {
   float timestamp = 0;
+
+  // Use the correct regex for this architecture.
+  pcrecpp::RE eip_re(kTimestampRegex + pcRegex[GetArch()],

[Michael Krebs, 2011/03/29 02:20:18]

This very well may be okay (I'm new to what the standards are here), but
personally I'm inclined to say that a check should be put before pcRegex[] is
indexed.  For example, if an architecture is added but someone forgets to update
pcRegex[], then this will [probably] get undesired results.

[sjg, 2011/04/01 20:04:15]

I would hope that the compiler will complain if you forget since pcRegex[] has a
fixed number of members.

[Michael Krebs, 2011/04/05 00:43:56]

My concern was more like what Ken just mentioned.. that you would get an empty
string out of pcRegex[] if an architecture was added but pcRegex[] was not
updated.

Your pcRegex[] will always have archCount number of entries.  Therefore, if an
architecture is added to enum ArchKind the size of pcRegex[] will automatically
increase -- so you at least won't get an out-of-bounds access at run-time -- but
you could get the undesired results.

For example, if someone were to add "archPowerPC" after archX86, but forgot to
update pcRegex[], then this code would return a regex of " Unsupported
architecture" for it, and the compiler would not complain.  If "archSH" was then
added after that, pcRegex[archSH] would return an empty string.

+                     pcrecpp::MULTILINE());
+
   while (eip_re.FindAndConsume(&kernel_dump, &timestamp, crashing_function)) {
     if (print_diagnostics) {
       printf("@%f: found crashing function %s\n",