Provide mprotect syscall

Provide mprotect syscall

Expose the mprotect syscall which is a part of experimental IRT interface.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=895
TEST= glibc trybots

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

[Roland McGrath, 2013-02-28 22:54:09]

The code itself looks OK but I am not convinced we want to have libc using dev
interfaces.

[bsy, 2013-02-28 23:04:09]

it sounds like we need to promote this to a stable interface.  objections?


On Thu, Feb 28, 2013 at 2:54 PM, <mcgrathr@chromium.org> wrote:

> The code itself looks OK but I am not convinced we want to have libc using
> dev
> interfaces.
>
>
https://codereview.chromium.**org/12382030/<https://codereview.chromium.org/1...
>



-- 
bennet s yee
i usually don't capitalize due to mild tendonitis

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

[Petr Hosek, 2013-03-01 02:01:57]

I'm fine with that, the question whether it should stay as a separate
irt_mprotect interface (i.e. just drop the dev part) or whether we should
include mprotect in irt_memory interface and use the IRT versioning?

On 2013/02/28 23:04:09, bsy wrote:
> it sounds like we need to promote this to a stable interface.  objections?
> 
> 
> On Thu, Feb 28, 2013 at 2:54 PM, <mailto:mcgrathr@chromium.org> wrote:
> 
> > The code itself looks OK but I am not convinced we want to have libc using
> > dev
> > interfaces.
> >
> >
>
https://codereview.chromium.**org/12382030/%3Chttps://codereview.chromium.org...>
> >
> 
> 
> 
> -- 
> bennet s yee
> i usually don't capitalize due to mild tendonitis
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
>

[Mark Seaborn, 2013-03-01 15:03:36]

On 28 February 2013 18:01, <phosek@chromium.org> wrote:

> I'm fine with that, the question whether it should stay as a separate
> irt_mprotect interface (i.e. just drop the dev part) or whether we should
> include mprotect in irt_memory interface and use the IRT versioning?
>

Whichever we do, we should ensure that newly-linked user programs that
don't use mprotect work with older IRT versions that don't provide mprotect
-- we shouldn't require a new IRT version unnecessarily.

I would prefer that we keep the separate irt_mprotect interface, because it
will simplify testing.  Otherwise, we will need to have tests to check 1)
that user programs can accept old and new versions of irt_memory, and 2)
that the IRT provides old and new versions of irt_memory.

Mark

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

[Roland McGrath, 2013-03-01 17:31:37]

I have a mild-to-moderate preference for a new version of irt_memory
instead of a separate query for one function.  My rationales are:

* We should organize the interfaces to optimize and simplify things for
  users of the newest version.  One query is better than two.  Things that
  intend to adapt to either new or old environments need to be prepared to
  do two queries, one of which might fail, either way.

* At some point we'll have interface changes that really are version
  replacements rather than just additions.  We need to exercise our muscles
  for doing this robustly, including testing issues.  Doing it the first
  time with this relatively trivial case will be easier and lower-impact
  than doing it for the first time later on when we have a bigger and more
  important interface change.

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

[Petr Hosek, 2013-03-01 19:21:24]

I agree with Roland, providing a new version of irt_memory feels like a cleaner
solution compared to a separate interface. This is also a nice opportunity to
exercise (and debug) the interface versioning mechanism.

The question is how exactly should the new version of irt_memory look like which
is a follow up to discussion we already had in
https://codereview.chromium.org/11141016/.

The incorrect solution I've used was:

#define NACL_IRT_MEMORY_v0_1    "nacl-irt-memory-0.1"
#define NACL_IRT_MEMORY_v0_2    "nacl-irt-memory-0.2"
struct nacl_irt_memory {
  int (*sysbrk)(void **newbrk);
  int (*mmap)(void **addr, size_t len, int prot, int flags, int fd, off_t off);
  int (*munmap)(void *addr, size_t len);
  int (*mprotect)(void *addr, size_t len, int prot);
};

Then Bennet suggested the following:

struct nacl_irt_memory {
  int (*sysbrk)(...);
  int (*mmap)(...);
  int (*munmap)(...);
};

struct nacl_irt_memory_v0_2 {
  struct nacl_irt_memory base;
  int (*mprotect)(...);
};

However, we never really agreed on what should be the right solution.

[Roland McGrath, 2013-03-01 19:37:44]

