Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6599022:
Add ARM support (Closed)
|
Created:
9 years, 10 months ago by sjg Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org Visibility:
Public. |
DescriptionAdd ARM support
Adds support for scanning ARM backtraces so that kernel crash reporting
can work on ARM.
BUG=chromium-os:12454
TEST=cat /sys/kernel/debug/preserved/kcrash > x
crash_reporter -generate_kernel_signature x
Verify that it finds a backtrace properly.
FEATURES=test emerge-x86-mario crash-reporter
- check all tests pass (this will run both ARM and X86 tests)
Change-Id: I4dc6d7a2bae53d05883da9425ec8e9ac4a5c2bba
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=a0a089a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Tidied up architecture variations #
Total comments: 8
Patch Set 3 : Add tests for ARM #
Total comments: 22
Patch Set 4 : Refactored tests a little, var change, comments, nits #
Total comments: 6
Patch Set 5 : Further checking, renamed common function #Patch Set 6 : Use Enable() function to check architecture #
Total comments: 4
Patch Set 7 : Removed 3 tests from ARM as they don't really test anything new #
Messages
Total messages: 22 (0 generated)
Please add at least one ARM unit test to kernel_collector_test. http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc#newcode105 kernel_collector.cc:105: "\\s+\\[<[[:xdigit:]]+>\\]" // Matches " [<7937bcee>]" This went from hard to understand to really opaque. :) Adding more to the regular expression for every new kind of architecture could get really hard to maintain. Could we initialize kernel collector with an architecture code and have it pick dynamically which regexps to use based on that? This way we can keep the regexps simple and still allow us to test and compile for all architecture at once. http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc#newcode156 kernel_collector.cc:156: pcrecpp::RE eip_re(kTimestampRegex + " (EIP: \\[<.*>\\]|PC is at) ([^\\+ ]+).*", Same comment here.
No new version yet, just a question - thanks. http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc#newcode105 kernel_collector.cc:105: "\\s+\\[<[[:xdigit:]]+>\\]" // Matches " [<7937bcee>]" On 2011/03/07 23:16:58, kmixter1 wrote: > This went from hard to understand to really opaque. :) > > Adding more to the regular expression for every new kind of architecture could > get really hard to maintain. Could we initialize kernel collector with an > architecture code and have it pick dynamically which regexps to use based on > that? This way we can keep the regexps simple and still allow us to test and > compile for all architecture at once. My schange here were: a. to allow 1 or 2 spaces at the start, instead of requiring 2. b. Change .* to be more specific, so it matches hex digits (which I argue is sensible anyway) The intent in the kernel is that the format is the same for each arch, but I would argue that the original expression here was too specific. I don't think we want to have different specs for different archs. Anyway, to handle ARM we would need to change the code also, since the first line includes a 'Stack backtrace;' string too! http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc#newcode156 kernel_collector.cc:156: pcrecpp::RE eip_re(kTimestampRegex + " (EIP: \\[<.*>\\]|PC is at) ([^\\+ ]+).*", On 2011/03/07 23:16:58, kmixter1 wrote: > Same comment here. I think this is fair enough, and the kernel will not like start calling EIP the PC on x86 :-) Do you know where I can find out the current architecture, preferably in the cros lib?
http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc#newcode105 kernel_collector.cc:105: "\\s+\\[<[[:xdigit:]]+>\\]" // Matches " [<7937bcee>]" On 2011/03/15 19:39:35, sjg wrote: > On 2011/03/07 23:16:58, kmixter1 wrote: > > This went from hard to understand to really opaque. :) > > > > Adding more to the regular expression for every new kind of architecture could > > get really hard to maintain. Could we initialize kernel collector with an > > architecture code and have it pick dynamically which regexps to use based on > > that? This way we can keep the regexps simple and still allow us to test and > > compile for all architecture at once. > > My schange here were: > > a. to allow 1 or 2 spaces at the start, instead of requiring 2. > > b. Change .* to be more specific, so it matches hex digits (which I argue is > sensible anyway) > > The intent in the kernel is that the format is the same for each arch, but I > would argue that the original expression here was too specific. I don't think we > want to have different specs for different archs. Anyway, to handle ARM we would > need to change the code also, since the first line includes a 'Stack backtrace;' > string too! Yeah, I didn't closely enough - it was already opaque. :) It's fine - thanks for adding the comments as well. http://codereview.chromium.org/6599022/diff/1/kernel_collector.cc#newcode156 kernel_collector.cc:156: pcrecpp::RE eip_re(kTimestampRegex + " (EIP: \\[<.*>\\]|PC is at) ([^\\+ ]+).*", On 2011/03/15 19:39:35, sjg wrote: > On 2011/03/07 23:16:58, kmixter1 wrote: > > Same comment here. > > I think this is fair enough, and the kernel will not like start calling EIP the > PC on x86 :-) > > Do you know where I can find out the current architecture, preferably in the > cros lib? You can't rely on libcros/Chrome since it's not necessarily running. I didn't see anything in libbase or libchromeos to do it. You might check src/common or consider adding it there. If you don't want to, you can add it in crash_collector.cc. You might either want to use uname(2) to get a machine string and convert that to an enumerator, or you could just rely on the compiler: #if defined(COMPILER_GCC) && defined(ARCH_CPU_X86_FAMILY) return X86; #elif defined(COMPILER_GCC) && defined(ARCH_CPU_ARM_FAMILY) return ARM;
PTAL thanks
Here it is! Thanks for taking a look - Simon
On 2011/03/28 23:15:23, sjg wrote: > Here it is! Thanks for taking a look - Simon As Ken mentioned, please add a unit test to kernel_collector_test.cc for ARM.
Here are my thoughts. It may be that nothing needs to be changed (other than adding a test, as the others have mentioned), since I'm still learning the standards/style around here. http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode22 kernel_collector.cc:22: // to count towards the not be gesignature of the kcrash. Was this an intentional comment change? http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode34 kernel_collector.cc:34: static const std::string pcRegex[KernelCollector::archCount] = { 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. http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode37 kernel_collector.cc:37: " Unsupported architecture" 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). http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode180 kernel_collector.cc:180: pcrecpp::RE eip_re(kTimestampRegex + pcRegex[GetArch()], 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.
PTAL thanks. http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode22 kernel_collector.cc:22: // to count towards the not be gesignature of the kcrash. On 2011/03/29 02:20:18, Michael Krebs wrote: > Was this an intentional comment change? Nope. It seems to be gone now so must have been a typo. http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode37 kernel_collector.cc:37: " Unsupported architecture" On 2011/03/29 02:20:18, Michael Krebs wrote: > 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). Have added a warning in the constructor and a test to catch it. http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode180 kernel_collector.cc:180: pcrecpp::RE eip_re(kTimestampRegex + pcRegex[GetArch()], On 2011/03/29 02:20:18, Michael Krebs wrote: > 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. I would hope that the compiler will complain if you forget since pcRegex[] has a fixed number of members.
ping - any thoughts on this please?
http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode28 kernel_collector.cc:28: * For ARM we see: Could you change the comment to be more descriptive (contents of dmesg indicating the PC of the panic on ARM)? http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode34 kernel_collector.cc:34: static const std::string pcRegex[KernelCollector::archCount] = { I suspect that if archCount is greater than the number of initializers, this will silently work and set strings to empty string as per the default initializer. Better to do a check in initializer if this is what you're trying to guard against. Also, this should be named something more like s_pc_regexp. I agree with Mike though that pcRegex should just be a const char *[]. kTimestampRegex should have been const char * as well - I just didn't realize this was a style rule when I wrote the code. http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode116 kernel_collector.cc:116: // For ARM: Could you add the x86 example? It may be easier to understand then why the second line of the regexp is what it is. http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode121 kernel_collector.cc:121: "([\\s\\?(]+)" // Matches " ? (" Matches " ? (" (ARM) or " ? " (x86). http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode122 kernel_collector.cc:122: "([^\\+ )]+)"); // Matches until \ + space ) nit: it took me a while to parse this comment. Maybe matches until a delimiter? http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h File kernel_collector.h (right): http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode23 kernel_collector.h:23: archArm, // ARM variant. nit: the comments seem superfluous here http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode25 kernel_collector.h:25: archUnknown, // We don't know. nit: In this code base unknown is usually the zero value. http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode78 kernel_collector.h:78: // Returns the architecture kind for which we are built - enum ArchKind add period. http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode84 kernel_collector.h:84: enum ArchKind arch_; // The architecture we are working with period. Maybe say the architecture of kernel dump strings. http://codereview.chromium.org/6599022/diff/16001/kernel_collector_test.cc File kernel_collector_test.cc (right): http://codereview.chromium.org/6599022/diff/16001/kernel_collector_test.cc#ne... kernel_collector_test.cc:213: EXPECT_TRUE(collector_.GetArch() != KernelCollector::archUnknown); I would separate this into a separate test function. http://codereview.chromium.org/6599022/diff/16001/kernel_collector_test.cc#ne... kernel_collector_test.cc:220: const char kPCButNoStack[] = Instead of copying this code directly from the x86 function, can you refactor it into a separate function that both call?
http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/6001/kernel_collector.cc#newcode180 kernel_collector.cc:180: pcrecpp::RE eip_re(kTimestampRegex + pcRegex[GetArch()], On 2011/04/01 20:04:15, sjg wrote: > On 2011/03/29 02:20:18, Michael Krebs wrote: > > 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. > > I would hope that the compiler will complain if you forget since pcRegex[] has a > fixed number of members. 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.
PTAL thanks http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode28 kernel_collector.cc:28: * For ARM we see: On 2011/04/05 00:09:26, kmixter1 wrote: > Could you change the comment to be more descriptive (contents of dmesg > indicating the PC of the panic on ARM)? done http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode34 kernel_collector.cc:34: static const std::string pcRegex[KernelCollector::archCount] = { On 2011/04/05 00:09:26, kmixter1 wrote: > I suspect that if archCount is greater than the number of initializers, this > will silently work and set strings to empty string as per the default > initializer. Better to do a check in initializer if this is what you're trying > to guard against. Also, this should be named something more like s_pc_regexp. I > agree with Mike though that pcRegex should just be a const char *[]. > kTimestampRegex should have been const char * as well - I just didn't realize > this was a style rule when I wrote the code. OK have changed the varname and added a check in constructor. http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode116 kernel_collector.cc:116: // For ARM: On 2011/04/05 00:09:26, kmixter1 wrote: > Could you add the x86 example? It may be easier to understand then why the > second line of the regexp is what it is. It is above - I have changed it around to be more explicit. http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode121 kernel_collector.cc:121: "([\\s\\?(]+)" // Matches " ? (" On 2011/04/05 00:09:26, kmixter1 wrote: > Matches " ? (" (ARM) or " ? " (x86). done http://codereview.chromium.org/6599022/diff/16001/kernel_collector.cc#newcode122 kernel_collector.cc:122: "([^\\+ )]+)"); // Matches until \ + space ) On 2011/04/05 00:09:26, kmixter1 wrote: > nit: it took me a while to parse this comment. Maybe matches until a delimiter? done http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h File kernel_collector.h (right): http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode23 kernel_collector.h:23: archArm, // ARM variant. On 2011/04/05 00:09:26, kmixter1 wrote: > nit: the comments seem superfluous here removed http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode25 kernel_collector.h:25: archUnknown, // We don't know. On 2011/04/05 00:09:26, kmixter1 wrote: > nit: In this code base unknown is usually the zero value. done http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode78 kernel_collector.h:78: // Returns the architecture kind for which we are built - enum ArchKind On 2011/04/05 00:09:26, kmixter1 wrote: > add period. done http://codereview.chromium.org/6599022/diff/16001/kernel_collector.h#newcode84 kernel_collector.h:84: enum ArchKind arch_; // The architecture we are working with On 2011/04/05 00:09:26, kmixter1 wrote: > period. Maybe say the architecture of kernel dump strings. done http://codereview.chromium.org/6599022/diff/16001/kernel_collector_test.cc File kernel_collector_test.cc (right): http://codereview.chromium.org/6599022/diff/16001/kernel_collector_test.cc#ne... kernel_collector_test.cc:213: EXPECT_TRUE(collector_.GetArch() != KernelCollector::archUnknown); On 2011/04/05 00:09:26, kmixter1 wrote: > I would separate this into a separate test function. done http://codereview.chromium.org/6599022/diff/16001/kernel_collector_test.cc#ne... kernel_collector_test.cc:220: const char kPCButNoStack[] = On 2011/04/05 00:09:26, kmixter1 wrote: > Instead of copying this code directly from the x86 function, can you refactor it > into a separate function that both call? OK have done this. In fact I could create different traces for ARM to make the test more real, but I don't see any advantage in that given what we are testing.
LGTM with nits http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc#newcode49 kernel_collector.cc:49: if (arch_ == archUnknown || !s_pc_regex[arch_]) Shouldn't there be a check of: if (sizeof(s_pc_regex) / sizeof(s_pc_regex[0]) != archCount) also it seems like you should check arch_ < archCount Also, local style is to always explicitly check NULL (s_pc_regexp[arch_] == NULL) http://codereview.chromium.org/6599022/diff/22001/kernel_collector_test.cc File kernel_collector_test.cc (right): http://codereview.chromium.org/6599022/diff/22001/kernel_collector_test.cc#ne... kernel_collector_test.cc:347: void KernelCollectorTest::DoCommonTests() { Could you add something with ComputeKernelStackSignature in the name? One option is to add a new fixture class ComputeKernelStackSignatureTest and have DoCommonTests within it. Also, local style is to put common code above code that references it.
Updated for review comments. Had a small Q though - see comments. http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc#newcode49 kernel_collector.cc:49: if (arch_ == archUnknown || !s_pc_regex[arch_]) On 2011/04/05 17:56:52, kmixter1 wrote: > Shouldn't there be a check of: > > if (sizeof(s_pc_regex) / sizeof(s_pc_regex[0]) != archCount) > > also it seems like you should check arch_ < archCount > > Also, local style is to always explicitly check NULL (s_pc_regexp[arch_] == > NULL) done. We are putting on the belts and braces now. I hope there are a lot more architectures. I have changed it to leave arch_ set to archUknnown if it generates any warnings. This will cause a test failure followed by a segfault (accessing NULL string for regex). I hope this is OK? http://codereview.chromium.org/6599022/diff/22001/kernel_collector_test.cc File kernel_collector_test.cc (right): http://codereview.chromium.org/6599022/diff/22001/kernel_collector_test.cc#ne... kernel_collector_test.cc:347: void KernelCollectorTest::DoCommonTests() { On 2011/04/05 17:56:52, kmixter1 wrote: > Could you add something with ComputeKernelStackSignature in the name? One > option is to add a new fixture class ComputeKernelStackSignatureTest and have > DoCommonTests within it. Also, local style is to put common code above code > that references it. OK I just renamed the function.
LGTM assuming kmixter1 still approves. http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc#newcode49 kernel_collector.cc:49: if (arch_ == archUnknown || !s_pc_regex[arch_]) On 2011/04/05 18:25:54, sjg wrote: > On 2011/04/05 17:56:52, kmixter1 wrote: > > Shouldn't there be a check of: > > > > if (sizeof(s_pc_regex) / sizeof(s_pc_regex[0]) != archCount) > > > > also it seems like you should check arch_ < archCount > > > > Also, local style is to always explicitly check NULL (s_pc_regexp[arch_] == > > NULL) > > done. We are putting on the belts and braces now. I hope there are a lot more > architectures. > > I have changed it to leave arch_ set to archUknnown if it generates any > warnings. This will cause a test failure followed by a segfault (accessing NULL > string for regex). I hope this is OK? Re: The first archCount check: given the current definition of s_pc_regex[] it will always be of size archCount. Instead, maybe it can just be "... char *s_pc_regex[] = {" and then the following check can be used to fail at build time: COMPILE_ASSERT(arraysize(s_pc_regexp) == archCount, missing_arch_pc_regexp); Regarding the segfault, I wouldn't normally think that'd be okay, but kmixter would probably know with respect to this.
http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc File kernel_collector.cc (right): http://codereview.chromium.org/6599022/diff/22001/kernel_collector.cc#newcode49 kernel_collector.cc:49: if (arch_ == archUnknown || !s_pc_regex[arch_]) On 2011/04/05 21:02:15, Michael Krebs wrote: > On 2011/04/05 18:25:54, sjg wrote: > > On 2011/04/05 17:56:52, kmixter1 wrote: > > > Shouldn't there be a check of: > > > > > > if (sizeof(s_pc_regex) / sizeof(s_pc_regex[0]) != archCount) > > > > > > also it seems like you should check arch_ < archCount > > > > > > Also, local style is to always explicitly check NULL (s_pc_regexp[arch_] == > > > NULL) > > > > done. We are putting on the belts and braces now. I hope there are a lot more > > architectures. > > > > I have changed it to leave arch_ set to archUknnown if it generates any > > warnings. This will cause a test failure followed by a segfault (accessing > NULL > > string for regex). I hope this is OK? > > Re: The first archCount check: given the current definition of s_pc_regex[] it > will always be of size archCount. Instead, maybe it can just be "... char > *s_pc_regex[] = {" and then the following check can be used to fail at build > time: > > COMPILE_ASSERT(arraysize(s_pc_regexp) == archCount, missing_arch_pc_regexp); > > Regarding the segfault, I wouldn't normally think that'd be okay, but kmixter > would probably know with respect to this. Agreed - segv is not a good diagnostic. Can we just move the setting of architecture and checking into Enable() and return false if it fails?
PTAL I have moved the check into Enable() and added the compiler assert.
LGTM
http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc File kernel_collector_test.cc (right): http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc#ne... kernel_collector_test.cc:252: const char kPCButNoStack[] = So kPCButNoStack/kPCLineTooOld/kExamplePanicOnly are still duplicated between x86 and arm. CAn't they be refactored out into the ocmmon function too? http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc#ne... kernel_collector_test.cc:269: // Panic without EIP line. The EIP typo will go away if you combine these. :)
I think you are seeing duplications where there aren't any. Or are you suggesting parameterizing the dumps? PTAL thanks http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc File kernel_collector_test.cc (right): http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc#ne... kernel_collector_test.cc:252: const char kPCButNoStack[] = On 2011/04/05 23:29:11, kmixter1 wrote: > So kPCButNoStack/kPCLineTooOld/kExamplePanicOnly are still duplicated between > x86 and arm. CAn't they be refactored out into the ocmmon function too? They look different to me? http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc#ne... kernel_collector_test.cc:269: // Panic without EIP line. On 2011/04/05 23:29:11, kmixter1 wrote: > The EIP typo will go away if you combine these. :) Ooops, saw that a while back and forgot to fix...
LGTM - Yeah, I see now, they're different because they parenthesize the function names for ARM and x86 differently. Since they're just testing edge conditions, they really don't need to be done for both x86 and ARM. I'd prefer if you removed those test cases from the ARM test function and just ran the common ones. But either way is fine. On Tue, Apr 5, 2011 at 7:02 PM, <sjg@chromium.org> wrote: > I think you are seeing duplications where there aren't any. Or are you > suggesting parameterizing the dumps? > > PTAL thanks > > > > http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc > File kernel_collector_test.cc (right): > > http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc#ne... > kernel_collector_test.cc:252: const char kPCButNoStack[] = > On 2011/04/05 23:29:11, kmixter1 wrote: >> >> So kPCButNoStack/kPCLineTooOld/kExamplePanicOnly are still duplicated > > between >> >> x86 and arm. CAn't they be refactored out into the ocmmon function > > too? > > They look different to me? > > http://codereview.chromium.org/6599022/diff/18006/kernel_collector_test.cc#ne... > kernel_collector_test.cc:269: // Panic without EIP line. > On 2011/04/05 23:29:11, kmixter1 wrote: >> >> The EIP typo will go away if you combine these. :) > > Ooops, saw that a while back and forgot to fix... > > http://codereview.chromium.org/6599022/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
