[wasm] calculate wasm mem size base explicitly

src/assembler.cc

 // Copyright (c) 1994-2006 Sun Microsystems Inc.
 // All Rights Reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 // - Redistributions of source code must retain the above copyright notice,
 // this list of conditions and the following disclaimer.
 //
@@ skipping 333 matching lines @@
 
 const int kCodeWithIdTag = 0;
 const int kDeoptReasonTag = 1;
 
 void RelocInfo::update_wasm_memory_reference(
     Address old_base, Address new_base, uint32_t old_size, uint32_t new_size,
     ICacheFlushMode icache_flush_mode) {
   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 {
     UNREACHABLE();
   }
   if (icache_flush_mode != SKIP_ICACHE_FLUSH) {
     Assembler::FlushICache(isolate_, pc_, sizeof(int64_t));
   }
 }
 
@@ skipping 1582 matching lines @@
 
 
 void Assembler::DataAlign(int m) {
   DCHECK(m >= 2 && base::bits::IsPowerOfTwo32(m));
   while ((pc_offset() & (m - 1)) != 0) {
     db(0);
   }
 }
 }  // namespace internal
 }  // namespace v8