Put stack first when not optimizing

Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177

Author: sbc100

ChangeLog.md

@@ -20,6 +20,10 @@ See docs/process.md for more on how version tagging works.
 
 3.1.25 (in development)
 -----------------------
+- In non-optimizing builds we emscripten will now place the stack first in
+  memory, before global data.  This is to get more accurate stack overflow
+  errors (since overflow will trap rather currpting global data first).  Most
+  programs should not be effected by this change. (#18154)
 - The `TOTAL_STACK` setting was renamed to `STACK_SIZE`.  The old name will
   continue to work as an alias. (#18128)
 - Exporting `print`/`printErr` via `-sEXPORTED_RUNTIME_METHODS` is deprecated in

emcc.py

@@ -1814,6 +1814,14 @@ def phase_linker_setup(options, state, newargs):
   if not settings.AUTO_JS_LIBRARIES:
     default_setting('USE_SDL', 0)
 
+  if 'GLOBAL_BASE' not in user_settings and not settings.SHRINK_LEVEL and not settings.OPT_LEVEL:
+    # When optimizing for size it helps to put static data first before
+    # the stack (sincs this makes instructions for accessing this data
+    # use a smaller LEB encoding).
+    # However, for debugability is better to have the stack come first
+    # (becuase stack overflows will trap rather than corrupting data).
+    settings.STACK_FIRST = True
+
   # Default to TEXTDECODER=2 (always use TextDecoder to decode UTF-8 strings)
   # in -Oz builds, since custom decoder for UTF-8 takes up space.
   # In pthreads enabled builds, TEXTDECODER==2 may not work, see
@@ -2005,7 +2013,8 @@ def phase_linker_setup(options, state, newargs):
     settings.REQUIRED_EXPORTS += [
       'emscripten_stack_get_end',
       'emscripten_stack_get_free',
-      'emscripten_stack_get_base'
+      'emscripten_stack_get_base',
+      'emscripten_stack_get_current',
     ]
 
     # We call one of these two functions during startup which caches the stack limits
@@ -2150,7 +2159,7 @@ def phase_linker_setup(options, state, newargs):
     exit_with_error('cannot have both DYNAMIC_EXECUTION=0 and RELOCATABLE enabled at the same time, since RELOCATABLE needs to eval()')
 
   if settings.SIDE_MODULE and 'GLOBAL_BASE' in user_settings:
-    exit_with_error('Cannot set GLOBAL_BASE when building SIDE_MODULE')
+    exit_with_error('GLOBAL_BASE is not compatible with SIDE_MODULE')
 
   if options.use_preload_plugins or len(options.preload_files) or len(options.embed_files):
     if settings.NODERAWFS:
@@ -2498,6 +2507,7 @@ def check_memory_setting(setting):
     # We start our global data after the shadow memory.
     # We don't need to worry about alignment here.  wasm-ld will take care of that.
     settings.GLOBAL_BASE = shadow_size
+    settings.STACK_FIRST = False
 
     if not settings.ALLOW_MEMORY_GROWTH:
       settings.INITIAL_MEMORY = total_mem

src/library.js

@@ -3442,6 +3442,14 @@ mergeInto(LibraryManager.library, {
     if (e instanceof ExitStatus || e == 'unwind') {
       return EXITSTATUS;
     }
+#if STACK_OVERFLOW_CHECK
+    checkStackCookie();
+    if (e instanceof WebAssembly.RuntimeError) {
+      if (_emscripten_stack_get_current() <= 0) {
+        err('Stack overflow detected.  You can try increasing -sSTACK_SIZE (currently set to ' + STACK_SIZE + ')');
+      }
+    }
+#endif
 #if MINIMAL_RUNTIME
     throw e;
 #else

src/runtime_stack_check.js

@@ -14,6 +14,12 @@ function writeStackCookie() {
 #if ASSERTIONS
   assert((max & 3) == 0);
 #endif
+  // If the stack ends at address zero we write our cookies 4 bytes into the
+  // stack.  This prevents interference with the (separate) address-zero check
+  // below.
+  if (max == 0) {
+    max += 4;
+  }
   // The stack grow downwards towards _emscripten_stack_get_end.
   // We write cookies to the final two words in the stack and detect if they are
   // ever overwritten.
@@ -33,6 +39,9 @@ function checkStackCookie() {
 #if RUNTIME_DEBUG
   dbg('checkStackCookie: ' + max.toString(16));
 #endif
+  if (max == 0) {
+    max += 4;
+  }
   var cookie1 = {{{ makeGetValue('max', 0, 'u32') }}};
   var cookie2 = {{{ makeGetValue('max', 4, 'u32') }}};
   if (cookie1 != 0x2135467 || cookie2 != 0x89BACDFE) {

src/settings_internal.js

@@ -244,3 +244,5 @@ var ALL_INCOMING_MODULE_JS_API = [];
 // when weak symbols are undefined.  Only applies in the case of dyanmic linking
 // (MAIN_MODULE).
 var WEAK_IMPORTS = [];
+
+var STACK_FIRST = false;

test/core/stack_overflow.cpp renamed to test/core/stack_overflow.c

@@ -6,7 +6,8 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
-#include <emscripten.h>
+#include <emscripten/emscripten.h>
+#include <emscripten/stack.h>
 
 void recurse(unsigned long x);
 
@@ -18,7 +19,7 @@ void act(volatile unsigned long *a) {
 }
 
 void recurse(volatile unsigned long x) {
-  printf("recurse %ld\n", x);
+  printf("recurse %ld sp=%#lx\n", x, emscripten_stack_get_current());
   volatile unsigned long a = x;
   volatile char buffer[1000*1000];
   buffer[x/2] = 0;
@@ -36,4 +37,3 @@ void recurse(volatile unsigned long x) {
 int main() {
   recurse(1000*1000);
 }
-

test/core/test_emmalloc_trim.out

@@ -1,35 +1,35 @@
 heap size: 134217728
 dynamic heap 0: 4096
 free dynamic memory 0: 4096
-unclaimed heap memory 0: 2142171136
-sbrk 0: 0x502000
+unclaimed heap memory 0: 2142175232
+sbrk 0: 0x501000
 1
 dynamic heap 1: 37752832
 free dynamic memory 1: 4096
-unclaimed heap memory 1: 2104422400
-sbrk 1: 0x2902000
+unclaimed heap memory 1: 2104426496
+sbrk 1: 0x2901000
 1st trim: 1
 dynamic heap 1: 37752832
 free dynamic memory 1: 0
-unclaimed heap memory 1: 2104422400
-sbrk 1: 0x2902000
+unclaimed heap memory 1: 2104426496
+sbrk 1: 0x2901000
 2nd trim: 0
 dynamic heap 2: 37752832
 free dynamic memory 2: 0
-unclaimed heap memory 2: 2104422400
-sbrk 2: 0x2902000
+unclaimed heap memory 2: 2104426496
+sbrk 2: 0x2901000
 3rd trim: 1
 dynamic heap 3: 33656832
 free dynamic memory 3: 102400
-unclaimed heap memory 3: 2104422400
-sbrk 3: 0x2902000
+unclaimed heap memory 3: 2104426496
+sbrk 3: 0x2901000
 4th trim: 0
 dynamic heap 4: 33656832
 free dynamic memory 4: 102400
-unclaimed heap memory 4: 2104422400
-sbrk 4: 0x2902000
+unclaimed heap memory 4: 2104426496
+sbrk 4: 0x2901000
 5th trim: 1
 dynamic heap 5: 33558528
 free dynamic memory 5: 0
-unclaimed heap memory 5: 2104422400
-sbrk 5: 0x2902000
+unclaimed heap memory 5: 2104426496
+sbrk 5: 0x2901000

test/core/test_stack_placement.c

@@ -7,6 +7,7 @@
 
 #include <stdio.h>
 #include <assert.h>
+#include <emscripten/stack.h>
 
 /* We had a regression where the stack position was not taking into account
  * the data and bss.  This test runs with 1024 byte stack which given that bug
@@ -22,7 +23,14 @@ int main(int argc, char* argv[]) {
   int* bss_address = &static_bss[BSS_BYTES-1];
   int* stack_address = &stack_var;
   printf("data: %p bss: %p stack: %p\n", data_address, bss_address, stack_address);
-  assert(stack_address > bss_address);
+  // Stack can either come after BSS or before data (In debug builds we link
+  // with --stack-first).
+  int stack_first = emscripten_stack_get_end() == 0;
+  if (stack_first) {
+    assert(stack_address < data_address);
+  } else {
+    assert(stack_address > bss_address);
+  }
   assert(bss_address > data_address);
   printf("success.\n");
   return 0;

test/other/metadce/test_metadce_hello_O0.exports

@@ -3,6 +3,7 @@ __indirect_function_table
 __wasm_call_ctors
 dynCall_jiji
 emscripten_stack_get_base
+emscripten_stack_get_current
 emscripten_stack_get_end
 emscripten_stack_get_free
 emscripten_stack_init

test/other/metadce/test_metadce_hello_O0.funcs

@@ -23,6 +23,7 @@ $__wasi_syscall_ret
 $__wasm_call_ctors
 $dynCall_jiji
 $emscripten_stack_get_base
+$emscripten_stack_get_current
 $emscripten_stack_get_end
 $emscripten_stack_get_free
 $emscripten_stack_init

test/other/metadce/test_metadce_hello_O0.jssize

@@ -1 +1 @@
-26256
+26641

test/other/metadce/test_metadce_hello_O0.size

@@ -1 +1 @@
-12058
+12172

test/other/metadce/test_metadce_minimal_O0.exports

@@ -3,6 +3,7 @@ __indirect_function_table
 __wasm_call_ctors
 add
 emscripten_stack_get_base
+emscripten_stack_get_current
 emscripten_stack_get_end
 emscripten_stack_get_free
 emscripten_stack_init

test/other/metadce/test_metadce_minimal_O0.funcs

@@ -8,6 +8,7 @@ $__unlockfile
 $__wasm_call_ctors
 $add
 $emscripten_stack_get_base
+$emscripten_stack_get_current
 $emscripten_stack_get_end
 $emscripten_stack_get_free
 $emscripten_stack_init

test/other/metadce/test_metadce_minimal_O0.jssize

@@ -1 +1 @@
-22987
+23200

test/other/metadce/test_metadce_minimal_O0.size

@@ -1 +1 @@
-865
+919

test/other/test_unoptimized_code_size.js.size

@@ -1 +1 @@
-66886
+67655

test/other/test_unoptimized_code_size.wasm.size

@@ -1 +1 @@
-12091
+12205

test/other/test_unoptimized_code_size_no_asserts.wasm.size

@@ -1 +1 @@
-11580
+11644

test/other/test_unoptimized_code_size_strict.js.size

@@ -1 +1 @@
-65752
+66521

test/other/test_unoptimized_code_size_strict.wasm.size

@@ -1 +1 @@
-12091
+12205

test/test_core.py

@@ -2949,7 +2949,7 @@ def test_bsearch(self):
 
   def test_stack_overflow(self):
     self.set_setting('ASSERTIONS', 2)
-    self.do_runf(test_file('core/stack_overflow.cpp'), 'Aborted(stack overflow', assert_returncode=NON_ZERO)
+    self.do_runf(test_file('core/stack_overflow.c'), 'Aborted(stack overflow', assert_returncode=NON_ZERO)
 
   def test_stackAlloc(self):
     self.do_core_test('stackAlloc.cpp')

test/test_other.py

@@ -8597,8 +8597,8 @@ def test_js_optimizer_parse_error(self):
     stderr = self.expect_fail([EMXX, 'src.cpp', '-O2'])
     # wasm backend output doesn't have spaces in the EM_ASM function bodies
     self.assertContained(('''
-  1025: () => { var x = !<->5.; }
-                         ^
+  5242881: () => { var x = !<->5.; }
+                            ^
 '''), stderr)
 
   def test_js_optimizer_chunk_size_determinism(self):
@@ -12674,3 +12674,10 @@ def test_em_js_deps(self):
     }
     ''')
     self.do_runf('f2.c', emcc_args=['f1.c'])
+
+  def test_stack_overflow(self):
+    self.set_setting('STACK_OVERFLOW_CHECK', 1)
+    self.emcc_args += ['-O1', '--profiling-funcs']
+    self.do_runf(test_file('core/stack_overflow.c'),
+                 'Stack overflow detected.  You can try increasing -sSTACK_SIZE',
+                 assert_returncode=NON_ZERO)

tools/building.py

@@ -225,8 +225,11 @@ def lld_flags_for_executable(external_symbols):
       cmd.append('--max-memory=%d' % settings.INITIAL_MEMORY)
     elif settings.MAXIMUM_MEMORY != -1:
       cmd.append('--max-memory=%d' % settings.MAXIMUM_MEMORY)
-    if not settings.RELOCATABLE:
-      cmd.append('--global-base=%s' % settings.GLOBAL_BASE)
+
+  if settings.STACK_FIRST:
+    cmd.append('--stack-first')
+  elif not settings.RELOCATABLE:
+    cmd.append('--global-base=%s' % settings.GLOBAL_BASE)
 
   return cmd