[wasm] Change the constant kV8MaxWasmMemoryPages to a command line flag.

[wasm] Change the constant kV8MaxWasmMemoryPages to a command line flag.

The hardcoded constant caused a problem for the wasm fuzzer because
when the maximum memory was allocated in a test case, clusterfuzz ran
out of memory. with the command line flag we can set a lower limit
for the fuzzer.

The flag has the value of the constant as its default value, so that
for everything but the fuzzers nothing should change.

R=titzer@chromium.org
BUG=chromium:676888
Review-Url: https://codereview.chromium.org/2626313003
Cr-Commit-Position: refs/heads/master@{#42599}
Committed: https://chromium.googlesource.com/v8/v8/+/94266b7d86cfb7201461a9af2bfe2ec3268a70e3

[ahaas, 2017-01-13 14:36:18]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 14:36:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 14:41:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 14:41:01]

Dry run: Try jobs failed on following builders:
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[ahaas, 2017-01-13 14:49:41]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 14:49:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-01-13 14:51:50]

On 2017/01/13 at 14:36:17, ahaas wrote:
> 

Fixed a typo.

[titzer, 2017-01-13 14:54:22]

https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:502: DEFINE_INT(wasm_max_mem_pages, 16384, "maximum
memory size of a wasm instance")
You can actually define this as a uint32_t (which will save you all the casts
elsewhere), but it will parse (in flags.cc) as an int. Since we are already
doing a static_cast<int> there, we might actually already have a bug there if
someone passes a negative number or an out-of-range number. Or you could try to
introduce unsigned types to flags.cc...

[commit-bot: I haz the power, 2017-01-13 15:20:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 15:20:20]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-01-16 16:56:07]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 16:56:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 17:25:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:25:51]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-01-17 09:09:55]

https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:502: DEFINE_INT(wasm_max_mem_pages, 16384, "maximum
memory size of a wasm instance")
On 2017/01/13 at 14:54:22, titzer wrote:
> You can actually define this as a uint32_t (which will save you all the casts
elsewhere), but it will parse (in flags.cc) as an int. Since we are already
doing a static_cast<int> there, we might actually already have a bug there if
someone passes a negative number or an out-of-range number. Or you could try to
introduce unsigned types to flags.cc...

I think I did that now, but I'm not sure how flags.cc works in detail.

[titzer, 2017-01-17 09:34:01]

On 2017/01/17 09:09:55, ahaas wrote:
> https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h
> File src/flag-definitions.h (right):
> 
>
https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h#...
> src/flag-definitions.h:502: DEFINE_INT(wasm_max_mem_pages, 16384, "maximum
> memory size of a wasm instance")
> On 2017/01/13 at 14:54:22, titzer wrote:
> > You can actually define this as a uint32_t (which will save you all the
casts
> elsewhere), but it will parse (in flags.cc) as an int. Since we are already
> doing a static_cast<int> there, we might actually already have a bug there if
> someone passes a negative number or an out-of-range number. Or you could try
to
> introduce unsigned types to flags.cc...
> 
> I think I did that now, but I'm not sure how flags.cc works in detail.

It will parse it as TYPE_INT, which means it will allow signed numbers, which
will wrap around...which is kind of surprising :/ I am not sure if it is
possible to enforce boundaries on command line flags.

[ahaas, 2017-01-23 09:50:02]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 09:50:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-01-23 10:00:12]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 10:00:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-01-23 10:01:40]

