Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11639038:
ucontext_t support for Android x86. (Closed)
|
Created:
8 years ago by yfw.chromium Modified:
7 years, 5 months ago CC:
chromium-reviews, agl, jln+watch_chromium.org, shashi, digit1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionucontext_t support for Android x86.
Android x86 has not support for ucontext_t yet. Add local definition
to make content_shell build correctly for Android x86.
TEST=build x86 content shell.
NOTRY=true
BUG=
Patch Set 1 #
Total comments: 10
Patch Set 2 : ucontext_t support for Android x86. #Patch Set 3 : ucontext_t support for Android x86. #
Total comments: 29
Patch Set 4 : ucontext_t support for Android x86. #Patch Set 5 : ucontext_t support for Android x86 #Patch Set 6 : ucontext_t support for Android x86 #
Messages
Total messages: 18 (0 generated)
Hi, Could you please help to review this change? It enable the content_shell_apk build for android x86. Thanks. Regards yfw
https://codereview.chromium.org/11639038/diff/1/sandbox/linux/seccomp-bpf/san... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://codereview.chromium.org/11639038/diff/1/sandbox/linux/seccomp-bpf/san... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:24: #elif defined(__i386__) Since you picked a file name that was independent of the flavor of the architecture, you should probably include this file both for __i386__ and for __x86_64__ Of course, you should also make sure that the file behaves correctly for these platforms ... or, if you cannot test that, cause it to trigger an #error if the flavor is currently unsupported. That would give people a heads-up where they need to start fixing things, when they want to support additional platform configurations. https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... File sandbox/linux/services/android_x86_ucontext.h (right): https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:10: #if !defined(__BIONIC_HAVE_UCONTEXT_T) Can you please find another reviewer that can verify this is the correct thing to do? I don't know enough about Android to have any opinion. https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:17: uint32_t gregs[32]; Did you actually test this? This looks wrong to me. On i386, I would expect to see 19 general purpose registers, and on x86_64 I would expect to see 23. But this is just from reading the sources, without actually testing things myself. https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:17: uint32_t gregs[32]; This should be a "greg_t" instead of a "uint32_t", shouldn't it? https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:20: uint32_t cr2; I think, these should be "unsigned long" instead of "uint32_t", in order to make things work both on i386 and x86-64. But, again, I haven't actually tested this. I might have gotten this wrong. You should definitely test this. https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:37: sigset_t uc_sigmask; We really have to have a unittest for this. If you had a proper test, we would have caught the problem of having the wrong number of general purpose registers. Can you write a test that (spot) checks the general purpose registers and the signal mask? I think, you can do all of this without any assembly coding. You bind a local variable to a particular CPU register (you might want to check several), you then raise a signal by triggering an error condition (e.g. a segmentation fault). Inside the signal handler, you verify that the saved CPU register has the value that you expect. You then use siglongjmp() to return from the signal handler, as that allows you to return without re-executing the faulting instruction. For the signal mask, you can do the same thing, by setting the mask outside of the signal handler and then verifying its value inside the signal handler. Make sure this test works for all of our commonly supported platforms (i.e. i386, x86-64, and ARM). It should probably go into something like ucontext_unittest.cc. And it should test with the implementation of ucontext_t that we pick by default for this particular platform. I.e. on non-Android builds, we would pick the implementation in glibc. But on Android builds, we would pick your implementation.
On 2012/12/21 02:03:54, Markus (顧孟勤) wrote: > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/seccomp-bpf/san... > File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/seccomp-bpf/san... > sandbox/linux/seccomp-bpf/sandbox_bpf.cc:24: #elif defined(__i386__) > Since you picked a file name that was independent of the flavor of the > architecture, you should probably include this file both for __i386__ and for > __x86_64__ > > Of course, you should also make sure that the file behaves correctly for these > platforms ... or, if you cannot test that, cause it to trigger an #error if the > flavor is currently unsupported. That would give people a heads-up where they > need to start fixing things, when they want to support additional platform > configurations. > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... > File sandbox/linux/services/android_x86_ucontext.h (right): > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... > sandbox/linux/services/android_x86_ucontext.h:10: #if > !defined(__BIONIC_HAVE_UCONTEXT_T) > Can you please find another reviewer that can verify this is the correct thing > to do? I don't know enough about Android to have any opinion. > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... > sandbox/linux/services/android_x86_ucontext.h:17: uint32_t gregs[32]; > Did you actually test this? This looks wrong to me. > > On i386, I would expect to see 19 general purpose registers, and on x86_64 I > would expect to see 23. But this is just from reading the sources, without > actually testing things myself. > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... > sandbox/linux/services/android_x86_ucontext.h:17: uint32_t gregs[32]; > This should be a "greg_t" instead of a "uint32_t", shouldn't it? > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... > sandbox/linux/services/android_x86_ucontext.h:20: uint32_t cr2; > I think, these should be "unsigned long" instead of "uint32_t", in order to make > things work both on i386 and x86-64. > > But, again, I haven't actually tested this. I might have gotten this wrong. You > should definitely test this. > > https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... > sandbox/linux/services/android_x86_ucontext.h:37: sigset_t uc_sigmask; > We really have to have a unittest for this. If you had a proper test, we would > have caught the problem of having the wrong number of general purpose registers. > > Can you write a test that (spot) checks the general purpose registers and the > signal mask? > > I think, you can do all of this without any assembly coding. > > You bind a local variable to a particular CPU register (you might want to check > several), you then raise a signal by triggering an error condition (e.g. a > segmentation fault). Inside the signal handler, you verify that the saved CPU > register has the value that you expect. You then use siglongjmp() to return from > the signal handler, as that allows you to return without re-executing the > faulting instruction. > > For the signal mask, you can do the same thing, by setting the mask outside of > the signal handler and then verifying its value inside the signal handler. > > Make sure this test works for all of our commonly supported platforms (i.e. > i386, x86-64, and ARM). It should probably go into something like > ucontext_unittest.cc. > > And it should test with the implementation of ucontext_t that we pick by default > for this particular platform. I.e. on non-Android builds, we would pick the > implementation in glibc. But on Android builds, we would pick your > implementation. Thanks a lot for the comments. I will update the patch later.
https://codereview.chromium.org/11639038/diff/1/sandbox/linux/seccomp-bpf/san... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://codereview.chromium.org/11639038/diff/1/sandbox/linux/seccomp-bpf/san... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:24: #elif defined(__i386__) On 2012/12/21 02:03:54, Markus (顧孟勤) wrote: > Since you picked a file name that was independent of the flavor of the > architecture, you should probably include this file both for __i386__ and for > __x86_64__ > > Of course, you should also make sure that the file behaves correctly for these > platforms ... or, if you cannot test that, cause it to trigger an #error if the > flavor is currently unsupported. That would give people a heads-up where they > need to start fixing things, when they want to support additional platform > configurations. Android just has i386 ABI support now. So we don't need to take care of x86_64 for Android. I will introduce #error for other platform. And yes. x86_64 may be supported by Android in the future. But I suppose Android bionic should have ucontext definition for x86_64 at that time. So we could remove all these local definition and use bionic https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... File sandbox/linux/services/android_x86_ucontext.h (right): https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:10: #if !defined(__BIONIC_HAVE_UCONTEXT_T) On 2012/12/21 02:03:54, Markus (顧孟勤) wrote: > Can you please find another reviewer that can verify this is the correct thing > to do? I don't know enough about Android to have any opinion. This was introduced in Android with this change: https://android-review.googlesource.com/#/c/38875/4/libc/include/sys/cdefs.h And several Android components use this as well. So I suppose it's safe to use it here. And I don't know who should be involved here as well. https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:17: uint32_t gregs[32]; On 2012/12/21 02:03:54, Markus (顧孟勤) wrote: > This should be a "greg_t" instead of a "uint32_t", shouldn't it? Will change 32 to 19 and use greg_t. https://codereview.chromium.org/11639038/diff/1/sandbox/linux/services/androi... sandbox/linux/services/android_x86_ucontext.h:20: uint32_t cr2; On 2012/12/21 02:03:54, Markus (顧孟勤) wrote: > I think, these should be "unsigned long" instead of "uint32_t", in order to make > things work both on i386 and x86-64. > > But, again, I haven't actually tested this. I might have gotten this wrong. You > should definitely test this. OK.
New patchset is uploaded to address the comments. ChangeLog: unit test for ucontext is added. NOTE: The test case can't pass x86 build because of this android x86 ndk issue: http://code.google.com/p/android/issues/detail?id=19851 The ARM emu has issue to run the unit test. I will find a real arm device to run unit test later. Please review the code first. Hold the patch merging till we get the test result on real ARM device.
The unit tests runs OK on ARM devices. Can't pass x86 android build because of x86 android NDK issue.
On 2013/01/04 06:03:08, yfw.chromium wrote: > The unit tests runs OK on ARM devices. Can't pass x86 android build because of > x86 android NDK issue. Your CL looks generally OK. I haven't looked in detail yet, and there might be a handful of nitpicks, though. I'll do a casual review shortly. I'll hold off on a really deep review for the time being, as your work overlaps with work the Julien is currently doing. He is refactoring several of these files. In fact, it might be most expedient if he merged your very good patches directly into the CL that he has pending. Of course, I'll ask him to give you full credit for the work done. Also, I honestly don't know what I need to do to run these tests locally. If you happen to have a link to some document that walks me through setting up a test environment for Android x86, that would be greatly appreciated.
Let's get your CL in shape and land it, unless you're busy (we need this fairly quickly), in which case I'll take it over. 1. Please use BUG=166704 in your commit message 2. Please don't use NOTRY=true on such a hairy change. We'll NOTRY=true it later if and only if it fails because of Windows or some other unrelated reason. 3. Please rebase your change on the latest Chromium. You'll see that compile_seccomp_bpf in the GYP file has changed and you'll need to re-enable x86 Otherwise it looks genereally good, thanks for working on this! There are a few other comments below, the biggest of them is that I would like once general android_ucontext.h file to include, regardless of the architecture. https://chromiumcodereview.appspot.com/11639038/diff/9001/build/android/run_t... File build/android/run_tests.py (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/build/android/run_t... build/android/run_tests.py:72: 'sandbox_linux_unittests', Don't touch this file at the moment. We already run this test on the FYI bot and we can enable it by default in another CL, once everything is stabilized. https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/sandb... File sandbox/linux/sandbox_linux.gypi (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/sandb... sandbox/linux/sandbox_linux.gypi:13: ['(OS=="linux" or OS=="android") and (target_arch=="ia32" ' Update your repo and you'll see that you need to re-enable x86 here. https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/secco... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/secco... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:21: #if defined(OS_ANDROID) Can you move most of what's below into a global services/android_ucontext.h ? https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... File sandbox/linux/services/android_x86_ucontext.h (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... sandbox/linux/services/android_x86_ucontext.h:11: #if !defined(__BIONIC_HAVE_UCONTEXT_T) That's lovely! Do you mind patching it on the arm version as well ? Even better: have it in a global android_ucontext.h that includes the other files. https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... File sandbox/linux/services/ucontext_unittest.cc (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... sandbox/linux/services/ucontext_unittest.cc:19: namespace base { You're in sandbox. Please look at any other _unittest.c file in Sandbox. https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... sandbox/linux/services/ucontext_unittest.cc:50: TEST_F(ucontext_test, TestUcontext) { Please use a SANDBOX_TEST so that you get to run in your own process etc. Otherwise other tests will share the side effects of this.
https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... File sandbox/linux/services/ucontext_unittest.cc (right): https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:13: #else This is a useful unittest even if we are building for Linux as opposed to Android. We should make sure it is used on Linux and then, of course, we have to make sure it runs on both i386 and x86-64, as well as ARM. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:20: namespace android { This should not be Android specific code. Just follow the examples in the other unittests that we already have. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:22: typedef testing::Test ucontext_test; This sounds wrong, as far as our style guide is concerned. But Julien is that style-guide-god -- he probably can tell you better. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:30: static int par5_v = 0x00; Our style guide prefers the use of anonymous namespaces instead of "static". Also, these should be named something like kExpectedValueParm1, or some other similarly appropriate name. And they should be marked as "const". https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:32: void sig_action(int n, siginfo_t *siginfo, void* context) While I personally think our style guide is mistaken in where it wants the "*", at the very least make sure you are consistent in your choice of where you put it. The style guide wants a different capitalization for this function name. Also, this function should be in an anonymous namespace. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:33: { The brace should probably be at the end of the previous line. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:34: ucontext_t *ctx = (ucontext_t *)context; According to our style guide, we should be using C++ style casts, rather than C casts. Maybe, rename "context" to "void_context"; and "ctx" to "context"? That makes it a little more clear what is happening here. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:38: sigaddset(&set, SIGFPE); This is a little fragile. Both glibc and the kernel like to intercept sigprocmask() and make some of the signal bits immutable. The code would be more reliable, if you queried the current signal mask prior to raising the signal. Then store it in a global variable that you can check from within the signal handler. Or if you don't want to use a global variable, store a pointer to it in a CPU register. After all, you are looking at CPU registers already. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:46: EXPECT_EQ(ctx->uc_sigmask, set); Use the comparison macros that we wrote for the sandbox. They can safely be called from within a sub-function or a signal handler. The EXPECT_XXX() macros OTOH must not be called from a signal handler. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:50: TEST_F(ucontext_test, TestUcontext) { Use the test macros that we wrote for the sandbox. They do an extra fork(). This allows you to make global state changes (such as messing with signal handlers), without breaking other tests. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:52: struct sigaction act; If you write "struct sigaction act = { }", you don't need the memset() https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:59: act.sa_flags = SA_RESTART | SA_SIGINFO; Why do you set SA_RESTART? I don't think that even makes sense on SIGSEGV -- or am I missing something. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:72: #if defined(__i386__) I really would prefer if you tried to avoid using assembly. It just makes it easier for other people to modify the code. But if you absolutely have to use assembly, you have to make sure you set the appropriate constraints to inform the compiler that you just clobbered a whole bunch of registers. Try: register int ebx asm("ebx") = kExpectedValueParm1; Not all of the registers will be available to you, as the compiler needs some for internal uses. Make sure you compile with -fPIC and with frame pointers in order to make sure your code is compatible with these flags. In order to raise a signal, you can do: *(volatile char *)0 = 0; // Trigger a SIGSEGV https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:91: } How about you change one or more of the CPU registers from within the signal handler. Then you can verify that the change took place and you also verify that the signal handler ran. Although, come to think of it ... this might not work with sigsetjmp() :-( That's really too bad. Maybe we should try to get rid of the sigsetjmp(); not sure how to do that cleanly, though.
David: I was told to add you to this thread. This more include files hackery to compile sandbox/ on x86 Android. YFW: if you don't have time to work on it, please let us know ASAP, there are a few things that are blocking on this and I really would like to have something ready soon. History teaches us that changes like this inevitably end-up breaking someone's build and that we need a few itereations :)
On 2013/01/08 23:04:17, Julien Tinnes wrote: > David: I was told to add you to this thread. > > This more include files hackery to compile sandbox/ on x86 Android. > > YFW: if you don't have time to work on it, please let us know ASAP, there are a > few things that are blocking on this and I really would like to have something > ready soon. History teaches us that changes like this inevitably end-up breaking > someone's build and that we need a few itereations :) Hi Julien, I could work on it. But as newbie, it takes longer time to make everything done (especially for test suite. It took longest my time. :)). So you could take over. Regards yfw (Yin, Fengwei)
Thanks a lot for your guys comments. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... File sandbox/linux/services/ucontext_unittest.cc (right): https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:34: ucontext_t *ctx = (ucontext_t *)context; On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > According to our style guide, we should be using C++ style casts, rather than C > casts. > > Maybe, rename "context" to "void_context"; and "ctx" to "context"? That makes it > a little more clear what is happening here. Yes. several C vs C++. :). https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:38: sigaddset(&set, SIGFPE); On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > This is a little fragile. Both glibc and the kernel like to intercept > sigprocmask() and make some of the signal bits immutable. > > The code would be more reliable, if you queried the current signal mask prior to > raising the signal. Then store it in a global variable that you can check from > within the signal handler. No. If kernel changes the signal mask between we store it and signal handle is called. The thing is still broken. Frankly, I don't think kernel could play with signal mask a lot. Not sure about glibc. > > Or if you don't want to use a global variable, store a pointer to it in a CPU > register. After all, you are looking at CPU registers already. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:50: TEST_F(ucontext_test, TestUcontext) { On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > Use the test macros that we wrote for the sandbox. They do an extra fork(). This > allows you to make global state changes (such as messing with signal handlers), > without breaking other tests. So I suppose that it's ok child process receiving signal like SIGSEGV and we could remove sigsetjmp/siglongjmp. That's good. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:59: act.sa_flags = SA_RESTART | SA_SIGINFO; On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > Why do you set SA_RESTART? I don't think that even makes sense on SIGSEGV -- or > am I missing something. I tried to minimize the impact of the test (Before I is told to use sandbox unittest). https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:72: #if defined(__i386__) On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > I really would prefer if you tried to avoid using assembly. It just makes it > easier for other people to modify the code. > > But if you absolutely have to use assembly, you have to make sure you set the > appropriate constraints to inform the compiler that you just clobbered a whole > bunch of registers. > > Try: > > register int ebx asm("ebx") = kExpectedValueParm1; > > Not all of the registers will be available to you, as the compiler needs some > for internal uses. > > Make sure you compile with -fPIC and with frame pointers in order to make sure > your code is compatible with these flags. > > In order to raise a signal, you can do: > > *(volatile char *)0 = 0; // Trigger a SIGSEGV No. toolchain could use registers for *(volatile char *)0 = 0; and it heavily depends on the toolchain. Which make hard to select which register should be checked. That's why I use assembly here. register int ebx asm("ebx") = kExpectedValueParm1; doesn't work either because the registers could be reused by toolchain again. I checked it on android x86/ARM. Clobbering registers is not big deal in this special case because the signal handler will restore the saved context which include original value for registers. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:72: #if defined(__i386__) On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > I really would prefer if you tried to avoid using assembly. It just makes it > easier for other people to modify the code. > > But if you absolutely have to use assembly, you have to make sure you set the > appropriate constraints to inform the compiler that you just clobbered a whole > bunch of registers. > > Try: > > register int ebx asm("ebx") = kExpectedValueParm1; > > Not all of the registers will be available to you, as the compiler needs some > for internal uses. > > Make sure you compile with -fPIC and with frame pointers in order to make sure > your code is compatible with these flags. > > In order to raise a signal, you can do: > > *(volatile char *)0 = 0; // Trigger a SIGSEGV Done. https://codereview.chromium.org/11639038/diff/9001/sandbox/linux/services/uco... sandbox/linux/services/ucontext_unittest.cc:91: } On 2013/01/08 22:30:17, Markus (顧孟勤) wrote: > How about you change one or more of the CPU registers from within the signal > handler. Then you can verify that the change took place and you also verify that > the signal handler ran. > > Although, come to think of it ... this might not work with sigsetjmp() :-( > That's really too bad. Maybe we should try to get rid of the sigsetjmp(); not > sure how to do that cleanly, though. If want to make sure the signal handler is called, use a global flag?
lgtm for the ucontext definitions, I don't have any opinion on the unit test itself. https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/secco... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/secco... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:27: #error "Lack ucontext_t definition for mips on Android" If you need a MIPS version, please see https://android-review.googlesource.com/#/c/38875/4/libc/arch-mips/include/ma... (this patch was later reverted because it broke something in the internal tree, but I plan to revive it some time in the future). https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... File sandbox/linux/services/android_x86_ucontext.h (right): https://chromiumcodereview.appspot.com/11639038/diff/9001/sandbox/linux/servi... sandbox/linux/services/android_x86_ucontext.h:42: } ucontext_t; that looks good to me too :)
Don't know why the new patch is created as: https://codereview.chromium.org/11828016 The new patch just passed the x86 build check. No device available to test it. Especially for unittest part. I suppose jln will take it over. :)
On 2013/01/09 11:27:21, yfw.chromium wrote: > Don't know why the new patch is created as: > https://codereview.chromium.org/11828016 > > The new patch just passed the x86 build check. No device available to test it. > Especially for unittest part. > > I suppose jln will take it over. :) For some reason the review tool doesn't let me look at your new CL. I imagine that the reason why it generated a new CL is because you switched branches. Go back to your old branch (or create a new one via git checkout -b new_branch) and use "git cl issue 11639038" to set the issue number to this one. Then you can "git cl upload".
On 2013/01/09 21:18:24, Julien Tinnes wrote: > On 2013/01/09 11:27:21, yfw.chromium wrote: > > Don't know why the new patch is created as: > > https://codereview.chromium.org/11828016 > > > > The new patch just passed the x86 build check. No device available to test it. > > Especially for unittest part. > > > > I suppose jln will take it over. :) > > For some reason the review tool doesn't let me look at your new CL. > > I imagine that the reason why it generated a new CL is because you switched > branches. Go back to your old branch (or create a new one via git checkout -b > new_branch) and use "git cl issue 11639038" to set the issue number to this one. > Then you can "git cl upload". Yes. That's the reason. And I guess the stupid thing makes you unseen to the new issue is I forgot publishing it. I will retry to update the patch. Frankly, I suppose you could take the change except the unittest. Just skip my unittest.
Add android_ucontext.h according to jln comments. Removed the sigsetjmp/siglongjmp which could make the unittest build-able for android x86. But: the unittest shouldn't be trusted. It's just hang when I tried it on x86 devices. But I hope the other changes except unittest could help.
Changes: 1. unittest uses SANDBOX_DEATH_TEST. 2. Fixed typo in DEATH_BY_SIGNAL definition. build for ARM/x86 android. Unittest passed on x86 devices. ARM device was not tested. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
