[wasm] calculate wasm mem size base explicitly

src/assembler.cc

Index: src/assembler.cc
diff --git a/src/assembler.cc b/src/assembler.cc
index e97b880ebc38dfabc49c87850047f82569e3be3a..feeef0c5729c380b784bd500dae810bf4e827515 100644
--- a/src/assembler.cc
+++ b/src/assembler.cc
@@ -351,17 +351,18 @@ void RelocInfo::update_wasm_memory_reference(
   DCHECK(IsWasmMemoryReference(rmode_) || IsWasmMemorySizeReference(rmode_));
   if (IsWasmMemoryReference(rmode_)) {
     Address updated_reference;
+    DCHECK_GE(wasm_memory_reference(), old_base);
     updated_reference = new_base + (wasm_memory_reference() - old_base);
     // The reference is not checked here but at runtime. Validity of references
     // may change over time.
     unchecked_update_wasm_memory_reference(updated_reference,
                                            icache_flush_mode);
   } else if (IsWasmMemorySizeReference(rmode_)) {
-    uint32_t updated_size_reference;
-    DCHECK(old_size == 0 || wasm_memory_size_reference() <= old_size);
-    updated_size_reference =
-        new_size + (wasm_memory_size_reference() - old_size);
-    DCHECK(updated_size_reference <= new_size);
+    uint32_t current_size_reference = wasm_memory_size_reference();
+    DCHECK(old_size == 0 || current_size_reference <= old_size);
+    uint32_t offset = old_size - current_size_reference;
+    DCHECK_GE(new_size, offset);

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

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]

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]

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?

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

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.

+    uint32_t updated_size_reference = new_size - offset;
     unchecked_update_wasm_memory_size(updated_size_reference,
                                       icache_flush_mode);
   } else {