i#1680 large pages: Add code to discover page size.

Commit log for first patchset:
---------------
i#1680 large pages: Add code to discover page size.

The stack pointer is passed as a third argument to relocate_dynamorio
and on Linux this is used to search the auxiliary vector for AT_PAGESZ.
On MacOS, and on Linux if for some reason os_page_size is called
before relocate_dynamorio, the page size is discovered using the
system calls mmap and munmap, which should work on any POSIX system.
---------------

[bruening, 2016-10-12 16:03:36]

The title of the commit message should prob say "on unix".

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1594
core/unix/loader.c:1594: for (++p; p[0] != 0; p += 2) {
Use proper types: ELF_AUXV_TYPE *

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1611
core/unix/loader.c:1611: set_page_size(sp);
What about late injection, or standalone DR, or static DR?  For the first two,
_init() takes in envp on all but Android and we should walk auxv there as well,
probably best in some foo_init() that is then shared among all 3: which foo is
determined by how early it has to be.  If prior to options_init() it might need
to be a special init like syscalls_init() is for Windows.  In fact maybe
syscalls_init() should turn into some cross-platform os_config_init() or sthg.

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9625
core/unix/os.c:9625: * discovered in any other way, such as from AT_PAGESZ.
Mac OSX should use sysctl_query on hw.pagesize

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9631
core/unix/os.c:9631: for (size = 1; size * 2 > 0; size *= 2) {
May as well start higher and avoid a handful of syscalls.
Seems best to start at 4096 and then go up or down from there.
Yes, it does seem worth some simple optimizing: it doesn't seem good to spend 35
(!) syscalls at init every time -- we do care about init perf, and in this CL
this is not only used as a last resort: it is the primary mechanism for
everything except Linux early injection (so all of Mac, all standalone DR usage,
all static DR usage, all late injection which it's true we don't support much
anymore).

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9634
core/unix/os.c:9634: if ((ptr_uint_t)addr >= (ptr_uint_t)-4096) /* mmap failed
*/
Use mmap_syscall_succeeded

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9651
core/unix/os.c:9651: static size_t page_size = 0;
style: no variable or type decls in the middle of functions

"12. Macros and globals should be declared either at the top of the file or at
the top of a related group of functions that are delineated by a banner comment.
Do not place global declarations or defines at random places in the middle of a
file."

https://codereview.appspot.com/313800043/diff/1/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os_private.h#newcod...
core/unix/os_private.h:227: void os_set_page_size(size_t);
style: param name

[Edmund.Grimley.Evans, 2016-10-12 17:11:06]

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1611
core/unix/loader.c:1611: set_page_size(sp);
On 2016/10/12 16:03:36, bruening wrote:
> What about late injection, or standalone DR, or static DR?  For the first two,
> _init() takes in envp on all but Android and we should walk auxv there as
well,
> probably best in some foo_init() that is then shared among all 3: which foo is
> determined by how early it has to be.  If prior to options_init() it might
need
> to be a special init like syscalls_init() is for Windows.  In fact maybe
> syscalls_init() should turn into some cross-platform os_config_init() or sthg.

Does all this have to be done in the same commit? I think I've set up an
infrastructure that allows people to fill in the other cases later. It might be
easier to find bugs later if we don't do everything in one commit.

Note that the page size is used everywhere, including in relocate_dynamorio,
which itself has to execute before you start using global variables, so if
you're going to get the page size from the auxiliary vector then it has to be
just about the very first thing you do.

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9634
core/unix/os.c:9634: if ((ptr_uint_t)addr >= (ptr_uint_t)-4096) /* mmap failed
*/
On 2016/10/12 16:03:36, bruening wrote:
> Use mmap_syscall_succeeded

It would crash because of the ASSERT_CURIOSITY. This code is executed before
DynamoRIO has relocated itself.

[Edmund.Grimley.Evans, 2016-10-12 17:26:12]

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9634
core/unix/os.c:9634: if ((ptr_uint_t)addr >= (ptr_uint_t)-4096) /* mmap failed
*/
On 2016/10/12 17:11:06, Edmund.Grimley.Evans wrote:
> On 2016/10/12 16:03:36, bruening wrote:
> > Use mmap_syscall_succeeded
> 
> It would crash because of the ASSERT_CURIOSITY. This code is executed before
> DynamoRIO has relocated itself.

That's not quite what I meant... If you were to remove the code that looks up
the page size in the auxiliary vector then this code would be executed before
DynamoRIO has relocated itself, and it would crash, perhaps. The point is that I
want os_find_page_size to be totally robust and work wherever it is called from.
Perhaps I should just add a reference to mmap_syscall_succeeded in a comment.

[bruening, 2016-10-12 17:29:14]

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1611
core/unix/loader.c:1611: set_page_size(sp);
On 2016/10/12 17:11:05, Edmund.Grimley.Evans wrote:
> On 2016/10/12 16:03:36, bruening wrote:
> > What about late injection, or standalone DR, or static DR?  For the first
two,
> > _init() takes in envp on all but Android and we should walk auxv there as
> well,
> > probably best in some foo_init() that is then shared among all 3: which foo
is
> > determined by how early it has to be.  If prior to options_init() it might
> need
> > to be a special init like syscalls_init() is for Windows.  In fact maybe
> > syscalls_init() should turn into some cross-platform os_config_init() or
sthg.
> 
> Does all this have to be done in the same commit? I think I've set up an
> infrastructure that allows people to fill in the other cases later. It might
be
> easier to find bugs later if we don't do everything in one commit.
> 
> Note that the page size is used everywhere, including in relocate_dynamorio,
> which itself has to execute before you start using global variables, so if
> you're going to get the page size from the auxiliary vector then it has to be
> just about the very first thing you do.

We're talking about a call to the set_page_size routine from one other place: it
is not a huge change, and the other configurations are all tested in the same
test suite.  But if it's going to be split and this CL will only walk auxv for
Linux early injection, IMHO the "last resort" code should not be called for the
other cases and 4096 should be returned (with perhaps a debug-only call).

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9634
core/unix/os.c:9634: if ((ptr_uint_t)addr >= (ptr_uint_t)-4096) /* mmap failed
*/
On 2016/10/12 17:11:06, Edmund.Grimley.Evans wrote:
> On 2016/10/12 16:03:36, bruening wrote:
> > Use mmap_syscall_succeeded
> 
> It would crash because of the ASSERT_CURIOSITY. This code is executed before
> DynamoRIO has relocated itself.

It is?  It doesn't seem like it, and there are no comments saying that it's
fragile to prevent someone adding LOG or sthg.  If there's a reason to not use
the succeeded routine, a comment should explain it, and it would be nice to use
similar-looking code to mmap_syscall_succeeded for consistency (i.e.,
ptr_int_t).

[Edmund.Grimley.Evans, 2016-10-12 18:36:42]

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1611
core/unix/loader.c:1611: set_page_size(sp);
On 2016/10/12 17:29:14, bruening wrote:
> On 2016/10/12 17:11:05, Edmund.Grimley.Evans wrote:
> > On 2016/10/12 16:03:36, bruening wrote:
> > > What about late injection, or standalone DR, or static DR?  For the first
> two,
> > > _init() takes in envp on all but Android and we should walk auxv there as
> > well,
> > > probably best in some foo_init() that is then shared among all 3: which
foo
> is
> > > determined by how early it has to be.  If prior to options_init() it might
> > need
> > > to be a special init like syscalls_init() is for Windows.  In fact maybe
> > > syscalls_init() should turn into some cross-platform os_config_init() or
> sthg.
> > 
> > Does all this have to be done in the same commit? I think I've set up an
> > infrastructure that allows people to fill in the other cases later. It might
> be
> > easier to find bugs later if we don't do everything in one commit.
> > 
> > Note that the page size is used everywhere, including in relocate_dynamorio,
> > which itself has to execute before you start using global variables, so if
> > you're going to get the page size from the auxiliary vector then it has to
be
> > just about the very first thing you do.
> 
> We're talking about a call to the set_page_size routine from one other place:
it
> is not a huge change, and the other configurations are all tested in the same
> test suite.  But if it's going to be split and this CL will only walk auxv for
> Linux early injection, IMHO the "last resort" code should not be called for
the
> other cases and 4096 should be returned (with perhaps a debug-only call).

I'll look into that. Presumably all these cases are invoked from "make test" so
I can find them that way. But will it be possible to obtain the initial SP in
each of these cases? Or do some of these cases use the C library's start-up
code? If they do, then presumably I should call sysconf instead.

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9634
core/unix/os.c:9634: if ((ptr_uint_t)addr >= (ptr_uint_t)-4096) /* mmap failed
*/
On 2016/10/12 17:29:14, bruening wrote:
> On 2016/10/12 17:11:06, Edmund.Grimley.Evans wrote:
> > On 2016/10/12 16:03:36, bruening wrote:
> > > Use mmap_syscall_succeeded
> > 
> > It would crash because of the ASSERT_CURIOSITY. This code is executed before
> > DynamoRIO has relocated itself.
> 
> It is?  It doesn't seem like it, and there are no comments saying that it's
> fragile to prevent someone adding LOG or sthg.  If there's a reason to not use
> the succeeded routine, a comment should explain it, and it would be nice to
use
> similar-looking code to mmap_syscall_succeeded for consistency (i.e.,
> ptr_int_t).

Yes, I should add a warning comment like the comment at the top of
relocate_dynamorio.

I'd prefer to use ptr_uint_t in both places for mmap_syscall_succeeded because
casts to signed types tend to give undefined behaviour

[bruening, 2016-10-12 19:05:38]

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1611
core/unix/loader.c:1611: set_page_size(sp);
On 2016/10/12 18:36:42, Edmund.Grimley.Evans wrote:
> On 2016/10/12 17:29:14, bruening wrote:
> > On 2016/10/12 17:11:05, Edmund.Grimley.Evans wrote:
> > > On 2016/10/12 16:03:36, bruening wrote:
> > > > What about late injection, or standalone DR, or static DR?  For the
first
> > two,
> > > > _init() takes in envp on all but Android and we should walk auxv there
as
> > > well,
> > > > probably best in some foo_init() that is then shared among all 3: which
> foo
> > is
> > > > determined by how early it has to be.  If prior to options_init() it
might
> > > need
> > > > to be a special init like syscalls_init() is for Windows.  In fact maybe
> > > > syscalls_init() should turn into some cross-platform os_config_init() or
> > sthg.
> > > 
> > > Does all this have to be done in the same commit? I think I've set up an
> > > infrastructure that allows people to fill in the other cases later. It
might
> > be
> > > easier to find bugs later if we don't do everything in one commit.
> > > 
> > > Note that the page size is used everywhere, including in
relocate_dynamorio,
> > > which itself has to execute before you start using global variables, so if
> > > you're going to get the page size from the auxiliary vector then it has to
> be
> > > just about the very first thing you do.
> > 
> > We're talking about a call to the set_page_size routine from one other
place:
> it
> > is not a huge change, and the other configurations are all tested in the
same
> > test suite.  But if it's going to be split and this CL will only walk auxv
for
> > Linux early injection, IMHO the "last resort" code should not be called for
> the
> > other cases and 4096 should be returned (with perhaps a debug-only call).
> 
> I'll look into that. Presumably all these cases are invoked from "make test"
so
> I can find them that way. But will it be possible to obtain the initial SP in
> each of these cases? Or do some of these cases use the C library's start-up
> code? If they do, then presumably I should call sysconf instead.

AFAIK the envp passed to _init or .init_array can be assumed to always be on the
stack and usable to find auxv.  Maybe there's a corner case where a setenv in
another constructor affects it if the top-level caller is re-reading environ?!?

[Edmund.Grimley.Evans, 2016-10-13 16:16:05]

Commit log for latest patchset:
---------------
i#1680 large pages: Add code to discover page size on Linux.

The stack pointer is passed as a third argument to relocate_dynamorio
and on Linux this is used to search the auxiliary vector for AT_PAGESZ.
Similarly, there are calls to os_page_size_init in dynamorio_app_init
and standalone_init to do the same thing. On MacOS, and on Linux if
for some reason os_page_size is called before these initialisations,
the page size is discovered using the system calls mmap and munmap,
which should work on any POSIX system.

Review-URL: https://codereview.appspot.com/313800043
---------------

[Edmund.Grimley.Evans, 2016-10-13 16:37:21]

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/loader.c#newcode1594
core/unix/loader.c:1594: for (++p; p[0] != 0; p += 2) {
On 2016/10/12 16:03:36, bruening wrote:
> Use proper types: ELF_AUXV_TYPE *

Acknowledged.

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9651
core/unix/os.c:9651: static size_t page_size = 0;
On 2016/10/12 16:03:36, bruening wrote:
> style: no variable or type decls in the middle of functions
> 
> "12. Macros and globals should be declared either at the top of the file or at
> the top of a related group of functions that are delineated by a banner
comment.
> Do not place global declarations or defines at random places in the middle of
a
> file."

Acknowledged.

https://codereview.appspot.com/313800043/diff/1/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os_private.h#newcod...
core/unix/os_private.h:227: void os_set_page_size(size_t);
On 2016/10/12 16:03:36, bruening wrote:
> style: param name

Acknowledged.

[bruening, 2016-10-14 16:29:03]

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9625
core/unix/os.c:9625: * discovered in any other way, such as from AT_PAGESZ.
On 2016/10/12 16:03:36, bruening wrote:
> Mac OSX should use sysctl_query on hw.pagesize

Was this overlooked?  I see no response.  This is one line of code, is much
cleaner than a loop of mmaps, and we have "trybot" support for testing Mac.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode2589
core/unix/os.c:2589: (void)err;
The original seems simpler and more straightforward, avoiding this temporary and
the cast on line 2586 -- I see no positives and all negatives from this change,
right?

[Edmund.Grimley.Evans, 2016-10-14 16:38:19]

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9625
core/unix/os.c:9625: * discovered in any other way, such as from AT_PAGESZ.
On 2016/10/14 16:29:03, bruening wrote:
> On 2016/10/12 16:03:36, bruening wrote:
> > Mac OSX should use sysctl_query on hw.pagesize
> 
> Was this overlooked?  I see no response.  This is one line of code, is much
> cleaner than a loop of mmaps, and we have "trybot" support for testing Mac.

I left out the Mac bit. I thought it would be better as a separate patch.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode2589
core/unix/os.c:2589: (void)err;
On 2016/10/14 16:29:03, bruening wrote:
> The original seems simpler and more straightforward, avoiding this temporary
and
> the cast on line 2586 -- I see no positives and all negatives from this
change,
> right?

I am trying to avoid undefined behaviour. I'm not sure what the C standard
guarantees about casting a pointer to a signed type. You can't safely cast an
unsigned type to a signed type...

[bruening, 2016-10-14 17:11:41]

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9625
core/unix/os.c:9625: * discovered in any other way, such as from AT_PAGESZ.
On 2016/10/14 16:38:18, Edmund.Grimley.Evans wrote:
> On 2016/10/14 16:29:03, bruening wrote:
> > On 2016/10/12 16:03:36, bruening wrote:
> > > Mac OSX should use sysctl_query on hw.pagesize
> > 
> > Was this overlooked?  I see no response.  This is one line of code, is much
> > cleaner than a loop of mmaps, and we have "trybot" support for testing Mac.
> 
> I left out the Mac bit. I thought it would be better as a separate patch.

Please let the reviewer know -- typically each comment is marked Done or Ack or
replied to, and an XXX comment put in the code (and sometimes the issue and
commit msg).

The fear is that this will simply be forgotten, and we'll be stuck with a kludgy
mechanism on Mac where we used to have a nice constant and where there's a clean
general solution.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode2589
core/unix/os.c:2589: (void)err;
On 2016/10/14 16:38:18, Edmund.Grimley.Evans wrote:
> On 2016/10/14 16:29:03, bruening wrote:
> > The original seems simpler and more straightforward, avoiding this temporary
> and
> > the cast on line 2586 -- I see no positives and all negatives from this
> change,
> > right?
> 
> I am trying to avoid undefined behaviour. I'm not sure what the C standard
> guarantees about casting a pointer to a signed type. You can't safely cast an
> unsigned type to a signed type...

From a pointer, there is no difference I have heard of in casting to signed vs
unsigned pointer-sized integers:

C standard 6.3.2.3:

"Any pointer type may be converted to an integer type. Except as previously
specified, the result is implementation-defined. If the result cannot be
represented in the integer type, the behavior is undefined."

C++:

"A pointer converted to an integer of sufficient size (if any such exists on the
implementation) and back to the same pointer type will have its original value;
mappings between pointers and integers are otherwise
implementation-defined."

[Edmund.Grimley.Evans, 2016-10-14 18:09:49]

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9625
core/unix/os.c:9625: * discovered in any other way, such as from AT_PAGESZ.
On 2016/10/14 17:11:41, bruening wrote:
> On 2016/10/14 16:38:18, Edmund.Grimley.Evans wrote:
> > On 2016/10/14 16:29:03, bruening wrote:
> > > On 2016/10/12 16:03:36, bruening wrote:
> > > > Mac OSX should use sysctl_query on hw.pagesize
> > > 
> > > Was this overlooked?  I see no response.  This is one line of code, is
much
> > > cleaner than a loop of mmaps, and we have "trybot" support for testing
Mac.
> > 
> > I left out the Mac bit. I thought it would be better as a separate patch.
> 
> Please let the reviewer know -- typically each comment is marked Done or Ack
or
> replied to, and an XXX comment put in the code (and sometimes the issue and
> commit msg).
> 
> The fear is that this will simply be forgotten, and we'll be stuck with a
kludgy
> mechanism on Mac where we used to have a nice constant and where there's a
clean
> general solution.

I could make os_find_page_size always return 4096 on Mac, with a FIXME. Or I
could try playing with the trybot (though Travis was very slow on Mac today, I
noticed). It'll have to wait till Monday now in any case.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode2589
core/unix/os.c:2589: (void)err;
On 2016/10/14 17:11:41, bruening wrote:
> On 2016/10/14 16:38:18, Edmund.Grimley.Evans wrote:
> > On 2016/10/14 16:29:03, bruening wrote:
> > > The original seems simpler and more straightforward, avoiding this
temporary
> > and
> > > the cast on line 2586 -- I see no positives and all negatives from this
> > change,
> > > right?
> > 
> > I am trying to avoid undefined behaviour. I'm not sure what the C standard
> > guarantees about casting a pointer to a signed type. You can't safely cast
an
> > unsigned type to a signed type...
> 
> From a pointer, there is no difference I have heard of in casting to signed vs
> unsigned pointer-sized integers:
> 
> C standard 6.3.2.3:
> 
> "Any pointer type may be converted to an integer type. Except as previously
> specified, the result is implementation-defined. If the result cannot be
> represented in the integer type, the behavior is undefined."
> 
> C++:
> 
> "A pointer converted to an integer of sufficient size (if any such exists on
the
> implementation) and back to the same pointer type will have its original
value;
> mappings between pointers and integers are otherwise
> implementation-defined."

I'm generally nervous about signed types because of the weird bugs they cause
when you have an enthusiastically optimising compiler, and I've always stuck to
uintptr_t myself when I have to cast a pointer to an integer, but I'll leave the
existing code if you prefer. I hope it won't cause any problems with future
compilers. It is at least "implementation defined" rather than "undefined", I
think...

[bruening, 2016-10-15 01:25:52]

Looks like it won't build on Windows

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/1/core/unix/os.c#newcode9625
core/unix/os.c:9625: * discovered in any other way, such as from AT_PAGESZ.
On 2016/10/14 18:09:49, Edmund.Grimley.Evans wrote:
> On 2016/10/14 17:11:41, bruening wrote:
> > On 2016/10/14 16:38:18, Edmund.Grimley.Evans wrote:
> > > On 2016/10/14 16:29:03, bruening wrote:
> > > > On 2016/10/12 16:03:36, bruening wrote:
> > > > > Mac OSX should use sysctl_query on hw.pagesize
> > > > 
> > > > Was this overlooked?  I see no response.  This is one line of code, is
> much
> > > > cleaner than a loop of mmaps, and we have "trybot" support for testing
> Mac.
> > > 
> > > I left out the Mac bit. I thought it would be better as a separate patch.
> > 
> > Please let the reviewer know -- typically each comment is marked Done or Ack
> or
> > replied to, and an XXX comment put in the code (and sometimes the issue and
> > commit msg).
> > 
> > The fear is that this will simply be forgotten, and we'll be stuck with a
> kludgy
> > mechanism on Mac where we used to have a nice constant and where there's a
> clean
> > general solution.
> 
> I could make os_find_page_size always return 4096 on Mac, with a FIXME. Or I
> could try playing with the trybot (though Travis was very slow on Mac today, I
> noticed). It'll have to wait till Monday now in any case.

I'm happy with Mac in a separate CL as long as this CL has a comment about it.

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode370
core/dynamo.c:370: os_page_size_init(our_environ);
This will not build on Windows: "our_environ" does not exist there

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode841
core/dynamo.c:841: os_page_size_init(our_environ);
Ditto

https://codereview.appspot.com/313800043/diff/20001/core/os_shared.h
File core/os_shared.h (right):

https://codereview.appspot.com/313800043/diff/20001/core/os_shared.h#newcode513
core/os_shared.h:513: void os_page_size_init(char **env);
suggest: const char

https://codereview.appspot.com/313800043/diff/20001/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/loader.c#newcod...
core/unix/loader.c:1585: char **env = (char **)sp + argc + 2;
Worth a comment explaining why "2"

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode2589
core/unix/os.c:2589: (void)err;
On 2016/10/14 18:09:49, Edmund.Grimley.Evans wrote:
> On 2016/10/14 17:11:41, bruening wrote:
> > On 2016/10/14 16:38:18, Edmund.Grimley.Evans wrote:
> > > On 2016/10/14 16:29:03, bruening wrote:
> > > > The original seems simpler and more straightforward, avoiding this
> temporary
> > > and
> > > > the cast on line 2586 -- I see no positives and all negatives from this
> > > change,
> > > > right?
> > > 
> > > I am trying to avoid undefined behaviour. I'm not sure what the C standard
> > > guarantees about casting a pointer to a signed type. You can't safely cast
> an
> > > unsigned type to a signed type...
> > 
> > From a pointer, there is no difference I have heard of in casting to signed
vs
> > unsigned pointer-sized integers:
> > 
> > C standard 6.3.2.3:
> > 
> > "Any pointer type may be converted to an integer type. Except as previously
> > specified, the result is implementation-defined. If the result cannot be
> > represented in the integer type, the behavior is undefined."
> > 
> > C++:
> > 
> > "A pointer converted to an integer of sufficient size (if any such exists on
> the
> > implementation) and back to the same pointer type will have its original
> value;
> > mappings between pointers and integers are otherwise
> > implementation-defined."
> 
> I'm generally nervous about signed types because of the weird bugs they cause
> when you have an enthusiastically optimising compiler, and I've always stuck
to
> uintptr_t myself when I have to cast a pointer to an integer, but I'll leave
the
> existing code if you prefer. I hope it won't cause any problems with future
> compilers. It is at least "implementation defined" rather than "undefined", I
> think...

