added several system call hooks for future sockets implementation

added several system call hooks for future sockets implementation

FOA, select or poll would be implemented, of course.
Should I add __nacl_irt_... call hooks for common berkeley socket calls (accept, bind, listen, connect, send, recv, gethostby[name,addr], sendto, recvfrom, [set,get]sockopt)?

BUG=
TEST=

Committed: https://git.chromium.org/gitweb?p=native_client/nacl-glibc.git;a=commit;h=0533bbb

[vissi, 2012-04-02 12:54:22]

Hi,
I added several __nacl_irt hooks for future sockets implementation.

[vissi, 2012-04-02 12:58:48]

On 2012/04/02 12:54:22, vissi wrote:
> Hi,
> I added several __nacl_irt hooks for future sockets implementation.

Rebased to HEAD, so that current patch did not contain lines from previous one.

[pasko-google - do not use, 2012-04-02 13:00:36]

I would prefer if all hooks you add get tested first, then new set of hooks gets
added. Please, add tests for your previous hooks into nacl-mounts and get them
run on naclports bots, then continue with new shiny hooks.

A good test would be using nacl-mounts, linked with user code that does not use
the hooks directly.

[vissi, 2012-04-09 11:34:58]

On 2012/04/02 13:00:36, pasko wrote:
> I would prefer if all hooks you add get tested first, then new set of hooks
gets
> added. Please, add tests for your previous hooks into nacl-mounts and get them
> run on naclports bots, then continue with new shiny hooks.
> 
> A good test would be using nacl-mounts, linked with user code that does not
use
> the hooks directly.

Hi,
I've added more calls to this patch set, going to prepare handlers in
nacl-mounts.
For now calls missing are gethostby[name,addr], getaddrinfo (sb knows, which is
better?), [set,get]sockopt.

[Evgeniy Stepanov, 2012-04-09 11:45:13]

https://chromiumcodereview.appspot.com/9969049/diff/5001/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://chromiumcodereview.appspot.com/9969049/diff/5001/sysdeps/nacl/sysdep....
sysdeps/nacl/sysdep.h:268: *err = __nacl_irt_epoll_pwait (epfd, events,
maxevents, timeout, sigmask, sigsetsize);
80 chars

https://chromiumcodereview.appspot.com/9969049/diff/5001/sysdeps/nacl/sysdep....
sysdeps/nacl/sysdep.h:1195: void *sigmask) // const sigset_t *sigmask?
There were tabs in the original file?? Please replace them with spaces while you
are at it.

https://chromiumcodereview.appspot.com/9969049/diff/5001/sysdeps/nacl/sysdep....
sysdeps/nacl/sysdep.h:1197: *err = __nacl_irt_pselect (nfds, readfds, writefds,
exceptfds, timeout, sigmask);
80 chars here and below

[khim, 2012-04-09 12:21:11]

https://chromiumcodereview.appspot.com/9969049/diff/5001/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://chromiumcodereview.appspot.com/9969049/diff/5001/sysdeps/nacl/sysdep....
sysdeps/nacl/sysdep.h:1195: void *sigmask) // const sigset_t *sigmask?
On 2012/04/09 11:45:13, Evgeniy Stepanov wrote:
> There were tabs in the original file?? Please replace them with spaces while
you
> are at it.

Hold your horses! This is GLibC, not NaCl.

native_client/tools/SRC/glibc$ find -type f -print0 | xargs -0 grep -l $'\t' |
wc -l
6163

[Evgeniy Stepanov, 2012-04-09 12:31:26]

Per offline discussion, we'd like to be more like linux. I.e., replace all these
new syscalls with "socketcall" and pull in the sysv implementation of sockets to
sysdeps/nacl.

[Mark Seaborn, 2012-04-09 15:20:08]

On 9 April 2012 05:31, <eugenis@google.com> wrote:

> Per offline discussion, we'd like to be more like linux. I.e., replace all
> these
> new syscalls with "socketcall" and pull in the sysv implementation of
> sockets to
> sysdeps/nacl.
>

Do you mean the "socketcall" syscall that several socket calls (socket(),
connect(), bind() etc.) are routed through?  That syscall only exists on
x86-32 Linux.  It is not something we should be copying for NaCl.  There is
no reason for routing many socket function calls through a single function
or syscall.  That approach was dropped on x86-64 Linux where there are
separate syscalls for socket(), connect(), bind() etc.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[vissi, 2012-04-11 13:28:06]

On 2012/04/09 12:31:26, Evgeniy Stepanov wrote:
> Per offline discussion, we'd like to be more like linux. I.e., replace all
these
> new syscalls with "socketcall" and pull in the sysv implementation of sockets
to
> sysdeps/nacl.
hi,
now I added the calls to syscalls.list (as in x86_64). Going to write
nacl-mounts interceptors now.

[vissi, 2012-04-12 16:57:48]

