Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
125533004:
Initial implementation of IRT for non-SFI mode. (Closed)
|
Created:
6 years, 11 months ago by hidehiko Modified:
6 years, 11 months ago CC:
chromium-reviews, hamaji Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of IRT for non-SFI mode.
This CL implements the mechanism to pass an irt querying function to the plugin
process via AT_SYSINFO. Also, as the first step to implement many irt functions
for non-SFI mode, this CL introduces nacl_irt_basic.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=Tried to call a newly added function from plugin via AT_SYSINFO.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244144
Patch Set 1 #
Total comments: 23
Patch Set 2 : #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : #
Messages
Total messages: 9 (0 generated)
Could you take a look?
https://codereview.chromium.org/125533004/diff/1/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/125533004/diff/1/components/nacl.gyp#newcode194 components/nacl.gyp:194: 'nacl/loader/nonsfi/irt/irt_basic.cc', Could you put these under nonsfi/ without creating an irt/ subdir? This is quite deeply nested already. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:20: // Note: when we start to support threads, here we need to join them. That's not the case. There's no need to join threads, and you can't join threads that don't terminate anyway. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:21: ::exit(status); This should really call _exit(), because exit() does some finalisation that can break running threads. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:22: while (1) *(volatile int *) 0 = 0; // Crash. Not really necessary, since _exit() is generally declared as "noreturn", so this will get optimised away by the compiler anyway. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:36: *ticks = ::clock(); // No failure. The man page says this can return (clock_t)-1 on failure... https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:42: // We should be able to call nanosleep(NULL, &host_rem) for this. Why do you expect req==NULL to work? This isn't part of the NaCl IRT interface, so I don't think you should check for this. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:75: *value = ::sysconf(name); Calling through to sysconf() isn't valid because the NaCl interface has a different set of supported values. This should handle NACL_ABI__SC_PAGESIZE and NACL_ABI__SC_NPROCESSORS_ONLN. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_interfaces.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_interfaces.cc:12: extern const struct nacl_irt_basic kIrtBasic; This is already declared in the header https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_interfaces.cc:25: NaClInterfaceTable(const char* name, const T& value) I'm pretty sure using a constructor like this makes kIrtInterfaces non-POD and adds a global initialiser, which will fail the perf check for the global initialiser count. Just use the {} syntax for initialisation instead.
https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:42: // We should be able to call nanosleep(NULL, &host_rem) for this. On 2014/01/07 09:45:06, Mark Seaborn wrote: > Why do you expect req==NULL to work? This isn't part of the NaCl IRT interface, > so I don't think you should check for this. Let me reply as I wrote this. I think both linux and nacl service runtime don't crash for req==NULL. I think NaClSysNanosleep returns -NACL_ABI_EFAULT for NULL req. If you think this compatibility for error case is not important, I'm fine with removing this. We will workaround this issue in our side.
Thank you for review. Could you take another look? https://codereview.chromium.org/125533004/diff/1/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/125533004/diff/1/components/nacl.gyp#newcode194 components/nacl.gyp:194: 'nacl/loader/nonsfi/irt/irt_basic.cc', On 2014/01/07 09:45:06, Mark Seaborn wrote: > Could you put these under nonsfi/ without creating an irt/ subdir? This is > quite deeply nested already. Done. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:20: // Note: when we start to support threads, here we need to join them. On 2014/01/07 09:45:06, Mark Seaborn wrote: > That's not the case. There's no need to join threads, and you can't join > threads that don't terminate anyway. Oops, I misunderstood. Done. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:21: ::exit(status); On 2014/01/07 09:45:06, Mark Seaborn wrote: > This should really call _exit(), because exit() does some finalisation that can > break running threads. Done. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:22: while (1) *(volatile int *) 0 = 0; // Crash. On 2014/01/07 09:45:06, Mark Seaborn wrote: > Not really necessary, since _exit() is generally declared as "noreturn", so this > will get optimised away by the compiler anyway. Ok, removed. This is extracted from native_client/src/untrusted/irt/irt_basic.c. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:36: *ticks = ::clock(); // No failure. On 2014/01/07 09:45:06, Mark Seaborn wrote: > The man page says this can return (clock_t)-1 on failure... Good catch. I just mimic'ed native_client/src/trusted/service_runtime/posix/nacl_syscall_impl.c and native_client/src/untrusted/irt/irt_basic.c&l=21 ... There seem no errno change spec for clock(). So I use EOVERFLOW for the error code, according to man's comment. WDYT? https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:42: // We should be able to call nanosleep(NULL, &host_rem) for this. On 2014/01/07 11:32:09, hamaji wrote: > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > Why do you expect req==NULL to work? This isn't part of the NaCl IRT > interface, > > so I don't think you should check for this. > > Let me reply as I wrote this. > > I think both linux and nacl service runtime don't crash for req==NULL. I think > NaClSysNanosleep returns -NACL_ABI_EFAULT for NULL req. If you think this > compatibility for error case is not important, I'm fine with removing this. We > will workaround this issue in our side. Thank you for your comment, Shinichiro. So, according to man of nanosleep, it returns EFAULT when "Problem with copying information from user space", and IMHO, req==NULL is such a case. I removed comments, because it seems not directly related to nacl, but kept the code as is. I'm also fine to remove it, as Shinichiro said above. WDYT, Mark? https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:75: *value = ::sysconf(name); On 2014/01/07 09:45:06, Mark Seaborn wrote: > Calling through to sysconf() isn't valid because the NaCl interface has a > different set of supported values. This should handle NACL_ABI__SC_PAGESIZE and > NACL_ABI__SC_NPROCESSORS_ONLN. Great to know. Done. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_interfaces.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_interfaces.cc:12: extern const struct nacl_irt_basic kIrtBasic; On 2014/01/07 09:45:06, Mark Seaborn wrote: > This is already declared in the header Oops. I misoperated... https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_interfaces.cc:25: NaClInterfaceTable(const char* name, const T& value) On 2014/01/07 09:45:06, Mark Seaborn wrote: > I'm pretty sure using a constructor like this makes kIrtInterfaces non-POD and > adds a global initialiser, which will fail the perf check for the global > initialiser count. Just use the {} syntax for initialisation instead. I see, done.
On 2014/01/07 11:52:29, hidehiko wrote: > Thank you for review. Could you take another look? > > https://codereview.chromium.org/125533004/diff/1/components/nacl.gyp > File components/nacl.gyp (right): > > https://codereview.chromium.org/125533004/diff/1/components/nacl.gyp#newcode194 > components/nacl.gyp:194: 'nacl/loader/nonsfi/irt/irt_basic.cc', > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > Could you put these under nonsfi/ without creating an irt/ subdir? This is > > quite deeply nested already. > > Done. > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > File components/nacl/loader/nonsfi/irt/irt_basic.cc (right): > Hi Mark, Friendly ping? - hidehiko > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_basic.cc:20: // Note: when we start to > support threads, here we need to join them. > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > That's not the case. There's no need to join threads, and you can't join > > threads that don't terminate anyway. > > Oops, I misunderstood. Done. > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_basic.cc:21: ::exit(status); > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > This should really call _exit(), because exit() does some finalisation that > can > > break running threads. > > Done. > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_basic.cc:22: while (1) *(volatile int *) 0 > = 0; // Crash. > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > Not really necessary, since _exit() is generally declared as "noreturn", so > this > > will get optimised away by the compiler anyway. > > Ok, removed. This is extracted from native_client/src/untrusted/irt/irt_basic.c. > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_basic.cc:36: *ticks = ::clock(); // No > failure. > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > The man page says this can return (clock_t)-1 on failure... > > Good catch. I just mimic'ed > native_client/src/trusted/service_runtime/posix/nacl_syscall_impl.c and > native_client/src/untrusted/irt/irt_basic.c&l=21 ... > There seem no errno change spec for clock(). So I use EOVERFLOW for the error > code, according to man's comment. WDYT? > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_basic.cc:42: // We should be able to call > nanosleep(NULL, &host_rem) for this. > On 2014/01/07 11:32:09, hamaji wrote: > > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > > Why do you expect req==NULL to work? This isn't part of the NaCl IRT > > interface, > > > so I don't think you should check for this. > > > > Let me reply as I wrote this. > > > > I think both linux and nacl service runtime don't crash for req==NULL. I think > > NaClSysNanosleep returns -NACL_ABI_EFAULT for NULL req. If you think this > > compatibility for error case is not important, I'm fine with removing this. We > > will workaround this issue in our side. > > Thank you for your comment, Shinichiro. > So, according to man of nanosleep, it returns EFAULT when "Problem with copying > information from user space", and IMHO, req==NULL is such a case. > I removed comments, because it seems not directly related to nacl, but kept the > code as is. > I'm also fine to remove it, as Shinichiro said above. WDYT, Mark? > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_basic.cc:75: *value = ::sysconf(name); > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > Calling through to sysconf() isn't valid because the NaCl interface has a > > different set of supported values. This should handle NACL_ABI__SC_PAGESIZE > and > > NACL_ABI__SC_NPROCESSORS_ONLN. > > Great to know. Done. > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > File components/nacl/loader/nonsfi/irt/irt_interfaces.cc (right): > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_interfaces.cc:12: extern const struct > nacl_irt_basic kIrtBasic; > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > This is already declared in the header > > Oops. I misoperated... > > https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... > components/nacl/loader/nonsfi/irt/irt_interfaces.cc:25: NaClInterfaceTable(const > char* name, const T& value) > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > I'm pretty sure using a constructor like this makes kIrtInterfaces non-POD and > > adds a global initialiser, which will fail the perf check for the global > > initialiser count. Just use the {} syntax for initialisation instead. > > I see, done.
LGTM https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:36: *ticks = ::clock(); // No failure. On 2014/01/07 11:52:29, hidehiko wrote: > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > The man page says this can return (clock_t)-1 on failure... > > Good catch. I just mimic'ed > native_client/src/trusted/service_runtime/posix/nacl_syscall_impl.c and > native_client/src/untrusted/irt/irt_basic.c&l=21 ... > There seem no errno change spec for clock(). So I use EOVERFLOW for the error > code, according to man's comment. WDYT? Oh, you're right. http://pubs.opengroup.org/onlinepubs/007904975/functions/clock.html doesn't specify that errno should be set on error, and neither does "man 3 clock" on Linux. So the original code is OK -- sorry for nit-picking. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:42: // We should be able to call nanosleep(NULL, &host_rem) for this. On 2014/01/07 11:52:29, hidehiko wrote: > On 2014/01/07 11:32:09, hamaji wrote: > > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > > Why do you expect req==NULL to work? This isn't part of the NaCl IRT > > > interface, so I don't think you should check for this. > > > > Let me reply as I wrote this. > > > > I think both linux and nacl service runtime don't crash for req==NULL. I think > > NaClSysNanosleep returns -NACL_ABI_EFAULT for NULL req. If you think this > > compatibility for error case is not important, I'm fine with removing this. We > > will workaround this issue in our side. Out of curiosity, what's the case you need the workaround for? Is there some code that passes NULL here? > Thank you for your comment, Shinichiro. > So, according to man of nanosleep, it returns EFAULT when "Problem with copying > information from user space", and IMHO, req==NULL is such a case. > I removed comments, because it seems not directly related to nacl, but kept the > code as is. > I'm also fine to remove it, as Shinichiro said above. WDYT, Mark? If we checked for a NULL pointer here, we'd have to check for NULL pointers in every IRT call, but that would just be working around broken programs. service_runtime happens to check for NULL, but that's rather unfortunate. I filed a related bug about this ages ago: https://code.google.com/p/nativeclient/issues/detail?id=1046 Linux returns EFAULT on any error copying to/from user space and doesn't check for NULL specifically, but handling all copying errors is not very practical for NaCl. So yes, please do remove the NULL check. :-) https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:21: ::_exit(status); Maybe leave off the "::" prefixes unless there's a particular reason for them? It would make the source a little inconsistent if others add more code that doesn't use "::" prefixes (which would be quite reasonable to do). https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:26: if (::gettimeofday(&host_tv, NULL) == -1) Nit: in general, "!= 0" is slightly more defensive for error checks https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:50: if (::nanosleep(&host_req, &host_rem) == -1) "!= 0" (nit) https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:79: errno = EINVAL; Just "return EINVAL" here?
Thank you for review! Submitting... https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... File components/nacl/loader/nonsfi/irt/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:36: *ticks = ::clock(); // No failure. On 2014/01/10 05:12:20, Mark Seaborn wrote: > On 2014/01/07 11:52:29, hidehiko wrote: > > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > > The man page says this can return (clock_t)-1 on failure... > > > > Good catch. I just mimic'ed > > native_client/src/trusted/service_runtime/posix/nacl_syscall_impl.c and > > native_client/src/untrusted/irt/irt_basic.c&l=21 ... > > There seem no errno change spec for clock(). So I use EOVERFLOW for the error > > code, according to man's comment. WDYT? > > Oh, you're right. > http://pubs.opengroup.org/onlinepubs/007904975/functions/clock.html doesn't > specify that errno should be set on error, and neither does "man 3 clock" on > Linux. So the original code is OK -- sorry for nit-picking. Done. https://codereview.chromium.org/125533004/diff/1/components/nacl/loader/nonsf... components/nacl/loader/nonsfi/irt/irt_basic.cc:42: // We should be able to call nanosleep(NULL, &host_rem) for this. On 2014/01/10 05:12:20, Mark Seaborn wrote: > On 2014/01/07 11:52:29, hidehiko wrote: > > On 2014/01/07 11:32:09, hamaji wrote: > > > On 2014/01/07 09:45:06, Mark Seaborn wrote: > > > > Why do you expect req==NULL to work? This isn't part of the NaCl IRT > > > > interface, so I don't think you should check for this. > > > > > > Let me reply as I wrote this. > > > > > > I think both linux and nacl service runtime don't crash for req==NULL. I > think > > > NaClSysNanosleep returns -NACL_ABI_EFAULT for NULL req. If you think this > > > compatibility for error case is not important, I'm fine with removing this. > We > > > will workaround this issue in our side. > > Out of curiosity, what's the case you need the workaround for? Is there some > code that passes NULL here? > It turned out that it was just our test code. We'll fix our side separately. > > Thank you for your comment, Shinichiro. > > So, according to man of nanosleep, it returns EFAULT when "Problem with > copying > > information from user space", and IMHO, req==NULL is such a case. > > I removed comments, because it seems not directly related to nacl, but kept > the > > code as is. > > I'm also fine to remove it, as Shinichiro said above. WDYT, Mark? > > If we checked for a NULL pointer here, we'd have to check for NULL pointers in > every IRT call, but that would just be working around broken programs. > service_runtime happens to check for NULL, but that's rather unfortunate. > > I filed a related bug about this ages ago: > https://code.google.com/p/nativeclient/issues/detail?id=1046 > > Linux returns EFAULT on any error copying to/from user space and doesn't check > for NULL specifically, but handling all copying errors is not very practical for > NaCl. > > So yes, please do remove the NULL check. :-) Ok, done. https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_basic.cc (right): https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:21: ::_exit(status); On 2014/01/10 05:12:20, Mark Seaborn wrote: > Maybe leave off the "::" prefixes unless there's a particular reason for them? > It would make the source a little inconsistent if others add more code that > doesn't use "::" prefixes (which would be quite reasonable to do). Done. https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:26: if (::gettimeofday(&host_tv, NULL) == -1) On 2014/01/10 05:12:20, Mark Seaborn wrote: > Nit: in general, "!= 0" is slightly more defensive for error checks Done. https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:50: if (::nanosleep(&host_req, &host_rem) == -1) On 2014/01/10 05:12:20, Mark Seaborn wrote: > "!= 0" (nit) Done. https://codereview.chromium.org/125533004/diff/100001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_basic.cc:79: errno = EINVAL; On 2014/01/10 05:12:20, Mark Seaborn wrote: > Just "return EINVAL" here? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/125533004/250001
Message was sent while issue was closed.
Change committed as 244144 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