On Fri, Mar 1, 2013 at 11:21 AM,  <phosek@chromium.org> wrote:
> The question is how exactly should the new version of irt_memory look
> like which is a follow up to discussion we already had in
> https://codereview.chromium.org/11141016/.
>
> The incorrect solution I've used was:
>
> #define NACL_IRT_MEMORY_v0_1    "nacl-irt-memory-0.1"
> #define NACL_IRT_MEMORY_v0_2    "nacl-irt-memory-0.2"
> struct nacl_irt_memory {
>   int (*sysbrk)(void **newbrk);
>   int (*mmap)(void **addr, size_t len, int prot, int flags, int fd, off_t
> off);
>   int (*munmap)(void *addr, size_t len);
>
>   int (*mprotect)(void *addr, size_t len, int prot);
> };
>
> Then Bennet suggested the following:
>
> struct nacl_irt_memory {
>   int (*sysbrk)(...);
>   int (*mmap)(...);
>   int (*munmap)(...);
> };
>
> struct nacl_irt_memory_v0_2 {
>   struct nacl_irt_memory base;
>   int (*mprotect)(...);
> };
>
> However, we never really agreed on what should be the right solution.

When we're just extending an existing interface with additional calls, we
certainly want to make the new struct one with a prefix that exactly
matches the old one.  However, IMHO we should also endeavor to avoid making
it baroque for those using only the newest interface.  Having to use
ptr->foo for some calls and ptr->base.foo for others is quite ugly.

There is also the issue of struct naming.  In the PPAPI interfaces (on
which our whole interface-query scheme is loosely based), I think they've
chosen to make the latest version of an interface always have a simple name
without a version number, and only the compatibility interface structs have
a version number in their name.

Perhaps there is some combination of typedefs, anonymous unions/structs,
etc. that can make this all more reasonable to use.  It needs more
experimentation and thought.

At the end of the day, if we wind up having duplicative struct definitions
where we only ensure that one is a prefix of the other via tests rather
than the type system, I think it is better to impose that burden on
ourselves than to impose a burden of baroque source APIs on the users of
this public interface.

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

[Petr Hosek, 2013-03-04 01:03:10]

I've been playing with the IRT over the weekend. I definitely prefer having an
entirely new struct and (if needed) ensuring that the old one is a prefix of the
new through test rather nesting those structs. I think we want to avoid ending
up with "matryoshka"-like structs with several layers of nesting.

Regarding naming, I've been checking the PPAPI and if I understood it correctly,
they use explicit versioning i.e. even the latest version of an interface has
the version in its name, but they typedef the latest version so when you use the
name without version, you'll get the latest version. I start to think this might
the best solution for NaCl as well. At the moment, we have calls like:

struct nacl_irt_memory memory;
__nacl_irt_query(NACL_IRT_MEMORY_v0_1, &memory, sizeof(memory));

This fails at runtime because query for NACL_IRT_MEMORY_v0_1 returns struct
nacl_irt_memory_v0_1 which size is now different than of struct nacl_irt_memory
(i.e. which is now v0_2). That's certainly something we don't want.

By making the struct names version-explicit and typedefing the latest version,
you have two options: (1) always use the latest version (if you feel brave
enough), or (2) use a specific version. This feels like an ideal compromise.

Did I miss something?

[Roland McGrath, 2013-03-04 17:12:07]

Sounds reasonable to me.

[bsy, 2013-03-04 17:39:11]

i agree w/ mcgrathr that .base etc shouldn't be exposed.  it's hair that
developers -- even if most won't have to use the raw irt interface --
shouldn't have to deal with.

rather that just testing that a new extended version of an interface has as
a prefix the older version, i would suggest the following

#define IRT_FOO_INTERFACE_V0_MEMBERS \
  int (*irt_foo_bar)(...); \
  int (*irt_foo_baz)(...); \
  int (*irt_foo_quux)(...);

#define IRT_FOO_INTERFACE_V1_MEMBERS \
  IRT_FOO_INTERFACE_V0_MEMBERS \
  int (*irt_foo_ran_out_of_meta_syntactic_variables_1)(...); \
  int (*irt_foo_ran_out_of_meta_syntactic_variables_2)(...);

and of course,

struct irt_foo_v0 {
  IRT_FOO_INTERFACE_V0_MEMBERS
};