On 2012/04/11 13:28:06, vissi wrote:
> On 2012/04/09 12:31:26, Evgeniy Stepanov wrote:
> > Per offline discussion, we'd like to be more like linux. I.e., replace all
> these
> > new syscalls with "socketcall" and pull in the sysv implementation of
sockets
> to
> > sysdeps/nacl.
> hi,
> now I added the calls to syscalls.list (as in x86_64). Going to write
> nacl-mounts interceptors now.

fixed interceptors w/ @khim's advice. See
https://chromiumcodereview.appspot.com/10067012 for headers fixes.

[pasko-google - do not use, 2012-04-13 08:44:13]

I am reluctant to approving a change based on syscalls.list. Especially via
extending argument parsing in make-syscalls.sh in undocumented way.
Additionally, this approach has not been used in nacl-glibc making it more
complicated to grep though the sources. This does not make us closer to
upstream, nor makes it easier to understand and support.

Also, for my +1 there should be a test to use this functionality at least
uploaded (better be very ready to commit).

Also, previous glibc work was not propagated to the SDK and naclports buildbots.
I think we should go through this excercise first to set up incremental
development in the toolchain. Procrastinating the and demos to the last day is
tempting, but it leads to a risk that we won't have anything working at the
C-Day.

Commit early, commit often.

I will leave responsibility to approve/disapprove this to khim@.

[pasko-google - do not use, 2012-04-13 08:44:26]

https://chromiumcodereview.appspot.com/9969049/diff/15001/make_sysd_rules.py
File make_sysd_rules.py (right):

https://chromiumcodereview.appspot.com/9969049/diff/15001/make_sysd_rules.py#...
make_sysd_rules.py:94: #    "socket/accept.c",
commenting code is not a good way forward, just remove lines if you need to,
they remain in revision history

[khim, 2012-04-13 09:07:10]

On 2012/04/13 08:44:13, pasko wrote:
> I am reluctant to approving a change based on syscalls.list. Especially via
> extending argument parsing in make-syscalls.sh in undocumented way.
> Additionally, this approach has not been used in nacl-glibc making it more
> complicated to grep though the sources.

Actually this approach IS used in nacl-glibc (along with another way which uses
manually-created files) as I've found out when I've played with naclports.

I've fixed it for IRT-supplied syscalls:
  http://codereview.chromium.org/7737022

Unfortunately my patch had no handling for some types (I had no way
to test them) thus this patch adds the required changes.

> This does not make us closer to upstream, nor makes it easier to understand
and support.

It DOES make us closer to upstream. Upstream uses this same script to handle
most syscalls and we are reusing the upstream definitions of syscalls - this
should make it less error-prone.

> 
> Also, for my +1 there should be a test to use this functionality at least
> uploaded (better be very ready to commit).
> 
> Also, previous glibc work was not propagated to the SDK and naclports
buildbots.
> I think we should go through this excercise first to set up incremental
> development in the toolchain. Procrastinating the and demos to the last day is
> tempting, but it leads to a risk that we won't have anything working at the
> C-Day.
> 
> Commit early, commit often.
> 
> I will leave responsibility to approve/disapprove this to khim@.

I have slightly different concern: today you can build nacl-glibc using ANY
version of Linux headers (at least in theory). This change will mean that one
will need to use our very own specially-patched version of headers. This
violates the principle of least surprise and can be easily fixed: we just need
to add defines to appropriate headers in out port of nacl-glibc, not in
linux-headers repo.

[pasko-google - do not use, 2012-04-13 09:30:24]

On 2012/04/13 09:07:10, khim wrote:
> > This does not make us closer to upstream, nor makes it easier to understand
> and support.
> 
> It DOES make us closer to upstream. Upstream uses this same script to handle
> most syscalls and we are reusing the upstream definitions of syscalls - this
> should make it less error-prone.

Given khim@ is able to support syscalls generation for IRT, I am OK with that.
 
> > Also, for my +1 there should be a test to use this functionality at least
> > uploaded (better be very ready to commit).
> > 
> > Also, previous glibc work was not propagated to the SDK and naclports
> buildbots.
> > I think we should go through this excercise first to set up incremental
> > development in the toolchain. Procrastinating the and demos to the last day
is
> > tempting, but it leads to a risk that we won't have anything working at the
> > C-Day.
> > 
> > Commit early, commit often.
> > 
> > I will leave responsibility to approve/disapprove this to khim@.
> 
> I have slightly different concern: today you can build nacl-glibc using ANY
> version of Linux headers (at least in theory). This change will mean that one
> will need to use our very own specially-patched version of headers. This
> violates the principle of least surprise and can be easily fixed: we just need
> to add defines to appropriate headers in out port of nacl-glibc, not in
> linux-headers repo.

Yes, macros related to linux headers must go into glibc

[vissi, 2012-04-13 10:36:09]

