Incorporate suggestions from code review

* MEM_INIT_METHOD != 1 with --with-memory-file 1 now triggers an assertion
* Consistently return '' instead of m.group(0) if there is no initializer
* Strip trailing zeros for emterpreter as well
* Include crc32 in literal only if it gets verified
* Enable assertions for the asm2m test run in general
* Disable assertions for one test case, fnmatch, to cover that as well
* Include the asm2m run name in two lists of run modes
* Add browser test to verify all pairs of bytes get encoded correctly
* Add browser test to verify that a >32M initializer works without chunking
* Omit duplicate var declaration for the memoryInitializer variable
* Minor comments and syntax improvements
* Capture the memory_init_file setting by its MEM_INIT_METHOD value.
* Drop special handling for emterpreter, which shouldn't be needed any more.

Author: gagern

emcc

@@ -1291,8 +1291,8 @@ try:
   extra_args = [] if not js_libraries else ['--libraries', ','.join(map(os.path.abspath, js_libraries))]
   if memory_init_file:
     shared.Settings.MEM_INIT_METHOD = 1
-  elif shared.Settings.MEM_INIT_METHOD == 1:
-    shared.Settings.MEM_INIT_METHOD = 0
+  else:
+    assert shared.Settings.MEM_INIT_METHOD != 1
   final = shared.Building.emscripten(final, append_ext=False, extra_args=extra_args)
   if DEBUG: save_intermediate('original')
 
@@ -1344,46 +1344,47 @@ try:
 
   js_transform_tempfiles = [final]
 
-  if memory_init_file or shared.Settings.MEM_INIT_METHOD == 2:
+  if shared.Settings.MEM_INIT_METHOD > 0:
     memfile = target + '.mem'
     shared.try_delete(memfile)
     def repl(m):
       # handle chunking of the memory initializer
-      s = m.groups(0)[0]
-      if len(s) == 0 and not shared.Settings.EMTERPRETIFY: return m.group(0) # emterpreter must have a mem init file; otherwise, don't emit 0-size ones
+      s = m.group(1)
+      if len(s) == 0: return '' # don't emit 0-size ones
       membytes = [int(x or '0') for x in s.split(',')]
-      if not shared.Settings.EMTERPRETIFY:
-        while membytes and membytes[-1] == 0:
-          membytes.pop()
-        if not membytes:
-          return '';
+      while membytes and membytes[-1] == 0:
+        membytes.pop()
+      if not membytes: return ''
       if not memory_init_file:
-        crcTable = []
-        for i in range(256):
-          crc = i
-          for bit in range(8):
-            crc = (crc >> 1) ^ ((crc & 1) * 0xedb88320)
-          crcTable.append(crc)
-        crc = 0xffffffff
+        # memory initializer in a string literal
         s = list(membytes)
-        n = len(s)
-        crc = crcTable[(crc ^ n) & 0xff] ^ (crc >> 8)
-        crc = crcTable[(crc ^ (n >> 8)) & 0xff] ^ (crc >> 8)
-        for i in s:
-          crc = crcTable[(crc ^ i) & 0xff] ^ (crc >> 8)
-        for i in range(4):
-          s.append((crc >> (8 * i)) & 0xff)
+        if shared.Settings.ASSERTIONS:
+          # append checksum of length and content
+          crcTable = []
+          for i in range(256):
+            crc = i
+            for bit in range(8):
+              crc = (crc >> 1) ^ ((crc & 1) * 0xedb88320)
+            crcTable.append(crc)
+          crc = 0xffffffff
+          n = len(s)
+          crc = crcTable[(crc ^ n) & 0xff] ^ (crc >> 8)
+          crc = crcTable[(crc ^ (n >> 8)) & 0xff] ^ (crc >> 8)
+          for i in s:
+            crc = crcTable[(crc ^ i) & 0xff] ^ (crc >> 8)
+          for i in range(4):
+            s.append((crc >> (8 * i)) & 0xff)
         s = ''.join(map(chr, s))
         s = s.replace('\\', '\\\\').replace("'", "\\'")
         s = s.replace('\n', '\\n').replace('\r', '\\r')
         def escape(x): return '\\x{:02x}'.format(ord(x.group()))
         s = re.sub('[\x80-\xff]', escape, s)
