[wasm] calculate wasm mem size base explicitly

[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned implicit conversions
when re-computing wasm mem sizes.

BUG=

Committed: https://crrev.com/a1784e87cd73ce6c19570c3d3c0e282a9fd1b739
Cr-Commit-Position: refs/heads/master@{#39509}

[Mircea Trofin, 2016-09-16 23:18:33]

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

[commit-bot: I haz the power, 2016-09-16 23:18:35]

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

[Mircea Trofin, 2016-09-16 23:35:14]

Description was changed from

==========
[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned conversions
when re-computing wasm mem sizes.

BUG=
==========

to

==========
[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned implicit conversions
when re-computing wasm mem sizes.

BUG=
==========

[Mircea Trofin, 2016-09-16 23:35:15]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org, gdeepti@chromium.org

[commit-bot: I haz the power, 2016-09-16 23:49:02]

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

[commit-bot: I haz the power, 2016-09-16 23:49:03]

Dry run: This issue passed the CQ dry run.

[bradnelson, 2016-09-17 00:03:11]

https://codereview.chromium.org/2349053002/diff/1/src/assembler.cc
File src/assembler.cc (left):

https://codereview.chromium.org/2349053002/diff/1/src/assembler.cc#oldcode361
src/assembler.cc:361: DCHECK(old_size == 0 || wasm_memory_size_reference() <=
old_size);
What happened to the old_size == 0 case?

[Mircea Trofin, 2016-09-17 00:06:15]

On 2016/09/17 00:03:11, bradnelson wrote:
> https://codereview.chromium.org/2349053002/diff/1/src/assembler.cc
> File src/assembler.cc (left):
> 
> https://codereview.chromium.org/2349053002/diff/1/src/assembler.cc#oldcode361
> src/assembler.cc:361: DCHECK(old_size == 0 || wasm_memory_size_reference() <=
> old_size);
> What happened to the old_size == 0 case?

If old_size==0, we can't even have size references. I chatted back and forth
with Deepti, 
we landed on "leave it", but then I wanted to see if anything would fail if I
pulled it out.

I'll let Deepti also chime in.

[gdeepti, 2016-09-17 00:15:14]

I'd prefer that we handle the old_size == 0 for the same reasons as I did when
we discussed it offline, namely that the old_size = 0 case still has edge cases
that need to be handled correctly, and it would be good to have this in till it
is feature complete and the test holes are fixed.

[Mircea Trofin, 2016-09-17 00:23:27]

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

[commit-bot: I haz the power, 2016-09-17 00:23:36]

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, 2016-09-17 00:50:30]

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

[commit-bot: I haz the power, 2016-09-17 00:50:31]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-09-17 01:25:58]

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc#newcod...
src/assembler.cc:364: DCHECK_GE(new_size, offset);
I'm confused by this check, could you explain why this shouldn't this be
DCHECK_GE(new_size, updated_size_reference) after updated_size reference is
calculated?

[Mircea Trofin, 2016-09-17 01:40:03]

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc#newcod...
src/assembler.cc:364: DCHECK_GE(new_size, offset);
On 2016/09/17 01:25:58, gdeepti wrote:
> I'm confused by this check, could you explain why this shouldn't this be
> DCHECK_GE(new_size, updated_size_reference) after updated_size reference is
> calculated? 

If new_size < offset, then the calculation of updated_size_reference would
suffer from the same problem we were trying to address - not relying on implicit
conversion semantics.

This way, if our expectations are passed (new_size >= offset), then
updated_size_reference must be >= 0, and also, from how updated_size_reference
is calculated, it's <= new_size.

[gdeepti, 2016-09-17 02:24:20]

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc#newcod...
src/assembler.cc:364: DCHECK_GE(new_size, offset);
On 2016/09/17 01:40:03, Mircea Trofin wrote:
> On 2016/09/17 01:25:58, gdeepti wrote:
> > I'm confused by this check, could you explain why this shouldn't this be
> > DCHECK_GE(new_size, updated_size_reference) after updated_size reference is
> > calculated? 
> 
> If new_size < offset, then the calculation of updated_size_reference would
> suffer from the same problem we were trying to address - not relying on
implicit
> conversion semantics.
> 
> This way, if our expectations are passed (new_size >= offset), then
> updated_size_reference must be >= 0, and also, from how updated_size_reference
> is calculated, it's <= new_size.

Offset in this case is the offset into memory relative to old size. Offset will
always be less than old size as there is already a DCHECK for
current_size_reference <= old_size and offset = old_size -
current_size_reference.  (If greater, it should be handled correctly in the
compiler with a bounds check). We do not rely on implicit conversion semantics
as this change calculates offset as  old_size - current_size_reference. For
grow_memory this will always pass as current_size_reference < old size <
new_size. My assumption was that these checks were being hardened to add the
appropriate checks for when new_size < old_size. Depending on the offset, it
will fail for new_size < old_size. I'm confused about the exact use case here -
will the offset be adjusted ahead of time so unchecked_update_wasm_memory_size
will only be called for offset < new_size?

[titzer, 2016-09-19 11:56:04]

On 2016/09/17 02:24:20, gdeepti wrote:
> https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc
> File src/assembler.cc (right):
> 
>
https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc#newcod...
> src/assembler.cc:364: DCHECK_GE(new_size, offset);
> On 2016/09/17 01:40:03, Mircea Trofin wrote:
> > On 2016/09/17 01:25:58, gdeepti wrote:
> > > I'm confused by this check, could you explain why this shouldn't this be
> > > DCHECK_GE(new_size, updated_size_reference) after updated_size reference
is
> > > calculated? 
> > 
> > If new_size < offset, then the calculation of updated_size_reference would
> > suffer from the same problem we were trying to address - not relying on
> implicit
> > conversion semantics.
> > 
> > This way, if our expectations are passed (new_size >= offset), then
> > updated_size_reference must be >= 0, and also, from how
updated_size_reference
> > is calculated, it's <= new_size.
> 
> Offset in this case is the offset into memory relative to old size. Offset
will
> always be less than old size as there is already a DCHECK for
> current_size_reference <= old_size and offset = old_size -
> current_size_reference.  (If greater, it should be handled correctly in the
> compiler with a bounds check). We do not rely on implicit conversion semantics
> as this change calculates offset as  old_size - current_size_reference. For
> grow_memory this will always pass as current_size_reference < old size <
> new_size. My assumption was that these checks were being hardened to add the
> appropriate checks for when new_size < old_size. Depending on the offset, it
> will fail for new_size < old_size. I'm confused about the exact use case here
-
> will the offset be adjusted ahead of time so unchecked_update_wasm_memory_size
> will only be called for offset < new_size?

lgtm.

I don't think this change conflicts with 0xC.

[Mircea Trofin, 2016-09-19 18:15:39]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-09-19 18:15:49]

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2349053002/diff/20001/src/assembler.cc#newcod...
src/assembler.cc:364: DCHECK_GE(new_size, offset);
On 2016/09/17 02:24:20, gdeepti wrote:
> On 2016/09/17 01:40:03, Mircea Trofin wrote:
> > On 2016/09/17 01:25:58, gdeepti wrote:
> > > I'm confused by this check, could you explain why this shouldn't this be
> > > DCHECK_GE(new_size, updated_size_reference) after updated_size reference
is
> > > calculated? 
> > 
> > If new_size < offset, then the calculation of updated_size_reference would
> > suffer from the same problem we were trying to address - not relying on
> implicit
> > conversion semantics.
> > 
> > This way, if our expectations are passed (new_size >= offset), then
> > updated_size_reference must be >= 0, and also, from how
updated_size_reference
> > is calculated, it's <= new_size.
> 
> Offset in this case is the offset into memory relative to old size. Offset
will
> always be less than old size as there is already a DCHECK for
> current_size_reference <= old_size and offset = old_size -
> current_size_reference.  (If greater, it should be handled correctly in the
> compiler with a bounds check). We do not rely on implicit conversion semantics
> as this change calculates offset as  old_size - current_size_reference. For
> grow_memory this will always pass as current_size_reference < old size <
> new_size. My assumption was that these checks were being hardened to add the
> appropriate checks for when new_size < old_size. Depending on the offset, it
> will fail for new_size < old_size. I'm confused about the exact use case here
-
> will the offset be adjusted ahead of time so unchecked_update_wasm_memory_size
> will only be called for offset < new_size?

It's just checking locally the assumption about the relation between new_size
and offset, validating that updated_size check is what we'd expect.

[commit-bot: I haz the power, 2016-09-19 18:15:50]

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, 2016-09-19 18:43:47]

Description was changed from

==========
[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned implicit conversions
when re-computing wasm mem sizes.

BUG=
==========

to

==========
[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned implicit conversions
when re-computing wasm mem sizes.

BUG=
==========

[commit-bot: I haz the power, 2016-09-19 18:43:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-19 21:01:35]

Description was changed from

==========
[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned implicit conversions
when re-computing wasm mem sizes.

BUG=
==========

to

==========
[wasm] calculate wasm mem size base explicitly

This CL avoids relying on signed/unsigned implicit conversions
when re-computing wasm mem sizes.

BUG=

Committed: https://crrev.com/a1784e87cd73ce6c19570c3d3c0e282a9fd1b739
Cr-Commit-Position: refs/heads/master@{#39509}
==========

[commit-bot: I haz the power, 2016-09-19 21:01:36]

Patchset 2 (id:??) landed as
https://crrev.com/a1784e87cd73ce6c19570c3d3c0e282a9fd1b739
Cr-Commit-Position: refs/heads/master@{#39509}