On 2017/01/17 at 09:34:01, titzer wrote:
> On 2017/01/17 09:09:55, ahaas wrote:
> > https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h
> > File src/flag-definitions.h (right):
> > 
> >
https://codereview.chromium.org/2626313003/diff/20001/src/flag-definitions.h#...
> > src/flag-definitions.h:502: DEFINE_INT(wasm_max_mem_pages, 16384, "maximum
> > memory size of a wasm instance")
> > On 2017/01/13 at 14:54:22, titzer wrote:
> > > You can actually define this as a uint32_t (which will save you all the
casts
> > elsewhere), but it will parse (in flags.cc) as an int. Since we are already
> > doing a static_cast<int> there, we might actually already have a bug there
if
> > someone passes a negative number or an out-of-range number. Or you could try
to
> > introduce unsigned types to flags.cc...
> > 
> > I think I did that now, but I'm not sure how flags.cc works in detail.
> 
> It will parse it as TYPE_INT, which means it will allow signed numbers, which
will wrap around...which is kind of surprising :/ I am not sure if it is
possible to enforce boundaries on command line flags.

I added proper support for unsigned int parameters now with proper bounds
checks. Values out-of-bounds default to the default value of the parameter.

[titzer, 2017-01-23 10:20:44]

Thanks for adding the unsigned parsing type.

LGTM with comments

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc
File src/flags.cc (right):

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc#newcode74
src/flags.cc:74: int* uint_variable() const {
Should this return an unsigned int*?

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc#newcode112
src/flags.cc:112: int uint_default() const {
unsigned int?

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc#newcode434
src/flags.cc:434: "Error: illegal value for flag %s of type %s\n"
Can you add a more helpful error message? E.g. that it must be between the
bounds min and max?

[commit-bot: I haz the power, 2017-01-23 10:29:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 10:29:09]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-01-23 11:32:58]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 11:33:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-01-23 11:47:37]

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc
File src/flags.cc (right):

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc#newcode74
src/flags.cc:74: int* uint_variable() const {
On 2017/01/23 at 10:20:44, titzer wrote:
> Should this return an unsigned int*?

Yes, I guess so. Thanks.

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc#newcode112
src/flags.cc:112: int uint_default() const {
On 2017/01/23 at 10:20:44, titzer wrote:
> unsigned int?

Done.

https://codereview.chromium.org/2626313003/diff/80001/src/flags.cc#newcode434
src/flags.cc:434: "Error: illegal value for flag %s of type %s\n"
On 2017/01/23 at 10:20:44, titzer wrote:
> Can you add a more helpful error message? E.g. that it must be between the
bounds min and max?

Done.

[commit-bot: I haz the power, 2017-01-23 12:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 12:07:28]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-01-23 12:07:47]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2017-01-23 12:07:47]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2626313003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-23 12:07:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 12:09:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485173267194380,
"parent_rev": "0d1e0a15214505f017da068cdcd9ca38c9cf53e9", "commit_rev":
"94266b7d86cfb7201461a9af2bfe2ec3268a70e3"}

[commit-bot: I haz the power, 2017-01-23 12:09:56]

Description was changed from

==========
[wasm] Change the constant kV8MaxWasmMemoryPages to a command line flag.

The hardcoded constant caused a problem for the wasm fuzzer because
when the maximum memory was allocated in a test case, clusterfuzz ran
out of memory. with the command line flag we can set a lower limit
for the fuzzer.

The flag has the value of the constant as its default value, so that
for everything but the fuzzers nothing should change.

R=titzer@chromium.org
BUG=chromium:676888
==========

to

==========
[wasm] Change the constant kV8MaxWasmMemoryPages to a command line flag.

The hardcoded constant caused a problem for the wasm fuzzer because
when the maximum memory was allocated in a test case, clusterfuzz ran
out of memory. with the command line flag we can set a lower limit
for the fuzzer.

The flag has the value of the constant as its default value, so that
for everything but the fuzzers nothing should change.

R=titzer@chromium.org
BUG=chromium:676888

Review-Url: https://codereview.chromium.org/2626313003
Cr-Commit-Position: refs/heads/master@{#42599}
Committed:
https://chromium.googlesource.com/v8/v8/+/94266b7d86cfb7201461a9af2bfe2ec3268...
==========

[commit-bot: I haz the power, 2017-01-23 12:09:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/94266b7d86cfb7201461a9af2bfe2ec3268...