-        return "var memoryInitializer = '%s';" % s
+        return "memoryInitializer = '%s';" % s
       open(memfile, 'wb').write(''.join(map(chr, membytes)))
       if DEBUG:
         # Copy into temp dir as well, so can be run there too
         shared.safe_copy(memfile, os.path.join(shared.get_emscripten_temp_dir(), os.path.basename(memfile)))
-      return 'var memoryInitializer = "%s";' % os.path.basename(memfile)
+      return 'memoryInitializer = "%s";' % os.path.basename(memfile)
     src = re.sub(shared.JS.memory_initializer_pattern, repl, open(final).read(), count=1)
     open(final + '.mem.js', 'w').write(src)
     final += '.mem.js'

src/postamble.js

@@ -7,8 +7,9 @@ if (memoryInitializer && !ENVIRONMENT_IS_PTHREAD) (function(s) {
 #else
 if (memoryInitializer) (function(s) {
 #endif
-  var i, n = s.length - 4;
+  var i, n = s.length;
 #if ASSERTIONS
+  n -= 4;
   var crc, bit, table = new Int32Array(256);
   for (i = 0; i < 256; ++i) {
     for (crc = i, bit = 0; bit < 8; ++bit)
@@ -18,12 +19,14 @@ if (memoryInitializer) (function(s) {
   crc = -1;
   crc = table[(crc ^ n) & 0xff] ^ (crc >>> 8);
   crc = table[(crc ^ (n >>> 8)) & 0xff] ^ (crc >>> 8);
-  for (i = 0; i < s.length; ++i)
+  for (i = 0; i < s.length; ++i) {
     crc = table[(crc ^ s.charCodeAt(i)) & 0xff] ^ (crc >>> 8);
+  }
   assert(crc === 0, "memory initializer checksum");
 #endif
-  for (i = 0; i < n; ++i)
+  for (i = 0; i < n; ++i) {
     HEAPU8[STATIC_BASE + i] = s.charCodeAt(i);
+  }
 })(memoryInitializer);
 #else
 #if MEM_INIT_METHOD == 1

tests/meminit_pairs.c

@@ -0,0 +1,18 @@
+unsigned char problematic[] = { 0x20, 0x7c, 0x02, 0x07, 0x5f, 0xa0, 0xdf };
+int main() {
+  unsigned char a, b;
+  int result = 0, i, j;
+  for (i = 0; i < sizeof(problematic); ++i) {
+    a = problematic[i] ^ 32;
+    for (j = 0; j < sizeof(problematic); ++j) {
+      b = problematic[j] ^ 32;
+      if (((const unsigned char)data[a][2*b]) != a ||
+          ((const unsigned char)data[a][2*b + 1]) != b) {
+        result = 1;
+        printf("data[0x%02x][0x%03x]=%x02x\n", a, 2*b, data[a][2*b]);
+        printf("data[0x%02x][0x%03x]=%x02x\n", a, 2*b + 1, data[a][2*b + 1]);
+      }
+    }
+  }
+  REPORT_RESULT()
+}

tests/parallel_test_core.py

@@ -14,7 +14,7 @@
 assert not os.environ.get('EM_SAVE_DIR'), 'Need separate directories to avoid the parallel tests clashing'
 
 # run slower ones first, to optimize total time
-optimal_order = ['asm3i', 'asm1i', 'asm2nn', 'asm3', 'asm2', 'asm2g', 'asm2f', 'asm1', 'default']
+optimal_order = ['asm3i', 'asm1i', 'asm2nn', 'asm3', 'asm2', 'asm2m', 'asm2g', 'asm2f', 'asm1', 'default']
 assert set(optimal_order) == set(test_modes), 'need to update the list of slowest modes'
 
 # set up a background thread to report progress

tests/runner.py

@@ -38,7 +38,7 @@ def path_from_root(*pathelems):
 
 # Core test runner class, shared between normal tests and benchmarks
 checked_sanity = False
-test_modes = ['default', 'asm1', 'asm2', 'asm3', 'asm2f', 'asm2g', 'asm1i', 'asm3i', 'asm2nn']
+test_modes = ['default', 'asm1', 'asm2', 'asm3', 'asm2f', 'asm2g', 'asm1i', 'asm3i', 'asm2m', 'asm2nn']
 test_index = 0
 
 use_all_engines = os.environ.get('EM_ALL_ENGINES') # generally js engines are equivalent, testing 1 is enough. set this

tests/test_browser.py

@@ -2637,3 +2637,26 @@ def test_pthread_file_io(self):
   # Test that it is possible to send a signal via calling alarm(timeout), which in turn calls to the signal handler set by signal(SIGALRM, func);
   def test_sigalrm(self):
     self.btest(path_from_root('tests', 'sigalrm.cpp'), expected='0', args=['-O3'])
+
+  def test_meminit_pairs(self):
+    d = 'const char *data[] = {\n  "'
+    d += '",\n  "'.join(''.join('\\x{:02x}\\x{:02x}'.format(i, j)
+                                for j in range(256)) for i in range(256))
+    with open(path_from_root('tests', 'meminit_pairs.c')) as f:
+      d += '"\n};\n' + f.read()
+    args = ["-O2", "--memory-init-file", "0", "-s", "MEM_INIT_METHOD=2", "-s", "ASSERTIONS=1"]
+    self.btest(d, expected='0', args=args + ["--closure", "0"])
+    self.btest(d, expected='0', args=args + ["--closure", "0", "-g"])
+    self.btest(d, expected='0', args=args + ["--closure", "1"])
+
+  def test_meminit_big(self):
+    d = 'const char *data[] = {\n  "'
+    d += '",\n  "'.join([''.join('\\x{:02x}\\x{:02x}'.format(i, j)
+                                 for j in range(256)) for i in range(256)]*256)
+    with open(path_from_root('tests', 'meminit_pairs.c')) as f:
+      d += '"\n};\n' + f.read()
+    assert len(d) > (1 << 27) # more than 32M memory initializer
+    args = ["-O2", "--memory-init-file", "0", "-s", "MEM_INIT_METHOD=2", "-s", "ASSERTIONS=1"]
+    self.btest(d, expected='0', args=args + ["--closure", "0"])
+    self.btest(d, expected='0', args=args + ["--closure", "0", "-g"])
+    self.btest(d, expected='0', args=args + ["--closure", "1"])

tests/test_core.py

@@ -4374,6 +4374,13 @@ def test_strstr(self):
     self.do_run_from_file(src, output)
 
   def test_fnmatch(self):
+    # Run one test without assertions, for additional coverage
+    assert 'asm2m' in test_modes
+    if self.run_name == 'asm2m':
+      i = self.emcc_args.index('ASSERTIONS=1')
+      assert i > 0 and self.emcc_args[i-1] == '-s'
+      self.emcc_args[i] = 'ASSERTIONS=0'
+
     test_path = path_from_root('tests', 'core', 'fnmatch')
     src, output = (test_path + s for s in ('.c', '.out'))
     self.do_run_from_file(src, output)
@@ -7387,7 +7394,7 @@ def setUp(self):
 asm2g = make_run("asm2g", compiler=CLANG, emcc_args=["-O2", "-g", "-s", "ASSERTIONS=1", "-s", "SAFE_HEAP=1"])
 asm1i = make_run("asm1i", compiler=CLANG, emcc_args=["-O1", '-s', 'EMTERPRETIFY=1'])
 asm3i = make_run("asm3i", compiler=CLANG, emcc_args=["-O3", '-s', 'EMTERPRETIFY=1'])
-asm2m = make_run("asm2m", compiler=CLANG, emcc_args=["-O2", "--memory-init-file", "0", "-s", "MEM_INIT_METHOD=2"])
+asm2m = make_run("asm2m", compiler=CLANG, emcc_args=["-O2", "--memory-init-file", "0", "-s", "MEM_INIT_METHOD=2", "-s", "ASSERTIONS=1"])
 
 # Legacy test modes - 
 asm2nn = make_run("asm2nn", compiler=CLANG, emcc_args=["-O2"], env={"EMCC_NATIVE_OPTIMIZER": "0"})