On 2012/04/13 09:30:24, pasko wrote:
> On 2012/04/13 09:07:10, khim wrote:
> > > This does not make us closer to upstream, nor makes it easier to
understand
> > and support.
> > 
> > It DOES make us closer to upstream. Upstream uses this same script to handle
> > most syscalls and we are reusing the upstream definitions of syscalls - this
> > should make it less error-prone.
> 
> Given khim@ is able to support syscalls generation for IRT, I am OK with that.
>  
> > > Also, for my +1 there should be a test to use this functionality at least
> > > uploaded (better be very ready to commit).
> > > 
> > > Also, previous glibc work was not propagated to the SDK and naclports
> > buildbots.
> > > I think we should go through this excercise first to set up incremental
> > > development in the toolchain. Procrastinating the and demos to the last
day
> is
> > > tempting, but it leads to a risk that we won't have anything working at
the
> > > C-Day.
> > > 
> > > Commit early, commit often.
> > > 
> > > I will leave responsibility to approve/disapprove this to khim@.
> > 
> > I have slightly different concern: today you can build nacl-glibc using ANY
> > version of Linux headers (at least in theory). This change will mean that
one
> > will need to use our very own specially-patched version of headers. This
> > violates the principle of least surprise and can be easily fixed: we just
need
> > to add defines to appropriate headers in out port of nacl-glibc, not in
> > linux-headers repo.
> 
> Yes, macros related to linux headers must go into glibc

moved macros to glibc, removed code that was commented out.

[Evgeniy Stepanov, 2012-04-13 11:49:35]

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/irt_sy...
File sysdeps/nacl/irt_syscalls.h (right):

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/irt_sy...
sysdeps/nacl/irt_syscalls.h:67: extern int (*__nacl_irt_recv) (int sockfd, void
*buf, size_t len, int flags, int* ret);
80 chars here and below

[khim, 2012-04-13 11:56:48]

Few minor nits, o/w LGTM

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/irt_sy...
File sysdeps/nacl/irt_syscalls.h (right):

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/irt_sy...
sysdeps/nacl/irt_syscalls.h:67: extern int (*__nacl_irt_recv) (int sockfd, void
*buf, size_t len, int flags, int* ret);
On 2012/04/13 11:49:35, Evgeniy Stepanov wrote:
> 80 chars here and below

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/sysdep...
sysdeps/nacl/sysdep.h:47: 
Why this line is here?

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/sysdep...
sysdeps/nacl/sysdep.h:244: // here flags is size
Remove this comment. It's not help future reader at all.

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/make-s...
File sysdeps/unix/make-syscalls.sh (left):

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/make-s...
sysdeps/unix/make-syscalls.sh:2: 
Please return these lines back, no need to create spurious changes between
nacl-glibc and upstream glibc.

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/make-s...
sysdeps/unix/make-syscalls.sh:5: 
Ditto.

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/sysv/l...
File sysdeps/unix/sysv/linux/sys/syscall.h (right):

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/sysv/l...
sysdeps/unix/sysv/linux/sys/syscall.h:26: # ifdef __i386__
#ifdef (without space before ifdef to line up with #endif)

https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/sysv/l...
sysdeps/unix/sysv/linux/sys/syscall.h:27: #define __NR_accept		325
Here it'll be better to use
#  define
but this is minor issue.

[vissi, 2012-04-13 12:12:36]

On 2012/04/13 11:56:48, khim wrote:
> Few minor nits, o/w LGTM
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/irt_sy...
> File sysdeps/nacl/irt_syscalls.h (right):
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/irt_sy...
> sysdeps/nacl/irt_syscalls.h:67: extern int (*__nacl_irt_recv) (int sockfd,
void
> *buf, size_t len, int flags, int* ret);
> On 2012/04/13 11:49:35, Evgeniy Stepanov wrote:
> > 80 chars here and below
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/sysdep.h
> File sysdeps/nacl/sysdep.h (right):
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/sysdep...
> sysdeps/nacl/sysdep.h:47: 
> Why this line is here?
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/nacl/sysdep...
> sysdeps/nacl/sysdep.h:244: // here flags is size
> Remove this comment. It's not help future reader at all.
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/make-s...
> File sysdeps/unix/make-syscalls.sh (left):
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/make-s...
> sysdeps/unix/make-syscalls.sh:2: 
> Please return these lines back, no need to create spurious changes between
> nacl-glibc and upstream glibc.
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/make-s...
> sysdeps/unix/make-syscalls.sh:5: 
> Ditto.
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/sysv/l...
> File sysdeps/unix/sysv/linux/sys/syscall.h (right):
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/sysv/l...
> sysdeps/unix/sysv/linux/sys/syscall.h:26: # ifdef __i386__
> #ifdef (without space before ifdef to line up with #endif)
> 
>
https://chromiumcodereview.appspot.com/9969049/diff/19001/sysdeps/unix/sysv/l...
> sysdeps/unix/sysv/linux/sys/syscall.h:27: #define __NR_accept		325
> Here it'll be better to use
> #  define
> but this is minor issue.

thx. fixed