i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

Commit log for first patchset:
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as dr_page_size()
in globals.h for DynamoRIO's internal use.

Modify non-core code to use the new API.

Currently the value returned by dr_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.
---------------

[zhaoqin, 2016-09-09 21:09:48]

LGTM with a few questions

https://codereview.appspot.com/309340043/diff/1/api/samples/instrace_x86.c
File api/samples/instrace_x86.c (right):

https://codereview.appspot.com/309340043/diff/1/api/samples/instrace_x86.c#ne...
api/samples/instrace_x86.c:294: size_t page_size = dr_page_size();
should we use a global variable within the file to used by _init and _exit?

https://codereview.appspot.com/309340043/diff/1/api/samples/memtrace_x86.c
File api/samples/memtrace_x86.c (right):

https://codereview.appspot.com/309340043/diff/1/api/samples/memtrace_x86.c#ne...
api/samples/memtrace_x86.c:346: size_t page_size = dr_page_size();
ditto about global variable.

https://codereview.appspot.com/309340043/diff/1/suite/tests/security-common/T...
File suite/tests/security-common/TestMemProtChg.c (right):

https://codereview.appspot.com/309340043/diff/1/suite/tests/security-common/T...
suite/tests/security-common/TestMemProtChg.c:112: /* The stack tests require a
variable-length array. */
can you add more explanation about in which case the variable-length array is
supported.

https://codereview.appspot.com/309340043/diff/1/suite/tests/security-common/T...
suite/tests/security-common/TestMemProtChg.c:140: /* Run the test on the static
buffer, again. */
add comment to explain that rerun the test on the static buffer for test output
check match.

https://codereview.appspot.com/309340043/diff/1/suite/tests/tools.h
File suite/tests/tools.h (right):

https://codereview.appspot.com/309340043/diff/1/suite/tests/tools.h#newcode107
suite/tests/tools.h:107: }
why not use dr_page_size directly as it is added in the API?

[Edmund.Grimley.Evans, 2016-09-09 21:29:21]

https://codereview.appspot.com/309340043/diff/1/api/samples/instrace_x86.c
File api/samples/instrace_x86.c (right):

https://codereview.appspot.com/309340043/diff/1/api/samples/instrace_x86.c#ne...
api/samples/instrace_x86.c:294: size_t page_size = dr_page_size();
On 2016/09/09 21:09:47, zhaoqin wrote:
> should we use a global variable within the file to used by _init and _exit?

I don't see why not. I'll try it...

https://codereview.appspot.com/309340043/diff/1/suite/tests/tools.h
File suite/tests/tools.h (right):

https://codereview.appspot.com/309340043/diff/1/suite/tests/tools.h#newcode107
suite/tests/tools.h:107: }
On 2016/09/09 21:09:48, zhaoqin wrote:
> why not use dr_page_size directly as it is added in the API?

