Implement nacl_irt_futex for non-sfi mode.

Implement nacl_irt_futex for non-sfi mode.

This CL implements nacl_irt_futex for non-sfi mode.

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

[hidehiko, 2014-01-15 14:40:31]

This CL is based on crrev.com/133683011, which is just sent to you for code
review today.
If you think the review for the based CL will affect to this CL, please feel
free to suspend the review for this CL. (I believe, it won't happen, though :-).

Thank you for your review in advance,
- hidehiko

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

LGTM

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

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:18: // Converts a pair of nacl's
timespec of absolute time and host's timespec of
Nit: capitalise as "NaCl"

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:20: // Returns true if the relative
time is non-negative. Otherwise false (and
Maybe just have the caller check whether the resulting tv_sec is non-negative? 
nacl_abi_time_t is defined as signed -- you can comment that you're assuming
that if you want.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:25: const int64 kNanoSecondsPerSecond
= 1000000000;
Maybe use "int64_t" rather than "int64"?  In Linux code, we can use the former
and it's more standard.  However, kNanoSecondsPerSecond can just be int
anyway...

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:26: int64 elapsed_nsec =
It's better to do what unsandboxed_irt.c does:

    reltime.tv_sec = abstime->tv_sec - time_now.tv_sec;
    reltime.tv_nsec = abstime->tv_nsec - time_now.tv_nsec;
    if (reltime.tv_nsec < 0) {
      reltime.tv_sec -= 1;
      reltime.tv_nsec += 1000000000;
    }

This doesn't involve multiplication and division, so it's more efficient.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:42: // futex syscall takes relative
timeout, but the ABI for irt's
Nit: capitalise as "IRT"

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:48: // Linux's FUTEX_WAIT returns
EINVAL for negative timeout, but it valid
How about: "but an absolute time that is in the past is a valid argument to
irt_futex_wait_abs()"

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:49: // on irt_futex_wait_abs, and a
caller expects ETIMEOUT.
ETIMEDOUT

[hidehiko, 2014-01-16 06:52:23]

Thank you for your review. Submitting...

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

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:18: // Converts a pair of nacl's
timespec of absolute time and host's timespec of
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> Nit: capitalise as "NaCl"

Done.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:20: // Returns true if the relative
time is non-negative. Otherwise false (and
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> Maybe just have the caller check whether the resulting tv_sec is non-negative?

> nacl_abi_time_t is defined as signed -- you can comment that you're assuming
> that if you want.

Done.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:25: const int64 kNanoSecondsPerSecond
= 1000000000;
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> Maybe use "int64_t" rather than "int64"?  In Linux code, we can use the former
> and it's more standard.  However, kNanoSecondsPerSecond can just be int
> anyway...

Acknowledged.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:26: int64 elapsed_nsec =
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> It's better to do what unsandboxed_irt.c does:
> 
>     reltime.tv_sec = abstime->tv_sec - time_now.tv_sec;
>     reltime.tv_nsec = abstime->tv_nsec - time_now.tv_nsec;
>     if (reltime.tv_nsec < 0) {
>       reltime.tv_sec -= 1;
>       reltime.tv_nsec += 1000000000;
>     }
> 
> This doesn't involve multiplication and division, so it's more efficient.

Done, assuming both input tv_nsec are non-negative.
BTW, just curious, the cost of "if + decrement + add" is cheaper than
"multiplication + division" in most cases? (I believe you, but I'd also like to
know the source so that I can make a criterion in my mind for future CLs :-).

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:42: // futex syscall takes relative
timeout, but the ABI for irt's
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> Nit: capitalise as "IRT"

Done.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:48: // Linux's FUTEX_WAIT returns
EINVAL for negative timeout, but it valid
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> How about: "but an absolute time that is in the past is a valid argument to
> irt_futex_wait_abs()"

Done.

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:49: // on irt_futex_wait_abs, and a
caller expects ETIMEOUT.
On 2014/01/15 19:27:54, Mark Seaborn wrote:
> ETIMEDOUT

Done.

[commit-bot: I haz the power, 2014-01-16 06:53:55]

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

[commit-bot: I haz the power, 2014-01-16 18:07:23]

Change committed as 245248

[Mark Seaborn, 2014-01-16 19:39:47]

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

https://codereview.chromium.org/139453002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/irt_futex.cc:26: int64 elapsed_nsec =
On 2014/01/16 06:52:23, hidehiko wrote:
> On 2014/01/15 19:27:54, Mark Seaborn wrote:
> > It's better to do what unsandboxed_irt.c does:
> > 
> >     reltime.tv_sec = abstime->tv_sec - time_now.tv_sec;
> >     reltime.tv_nsec = abstime->tv_nsec - time_now.tv_nsec;
> >     if (reltime.tv_nsec < 0) {
> >       reltime.tv_sec -= 1;
> >       reltime.tv_nsec += 1000000000;
> >     }
> > 
> > This doesn't involve multiplication and division, so it's more efficient.
> 
> Done, assuming both input tv_nsec are non-negative.

Right.  It's required that 0 <= tv_nsec < 1000000000.  Linux's futex syscall
returns EINVAL otherwise.  We should probably document this in irt.h.

I suppose it would be worthwhile checking for that condition here too, before
the calculation.  Want to add a check?

I haven't checked whether/where this constraint is specified in the pthread
standard.  Might be worth checking...


> BTW, just curious, the cost of "if + decrement + add" is cheaper than
> "multiplication + division" in most cases? (I believe you, but I'd also like
to
> know the source so that I can make a criterion in my mind for future CLs :-).

Yes, multiplication tends to use a lot more cycles, and division is even more
complex.  On x86-32 and ARM, 64-bit div/mod involve calling out to external
functions.

e.g. See the code for __moddi3 in "objdump -dr /usr/lib32/libgcc_s.so.1".

Checking for 10^9 and doing the carry is the common pattern in glibc.  For
example, see
https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/pthread_mutex_timedlock....