Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
543125:
Port linux_syscall_support and linux_dumper to support Linux ARM. Unit tests... (Closed)
|
Created:
10 years, 11 months ago by nealsid Modified:
7 years, 7 months ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
DescriptionPort linux_syscall_support and linux_dumper to support Linux ARM. Unit tests pass on ARM.
Patch Set 1 #Patch Set 2 : '' #
Total comments: 57
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 6
Messages
Total messages: 12 (0 generated)
http://codereview.chromium.org/543125/diff/3003/3010 File src/client/linux/Makefile (right): http://codereview.chromium.org/543125/diff/3003/3010#newcode60 src/client/linux/Makefile:60: rm -f $(UNITTEST_BIN) $(BREAKPAD_LIBRARY) $(LIB_CC_OBJ) $(LIB_C_OBJ) $(TEST_CC_OBJ) $(DUMPER_HELPER_TEST_BIN) core Please use spaces instead of tabs. http://codereview.chromium.org/543125/diff/3003/3006 File src/client/linux/minidump_writer/linux_dumper.h (right): http://codereview.chromium.org/543125/diff/3003/3006#newcode142 src/client/linux/minidump_writer/linux_dumper.h:142: bool stack_grows_down_; Are these really things that needs to be an instance variable? Can it be a class constant, or even a #defined variable? http://codereview.chromium.org/543125/diff/3003/3007 File src/client/linux/minidump_writer/linux_dumper_unittest.cc (right): http://codereview.chromium.org/543125/diff/3003/3007#newcode81 src/client/linux/minidump_writer/linux_dumper_unittest.cc:81: ASSERT_TRUE(false); Doesn't ASSERT have stream output? Eg, ASSERT_TRUE(false) << "Errno was: " << errno; http://codereview.chromium.org/543125/diff/3003/3007#newcode88 src/client/linux/minidump_writer/linux_dumper_unittest.cc:88: EXPECT_EQ((size_t)5, dumper.threads().size()); Can you make both this literal and the number_of_threads var into constants? Eg static const char kNumberOfThreadsStr = "5"; and static const int kNumberOfThreads = 5; http://codereview.chromium.org/543125/diff/3003/3007#newcode95 src/client/linux/minidump_writer/linux_dumper_unittest.cc:95: // one word off the stack to store it's thread id. it's -> its http://codereview.chromium.org/543125/diff/3003/3007#newcode151 src/client/linux/minidump_writer/linux_dumper_unittest.cc:151: if (linux_gate_loc) { You don't need this if() conditional anymore; the test will abort if it's NULL.
Looks pretty solid from my limited understanding of breakpad... Most of my comments are style based. http://codereview.chromium.org/543125/diff/3003/3010 File src/client/linux/Makefile (right): http://codereview.chromium.org/543125/diff/3003/3010#newcode25 src/client/linux/Makefile:25: DUMPER_HELPER_TEST_C_OBJ=$(patsubst %.cc, $(OBJ_DIR)/%.o,$(DUMPER_HELPER_TEST_C_SRC)) Since this is in a $() subshell expression, can we split the line and stay under 80-chars? Not sure if htat works in makefiles... http://codereview.chromium.org/543125/diff/3003/3008 File src/client/linux/handler/exception_handler.cc (right): http://codereview.chromium.org/543125/diff/3003/3008#newcode90 src/client/linux/handler/exception_handler.cc:90: return syscall(__NR_tgkill, tgid, tid, sig); yikes! Did this ever work before? http://codereview.chromium.org/543125/diff/3003/3008#newcode268 src/client/linux/handler/exception_handler.cc:268: #if !defined(__ARM_ARCH_7A__) Please add comments explaining the ifdefs...this is complicated enough already. http://codereview.chromium.org/543125/diff/3003/3009 File src/client/linux/handler/exception_handler.h (right): http://codereview.chromium.org/543125/diff/3003/3009#newcode150 src/client/linux/handler/exception_handler.h:150: struct _libc_fpstate float_state; What is this for? Is this because we will not be enabling floating-point emulation? Add a comment. http://codereview.chromium.org/543125/diff/3003/3005 File src/client/linux/minidump_writer/linux_dumper.cc (right): http://codereview.chromium.org/543125/diff/3003/3005#newcode87 src/client/linux/minidump_writer/linux_dumper.cc:87: int foo; I'm a bit unsure as to whether this is safe for testing stack directionality. Did you verify if we're guaranteed by spec as to the stack ordering of locals that are primitives? http://codereview.chromium.org/543125/diff/3003/3005#newcode362 src/client/linux/minidump_writer/linux_dumper.cc:362: // Move the stack pointer to the bottom of the page that it's in. why promote the stack direction & page_size code into member variables? They seem to only be used in this function. http://codereview.chromium.org/543125/diff/3003/3007 File src/client/linux/minidump_writer/linux_dumper_unittest.cc (right): http://codereview.chromium.org/543125/diff/3003/3007#newcode32 src/client/linux/minidump_writer/linux_dumper_unittest.cc:32: #include <sys/types.h> add newline to separate system includes from our code. http://codereview.chromium.org/543125/diff/3003/3007#newcode81 src/client/linux/minidump_writer/linux_dumper_unittest.cc:81: ASSERT_TRUE(false); On 2010/01/21 22:48:04, hannahtang.work wrote: > Doesn't ASSERT have stream output? Eg, ASSERT_TRUE(false) << "Errno was: " << > errno; There should also be a FAIL() macro that's better than asserting false is true. Also, consider strerror instead of just errno. FAIL() << "Exec failed: " << strerror(errno); http://codereview.chromium.org/543125/diff/3003/3007#newcode97 src/client/linux/minidump_writer/linux_dumper_unittest.cc:97: void* process_tid_location = (void *)(one_thread.regs.uregs[11] - 8); Is this true for the arm thumb ABI? I think the fp on arm thumb is r7...at least that's what piman said. http://codereview.chromium.org/543125/diff/3003/3007#newcode150 src/client/linux/minidump_writer/linux_dumper_unittest.cc:150: ASSERT_NE((void*)NULL, linux_gate_loc); Just ASSERT_TRUE(linux_gate_loc); It's a bit more idiomatic. http://codereview.chromium.org/543125/diff/3003/3011 File src/client/linux/minidump_writer/linux_dumper_unittest_helper.c (right): http://codereview.chromium.org/543125/diff/3003/3011#newcode32 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:32: // thread id. newline between preamble and code please. http://codereview.chromium.org/543125/diff/3003/3011#newcode39 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:39: void *threadfunction(void *data) { threadfunction -> thread_function http://codereview.chromium.org/543125/diff/3003/3011#newcode41 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:41: while(true) ; Do we need to busy loop this? Can we sleep(5) and make this still useable? Otherwise, the failure case is to burn the cpu. :) http://codereview.chromium.org/543125/diff/3003/3011#newcode47 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:47: printf("ERROR: number of threads is 0"); fprintf to stderr for errors please. :) stderr is unbuffered so we can be sure we will indeed see the output -- and see it when it's produced -- as opposed to stdout. http://codereview.chromium.org/543125/diff/3003/3011#newcode54 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:54: threadfunction(NULL); the pedant in me wants you to call join on all the threads in a loop. http://codereview.chromium.org/543125/diff/3003/3004 File src/common/linux/linux_syscall_support.h (right): http://codereview.chromium.org/543125/diff/3003/3004#newcode220 src/common/linux/linux_syscall_support.h:220: #if defined(__i386__) || defined(__ARM_ARCH_7A__) || defined(__ARM_ARCH_3__) \ I'm starting to think you need a BREAKPAD_ARMEL define that resolves to: defined(__ARM_ARCH_7a__) || defined(__ARM_ARCH_3__) http://codereview.chromium.org/543125/diff/3003/3004#newcode1530 src/common/linux/linux_syscall_support.h:1530: #elif defined(__ARM_ARCH_3__) || defined(__ARM_ARCH_7A__) Okay, I'm confused...sometimes you have these __ARM_ARCH_3__ and __ARM_ARCH_7a__ defines. Then somtimes you have these __ARM_EABI__ defines. Are we being clear about when we're doing something based on the arch type, and when we're doing it because of the abi type? This code here looks to be contingent on EABI, and not arm3 versus arm7a. Same question for actually every other ifdef... http://codereview.chromium.org/543125/diff/3003/3004#newcode2612 src/common/linux/linux_syscall_support.h:2612: #if !defined(__ARM_EABI__) Should this indentation be nested? Also, why bother with the !ARM_EABI? Why not just ARM_EABI and flip the order of the if/else? Add a comment explaining why it's safe to assume the other code for every other architecture. http://codereview.chromium.org/543125/diff/3003/3004#newcode2772 src/common/linux/linux_syscall_support.h:2772: #if !defined(__ARM_EABI__) Same comment as earlier about clarifying why it's safe to assume this code for everything other that ARM_EABI.
All the other nits have been picked, I think, LGTM aside from my few comments here. http://codereview.chromium.org/543125/diff/3003/3009 File src/client/linux/handler/exception_handler.h (right): http://codereview.chromium.org/543125/diff/3003/3009#newcode149 src/client/linux/handler/exception_handler.h:149: #if !defined(__ARM_ARCH_7A__) The cross-compile setup I'm using (Scratchbox, for targeting Maemo) is targeting __ARM_ARCH_6J__. Can we use __ARMEL__ or __ARM_EABI__ in all these ifdefs instead of 7A specifcially? I'm not an ARM wizard, but I suspect most of the things you're guarding on don't need to be quite that specific. http://codereview.chromium.org/543125/diff/3003/3004 File src/common/linux/linux_syscall_support.h (right): http://codereview.chromium.org/543125/diff/3003/3004#newcode80 src/common/linux/linux_syscall_support.h:80: defined(__mips__) || defined(__PPC__) || defined(__ARM_ARCH_7A__)) \ Again, I think you can get away with __ARMEL__ or __ARM_EABI__.
What is the difference between ARMEL and ARM_EABI anyways? Does ARMEL stand for ARM-Endian-Little? Just guessing... -Albert On Fri, Jan 29, 2010 at 12:43 PM, <ted.mielczarek@gmail.com> wrote: > All the other nits have been picked, I think, LGTM aside from my few > comments > here. > > > > > http://codereview.chromium.org/543125/diff/3003/3009 > File src/client/linux/handler/exception_handler.h (right): > > http://codereview.chromium.org/543125/diff/3003/3009#newcode149 > src/client/linux/handler/exception_handler.h:149: #if > !defined(__ARM_ARCH_7A__) > The cross-compile setup I'm using (Scratchbox, for targeting Maemo) is > targeting __ARM_ARCH_6J__. Can we use __ARMEL__ or __ARM_EABI__ in all > these ifdefs instead of 7A specifcially? I'm not an ARM wizard, but I > suspect most of the things you're guarding on don't need to be quite > that specific. > > > http://codereview.chromium.org/543125/diff/3003/3004 > File src/common/linux/linux_syscall_support.h (right): > > http://codereview.chromium.org/543125/diff/3003/3004#newcode80 > src/common/linux/linux_syscall_support.h:80: defined(__mips__) || > defined(__PPC__) || defined(__ARM_ARCH_7A__)) \ > Again, I think you can get away with __ARMEL__ or __ARM_EABI__. > > > http://codereview.chromium.org/543125 >
AIUI, yes. EABI is the "new" ABI, see for example http://wiki.debian.org/ArmEabiPort. -Ted On Fri, Jan 29, 2010 at 4:12 PM, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote: > What is the difference between ARMEL and ARM_EABI anyways? > > Does ARMEL stand for ARM-Endian-Little? Just guessing... > > -Albert > > > On Fri, Jan 29, 2010 at 12:43 PM, <ted.mielczarek@gmail.com> wrote: > >> All the other nits have been picked, I think, LGTM aside from my few >> comments >> here. >> >> >> >> >> http://codereview.chromium.org/543125/diff/3003/3009 >> File src/client/linux/handler/exception_handler.h (right): >> >> http://codereview.chromium.org/543125/diff/3003/3009#newcode149 >> src/client/linux/handler/exception_handler.h:149: #if >> !defined(__ARM_ARCH_7A__) >> The cross-compile setup I'm using (Scratchbox, for targeting Maemo) is >> targeting __ARM_ARCH_6J__. Can we use __ARMEL__ or __ARM_EABI__ in all >> these ifdefs instead of 7A specifcially? I'm not an ARM wizard, but I >> suspect most of the things you're guarding on don't need to be quite >> that specific. >> >> >> http://codereview.chromium.org/543125/diff/3003/3004 >> File src/common/linux/linux_syscall_support.h (right): >> >> http://codereview.chromium.org/543125/diff/3003/3004#newcode80 >> src/common/linux/linux_syscall_support.h:80: defined(__mips__) || >> defined(__PPC__) || defined(__ARM_ARCH_7A__)) \ >> Again, I think you can get away with __ARMEL__ or __ARM_EABI__. >> >> >> http://codereview.chromium.org/543125 >> > >
I've run tests on x86 but don't have access to my ARM board until tomorrow. Thanks! Neal http://codereview.chromium.org/543125/diff/3003/3010 File src/client/linux/Makefile (right): http://codereview.chromium.org/543125/diff/3003/3010#newcode25 src/client/linux/Makefile:25: DUMPER_HELPER_TEST_C_OBJ=$(patsubst %.cc, $(OBJ_DIR)/%.o,$(DUMPER_HELPER_TEST_C_SRC)) On 2010/01/22 20:45:38, awong wrote: > Since this is in a $() subshell expression, can we split the line and stay under > 80-chars? Not sure if htat works in makefiles... Done. http://codereview.chromium.org/543125/diff/3003/3010#newcode60 src/client/linux/Makefile:60: rm -f $(UNITTEST_BIN) $(BREAKPAD_LIBRARY) $(LIB_CC_OBJ) $(LIB_C_OBJ) $(TEST_CC_OBJ) $(DUMPER_HELPER_TEST_BIN) core On 2010/01/21 22:48:04, hannahtang.work wrote: > Please use spaces instead of tabs. makefiles require tabs http://codereview.chromium.org/543125/diff/3003/3008 File src/client/linux/handler/exception_handler.cc (right): http://codereview.chromium.org/543125/diff/3003/3008#newcode90 src/client/linux/handler/exception_handler.cc:90: return syscall(__NR_tgkill, tgid, tid, sig); On 2010/01/22 20:45:38, awong wrote: > yikes! Did this ever work before? I'm not sure why the compilation didn't emit a warning here on x86; possibly due to inlining. http://codereview.chromium.org/543125/diff/3003/3008#newcode268 src/client/linux/handler/exception_handler.cc:268: #if !defined(__ARM_ARCH_7A__) On 2010/01/22 20:45:38, awong wrote: > Please add comments explaining the ifdefs...this is complicated enough already. Done. http://codereview.chromium.org/543125/diff/3003/3009 File src/client/linux/handler/exception_handler.h (right): http://codereview.chromium.org/543125/diff/3003/3009#newcode149 src/client/linux/handler/exception_handler.h:149: #if !defined(__ARM_ARCH_7A__) On 2010/01/29 20:43:53, ted.mielczarek wrote: > The cross-compile setup I'm using (Scratchbox, for targeting Maemo) is targeting > __ARM_ARCH_6J__. Can we use __ARMEL__ or __ARM_EABI__ in all these ifdefs > instead of 7A specifcially? I'm not an ARM wizard, but I suspect most of the > things you're guarding on don't need to be quite that specific. Done. http://codereview.chromium.org/543125/diff/3003/3009#newcode150 src/client/linux/handler/exception_handler.h:150: struct _libc_fpstate float_state; On 2010/01/22 20:45:38, awong wrote: > What is this for? Is this because we will not be enabling floating-point > emulation? Add a comment. Done. http://codereview.chromium.org/543125/diff/3003/3005 File src/client/linux/minidump_writer/linux_dumper.cc (right): http://codereview.chromium.org/543125/diff/3003/3005#newcode87 src/client/linux/minidump_writer/linux_dumper.cc:87: int foo; On 2010/01/22 20:45:38, awong wrote: > I'm a bit unsure as to whether this is safe for testing stack directionality. > Did you verify if we're guaranteed by spec as to the stack ordering of locals > that are primitives? removed http://codereview.chromium.org/543125/diff/3003/3005#newcode362 src/client/linux/minidump_writer/linux_dumper.cc:362: // Move the stack pointer to the bottom of the page that it's in. On 2010/01/22 20:45:38, awong wrote: > why promote the stack direction & page_size code into member variables? They > seem to only be used in this function. removed http://codereview.chromium.org/543125/diff/3003/3006 File src/client/linux/minidump_writer/linux_dumper.h (right): http://codereview.chromium.org/543125/diff/3003/3006#newcode142 src/client/linux/minidump_writer/linux_dumper.h:142: bool stack_grows_down_; On 2010/01/21 22:48:04, hannahtang.work wrote: > Are these really things that needs to be an instance variable? Can it be a > class constant, or even a #defined variable? I removed stack_grows_down code and made them not instance variables. http://codereview.chromium.org/543125/diff/3003/3007 File src/client/linux/minidump_writer/linux_dumper_unittest.cc (right): http://codereview.chromium.org/543125/diff/3003/3007#newcode32 src/client/linux/minidump_writer/linux_dumper_unittest.cc:32: #include <sys/types.h> On 2010/01/22 20:45:38, awong wrote: > add newline to separate system includes from our code. Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode81 src/client/linux/minidump_writer/linux_dumper_unittest.cc:81: ASSERT_TRUE(false); On 2010/01/22 20:45:38, awong wrote: > On 2010/01/21 22:48:04, hannahtang.work wrote: > > Doesn't ASSERT have stream output? Eg, ASSERT_TRUE(false) << "Errno was: " << > > errno; > > There should also be a FAIL() macro that's better than asserting false is true. > > Also, consider strerror instead of just errno. > > FAIL() << "Exec failed: " << strerror(errno); Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode81 src/client/linux/minidump_writer/linux_dumper_unittest.cc:81: ASSERT_TRUE(false); On 2010/01/21 22:48:04, hannahtang.work wrote: > Doesn't ASSERT have stream output? Eg, ASSERT_TRUE(false) << "Errno was: " << > errno; Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode88 src/client/linux/minidump_writer/linux_dumper_unittest.cc:88: EXPECT_EQ((size_t)5, dumper.threads().size()); On 2010/01/21 22:48:04, hannahtang.work wrote: > Can you make both this literal and the number_of_threads var into constants? Eg > static const char kNumberOfThreadsStr = "5"; and static const int > kNumberOfThreads = 5; Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode95 src/client/linux/minidump_writer/linux_dumper_unittest.cc:95: // one word off the stack to store it's thread id. On 2010/01/21 22:48:04, hannahtang.work wrote: > it's -> its Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode97 src/client/linux/minidump_writer/linux_dumper_unittest.cc:97: void* process_tid_location = (void *)(one_thread.regs.uregs[11] - 8); On 2010/01/22 20:45:38, awong wrote: > Is this true for the arm thumb ABI? I think the fp on arm thumb is r7...at > least that's what piman said. Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode150 src/client/linux/minidump_writer/linux_dumper_unittest.cc:150: ASSERT_NE((void*)NULL, linux_gate_loc); On 2010/01/22 20:45:38, awong wrote: > Just ASSERT_TRUE(linux_gate_loc); > > It's a bit more idiomatic. Done. http://codereview.chromium.org/543125/diff/3003/3007#newcode151 src/client/linux/minidump_writer/linux_dumper_unittest.cc:151: if (linux_gate_loc) { On 2010/01/21 22:48:04, hannahtang.work wrote: > You don't need this if() conditional anymore; the test will abort if it's NULL. Done. http://codereview.chromium.org/543125/diff/3003/3011 File src/client/linux/minidump_writer/linux_dumper_unittest_helper.c (right): http://codereview.chromium.org/543125/diff/3003/3011#newcode32 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:32: // thread id. On 2010/01/22 20:45:38, awong wrote: > newline between preamble and code please. Done. http://codereview.chromium.org/543125/diff/3003/3011#newcode39 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:39: void *threadfunction(void *data) { On 2010/01/22 20:45:38, awong wrote: > threadfunction -> thread_function Done. http://codereview.chromium.org/543125/diff/3003/3011#newcode41 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:41: while(true) ; On 2010/01/22 20:45:38, awong wrote: > Do we need to busy loop this? Can we sleep(5) and make this still useable? > Otherwise, the failure case is to burn the cpu. :) Yeah, I can do this but then I have a pretty intricate dependency on how sleep leaves the stack setup. e.g. on x86 it goes sleep->nanosleep->kernel_vsyscall and puts the thread_id variable 48 bytes from the current EBP while the thread is sleeping. I made the test case always use EXPECT_* rather than ASSERT_* so that the kill() there is always executed. is that OK? http://codereview.chromium.org/543125/diff/3003/3011#newcode47 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:47: printf("ERROR: number of threads is 0"); On 2010/01/22 20:45:38, awong wrote: > fprintf to stderr for errors please. :) > > stderr is unbuffered so we can be sure we will indeed see the output -- and see > it when it's produced -- as opposed to stdout. Done. http://codereview.chromium.org/543125/diff/3003/3011#newcode54 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:54: threadfunction(NULL); On 2010/01/22 20:45:38, awong wrote: > the pedant in me wants you to call join on all the threads in a loop. how about starting them detached? http://codereview.chromium.org/543125/diff/3003/3004 File src/common/linux/linux_syscall_support.h (right): http://codereview.chromium.org/543125/diff/3003/3004#newcode80 src/common/linux/linux_syscall_support.h:80: defined(__mips__) || defined(__PPC__) || defined(__ARM_ARCH_7A__)) \ On 2010/01/29 20:43:53, ted.mielczarek wrote: > Again, I think you can get away with __ARMEL__ or __ARM_EABI__. Done. http://codereview.chromium.org/543125/diff/3003/3004#newcode220 src/common/linux/linux_syscall_support.h:220: #if defined(__i386__) || defined(__ARM_ARCH_7A__) || defined(__ARM_ARCH_3__) \ On 2010/01/22 20:45:38, awong wrote: > I'm starting to think you need a BREAKPAD_ARMEL define that resolves to: > defined(__ARM_ARCH_7a__) || defined(__ARM_ARCH_3__) i'd rather not change the existing ARM_ARCH_3 dependency to use the new EABI syscall interface, so I left them split. http://codereview.chromium.org/543125/diff/3003/3004#newcode1530 src/common/linux/linux_syscall_support.h:1530: #elif defined(__ARM_ARCH_3__) || defined(__ARM_ARCH_7A__) On 2010/01/22 20:45:38, awong wrote: > Okay, I'm confused...sometimes you have these __ARM_ARCH_3__ and __ARM_ARCH_7a__ > defines. Then somtimes you have these __ARM_EABI__ defines. > > Are we being clear about when we're doing something based on the arch type, and > when we're doing it because of the abi type? > > This code here looks to be contingent on EABI, and not arm3 versus arm7a. > > Same question for actually every other ifdef... Done. http://codereview.chromium.org/543125/diff/3003/3004#newcode2612 src/common/linux/linux_syscall_support.h:2612: #if !defined(__ARM_EABI__) On 2010/01/22 20:45:38, awong wrote: > Should this indentation be nested? It should have been in the previous upload, but I moved it completely outside the ifdef. > > Also, why bother with the !ARM_EABI? Why not just ARM_EABI and flip the order > of the if/else? > See above repsonse, it's not nested anymore. > Add a comment explaining why it's safe to assume the other code for every other > architecture. It isn't meant to be run for everything that's !__ARM_EABI__, but for the arcthiectures in the preceding "#if defined" clause (__i386__, __ARM_ARCH_3__, etc). the reason is that with the move to EABI, Linux added direct system calls for some socket operations that were previously accessed via socketcall. Socketcall() was removed when this happened. It's not clear to me what I should do for code that will end up being compiled on ARM_ARCH_3 with EABI. For now, duplicate symbol definitions will occur for the system calls that are both defined directly & via socketcall. http://codereview.chromium.org/543125/diff/3003/3004#newcode2772 src/common/linux/linux_syscall_support.h:2772: #if !defined(__ARM_EABI__) On 2010/01/22 20:45:38, awong wrote: > Same comment as earlier about clarifying why it's safe to assume this code for > everything other that ARM_EABI. the code that is in what's in the #if clause here is the system call for those architectures; with either EABI or 2.6, Linux changed to ugetrlimit.
http://codereview.chromium.org/543125/diff/10009/9004 File src/client/linux/minidump_writer/linux_dumper.h (left): http://codereview.chromium.org/543125/diff/10009/9004#oldcode69 src/client/linux/minidump_writer/linux_dumper.h:69: Somewhere along the way this got broken, this line needs to be #if defined(__i386). I'm failing to build on x86-64 with this change.
http://codereview.chromium.org/543125/diff/10009/9005 File src/client/linux/minidump_writer/linux_dumper_unittest.cc (right): http://codereview.chromium.org/543125/diff/10009/9005#newcode104 src/client/linux/minidump_writer/linux_dumper_unittest.cc:104: void* process_tid_location = (void *)(one_thread.regs.ebp - 8); This should be one_thread.regs.rbp. Also, this test fails for me on x86-64.
LGTM from my perspective with a few nits. Assuming Ted's comments are addressed, I'd say checkin and let's see what happens! :) Nice job btw. This is nasty code. -Albert http://codereview.chromium.org/543125/diff/3003/3011 File src/client/linux/minidump_writer/linux_dumper_unittest_helper.c (right): http://codereview.chromium.org/543125/diff/3003/3011#newcode41 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:41: while(true) ; On 2010/02/05 01:26:04, nealsid wrote: > On 2010/01/22 20:45:38, awong wrote: > > Do we need to busy loop this? Can we sleep(5) and make this still useable? > > Otherwise, the failure case is to burn the cpu. :) > > Yeah, I can do this but then I have a pretty intricate dependency on how sleep > leaves the stack setup. e.g. on x86 it goes sleep->nanosleep->kernel_vsyscall > and puts the thread_id variable 48 bytes from the current EBP while the thread > is sleeping. I made the test case always use EXPECT_* rather than ASSERT_* so > that the kill() there is always executed. is that OK? Sure...sounds sensible. Add a comment? :) Also, use "while (true) {}" instead http://codereview.chromium.org/543125/diff/3003/3011#newcode54 src/client/linux/minidump_writer/linux_dumper_unittest_helper.c:54: threadfunction(NULL); On 2010/02/05 01:26:04, nealsid wrote: > On 2010/01/22 20:45:38, awong wrote: > > the pedant in me wants you to call join on all the threads in a loop. > > how about starting them detached? joining is cleaner IMO since you force a known termination point for all threads. Otherwise, the thread cancelation happens post main. Don't realy care though. http://codereview.chromium.org/543125/diff/10009/9008 File src/client/linux/Makefile (right): http://codereview.chromium.org/543125/diff/10009/9008#newcode26 src/client/linux/Makefile:26: $(DUMPER_HELPER_TEST_C_SRC)) Are these suppsoed to be tabs in the line continuation? Wasn't sure if it mattered or what the convention in the rest of the tile is. http://codereview.chromium.org/543125/diff/10009/9003 File src/client/linux/minidump_writer/linux_dumper.cc (right): http://codereview.chromium.org/543125/diff/10009/9003#newcode370 src/client/linux/minidump_writer/linux_dumper.cc:370: const ptrdiff_t offset = stack_pointer - (uint8_t*) mapping->start_addr; so...we're assuming a single stack direction now? http://codereview.chromium.org/543125/diff/10009/9003#newcode382 src/client/linux/minidump_writer/linux_dumper.cc:382: unsigned long tmp = 55; Magic number alert! What's this number for? http://codereview.chromium.org/543125/diff/10009/9005 File src/client/linux/minidump_writer/linux_dumper_unittest.cc (right): http://codereview.chromium.org/543125/diff/10009/9005#newcode74 src/client/linux/minidump_writer/linux_dumper_unittest.cc:74: sprintf(kNumberOfThreadsArgument, "%d", kNumberOfThreadsInHelperProgram); Can you use StrintPrintf instead (if you have it)? Otherwise, can we shift to snprintf?
LGTM http://codereview.chromium.org/543125/diff/3003/3010 File src/client/linux/Makefile (right): http://codereview.chromium.org/543125/diff/3003/3010#newcode60 src/client/linux/Makefile:60: rm -f $(UNITTEST_BIN) $(BREAKPAD_LIBRARY) $(LIB_CC_OBJ) $(LIB_C_OBJ) $(TEST_CC_OBJ) $(DUMPER_HELPER_TEST_BIN) core On 2010/02/05 01:26:04, nealsid wrote: > On 2010/01/21 22:48:04, hannahtang.work wrote: > > Please use spaces instead of tabs. > > makefiles require tabs Thanks for the reminder, it's been a while since I've worked with Makefiles. :)
ping...noticed this is still open. Neal, did this get landed? Can we close? On 2010/02/08 23:01:39, hannahtang.work wrote: > LGTM > > http://codereview.chromium.org/543125/diff/3003/3010 > File src/client/linux/Makefile (right): > > http://codereview.chromium.org/543125/diff/3003/3010#newcode60 > src/client/linux/Makefile:60: rm -f $(UNITTEST_BIN) $(BREAKPAD_LIBRARY) > $(LIB_CC_OBJ) $(LIB_C_OBJ) $(TEST_CC_OBJ) $(DUMPER_HELPER_TEST_BIN) core > On 2010/02/05 01:26:04, nealsid wrote: > > On 2010/01/21 22:48:04, hannahtang.work wrote: > > > Please use spaces instead of tabs. > > > > makefiles require tabs > > Thanks for the reminder, it's been a while since I've worked with Makefiles. :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
