Implement nacl_irt_clock for non-sfi mode.

Implement nacl_irt_clock for non-sfi mode.

This CL implements nacl_irt_clock on non-sfi mode,
which is simply proxying to the glibc implementation.

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=245459

[hidehiko, 2014-01-15 14:00:27]

Thank you for your review in advance,
- hidehiko

[Mark Seaborn, 2014-01-15 19:49:27]

Typo in commit message: "simplify" -> "simply"

LGTM

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/abi_conversion.cc (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/abi_conversion.cc:11: #include
"native_client/src/trusted/service_runtime/include/sys/stat.h"
Add #include to cover nacl_abi_timespec?

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/abi_conversion.h (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/abi_conversion.h:17: void
NaClAbiTimeSpecToTimeSpec(const struct nacl_abi_timespec& nacl_timespec,
You might consider inlining these two functions, since they're trivial.

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/irt_clock.cc (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:37: int IrtClockGetRes(int clk_id,
struct nacl_abi_timespec* rem) {
Should be "res", not "rem"

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:46: if (rem)
Hmm, interesting that clock_getres() checks for NULL but clock_gettime()
doesn't.

I see that this is required by the standard:
http://pubs.opengroup.org/onlinepubs/009695399/functions/clock_getres.html

Can you add a comment here: "clock_getres() requires a NULL check but
clock_gettime() doesn't"

I think this means there is a NaCl bug.  Under NaCl, I think clock_getres(X,
NULL) will give an EFAULT error.  Could you check and file a NaCl bug, so that
we can make sure that NaCl and non-SFI modes give a consistent result in this
case?

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:66: // Both clock_getres and
clock_gettime should use nacl's ABI, nacl_clockid_t
You probably don't need to comment about the casting for all of these
interfaces, or you could just make the comment a little shorter, e.g. "cast away
distinction between foo* and nacl_abi_foo*".

[hidehiko, 2014-01-16 05:30:11]

Thank you for your review.
PTAL, especially for my reply for your comment about inlining
NaClAbiTimeSpecToTimeSpec?

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/abi_conversion.cc (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/abi_conversion.cc:11: #include
"native_client/src/trusted/service_runtime/include/sys/stat.h"
On 2014/01/15 19:49:28, Mark Seaborn wrote:
> Add #include to cover nacl_abi_timespec?

Done.

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/abi_conversion.h (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/abi_conversion.h:17: void
NaClAbiTimeSpecToTimeSpec(const struct nacl_abi_timespec& nacl_timespec,
On 2014/01/15 19:49:28, Mark Seaborn wrote:
> You might consider inlining these two functions, since they're trivial.

Personally, I like the current style to keep the header as simple as possible.
WDYT? Or, do you mean, removing these functions, and copy'n'pasting body of
these functions to the caller rather than sharing them?

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/irt_clock.cc (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:37: int IrtClockGetRes(int clk_id,
struct nacl_abi_timespec* rem) {
On 2014/01/15 19:49:28, Mark Seaborn wrote:
> Should be "res", not "rem"

Oops, done.

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:46: if (rem)
On 2014/01/15 19:49:28, Mark Seaborn wrote:
> Hmm, interesting that clock_getres() checks for NULL but clock_gettime()
> doesn't.
> 
> I see that this is required by the standard:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/clock_getres.html
> 
> Can you add a comment here: "clock_getres() requires a NULL check but
> clock_gettime() doesn't"
> 
> I think this means there is a NaCl bug.  Under NaCl, I think clock_getres(X,
> NULL) will give an EFAULT error.  Could you check and file a NaCl bug, so that
> we can make sure that NaCl and non-SFI modes give a consistent result in this
> case?

Done to add the comment.

What I found are:
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/pnac...

The first one seems to return EFAULT, and the latter one seems to be crashed?
I'll file a bug later.

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:66: // Both clock_getres and
clock_gettime should use nacl's ABI, nacl_clockid_t
On 2014/01/15 19:49:28, Mark Seaborn wrote:
> You probably don't need to comment about the casting for all of these
> interfaces, or you could just make the comment a little shorter, e.g. "cast
away
> distinction between foo* and nacl_abi_foo*".

Done. I'll send a follow up CL for other files, which are already landed.

[hidehiko, 2014-01-16 05:38:58]

On 2014/01/16 05:30:11, hidehiko wrote:
> Thank you for your review.
> PTAL, especially for my reply for your comment about inlining
> NaClAbiTimeSpecToTimeSpec?

FYI: filed a bug
https://code.google.com/p/nativeclient/issues/detail?id=3769

> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> File components/nacl/loader/nonsfi/abi_conversion.cc (right):
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/abi_conversion.cc:11: #include
> "native_client/src/trusted/service_runtime/include/sys/stat.h"
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > Add #include to cover nacl_abi_timespec?
> 
> Done.
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> File components/nacl/loader/nonsfi/abi_conversion.h (right):
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/abi_conversion.h:17: void
> NaClAbiTimeSpecToTimeSpec(const struct nacl_abi_timespec& nacl_timespec,
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > You might consider inlining these two functions, since they're trivial.
> 
> Personally, I like the current style to keep the header as simple as possible.
> WDYT? Or, do you mean, removing these functions, and copy'n'pasting body of
> these functions to the caller rather than sharing them?
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> File components/nacl/loader/nonsfi/irt_clock.cc (right):
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/irt_clock.cc:37: int IrtClockGetRes(int clk_id,
> struct nacl_abi_timespec* rem) {
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > Should be "res", not "rem"
> 
> Oops, done.
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/irt_clock.cc:46: if (rem)
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > Hmm, interesting that clock_getres() checks for NULL but clock_gettime()
> > doesn't.
> > 
> > I see that this is required by the standard:
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/clock_getres.html
> > 
> > Can you add a comment here: "clock_getres() requires a NULL check but
> > clock_gettime() doesn't"
> > 
> > I think this means there is a NaCl bug.  Under NaCl, I think clock_getres(X,
> > NULL) will give an EFAULT error.  Could you check and file a NaCl bug, so
that
> > we can make sure that NaCl and non-SFI modes give a consistent result in
this
> > case?
> 
> Done to add the comment.
> 
> What I found are:
>
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...
>
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/pnac...
> 
> The first one seems to return EFAULT, and the latter one seems to be crashed?
> I'll file a bug later.
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/irt_clock.cc:66: // Both clock_getres and
> clock_gettime should use nacl's ABI, nacl_clockid_t
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > You probably don't need to comment about the casting for all of these
> > interfaces, or you could just make the comment a little shorter, e.g. "cast
> away
> > distinction between foo* and nacl_abi_foo*".
> 
> Done. I'll send a follow up CL for other files, which are already landed.

[Mark Seaborn, 2014-01-16 19:14:13]

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/abi_conversion.h (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/abi_conversion.h:17: void
NaClAbiTimeSpecToTimeSpec(const struct nacl_abi_timespec& nacl_timespec,
On 2014/01/16 05:30:12, hidehiko wrote:
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > You might consider inlining these two functions, since they're trivial.
> 
> Personally, I like the current style to keep the header as simple as possible.
> WDYT?

That's fine; it was only a suggestion.

> Or, do you mean, removing these functions, and copy'n'pasting body of
> these functions to the caller rather than sharing them?

No, I meant putting the definition in the header with an "inline" declaration.

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/irt_clock.cc (right):

https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_clock.cc:66: // Both clock_getres and
clock_gettime should use nacl's ABI, nacl_clockid_t
On 2014/01/16 05:30:12, hidehiko wrote:
> On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > You probably don't need to comment about the casting for all of these
> > interfaces, or you could just make the comment a little shorter, e.g. "cast
> > away distinction between foo* and nacl_abi_foo*".
> 
> Done. I'll send a follow up CL for other files, which are already landed.

You don't have to change the existing ones unless you particularly want to.  I
was just suggesting that a boilerplate comment would make things easier for you.
:-)

[hidehiko, 2014-01-17 02:15:38]

Thank you for your reply!
Submitting...

- hidehiko

On 2014/01/16 19:14:13, Mark Seaborn wrote:
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> File components/nacl/loader/nonsfi/abi_conversion.h (right):
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/abi_conversion.h:17: void
> NaClAbiTimeSpecToTimeSpec(const struct nacl_abi_timespec& nacl_timespec,
> On 2014/01/16 05:30:12, hidehiko wrote:
> > On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > > You might consider inlining these two functions, since they're trivial.
> > 
> > Personally, I like the current style to keep the header as simple as
possible.
> > WDYT?
> 
> That's fine; it was only a suggestion.
> 
> > Or, do you mean, removing these functions, and copy'n'pasting body of
> > these functions to the caller rather than sharing them?
> 
> No, I meant putting the definition in the header with an "inline" declaration.
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> File components/nacl/loader/nonsfi/irt_clock.cc (right):
> 
>
https://codereview.chromium.org/139303003/diff/1/components/nacl/loader/nonsf...
> components/nacl/loader/nonsfi/irt_clock.cc:66: // Both clock_getres and
> clock_gettime should use nacl's ABI, nacl_clockid_t
> On 2014/01/16 05:30:12, hidehiko wrote:
> > On 2014/01/15 19:49:28, Mark Seaborn wrote:
> > > You probably don't need to comment about the casting for all of these
> > > interfaces, or you could just make the comment a little shorter, e.g.
"cast
> > > away distinction between foo* and nacl_abi_foo*".
> > 
> > Done. I'll send a follow up CL for other files, which are already landed.
> 
> You don't have to change the existing ones unless you particularly want to.  I
> was just suggesting that a boilerplate comment would make things easier for
you.
> :-)

[commit-bot: I haz the power, 2014-01-17 02:16:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/139303003/100001

[commit-bot: I haz the power, 2014-01-17 02:16:27]

Failed to apply patch for components/nacl/loader/nonsfi/irt_interfaces.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/nacl/loader/nonsfi/irt_interfaces.cc
  Hunk #1 FAILED at 24.
  1 out of 1 hunk FAILED -- saving rejects to file
components/nacl/loader/nonsfi/irt_interfaces.cc.rej

Patch:       components/nacl/loader/nonsfi/irt_interfaces.cc
Index: components/nacl/loader/nonsfi/irt_interfaces.cc
diff --git a/components/nacl/loader/nonsfi/irt_interfaces.cc
b/components/nacl/loader/nonsfi/irt_interfaces.cc
index
a7e7d1e2461077ab4f0a5ff39d8f0eeb8d29612d..13b812d3ade269330e20572ec8f13f9c14c5d056
100644
--- a/components/nacl/loader/nonsfi/irt_interfaces.cc
+++ b/components/nacl/loader/nonsfi/irt_interfaces.cc
@@ -24,6 +24,7 @@ struct NaClInterfaceTable {
 const NaClInterfaceTable kIrtInterfaces[] = {
   NACL_INTERFACE_TABLE(NACL_IRT_BASIC_v0_1, kIrtBasic),
   NACL_INTERFACE_TABLE(NACL_IRT_DEV_FDIO_v0_1, kIrtFdIO),
+  NACL_INTERFACE_TABLE(NACL_IRT_CLOCK_v0_1, kIrtClock),
 };
 #undef NACL_INTERFACE_TABLE

[commit-bot: I haz the power, 2014-01-17 03:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/139303003/240001

[commit-bot: I haz the power, 2014-01-17 03:54:39]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-17 03:56:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/139303003/240001

[commit-bot: I haz the power, 2014-01-17 07:09:35]

Change committed as 245459