Issue 540002: Fix three unit tests on recent ARM devices. (Closed)
|
Created:
11 years, 8 months ago by digit Modified:
11 years, 8 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionFix three unit tests on recent ARM devices. Three unit tests were failing on recent ARM devices (e.g. Galaxy Nexus or Nexus 4), while ran properly on older ones (e.g. Nexus S). The main issue is that the instruction cache needs to be explicitely cleared on ARM after writing machine code bytes to a malloc()-ed page with PROT_EXEC. Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #MessagesTotal messages: 11
sorry, had to update the issue, I made a typo when writing ted's email address during gcl upload :)
Sign in to reply to this message.
LGTM https://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception... File src/client/linux/handler/exception_handler_unittest.cc (right): https://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception... src/client/linux/handler/exception_handler_unittest.cc:59: void flush_instruction_cache(const char* memory, uint32_t memory_size) { Naming: FlushInstructionCache. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception... src/client/linux/handler/exception_handler_unittest.cc:64: reinterpret_cast<long>(memory + memory_size), Can you put the reinterpret_cast<> around memory only and not memory_size. That way, if someone changes memory to be a pointer of a different type, the memory_size argument won’t change meaning from bytes to something else. https://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception... src/client/linux/handler/exception_handler_unittest.cc:68: syscall(0xf0002, Is __ARM_NR_cacheflush available outside the kernel? No big deal if it isn’t, but writing it symbolically instead of using that constant would have saved me five minutes or so in finding the in-kernel implementation to validate that the syscall really wants start-end and not start-length, and to make sure the “0” third argument was correct. (Of course you got it right. I just wanted to check.) https://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception... src/client/linux/handler/exception_handler_unittest.cc:70: reinterpret_cast<long>(memory + memory_size), Same here.
Sign in to reply to this message.
Thanks for fixing this. I noticed these failing but didn't get around to investigating why.
Sign in to reply to this message.
http://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception_... File src/client/linux/handler/exception_handler_unittest.cc (right): http://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception_... src/client/linux/handler/exception_handler_unittest.cc:59: void flush_instruction_cache(const char* memory, uint32_t memory_size) { Damn, sorry, I'm switching all the time between projects with different coding styles, I always get confused. Thanks. http://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception_... src/client/linux/handler/exception_handler_unittest.cc:64: reinterpret_cast<long>(memory + memory_size), That's an excellent suggestion. Done! http://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception_... src/client/linux/handler/exception_handler_unittest.cc:68: syscall(0xf0002, On 2013/03/20 18:42:52, Mark Mentovai wrote: > Is __ARM_NR_cacheflush available outside the kernel? > I'm not sure about that. I don't have Linux/ARM Glibc distribution to test this (the constant certainly doesn't appear in the GLibc sources themselves). I have manually tested that the code works by running it under Android/ARM (after all, it uses the same kernel). I've uploaded another patchset where __ARM_NR_cacheflush is declared through #ifndef .. #define .. #endif to make this more explicit though. http://breakpad.appspot.com/540002/diff/1/src/client/linux/handler/exception_... src/client/linux/handler/exception_handler_unittest.cc:70: reinterpret_cast<long>(memory + memory_size), On 2013/03/20 18:42:52, Mark Mentovai wrote: > Same here. Done.
Sign in to reply to this message.
On 2013/03/20 19:44:20, Ted Mielczarek wrote: > Thanks for fixing this. I noticed these failing but didn't get around to > investigating why. Yeah, I admit it took me far too long to understand what was really happening. At first I suspected that the sequence 0xff, 0xff, 0xff, 0xff wasn't invalid on more recent CPUs, but changing this to something different (like UDF) or even to an explicit write to address 0 didn't seem to solve the issue :)
Sign in to reply to this message.
http://breakpad.appspot.com/540002/diff/6001/src/client/linux/handler/excepti... File src/client/linux/handler/exception_handler_unittest.cc (right): http://breakpad.appspot.com/540002/diff/6001/src/client/linux/handler/excepti... src/client/linux/handler/exception_handler_unittest.cc:69: # define __ARM_NR_cacheflush 0xf0002 You defined __ARM_NR_cacheflush… http://breakpad.appspot.com/540002/diff/6001/src/client/linux/handler/excepti... src/client/linux/handler/exception_handler_unittest.cc:71: syscall(__NR_ARM_cacheflush, begin, end, 0); …but used __NR_ARM_cacheflush.
Sign in to reply to this message.
Yup, this definitely was a good find. On Wed, Mar 20, 2013 at 5:24 PM, <digit@chromium.org> wrote: > On 2013/03/20 19:44:20, Ted Mielczarek wrote: > >> Thanks for fixing this. I noticed these failing but didn't get around >> > to > >> investigating why. >> > > Yeah, I admit it took me far too long to understand what was really > happening. At first I suspected that the sequence 0xff, 0xff, 0xff, 0xff > wasn't invalid on more recent CPUs, but changing this to something > different (like UDF) or even to an explicit write to address 0 didn't > seem to solve the issue :) > > > http://breakpad.appspot.com/**540002/<http://breakpad.appspot.com/540002/> >
Sign in to reply to this message.
latest patch was manually tested on Android (both the Android and GLibc codepaths). http://breakpad.appspot.com/540002/diff/6001/src/client/linux/handler/excepti... File src/client/linux/handler/exception_handler_unittest.cc (right): http://breakpad.appspot.com/540002/diff/6001/src/client/linux/handler/excepti... src/client/linux/handler/exception_handler_unittest.cc:71: syscall(__NR_ARM_cacheflush, begin, end, 0); Damn, great find, thanks.
Sign in to reply to this message.
This has been submitted as 1132. For some reason, "gcl commit" froze after the submit so I had to interrupt it. I'm manually closing this issue, thanks.
Sign in to reply to this message.
|