There's no addition or subtraction and so no risk of over/underflow here, and
IMHO the code is simpler and cleaner as signed, so I would vote for preserving
the original (with the 4096 subst).

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9636
core/unix/os.c:9636: /* Return true if size is a multiple of the page size . */
The comment about being fragile should be repeated here -- it will be quite easy
for someone to come change this function and add ASSERT or sthg.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9656
core/unix/os.c:9656: * discovered in any other way, such as from AT_PAGESZ. This
function may be
suggest: split off as separate line starting with XXX to make this visible, as
the loader.c ones are done

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9676
core/unix/os.c:9676: }
This looks good, just a few syscalls and it will have it.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9681
core/unix/os.c:9681: void
static

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9705
core/unix/os.c:9705: # define KERNEL_AT_PAGESZ 6
This should not be needed: we should use the official header. We're already
using various auxv AT_ constants elsewhere.  Why would this be different?

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9708
core/unix/os.c:9708: struct auxval { ptr_uint_t type, val; } *auxv;
?  As requested before, use the type we already have: ELF_AUXV_TYPE.  See
privload_setup_auxv.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9713
core/unix/os.c:9713: for (auxv = (struct auxval *)(env + 1); auxv->type != 0;
auxv++) {
Use AT_NULL

https://codereview.appspot.com/313800043/diff/20001/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os_private.h#ne...
core/unix/os_private.h:227: void os_set_page_size(size_t size);
This does not need to be exported anymore

https://codereview.appspot.com/313800043/diff/20001/core/win32/os.c
File core/win32/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/win32/os.c#newcode9161
core/win32/os.c:9161: {
Best to have a comment explaining that either it's empty b/c there will never be
anything to do, or it's not finished and this is temporarily empty

[Edmund.Grimley.Evans, 2016-10-15 07:01:01]

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9708
core/unix/os.c:9708: struct auxval { ptr_uint_t type, val; } *auxv;
On 2016/10/15 01:25:52, bruening wrote:
> ?  As requested before, use the type we already have: ELF_AUXV_TYPE.  See
> privload_setup_auxv.

I tried #including "module_elf.h", where ELF_AUXV_TYPE is defined, but that
caused a shitstorm of errors.

[Edmund.Grimley.Evans, 2016-10-17 11:16:49]

https://codereview.appspot.com/313800043/diff/20001/core/os_shared.h
File core/os_shared.h (right):

https://codereview.appspot.com/313800043/diff/20001/core/os_shared.h#newcode513
core/os_shared.h:513: void os_page_size_init(char **env);
On 2016/10/15 01:25:51, bruening wrote:
> suggest: const char

Done. Cast of our_environ is now required.

https://codereview.appspot.com/313800043/diff/20001/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/loader.c#newcod...
core/unix/loader.c:1585: char **env = (char **)sp + argc + 2;
On 2016/10/15 01:25:51, bruening wrote:
> Worth a comment explaining why "2"

Done.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9636
core/unix/os.c:9636: /* Return true if size is a multiple of the page size . */
On 2016/10/15 01:25:52, bruening wrote:
> The comment about being fragile should be repeated here -- it will be quite
easy
> for someone to come change this function and add ASSERT or sthg.

Done.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9656
core/unix/os.c:9656: * discovered in any other way, such as from AT_PAGESZ. This
function may be
On 2016/10/15 01:25:51, bruening wrote:
> suggest: split off as separate line starting with XXX to make this visible, as
> the loader.c ones are done

Done.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9681
core/unix/os.c:9681: void
On 2016/10/15 01:25:51, bruening wrote:
> static

Done.

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9705
core/unix/os.c:9705: # define KERNEL_AT_PAGESZ 6
On 2016/10/15 01:25:52, bruening wrote:
> This should not be needed: we should use the official header. We're already
> using various auxv AT_ constants elsewhere.  Why would this be different?

It's not different, but perhaps in principle one should distinguish between the
AT_PAGESZ used and exported by the kernel, and the AT_PAGESZ used and exported
by the C library. They are very unlikely to differ in this case, but perhaps in
the case of sigaction...

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9708
core/unix/os.c:9708: struct auxval { ptr_uint_t type, val; } *auxv;
On 2016/10/15 07:01:00, Edmund.Grimley.Evans wrote:
> I tried #including "module_elf.h", where ELF_AUXV_TYPE is defined, but that
> caused a shitstorm of errors.

But #include "module_private.h" works!

https://codereview.appspot.com/313800043/diff/20001/core/unix/os.c#newcode9713
core/unix/os.c:9713: for (auxv = (struct auxval *)(env + 1); auxv->type != 0;
auxv++) {
On 2016/10/15 01:25:51, bruening wrote:
> Use AT_NULL

Done (with #include "module_private.h").

https://codereview.appspot.com/313800043/diff/20001/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/313800043/diff/20001/core/unix/os_private.h#ne...
core/unix/os_private.h:227: void os_set_page_size(size_t size);
On 2016/10/15 01:25:52, bruening wrote:
> This does not need to be exported anymore

Done.

[Edmund.Grimley.Evans, 2016-10-17 13:15:29]

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode370
core/dynamo.c:370: os_page_size_init(our_environ);
On 2016/10/15 01:25:51, bruening wrote:
> This will not build on Windows: "our_environ" does not exist there

Added #ifdef UNIX

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode841
core/dynamo.c:841: os_page_size_init(our_environ);
On 2016/10/15 01:25:51, bruening wrote:
> Ditto

Added #ifdef UNIX

https://codereview.appspot.com/313800043/diff/20001/core/win32/os.c
File core/win32/os.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/win32/os.c#newcode9161
core/win32/os.c:9161: {
On 2016/10/15 01:25:52, bruening wrote:
> Best to have a comment explaining that either it's empty b/c there will never
be
> anything to do, or it's not finished and this is temporarily empty

I've made this function only defined on UNIX.

[Edmund.Grimley.Evans, 2016-10-17 15:10:37]

Commit log for latest patchset:
---------------
i#1680 large pages: Add code to discover page size on Unix.

The stack pointer is passed as a third argument to relocate_dynamorio
and on Linux this is used to search the auxiliary vector for AT_PAGESZ.
Similarly, there are calls to os_page_size_init in dynamorio_app_init
and standalone_init to do the same thing. On MacOS, and on Linux if
for some reason os_page_size is called before these initialisations,
the page size is discovered using the system calls mmap and munmap,
which should work on any POSIX system.

A later commit should get Mac OSX to use sysctl_query on hw.pagesize.

Review-URL: https://codereview.appspot.com/313800043
---------------

[bruening, 2016-10-17 16:59:34]

LGTM w/ a couple of comments

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode370
core/dynamo.c:370: os_page_size_init(our_environ);
On 2016/10/17 13:15:29, Edmund.Grimley.Evans wrote:
> On 2016/10/15 01:25:51, bruening wrote:
> > This will not build on Windows: "our_environ" does not exist there
> 
> Added #ifdef UNIX

I'm not sure this is the best solution as this is not a UNIX-only action: we
want to initialize the page size early on Windows too.  Maybe as a temporary
thing, but then there should be a comment about it being temporary w/ the i#. 
An alternative is making the arg IF_UNIX.

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode841
core/dynamo.c:841: os_page_size_init(our_environ);
On 2016/10/17 13:15:28, Edmund.Grimley.Evans wrote:
> On 2016/10/15 01:25:51, bruening wrote:
> > Ditto
> 
> Added #ifdef UNIX

Ditto

https://codereview.appspot.com/313800043/diff/40001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/313800043/diff/40001/core/unix/os.c#newcode9698
core/unix/os.c:9698: /* FIXME i#1680: On Mac OSX we should use sysctl_query on
hw.pagesize. */
Generally FIXME is for correctness issues; XXX for everything else.

[Edmund.Grimley.Evans, 2016-10-17 17:33:12]

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c#newcode370
core/dynamo.c:370: os_page_size_init(our_environ);
On 2016/10/17 16:59:33, bruening wrote:
> On 2016/10/17 13:15:29, Edmund.Grimley.Evans wrote:
> > On 2016/10/15 01:25:51, bruening wrote:
> > > This will not build on Windows: "our_environ" does not exist there
> > 
> > Added #ifdef UNIX
> 
> I'm not sure this is the best solution as this is not a UNIX-only action: we
> want to initialize the page size early on Windows too.  Maybe as a temporary
> thing, but then there should be a comment about it being temporary w/ the i#. 
> An alternative is making the arg IF_UNIX.

I'm not sure either, particularly since I don't know much about Windows or Mac,
but I think that on Windows and Mac the page size can just as easily be
initialised the first time that os_page_size is called (in order to query the
page size). It's only on Linux that we have to initialise the page size before
os_page_size is called for the first time in order to avoid the "experimental
mmap" method, because it's only on Linux that we use the auxiliary vector rather
than a system call: the auxiliary vector can only be examined when we have a
pointer to it; the system call can be used in any old context.

[bruening, 2016-10-17 18:10:42]

(On phone so replying top level)
Ok that makes sense.

On Monday, October 17, 2016, <Edmund.Grimley.Evans@gmail.com> wrote:

>
> https://codereview.appspot.com/313800043/diff/20001/core/dynamo.c
> File core/dynamo.c (right):
>
> https://codereview.appspot.com/313800043/diff/20001/core/dyn
> amo.c#newcode370
> core/dynamo.c:370: os_page_size_init(our_environ);
> On 2016/10/17 16:59:33, bruening wrote:
>
>> On 2016/10/17 13:15:29, Edmund.Grimley.Evans wrote:
>> > On 2016/10/15 01:25:51, bruening wrote:
>> > > This will not build on Windows: "our_environ" does not exist there
>> >
>> > Added #ifdef UNIX
>>
>
> I'm not sure this is the best solution as this is not a UNIX-only
>>
> action: we
>
>> want to initialize the page size early on Windows too.  Maybe as a
>>
> temporary
>
>> thing, but then there should be a comment about it being temporary w/
>>
> the i#.
>
>> An alternative is making the arg IF_UNIX.
>>
>
> I'm not sure either, particularly since I don't know much about Windows
> or Mac, but I think that on Windows and Mac the page size can just as
> easily be initialised the first time that os_page_size is called (in
> order to query the page size). It's only on Linux that we have to
> initialise the page size before os_page_size is called for the first
> time in order to avoid the "experimental mmap" method, because it's only
> on Linux that we use the auxiliary vector rather than a system call: the
> auxiliary vector can only be examined when we have a pointer to it; the
> system call can be used in any old context.
>
> https://codereview.appspot.com/313800043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "DynamoRIO Devs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to dynamorio-devs+unsubscribe@googlegroups.com.
> To post to this group, send email to dynamorio-devs@googlegroups.com.
> Visit this group at https://groups.google.com/group/dynamorio-devs.
> For more options, visit https://groups.google.com/d/optout.
>

[Edmund.Grimley.Evans, 2016-10-18 09:05:29]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/efa2d7b6a7ee5543d1ef7240a12afcf...

Final commit log: 
---------------
i#1680 large pages: Add code to discover page size on Unix.

The stack pointer is passed as a third argument to relocate_dynamorio
and on Linux this is used to search the auxiliary vector for AT_PAGESZ.
Similarly, there are calls to os_page_size_init in dynamorio_app_init
and standalone_init to do the same thing. On MacOS, and on Linux if
for some reason os_page_size is called before these initialisations,
the page size is discovered using the system calls mmap and munmap,
which should work on any POSIX system.

A later commit should get Mac OSX to use sysctl_query on hw.pagesize.

Review-URL: https://codereview.appspot.com/313800043
---------------