I think this file is also used by tests that don't use DR's API. So previously
there was an independent definition of PAGE_SIZE here, and now there's an
independent definition of page_size (which can probably be simpler than the
definition of dr_page_size because it's only used for the tests and therefore
doesn't need to be particularly efficient).

[bruening, 2016-09-10 00:24:10]

bug: there is no definition of dr_page_size on Windows

I thought I was the reviewer but then Qin's review went through so I didn't look
at everything.

https://codereview.appspot.com/309340043/diff/1/core/arch/proc.h
File core/arch/proc.h (right):

https://codereview.appspot.com/309340043/diff/1/core/arch/proc.h#newcode55
core/arch/proc.h:55: /**< Convenience macro to align to the start of a page of
memory.
Incorrect Doxygen comment: does this build the docs correctly?

https://codereview.appspot.com/309340043/diff/1/core/arch/proc.h#newcode56
core/arch/proc.h:56: * It uses a function call so be careful where performance
is critical. */
Comment style violation

https://codereview.appspot.com/309340043/diff/1/core/arch/proc.h#newcode517
core/arch/proc.h:517: /**< Returns the size of a page of memory. */
Incorrect comment type

https://codereview.appspot.com/309340043/diff/1/core/arch/proc.h#newcode519
core/arch/proc.h:519: dr_page_size(void);
Every function exported here starts with proc_, though OTOH if we started over
we might prefix everything with dr_ so maybe this is fine, but if this is not a
proc_ function and is not implemented in proc.c, should it be in some other
header?  Maybe not.

https://codereview.appspot.com/309340043/diff/1/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/1/core/globals.h#newcode347
core/globals.h:347: # define PAGE_SIZE dr_page_size()
Note that generally internal code does not use dr_* routines as those are
interfaces for outside code and often have extra layers or take in an opaque
dcontext.

The way this is generally done in existing code would be to have os_page_size(),
declared in os_shared.h and implemented in each of unix/ and win32/, and then
have instrument_api.h declare dr_page_size() which would then call
os_page_size().  WDYT?

[Edmund.Grimley.Evans, 2016-09-16 11:02:26]

https://codereview.appspot.com/309340043/diff/1/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/1/core/globals.h#newcode347
core/globals.h:347: # define PAGE_SIZE dr_page_size()
On 2016/09/10 00:24:09, bruening wrote:
> Note that generally internal code does not use dr_* routines as those are
> interfaces for outside code and often have extra layers or take in an opaque
> dcontext.
> 
> The way this is generally done in existing code would be to have
os_page_size(),
> declared in os_shared.h and implemented in each of unix/ and win32/, and then
> have instrument_api.h declare dr_page_size() which would then call
> os_page_size().  WDYT?

That sounds like a sensible suggestion. I'm about to submit a new version of
this commit.

If either of you can test it on Windows, that would be great. Note that the
patch has to be applied onto the latest master, or rebased.

[Edmund.Grimley.Evans, 2016-09-16 11:03:55]

Commit log for latest patchset:
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as os_page_size()
in globals.h for DynamoRIO's internal use.

Modify non-core code to use the new API.

Currently the value returned by os_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.

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

[zhaoqin, 2016-09-22 15:45:07]

On 2016/09/16 11:03:55, Edmund.Grimley.Evans wrote:
> Commit log for latest patchset:
> ---------------
> i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().
> 
> When the page size is not a compile-time constant it is misleading to
> provide the traditional PAGE_SIZE macro, so we provide a function
> instead, though we provisionally define PAGE_SIZE as os_page_size()
> in globals.h for DynamoRIO's internal use.
> 
> Modify non-core code to use the new API.
> 
> Currently the value returned by os_page_size() is always 4096. In future
> on AArch64 it will sometimes be 16384 or 65536.
> 
> Review-URL: https://codereview.appspot.com/309340043
> ---------------

Derek, I assume you will finish review this one.

[bruening, 2016-09-26 18:05:10]

Mostly looks good but there is one issue to discuss: whether it is safe to use a
PAGE_SIZE value coming from the build toolchain.  I do not think it is as it
could prevent running correctly on a differently-configured kernel.

https://codereview.appspot.com/309340043/diff/20001/api/samples/instrace_x86.c
File api/samples/instrace_x86.c (right):

https://codereview.appspot.com/309340043/diff/20001/api/samples/instrace_x86....
api/samples/instrace_x86.c:300: page_size = dr_page_size();
suggest: initialize in dr_client_main to avoid future bugs where someone
references page_size in some other function

https://codereview.appspot.com/309340043/diff/20001/api/samples/memtrace_x86.c
File api/samples/memtrace_x86.c (right):

https://codereview.appspot.com/309340043/diff/20001/api/samples/memtrace_x86....
api/samples/memtrace_x86.c:352: page_size = dr_page_size();
Ditto.

https://codereview.appspot.com/309340043/diff/20001/clients/drcov/postprocess...
File clients/drcov/postprocess/drcov2lcov.cpp (right):

https://codereview.appspot.com/309340043/diff/20001/clients/drcov/postprocess...
clients/drcov/postprocess/drcov2lcov.cpp:717: ASSERT(ALIGNED(size,
dr_page_size()), "Module size is not aligned");
This is a standalone API app -- it mostly does a normal init, but please
double-check that the page size will be initialized for it.  The docs do claim
that all API routines except certain ones it lists will work so the user will
expect it to work for standalone.

https://codereview.appspot.com/309340043/diff/20001/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/20001/core/globals.h#newcode342
core/globals.h:342: /* If PAGE_SIZE is not provided by the system, define it for
DynamoRIO's
We often build a binary that is meant to run on multiple systems and across
multiple kernel versions -- so it may not be wise to rely on a toolchain define,
just like we do not want to rely on the syscall numbers from the toolchain that
happens to be used to build as it is often missing things that we need to run on
newer kernels.

Consider an ARM64 kernel compiled to use 64K pages with a DR built vs a default
toolchain that has a PAGE_SIZE define -- instead of dynamically reading the page
size and working correctly on the differently-configured kernel, DR will fail.

I think it is safest to remove this ifndef check.

Perhaps os_page_size should be a static inline function, if performance is what
we're worried about.

https://codereview.appspot.com/309340043/diff/20001/core/globals.h#newcode345
core/globals.h:345: * a compile-time constant.
This last sentence seems to contradict itself?  "better to use the non-constant
function" even though "the size is a constant"?

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

https://codereview.appspot.com/309340043/diff/20001/core/os_shared.h#newcode54
core/os_shared.h:54: size_t os_page_size(void);
This fits better near get_memory_info() and other memory attribute routines
instead of up here with init/exit and TLS routines.

https://codereview.appspot.com/309340043/diff/20001/ext/drcontainers/drtable.c
File ext/drcontainers/drtable.c (right):

https://codereview.appspot.com/309340043/diff/20001/ext/drcontainers/drtable....
ext/drcontainers/drtable.c:41: #ifndef PAGE_SIZE
As earlier, I don't think this ifndef is safe

https://codereview.appspot.com/309340043/diff/20001/suite/tests/client_tools.h
File suite/tests/client_tools.h (right):

https://codereview.appspot.com/309340043/diff/20001/suite/tests/client_tools....
suite/tests/client_tools.h:42: #ifndef PAGE_SIZE
Same argument as before

https://codereview.appspot.com/309340043/diff/20001/suite/tests/tools.h
File suite/tests/tools.h (right):

https://codereview.appspot.com/309340043/diff/20001/suite/tests/tools.h#newco...
suite/tests/tools.h:98: #ifndef PAGE_SIZE
Ditto

https://codereview.appspot.com/309340043/diff/20001/suite/tests/tools.h#newco...
suite/tests/tools.h:111: * ASSERT(page_size() <= PAGE_SIZE_MAX).
Please just put this assert in INIT()

[Edmund.Grimley.Evans, 2016-09-26 20:18:17]

https://codereview.appspot.com/309340043/diff/20001/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/20001/core/globals.h#newcode342
core/globals.h:342: /* If PAGE_SIZE is not provided by the system, define it for
DynamoRIO's
On 2016/09/26 18:05:10, bruening wrote:
> We often build a binary that is meant to run on multiple systems and across
> multiple kernel versions -- so it may not be wise to rely on a toolchain
define,
> just like we do not want to rely on the syscall numbers from the toolchain
that
> happens to be used to build as it is often missing things that we need to run
on
> newer kernels.

There's a difference between wrong definitions and missing definitions. I'm not
sure to what extent you can expect binaries generated by a tool chain to work if
definitions supplied by that tool chain, and perhaps therefore assumptions made
by that tool chain, are wrong. Perhaps you're all right if you know you're not
using the standard libraries...

I wouldn't expect any tool chain targeting Linux arm64 to define PAGE_SIZE
because it's been known to be variable right from the start. For other Linux
architectures there may be the situation that old tool chains define PAGE_SIZE,
because at the time it was believed to be a universal constant for that
architecture, while modern kernels use a different value... but that would be a
problem affecting many users besides DynamoRIO...

I would guess that the safest thing to do would be to note that the tool chain
has supplied a value for PAGE_SIZE and give an error at run time if the value
discovered at run time is different from the value supplied by the tool chain.
Implementing that might be a bit fiddly because we'd have to be careful to
distinguish the tool chain's PAGE_SIZE from DynamoRIO's own definition and make
sure we're using the right one... Perhaps in that case one should rename
DynamoRIO's PAGE_SIZE with a search-and-replace...

I don't think any recent "Linux" tool chains define PAGE_SIZE, but I seem to
recall a mention in a comment that perhaps some Android tool chains define
PAGE_SIZE.

Perhaps the best thing to do is to remove all mention of PAGE_SIZE from
DynamoRIO's source, replacing the uses with dr_page_size() or os_page_size()?

[Edmund.Grimley.Evans, 2016-09-27 09:24:32]

I'm just going with #undef PAGE_SIZE for now.

[Edmund.Grimley.Evans, 2016-09-27 09:24:52]

Commit log for latest patchset:
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as os_page_size()
in globals.h for DynamoRIO's internal use.

Modify non-core code to use the new API.

Currently the value returned by os_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.

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

[bruening, 2016-09-28 19:53:57]

I think the drcov.fib change is not supposed to be here?

https://codereview.appspot.com/309340043/diff/20001/clients/drcov/postprocess...
File clients/drcov/postprocess/drcov2lcov.cpp (right):

https://codereview.appspot.com/309340043/diff/20001/clients/drcov/postprocess...
clients/drcov/postprocess/drcov2lcov.cpp:717: ASSERT(ALIGNED(size,
dr_page_size()), "Module size is not aligned");
On 2016/09/26 18:05:10, bruening wrote:
> This is a standalone API app -- it mostly does a normal init, but please
> double-check that the page size will be initialized for it.  The docs do claim
> that all API routines except certain ones it lists will work so the user will
> expect it to work for standalone.

Did you mean to mark Done or Ack?

https://codereview.appspot.com/309340043/diff/20001/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/20001/core/globals.h#newcode342
core/globals.h:342: /* If PAGE_SIZE is not provided by the system, define it for
DynamoRIO's
On 2016/09/26 20:18:17, Edmund.Grimley.Evans wrote:
> On 2016/09/26 18:05:10, bruening wrote:
> > We often build a binary that is meant to run on multiple systems and across
> > multiple kernel versions -- so it may not be wise to rely on a toolchain
> define,
> > just like we do not want to rely on the syscall numbers from the toolchain
> that
> > happens to be used to build as it is often missing things that we need to
run
> on
> > newer kernels.
> 
> There's a difference between wrong definitions and missing definitions. I'm
not
> sure to what extent you can expect binaries generated by a tool chain to work
if
> definitions supplied by that tool chain, and perhaps therefore assumptions
made
> by that tool chain, are wrong. Perhaps you're all right if you know you're not
> using the standard libraries...
> 
> I wouldn't expect any tool chain targeting Linux arm64 to define PAGE_SIZE
> because it's been known to be variable right from the start. For other Linux
> architectures there may be the situation that old tool chains define
PAGE_SIZE,
> because at the time it was believed to be a universal constant for that
> architecture, while modern kernels use a different value... but that would be
a
> problem affecting many users besides DynamoRIO...
> 
> I would guess that the safest thing to do would be to note that the tool chain
> has supplied a value for PAGE_SIZE and give an error at run time if the value
> discovered at run time is different from the value supplied by the tool chain.
> Implementing that might be a bit fiddly because we'd have to be careful to
> distinguish the tool chain's PAGE_SIZE from DynamoRIO's own definition and
make
> sure we're using the right one... Perhaps in that case one should rename
> DynamoRIO's PAGE_SIZE with a search-and-replace...
> 
> I don't think any recent "Linux" tool chains define PAGE_SIZE, but I seem to
> recall a mention in a comment that perhaps some Android tool chains define
> PAGE_SIZE.
> 
> Perhaps the best thing to do is to remove all mention of PAGE_SIZE from
> DynamoRIO's source, replacing the uses with dr_page_size() or os_page_size()?

I would not want to base anything on speculation on what a toolchain might or
might not do, without checks to verify.  Maybe undef PAGE_SIZE is simplest and
make os_page_size into static inline with a cached var value.

https://codereview.appspot.com/309340043/diff/20001/core/globals.h#newcode345
core/globals.h:345: * a compile-time constant.
On 2016/09/26 18:05:10, bruening wrote:
> This last sentence seems to contradict itself?  "better to use the
non-constant
> function" even though "the size is a constant"?

I see no reply or "Done". Re-reading it I think that what you mean is that a
non-macro define like PAGE_SIZE is expected to be a literal.  Clarifying the
comment would help.

https://codereview.appspot.com/309340043/diff/40001/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/globals.h#newcode342
core/globals.h:342: * version for DynamoRIO's internal use. Since PAGE_SIZE
looks like
OK, best to mark Done or sthg so the reviewer knows there are no loose ends that
were overlooked.

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
As with all new API routines, please add to the changelist in release.dox.

https://codereview.appspot.com/309340043/diff/40001/suite/tests/common/tool.d...
File suite/tests/common/tool.drcov.fib.template (left):

https://codereview.appspot.com/309340043/diff/40001/suite/tests/common/tool.d...
suite/tests/common/tool.drcov.fib.template:9: DA:73,1
?  How is this related to the page size?  Please separate this into a separate
CL.  I'm not sure about this b/c this test has been passing on all platforms --
it seems like this is going to break non-Windows.

[Edmund.Grimley.Evans, 2016-09-28 20:41:17]

https://codereview.appspot.com/309340043/diff/20001/clients/drcov/postprocess...
File clients/drcov/postprocess/drcov2lcov.cpp (right):

https://codereview.appspot.com/309340043/diff/20001/clients/drcov/postprocess...
clients/drcov/postprocess/drcov2lcov.cpp:717: ASSERT(ALIGNED(size,
dr_page_size()), "Module size is not aligned");
On 2016/09/28 19:53:56, bruening wrote:
> On 2016/09/26 18:05:10, bruening wrote:
> > This is a standalone API app -- it mostly does a normal init, but please
> > double-check that the page size will be initialized for it.  The docs do
claim
> > that all API routines except certain ones it lists will work so the user
will
> > expect it to work for standalone.
> 
> Did you mean to mark Done or Ack?

Ack, I guess. Currently dr_page_size() just returns 4096. I certainly intend it
to work whenever it's called.

https://codereview.appspot.com/309340043/diff/20001/core/globals.h
File core/globals.h (right):

https://codereview.appspot.com/309340043/diff/20001/core/globals.h#newcode345
core/globals.h:345: * a compile-time constant.
On 2016/09/28 19:53:57, bruening wrote:
> On 2016/09/26 18:05:10, bruening wrote:
> > This last sentence seems to contradict itself?  "better to use the
> non-constant
> > function" even though "the size is a constant"?
> 
> I see no reply or "Done". Re-reading it I think that what you mean is that a
> non-macro define like PAGE_SIZE is expected to be a literal.  Clarifying the
> comment would help.

Yes.

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/28 19:53:57, bruening wrote:
> As with all new API routines, please add to the changelist in release.dox.

I'll do that tomorrow...

https://codereview.appspot.com/309340043/diff/40001/suite/tests/common/tool.d...
File suite/tests/common/tool.drcov.fib.template (left):

https://codereview.appspot.com/309340043/diff/40001/suite/tests/common/tool.d...
suite/tests/common/tool.drcov.fib.template:9: DA:73,1
On 2016/09/28 19:53:57, bruening wrote:
> ?  How is this related to the page size?  Please separate this into a separate
> CL.  I'm not sure about this b/c this test has been passing on all platforms
--
> it seems like this is going to break non-Windows.

It's a consequence of putting the assert into INIT(), as you requested.

[Edmund.Grimley.Evans, 2016-09-28 21:06:20]

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/28 20:41:17, Edmund.Grimley.Evans wrote:
> On 2016/09/28 19:53:57, bruening wrote:
> > As with all new API routines, please add to the changelist in release.dox.
> 
> I'll do that tomorrow...

Presumably like this:

The changes between version \DR_VERSION and 6.2.0 include the
following minor compatibility changes:

 - Changed dr_app_stop() to send all threads native instead of only the
   calling thread.
 - Replace PAGE_SIZE with dr_page_size().

[bruening, 2016-09-28 21:55:54]

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h
File core/arch/proc.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h#newcode59
core/arch/proc.h:59: #define PAGE_START(x) (((ptr_uint_t)(x)) &
~(os_page_size()-1))
bug: this is pasted straight into an exported header file for clients, and
clients have no access to internal os_* symbols.

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h#newcode59
core/arch/proc.h:59: #define PAGE_START(x) (((ptr_uint_t)(x)) &
~(os_page_size()-1))
Maybe this should be adjacent to dr_page_size in the headers

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/28 21:06:20, Edmund.Grimley.Evans wrote:
> On 2016/09/28 20:41:17, Edmund.Grimley.Evans wrote:
> > On 2016/09/28 19:53:57, bruening wrote:
> > > As with all new API routines, please add to the changelist in release.dox.
> > 
> > I'll do that tomorrow...
> 
> Presumably like this:
> 
> The changes between version \DR_VERSION and 6.2.0 include the
> following minor compatibility changes:
> 
>  - Changed dr_app_stop() to send all threads native instead of only the
>    calling thread.
>  - Replace PAGE_SIZE with dr_page_size().

Hmm, this is going to be a bigger deal in compatibility breakage: many clients
are going to fail to build which will be a pain (quick glance shows 90 hits in
DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE that calls
dr_page_size().

[Edmund.Grimley.Evans, 2016-09-28 22:38:53]

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h
File core/arch/proc.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h#newcode59
core/arch/proc.h:59: #define PAGE_START(x) (((ptr_uint_t)(x)) &
~(os_page_size()-1))
On 2016/09/28 21:55:54, bruening wrote:
> bug: this is pasted straight into an exported header file for clients, and
> clients have no access to internal os_* symbols.

OK, I'll change it to dr_page_size and move it.

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/28 21:55:54, bruening wrote:
> >  - Replace PAGE_SIZE with dr_page_size().
> 
> Hmm, this is going to be a bigger deal in compatibility breakage: many clients
> are going to fail to build which will be a pain (quick glance shows 90 hits in
> DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE that
calls
> dr_page_size().

I would have thought not because:

* PAGE_SIZE is a commonly used name. If DynamoRIO exports an idiosyncratic
definition of PAGE_SIZE it may cause confusion when users find they are also
getting a different version of PAGE_SIZE from some other header file that has
nothing to do with DynamoRIO.

* If people want to avoid a big diff they can probably just insert "#define
PAGE_SIZE dr_page_size()" for themselves in an appropriate place in their client
code.

* People will in any case have to make changes where PAGE_SIZE appears in a
constant expression.

* Even if a simple substitution of dr_page_size() for PAGE_SIZE works it would
be better for people to briefly consider each instance because in some cases a
naive substitution could have a bad effect on performance.

[Edmund.Grimley.Evans, 2016-09-29 09:44:54]

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h
File core/arch/proc.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h#newcode59
core/arch/proc.h:59: #define PAGE_START(x) (((ptr_uint_t)(x)) &
~(os_page_size()-1))
> OK, I'll change it to dr_page_size and move it.

On second thoughts, I don't think PAGE_START was or should be in the API. I'll
put it next to os_page_size in os_shared.h.

[Edmund.Grimley.Evans, 2016-09-29 09:52:20]

Commit log for latest patchset:
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as os_page_size()
in globals.h for DynamoRIO's internal use.

Modify non-core code to use the new API.

Currently the value returned by os_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.

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

[bruening, 2016-09-29 15:47:19]

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h
File core/arch/proc.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h#newcode59
core/arch/proc.h:59: #define PAGE_START(x) (((ptr_uint_t)(x)) &
~(os_page_size()-1))
On 2016/09/29 09:44:54, Edmund.Grimley.Evans wrote:
> > OK, I'll change it to dr_page_size and move it.
> 
> On second thoughts, I don't think PAGE_START was or should be in the API. I'll
> put it next to os_page_size in os_shared.h.

It was, as you can see: it is inside a BEGIN block and has a doxygen doc comment
(though < is incorrect...).  Removing it will require a changelog entry and
another compatibility break.

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/28 22:38:53, Edmund.Grimley.Evans wrote:
> On 2016/09/28 21:55:54, bruening wrote:
> > >  - Replace PAGE_SIZE with dr_page_size().
> > 
> > Hmm, this is going to be a bigger deal in compatibility breakage: many
clients
> > are going to fail to build which will be a pain (quick glance shows 90 hits
in
> > DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE that
> calls
> > dr_page_size().
> 
> I would have thought not because:
> 
> * PAGE_SIZE is a commonly used name. If DynamoRIO exports an idiosyncratic
> definition of PAGE_SIZE it may cause confusion when users find they are also
> getting a different version of PAGE_SIZE from some other header file that has
> nothing to do with DynamoRIO.
> 
> * If people want to avoid a big diff they can probably just insert "#define
> PAGE_SIZE dr_page_size()" for themselves in an appropriate place in their
client
> code.
> 
> * People will in any case have to make changes where PAGE_SIZE appears in a
> constant expression.
> 
> * Even if a simple substitution of dr_page_size() for PAGE_SIZE works it would
> be better for people to briefly consider each instance because in some cases a
> naive substitution could have a bad effect on performance.

Sure, there are longer-term benefits, but they have to be balanced with user
pain.  It's a tradeoff.  People's client builds breaking will be a pain and
people will complain and not be happy about it (including me on the next update
for DrMemory).  We generally try hard to maintain compatibility.  It would feel
less bad if it accompanied a major version bump to 7.0.  If that's the route we
take, please consider any other A64 or otherwise changes that might break
compatibility, and let's get them in before this 7.0 release.  This should prob
go on the github release note page as well.  Maybe the next release should have
a "#ifndef PAGE_SIZE #define PAGE_SIZE removed_from_API_please_use_dr_page_size"
which is then removed in the subsequent release.

[bruening, 2016-09-29 16:02:23]

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/29 15:47:19, bruening wrote:
> On 2016/09/28 22:38:53, Edmund.Grimley.Evans wrote:
> > On 2016/09/28 21:55:54, bruening wrote:
> > > >  - Replace PAGE_SIZE with dr_page_size().
> > > 
> > > Hmm, this is going to be a bigger deal in compatibility breakage: many
> clients
> > > are going to fail to build which will be a pain (quick glance shows 90
hits
> in
> > > DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE that
> > calls
> > > dr_page_size().
> > 
> > I would have thought not because:
> > 
> > * PAGE_SIZE is a commonly used name. If DynamoRIO exports an idiosyncratic
> > definition of PAGE_SIZE it may cause confusion when users find they are also
> > getting a different version of PAGE_SIZE from some other header file that
has
> > nothing to do with DynamoRIO.
> > 
> > * If people want to avoid a big diff they can probably just insert "#define
> > PAGE_SIZE dr_page_size()" for themselves in an appropriate place in their
> client
> > code.
> > 
> > * People will in any case have to make changes where PAGE_SIZE appears in a
> > constant expression.
> > 
> > * Even if a simple substitution of dr_page_size() for PAGE_SIZE works it
would
> > be better for people to briefly consider each instance because in some cases
a
> > naive substitution could have a bad effect on performance.
> 
> Sure, there are longer-term benefits, but they have to be balanced with user
> pain.  It's a tradeoff.  People's client builds breaking will be a pain and
> people will complain and not be happy about it (including me on the next
update
> for DrMemory).  We generally try hard to maintain compatibility.  It would
feel
> less bad if it accompanied a major version bump to 7.0.  If that's the route
we
> take, please consider any other A64 or otherwise changes that might break
> compatibility, and let's get them in before this 7.0 release.  This should
prob
> go on the github release note page as well.  Maybe the next release should
have
> a "#ifndef PAGE_SIZE #define PAGE_SIZE
removed_from_API_please_use_dr_page_size"
> which is then removed in the subsequent release.

Please note that the existing compat change, dr_app_stop(), was explicitly
labeled as being subject to change in the future in its docs, which is why it is
less of a concern.

[Edmund.Grimley.Evans, 2016-09-29 16:08:46]

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h
File core/arch/proc.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/arch/proc.h#newcode59
core/arch/proc.h:59: #define PAGE_START(x) (((ptr_uint_t)(x)) &
~(os_page_size()-1))
On 2016/09/29 15:47:19, bruening wrote:
> On 2016/09/29 09:44:54, Edmund.Grimley.Evans wrote:
> > > OK, I'll change it to dr_page_size and move it.
> > 
> > On second thoughts, I don't think PAGE_START was or should be in the API.
I'll
> > put it next to os_page_size in os_shared.h.
> 
> It was, as you can see: it is inside a BEGIN block and has a doxygen doc
comment
> (though < is incorrect...).  Removing it will require a changelog entry and
> another compatibility break.

Yes, I modified release.dox in the latest patchset. I would have thought that
removing PAGE_START is probably the right thing to do if PAGE_SIZE is being
removed.

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/29 15:47:19, bruening wrote:
> Sure, there are longer-term benefits, but they have to be balanced with user
> pain.  It's a tradeoff.  People's client builds breaking will be a pain and
> people will complain and not be happy about it (including me on the next
update
> for DrMemory).  We generally try hard to maintain compatibility.  It would
feel
> less bad if it accompanied a major version bump to 7.0.  If that's the route
we
> take, please consider any other A64 or otherwise changes that might break
> compatibility, and let's get them in before this 7.0 release.

I can't at the moment think of any...

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

So, is the only remaining question here what to do about PAGE_SIZE and
PAGE_START in the API?

[bruening, 2016-10-03 01:17:13]

Let's get Qin's opinion.

We may want to have the next release be 7.0 anyway to reflect the big
feature of A64 support?

On Sat, Oct 1, 2016 at 4:52 AM, <Edmund.Grimley.Evans@gmail.com> wrote:

> So, is the only remaining question here what to do about PAGE_SIZE and
> PAGE_START in the API?
>
> https://codereview.appspot.com/309340043/
>

[zhaoqin, 2016-10-03 04:09:59]

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/09/29 15:47:19, bruening wrote:
> On 2016/09/28 22:38:53, Edmund.Grimley.Evans wrote:
> > On 2016/09/28 21:55:54, bruening wrote:
> > > >  - Replace PAGE_SIZE with dr_page_size().
> > > 
> > > Hmm, this is going to be a bigger deal in compatibility breakage: many
> clients
> > > are going to fail to build which will be a pain (quick glance shows 90
hits
> in
> > > DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE that
> > calls
> > > dr_page_size().
> > 
> > I would have thought not because:
> > 
> > * PAGE_SIZE is a commonly used name. If DynamoRIO exports an idiosyncratic
> > definition of PAGE_SIZE it may cause confusion when users find they are also
> > getting a different version of PAGE_SIZE from some other header file that
has
> > nothing to do with DynamoRIO.
> > 
> > * If people want to avoid a big diff they can probably just insert "#define
> > PAGE_SIZE dr_page_size()" for themselves in an appropriate place in their
> client
> > code.
> > 
> > * People will in any case have to make changes where PAGE_SIZE appears in a
> > constant expression.
> > 
> > * Even if a simple substitution of dr_page_size() for PAGE_SIZE works it
would
> > be better for people to briefly consider each instance because in some cases
a
> > naive substitution could have a bad effect on performance.
> 
> Sure, there are longer-term benefits, but they have to be balanced with user
> pain.  It's a tradeoff.  People's client builds breaking will be a pain and
> people will complain and not be happy about it (including me on the next
update
> for DrMemory).  We generally try hard to maintain compatibility.  It would
feel
> less bad if it accompanied a major version bump to 7.0.  If that's the route
we
> take, please consider any other A64 or otherwise changes that might break
> compatibility, and let's get them in before this 7.0 release.  This should
prob
> go on the github release note page as well.  Maybe the next release should
have
> a "#ifndef PAGE_SIZE #define PAGE_SIZE
removed_from_API_please_use_dr_page_size"
> which is then removed in the subsequent release.

I see reasonable from both side, could we comprise in the middle?
For example, have an macro something like DR_COMPATIBLE_PAGE_SIZE.
If it is defined, PAGE_SIZE is defined as dr_page_size(), and not otherwise.
So if a client want to use new DR without changing their own code, they have to
add -DDR_COMPATIBLE_PAGE_SIZE, otherwise, have to change the code.

[zhaoqin, 2016-10-03 04:10:48]

On 2016/10/03 04:09:59, zhaoqin wrote:
> https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
> File core/lib/instrument_api.h (right):
> 
>
https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
> core/lib/instrument_api.h:2189: dr_page_size(void);
> On 2016/09/29 15:47:19, bruening wrote:
> > On 2016/09/28 22:38:53, Edmund.Grimley.Evans wrote:
> > > On 2016/09/28 21:55:54, bruening wrote:
> > > > >  - Replace PAGE_SIZE with dr_page_size().
> > > > 
> > > > Hmm, this is going to be a bigger deal in compatibility breakage: many
> > clients
> > > > are going to fail to build which will be a pain (quick glance shows 90
> hits
> > in
> > > > DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE
that
> > > calls
> > > > dr_page_size().
> > > 
> > > I would have thought not because:
> > > 
> > > * PAGE_SIZE is a commonly used name. If DynamoRIO exports an idiosyncratic
> > > definition of PAGE_SIZE it may cause confusion when users find they are
also
> > > getting a different version of PAGE_SIZE from some other header file that
> has
> > > nothing to do with DynamoRIO.
> > > 
> > > * If people want to avoid a big diff they can probably just insert
"#define
> > > PAGE_SIZE dr_page_size()" for themselves in an appropriate place in their
> > client
> > > code.
> > > 
> > > * People will in any case have to make changes where PAGE_SIZE appears in
a
> > > constant expression.
> > > 
> > > * Even if a simple substitution of dr_page_size() for PAGE_SIZE works it
> would
> > > be better for people to briefly consider each instance because in some
cases
> a
> > > naive substitution could have a bad effect on performance.
> > 
> > Sure, there are longer-term benefits, but they have to be balanced with user
> > pain.  It's a tradeoff.  People's client builds breaking will be a pain and
> > people will complain and not be happy about it (including me on the next
> update
> > for DrMemory).  We generally try hard to maintain compatibility.  It would
> feel
> > less bad if it accompanied a major version bump to 7.0.  If that's the route
> we
> > take, please consider any other A64 or otherwise changes that might break
> > compatibility, and let's get them in before this 7.0 release.  This should
> prob
> > go on the github release note page as well.  Maybe the next release should
> have
> > a "#ifndef PAGE_SIZE #define PAGE_SIZE
> removed_from_API_please_use_dr_page_size"
> > which is then removed in the subsequent release.
> 
> I see reasonable from both side, could we comprise in the middle?
> For example, have an macro something like DR_COMPATIBLE_PAGE_SIZE.
> If it is defined, PAGE_SIZE is defined as dr_page_size(), and not otherwise.
> So if a client want to use new DR without changing their own code, they have
to
> add -DDR_COMPATIBLE_PAGE_SIZE, otherwise, have to change the code.

I mean compromise.

[bruening, 2016-10-03 04:33:49]

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/40001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2189: dr_page_size(void);
On 2016/10/03 04:09:59, zhaoqin wrote:
> On 2016/09/29 15:47:19, bruening wrote:
> > On 2016/09/28 22:38:53, Edmund.Grimley.Evans wrote:
> > > On 2016/09/28 21:55:54, bruening wrote:
> > > > >  - Replace PAGE_SIZE with dr_page_size().
> > > > 
> > > > Hmm, this is going to be a bigger deal in compatibility breakage: many
> > clients
> > > > are going to fail to build which will be a pain (quick glance shows 90
> hits
> > in
> > > > DrMemory alone).  Maybe it's better to provide a #define of PAGE_SIZE
that
> > > calls
> > > > dr_page_size().
> > > 
> > > I would have thought not because:
> > > 
> > > * PAGE_SIZE is a commonly used name. If DynamoRIO exports an idiosyncratic
> > > definition of PAGE_SIZE it may cause confusion when users find they are
also
> > > getting a different version of PAGE_SIZE from some other header file that
> has
> > > nothing to do with DynamoRIO.
> > > 
> > > * If people want to avoid a big diff they can probably just insert
"#define
> > > PAGE_SIZE dr_page_size()" for themselves in an appropriate place in their
> > client
> > > code.
> > > 
> > > * People will in any case have to make changes where PAGE_SIZE appears in
a
> > > constant expression.
> > > 
> > > * Even if a simple substitution of dr_page_size() for PAGE_SIZE works it
> would
> > > be better for people to briefly consider each instance because in some
cases
> a
> > > naive substitution could have a bad effect on performance.
> > 
> > Sure, there are longer-term benefits, but they have to be balanced with user
> > pain.  It's a tradeoff.  People's client builds breaking will be a pain and
> > people will complain and not be happy about it (including me on the next
> update
> > for DrMemory).  We generally try hard to maintain compatibility.  It would
> feel
> > less bad if it accompanied a major version bump to 7.0.  If that's the route
> we
> > take, please consider any other A64 or otherwise changes that might break
> > compatibility, and let's get them in before this 7.0 release.  This should
> prob
> > go on the github release note page as well.  Maybe the next release should
> have
> > a "#ifndef PAGE_SIZE #define PAGE_SIZE
> removed_from_API_please_use_dr_page_size"
> > which is then removed in the subsequent release.
> 
> I see reasonable from both side, could we comprise in the middle?
> For example, have an macro something like DR_COMPATIBLE_PAGE_SIZE.
> If it is defined, PAGE_SIZE is defined as dr_page_size(), and not otherwise.
> So if a client want to use new DR without changing their own code, they have
to
> add -DDR_COMPATIBLE_PAGE_SIZE, otherwise, have to change the code.

Something like how we handled DynamoRIO_REG_COMPATIBILITY?

[Edmund.Grimley.Evans, 2016-10-03 13:41:10]

Commit log for latest patchset:
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as os_page_size()
for DynamoRIO's internal use, and, if DynamoRIO_PAGE_SIZE_COMPATIBILITY
is set, we also define PAGE_SIZE as dr_page_size() for use by clients.

Similarly, PAGE_START is removed from the API, though if
DynamoRIO_PAGE_SIZE_COMPATIBILITY is set a definition in terms of
dr_page_size() is provided.

Modify non-core code to use the new API.

Currently the value returned by os_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.

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

[bruening, 2016-10-04 05:17:53]

I think we should auto-set DynamoRIO_PAGE_SIZE_COMPATIBILITY if the client
targets version 6.2 or earlier, just like we did with the reg enum (and mention
in release note too).

https://codereview.appspot.com/309340043/diff/80001/api/docs/release.dox
File api/docs/release.dox (right):

https://codereview.appspot.com/309340043/diff/80001/api/docs/release.dox#newc...
api/docs/release.dox:141: - Replace PAGE_SIZE with dr_page_size() and remove
PAGE_START from the API.
Inconsistent tense 2x: notice how all of the other entries are past tense. 
Should be Replaced, removed.

https://codereview.appspot.com/309340043/diff/80001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/80001/core/lib/instrument_api.h...
core/lib/instrument_api.h:2191: # define PAGE_SIZE dr_page_size()
This is intended to have a doxygen comment?

https://codereview.appspot.com/309340043/diff/80001/core/os_shared.h
File core/os_shared.h (right):

https://codereview.appspot.com/309340043/diff/80001/core/os_shared.h#newcode503
core/os_shared.h:503: /**
This should not be a doxygen comment.

[Edmund.Grimley.Evans, 2016-10-04 09:07:55]

Commit log for latest patchset:
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as os_page_size()
for DynamoRIO's internal use, and, if DynamoRIO_PAGE_SIZE_COMPATIBILITY
is set, we also define PAGE_SIZE as dr_page_size() for use by clients.

Similarly, PAGE_START is removed from the API, though if
DynamoRIO_PAGE_SIZE_COMPATIBILITY is set a definition in terms of
dr_page_size() is provided.

Modify non-core code to use the new API.

Currently the value returned by os_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.

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

[bruening, 2016-10-04 15:02:31]

Please mention the auto-set in the commit msg.

Contains a doxygen error -- see comment.

I assume the version-based auto-set was manually tested.

Otherwise LGTM

https://codereview.appspot.com/309340043/diff/100001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/100001/core/lib/instrument_api....
core/lib/instrument_api.h:2191: # undef PAGE_SIZE
This comment is on the undef -- I don't think it's going to work.  What does it
look like in the docs?  I would think it has to go on the define.  Please check
the resulting html.

[bruening, 2016-10-05 03:12:15]

https://codereview.appspot.com/309340043/diff/100001/make/DynamoRIOConfig.cma...
File make/DynamoRIOConfig.cmake.in (right):

https://codereview.appspot.com/309340043/diff/100001/make/DynamoRIOConfig.cma...
make/DynamoRIOConfig.cmake.in:155: #  set(DynamoRIO_REG_COMPATIBILITY ON)
Please document the new setting up here, just like this one.

Best to look at the CL that added this and see if there's anything else.

[Edmund.Grimley.Evans, 2016-10-05 10:04:43]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/326a0d5d0c79f6eb9f70ccee85b3572...

Final commit log: 
---------------
i#1680 large pages: In the API, replace PAGE_SIZE with dr_page_size().

When the page size is not a compile-time constant it is misleading to
provide the traditional PAGE_SIZE macro, so we provide a function
instead, though we provisionally define PAGE_SIZE as os_page_size()
for DynamoRIO's internal use, and, if DynamoRIO_PAGE_SIZE_COMPATIBILITY
is set, we also define PAGE_SIZE as dr_page_size() for use by clients.

Similarly, PAGE_START is removed from the API, though if
DynamoRIO_PAGE_SIZE_COMPATIBILITY is set a definition in terms of
dr_page_size() is provided.

DynamoRIO_PAGE_SIZE_COMPATIBILITY will be set automatically if the
client targets version 6.2 or earlier.

Modify non-core code to use the new API.

Currently the value returned by os_page_size() is always 4096. In future
on AArch64 it will sometimes be 16384 or 65536.

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

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

https://codereview.appspot.com/309340043/diff/100001/core/lib/instrument_api.h
File core/lib/instrument_api.h (right):

https://codereview.appspot.com/309340043/diff/100001/core/lib/instrument_api....
core/lib/instrument_api.h:2191: # undef PAGE_SIZE
On 2016/10/04 15:02:31, bruening wrote:
> This comment is on the undef -- I don't think it's going to work.  What does
it
> look like in the docs?  I would think it has to go on the define.  Please
check
> the resulting html.

It does work like that - provided API.doxy is modified to mention
DR_PAGE_SIZE_COMPATIBILITY - but I'll change it anyway.

[bruening, 2016-10-05 19:33:21]

The Windows build is still broken (after the 2 bug fix CL's that followed this
CL): several tests don't compile.  I'm trying to commit my own code and this
breakage is blocking me.

[Edmund.Grimley.Evans, 2016-10-05 19:40:42]

On 2016/10/05 19:33:21, bruening wrote:
> The Windows build is still broken (after the 2 bug fix CL's that followed this
> CL): several tests don't compile.  I'm trying to commit my own code and this
> breakage is blocking me.

What's broken? Is it visible on
https://build.chromium.org/p/client.dynamorio/one_line_per_build?

[bruening, 2016-10-05 20:15:00]

On Wed, Oct 5, 2016 at 3:40 PM, <Edmund.Grimley.Evans@gmail.com> wrote:

> On 2016/10/05 19:33:21, bruening wrote:
>
>> The Windows build is still broken (after the 2 bug fix CL's that
>>
> followed this
>
>> CL): several tests don't compile.  I'm trying to commit my own code
>>
> and this
>
>> breakage is blocking me.
>>
>
> What's broken?


At least 3 tests fail to build.

debug-internal-32: **** 6 build errors ****
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39) :
error C2057: expected constant expression
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39) :
error C2466: cannot allocate an array of constant size 0
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39) :
error C2133: 'buf2' : unknown size
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82) :
error C2057: expected constant expression
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82) :
error C2466: cannot allocate an array of constant size 0
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82) :
error C2133: 'buf2_stack' : unknown size
debug-internal-64: **** 8 build errors ****
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39) :
error C2057: expected constant expression
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39) :
error C2466: cannot allocate an array of constant size 0
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39) :
error C2133: 'buf2' : unknown size
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82) :
error C2057: expected constant expression
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82) :
error C2466: cannot allocate an array of constant size 0
        D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82) :
error C2133: 'buf2_stack' : unknown size
        D:\derek\dr\git\src\suite\tests\client-interface\segfault.c(140) :
error C2220: warning treated as error - no 'object' file generated

D:\derek\dr\git\src\suite\tests\security-common\TestMemProtChg.c(97) :
error C2220: warning treated as error - no 'object' file generated

That is fixable, but beyond that there is a test failure whose reason is
not obvious.  This test has never been flaky and its results here are
deterministic: by trying builds before and after (have to include all the
Windows fixes) it is repeatedly failing with the PAGE_SIZE changes and
working at the CL right before:

50: Test command:
D:\derek\dr\build_suite\build_debug-internal-32\bin32\drrun.exe "-s" "90"
"-quiet" "-debug" "-use_dll"
"D:/derek/dr/build_suite/build_debug-internal-32/lib32/debug/dynamorio.dll"
"-exit0" "-stderr_mask" "0xC" "-msgbox_mask" "0" "-dumpcore_mask" "0x7d"
"-staged" "-code_api" "--"
"D:/derek/dr/build_suite/build_debug-internal-32/suite/tests/bin/security-win32.aslr-ind.exe"
50: Test timeout computed to be: 1500
50: aslr-ind main()
50: ready to go 332
50: 16
50: at base, no ASLR
50: 16
50: *** invalid indirect call at preferred base!
50: 1169229136
50: *** invalid indirect call allowed!
50: indirect call done
50: starting bad return function
50: looking at ring
50: PRECIOUS in a DLL, ATTACK SUCCESSFUL!
1/1 Test #50: code_api|security-win32.aslr-ind ...***Failed  Required
regular expression not found.Regex=[^aslr-in? main\(\)
?eady to go 332
?6
?t base, no ASLR
?6
?*\*\* invalid indirect call at preferred base!
?
?*\*\* invalid indirect call allowed!
?ndirect call done
?tarting bad return function
?ooking at ring
?RECIOUS in a DLL, ATTACK SUCCESSFUL!
$
]  0.43 sec

This is already more time than I want to spend...

Is it visible on
> https://build.chromium.org/p/client.dynamorio/one_line_per_build?
>

Probably not, the test suite bots have been broken due to Chromium infra
changes that have been too painful to try and work around.  This is one
reason I went and set up Travis CI, though it unfortunately does not have
Windows support.

[bruening, 2016-10-05 20:19:51]

I have the test fixes in a local tree, but am considering reverting as the
easiest soln to fix the test failure and let me make forward progress.

On Wed, Oct 5, 2016 at 4:14 PM, Derek Bruening <bruening@google.com> wrote:

> On Wed, Oct 5, 2016 at 3:40 PM, <Edmund.Grimley.Evans@gmail.com> wrote:
>
>> On 2016/10/05 19:33:21, bruening wrote:
>>
>>> The Windows build is still broken (after the 2 bug fix CL's that
>>>
>> followed this
>>
>>> CL): several tests don't compile.  I'm trying to commit my own code
>>>
>> and this
>>
>>> breakage is blocking me.
>>>
>>
>> What's broken?
>
>
> At least 3 tests fail to build.
>
> debug-internal-32: **** 6 build errors ****
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39)
> : error C2057: expected constant expression
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39)
> : error C2466: cannot allocate an array of constant size 0
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39)
> : error C2133: 'buf2' : unknown size
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82)
> : error C2057: expected constant expression
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82)
> : error C2466: cannot allocate an array of constant size 0
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82)
> : error C2133: 'buf2_stack' : unknown size
> debug-internal-64: **** 8 build errors ****
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39)
> : error C2057: expected constant expression
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39)
> : error C2466: cannot allocate an array of constant size 0
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(39)
> : error C2133: 'buf2' : unknown size
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82)
> : error C2057: expected constant expression
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82)
> : error C2466: cannot allocate an array of constant size 0
>         D:\derek\dr\git\src\suite\tests\security-win32\TestNTFlush.c(82)
> : error C2133: 'buf2_stack' : unknown size
>         D:\derek\dr\git\src\suite\tests\client-interface\segfault.c(140)
> : error C2220: warning treated as error - no 'object' file generated
>         D:\derek\dr\git\src\suite\tests\security-common\TestMemProtChg.c(97)
> : error C2220: warning treated as error - no 'object' file generated
>
> That is fixable, but beyond that there is a test failure whose reason is
> not obvious.  This test has never been flaky and its results here are
> deterministic: by trying builds before and after (have to include all the
> Windows fixes) it is repeatedly failing with the PAGE_SIZE changes and
> working at the CL right before:
>
> 50: Test command: D:\derek\dr\build_suite\build_
> debug-internal-32\bin32\drrun.exe "-s" "90" "-quiet" "-debug" "-use_dll"
> "D:/derek/dr/build_suite/build_debug-internal-32/lib32/debug/dynamorio.dll"
> "-exit0" "-stderr_mask" "0xC" "-msgbox_mask" "0" "-dumpcore_mask" "0x7d"
> "-staged" "-code_api" "--" "D:/derek/dr/build_suite/
> build_debug-internal-32/suite/tests/bin/security-win32.aslr-ind.exe"
> 50: Test timeout computed to be: 1500
> 50: aslr-ind main()
> 50: ready to go 332
> 50: 16
> 50: at base, no ASLR
> 50: 16
> 50: *** invalid indirect call at preferred base!
> 50: 1169229136
> 50: *** invalid indirect call allowed!
> 50: indirect call done
> 50: starting bad return function
> 50: looking at ring
> 50: PRECIOUS in a DLL, ATTACK SUCCESSFUL!
> 1/1 Test #50: code_api|security-win32.aslr-ind ...***Failed  Required
> regular expression not found.Regex=[^aslr-in? main\(\)
> ?eady to go 332
> ?6
> ?t base, no ASLR
> ?6
> ?*\*\* invalid indirect call at preferred base!
> ?
> ?*\*\* invalid indirect call allowed!
> ?ndirect call done
> ?tarting bad return function
> ?ooking at ring
> ?RECIOUS in a DLL, ATTACK SUCCESSFUL!
> $
> ]  0.43 sec
>
> This is already more time than I want to spend...
>
> Is it visible on
>> https://build.chromium.org/p/client.dynamorio/one_line_per_build?
>>
>
> Probably not, the test suite bots have been broken due to Chromium infra
> changes that have been too painful to try and work around.  This is one
> reason I went and set up Travis CI, though it unfortunately does not have
> Windows support.
>

[Edmund.Grimley.Evans, 2016-10-05 20:39:36]

On 2016/10/05 20:19:51, bruening wrote:
> I have the test fixes in a local tree, but am considering reverting as the
> easiest soln to fix the test failure and let me make forward progress.

You should probably commit the fixes anyway, even if we have to revert
everything afterwards. At least the code will all be there in the history for
anyone who wants to investigate the obscure Windows test failure.

[bruening, 2016-10-05 21:16:01]

I fixed the test, but this was not a pleasant scenario: too much build and test
breakage and affecting of other devs' time.  It doesn't seem a very workable
model to change Windows-affecting code without being able to test it locally. 
Are you sure you can't get access to a Windows test env?  Any suggestions?

[Edmund.Grimley.Evans, 2016-10-07 10:22:39]

On 2016/10/05 21:16:01, bruening wrote:
> I fixed the test, but this was not a pleasant scenario: too much build and
test
> breakage and affecting of other devs' time.  It doesn't seem a very workable
> model to change Windows-affecting code without being able to test it locally. 
> Are you sure you can't get access to a Windows test env?  Any suggestions?

Thanks for fixing it. Fortunately these API-changing commits don't come too
often, but I'll see if I can get (remote) access to a Windows system for
testing. I'll also try to gather some ideas, in a brainstorming fashion, on what
we could perhaps do to make things easier...

[bruening, 2016-10-07 16:09:43]

On 2016/10/07 10:22:39, Edmund.Grimley.Evans wrote:
> On 2016/10/05 21:16:01, bruening wrote:
> > I fixed the test, but this was not a pleasant scenario: too much build and
> test
> > breakage and affecting of other devs' time.  It doesn't seem a very workable
> > model to change Windows-affecting code without being able to test it
locally. 
> > Are you sure you can't get access to a Windows test env?  Any suggestions?
> 
> Thanks for fixing it. Fortunately these API-changing commits don't come too
> often, but I'll see if I can get (remote) access to a Windows system for
> testing. I'll also try to gather some ideas, in a brainstorming fashion, on
what
> we could perhaps do to make things easier...

Beyond just Windows, it has been getting hard to do DR work due to the number of
platforms increasing, and it may not be possible going forward for each
contributor to have access to every platform.  It is now quite easy to break the
Mac clang build due its picky warnings while succeeding in the Linux clang and
gcc builds; easy to break AArch64 tests while succeeding elsewhere; easy to
break certain types of Windows builds while local Windows builds succeed; etc. 
Travis at least gives us "trybots" for Mac -- having trybots for AArch64 tests
(not just builds) and for Windows would help but may not be easy to set up for
free.