Put stack first when not optimizing (#18154)

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

.circleci/config.yml

@@ -551,6 +551,7 @@ jobs:
             core2.test_em_js
             core2.test_em_js_pthreads
             core2.test_unicode_js_library
+            other.test_stack_overflow
             other.test_dlmalloc_modes
             other.test_c_preprocessor
             other.test_prejs_unicode

ChangeLog.md

@@ -26,6 +26,11 @@ See docs/process.md for more on how version tagging works.
   that will allow the command to be replicated.  This can be useful for sharing
   reproduction cases with others (inspired by the lld option of the same name).
   (#18160)
+- In non-optimizing builds 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 corrupting global data first).  This should not
+  be a user-visible change (unless your program does something very odd such
+  depending on the specific location of stack data in memory). (#18154)
 
 3.1.25 - 11/08/22
 -----------------

emcc.py

@@ -1883,6 +1883,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
@@ -2074,7 +2082,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
@@ -2219,7 +2228,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:
@@ -2567,6 +2576,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,10 @@ function checkStackCookie() {
 #if RUNTIME_DEBUG
   dbg('checkStackCookie: ' + max.toString(16));
 #endif
+  // See writeStackCookie().
+  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 test/core/stack_overflow.ctest/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