struct irt_foo_v1 {
  IRT_FOO_INTERFACE_V1_MEMBERS
};

typedef struct irt_foo_v1 irt_foo;

(or something similar along these lines.)  this would make the
one-struct-is-a-prefix-of-another pretty much correct by construction, or
at least far less error prone.


On Mon, Mar 4, 2013 at 9:12 AM, <mcgrathr@chromium.org> wrote:

> Sounds reasonable to me.
>
>
https://codereview.chromium.**org/12382030/<https://codereview.chromium.org/1...
>



-- 
bennet s yee
i usually don't capitalize due to mild tendonitis

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

[Petr Hosek, 2013-03-04 19:37:31]

What if we want to change (or remove) the existing member?

[Petr Hosek, 2013-03-29 15:32:05]

I have updated the code to use the stable IRT interface. Please note that I have
also dropped the const modifier from INTERNAL_SYSCALL_mprotect_3 addr argument
since NaCl version of mprotect does not use it (following POSIX definition) and
I was getting a lot of warnings as a consequence. There was a suggestion to
follow the Linux version of mprotect and add the const modifier in NaCl
interface, but this would require a lot of changes all over sel_ldr. If that's
ever done, we can re-add const modifier to INTERNAL_SYSCALL_mprotect_3. I'm
happy to drop the last patchset if you don't like this change.

[Mark Seaborn, 2013-03-29 15:48:07]

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt.h
File sysdeps/nacl/irt.h (right):

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt.h#newcod...
sysdeps/nacl/irt.h:169: #define NACL_IRT_DEV_MPROTECT_v0_1
"nacl-irt-dev-mprotect-0.1"
You should remove this

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls.c
File sysdeps/nacl/irt_syscalls.c (right):

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls...
sysdeps/nacl/irt_syscalls.c:272: static int nacl_irt_mprotect (void *addr,
size_t len, int prot) {
Nit: put this after munmap()

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls...
sysdeps/nacl/irt_syscalls.c:399: int (*__nacl_irt_mprotect) (void *addr, size_t
len, int prot);
Put this after munmap() too

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls...
sysdeps/nacl/irt_syscalls.c:494: __nacl_irt_mprotect = nacl_irt_mprotect;
You shouldn't fall back to using the syscall directly if NACL_IRT_MEMORY_v0_2
isn't present.  You should fall back to a function that returns ENOSYS.

Falling back to direct syscalls should only happen if the IRT isn't present at
all (and this is deprecated anyway; we should remove this fallback code some
time).

[Petr Hosek, 2013-03-29 21:02:21]

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt.h
File sysdeps/nacl/irt.h (right):

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt.h#newcod...
sysdeps/nacl/irt.h:169: #define NACL_IRT_DEV_MPROTECT_v0_1
"nacl-irt-dev-mprotect-0.1"
On 2013/03/29 15:48:07, Mark Seaborn wrote:
> You should remove this

Removed.

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls.c
File sysdeps/nacl/irt_syscalls.c (right):

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls...
sysdeps/nacl/irt_syscalls.c:272: static int nacl_irt_mprotect (void *addr,
size_t len, int prot) {
On 2013/03/29 15:48:07, Mark Seaborn wrote:
> Nit: put this after munmap()

Moved.

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls...
sysdeps/nacl/irt_syscalls.c:399: int (*__nacl_irt_mprotect) (void *addr, size_t
len, int prot);
On 2013/03/29 15:48:07, Mark Seaborn wrote:
> Put this after munmap() too

Moved.

https://codereview.chromium.org/12382030/diff/26004/sysdeps/nacl/irt_syscalls...
sysdeps/nacl/irt_syscalls.c:494: __nacl_irt_mprotect = nacl_irt_mprotect;
On 2013/03/29 15:48:07, Mark Seaborn wrote:
> You shouldn't fall back to using the syscall directly if NACL_IRT_MEMORY_v0_2
> isn't present.  You should fall back to a function that returns ENOSYS.
> 
> Falling back to direct syscalls should only happen if the IRT isn't present at
> all (and this is deprecated anyway; we should remove this fallback code some
> time).

Fixed.

[Mark Seaborn, 2013-04-01 15:13:41]

LGTM

[Petr Hosek, 2013-04-04 15:04:12]

Committed patchset #4 manually as reb3343e (presubmit successful).