`wpt/test-wasm-jsapi` sometimes crashes on AIX

[richardlau]

Test

wpt/test-wasm-jsapi

Platform

AIX

Console output

21:53:20 ok 5280 wpt/test-user-timing # TODO : Fix flaky test
21:53:20   ---
21:53:20   duration_ms: 917.03200
21:53:20   ...
21:53:21 not ok 5281 wpt/test-wasm-jsapi
21:53:21   ---
21:53:21   duration_ms: 1557.51600
21:53:21   severity: crashed
21:53:21   exitcode: -5
21:53:21   stack: |-
21:53:21     [SKIPPED] esm-integration/global-exports-live-bindings.tentative.any.js: Live bindings unsupported pending V8 WebAssemblyModuleRecord
...
21:53:22     [PASS] basic 4
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     (node:29032854) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time
21:53:22     (Use `node --trace-warnings ...` to show where the warning was created)
21:53:22     (node:29032854) ExperimentalWarning: Importing WebAssembly module instances is an experimental feature and might change at any time
21:53:22     
21:53:22     
21:53:22     #
21:53:22     # Fatal error in , line 0
21:53:22     # Check failed: ptr == address.
21:53:22     #
21:53:22     #
21:53:22     #
21:53:22     #FailureMessage Object: 1135e9ad0
21:53:22     ----- Native stack trace -----
21:53:22     
21:53:22   ...
...

21:53:52 Failed tests:
21:53:52 out/Release/node --experimental-wasm-modules /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-power9/test/wpt/test-wasm-jsapi.mjs
21:53:52 gmake[1]: *** [Makefile:619: test-ci] Error 1

Build links

• https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix72-power9/61914/console
• https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix72-power9/61967/console
• https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix72-power9/61997/console

Additional information

We suspect it is the same check/comment

node/deps/v8/src/base/platform/platform-aix.cc

Lines 194 to 199 in 5b6091c

	// If this check fails it's most likely due to a racing condition where
	// another thread has mapped the same address right before we do.
	// Since this could cause hard-to-debug issues, potentially with security
	// impact, and we can't recover from this, the best we can do is abort the
	// process.
	CHECK_EQ(ptr, address);

as referenced in #62470.

[panva]

WPTs can be marked as flaky but that's not helpful if the whole test file crashes. Using #62517 we can now skip a subtest if it can be isolated by name. When the status file is cjs it can be skipped conditionally on aix only.

[richardlau]

We suspect it is the same check/comment

node/deps/v8/src/base/platform/platform-aix.cc

Lines 194 to 199 in 5b6091c

	// If this check fails it's most likely due to a racing condition where
	// another thread has mapped the same address right before we do.
	// Since this could cause hard-to-debug issues, potentially with security
	// impact, and we can't recover from this, the best we can do is abort the
	// process.
	CHECK_EQ(ptr, address);

@miladfarca suggested trying this patch:

diff --git a/src/base/platform/platform-aix.cc b/src/base/platform/platform-aix.cc
index ed4b5913387..53d7c8ce08b 100644
--- a/src/base/platform/platform-aix.cc
+++ b/src/base/platform/platform-aix.cc
@@ -166,39 +166,10 @@ Stack::StackSlot Stack::ObtainCurrentThreadStackStart() {
 
 // static
 bool OS::DecommitPages(void* address, size_t size) {
-  // The difference between this implementation and the alternative under
-  // platform-posix.cc is that on AIX, calling mmap on a pre-designated address
-  // with MAP_FIXED will fail and return -1 unless the application has requested
-  // SPEC1170 compliant behaviour:
-  // https://www.ibm.com/docs/en/aix/7.3?topic=m-mmap-mmap64-subroutine
-  // Therefore in case if failure we need to unmap the address before trying to
-  // map it again. The downside is another thread could place another mapping at
-  // the same address after the munmap but before the mmap, therefore a CHECK is
-  // also added to assure the address is mapped successfully. Refer to the
-  // comments under https://crrev.com/c/3010195 for more details.
-#define MMAP() \
-  mmap(address, size, PROT_NONE, MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)
   DCHECK_EQ(0, reinterpret_cast<uintptr_t>(address) % CommitPageSize());
   DCHECK_EQ(0, size % CommitPageSize());
-  void* ptr;
-  // Try without mapping first.
-  ptr = MMAP();
-  if (ptr != address) {
-    DCHECK_EQ(ptr, MAP_FAILED);
-    // Returns 0 when successful.
-    if (munmap(address, size)) {
-      return false;
-    }
-    // Try again after unmap.
-    ptr = MMAP();
-    // If this check fails it's most likely due to a racing condition where
-    // another thread has mapped the same address right before we do.
-    // Since this could cause hard-to-debug issues, potentially with security
-    // impact, and we can't recover from this, the best we can do is abort the
-    // process.
-    CHECK_EQ(ptr, address);
-  }
-#undef MMAP
+  if (mprotect(address, size, PROT_NONE) != 0) return false;
+  if (madvise(address, size, MADV_DONTNEED) != 0) return false;
   return true;
 }

Had to tweak it slightly to fix a cast error: https://ci.nodejs.org/job/node-stress-single-test/683/nodes=aix72-power9/console

19:50:42 ../deps/v8/src/base/platform/platform-aix.cc: In static member function 'static bool v8::base::OS::DecommitPages(void*, size_t)':
19:50:42 ../deps/v8/src/base/platform/platform-aix.cc:172:15: error: invalid conversion from 'void*' to 'caddr_t' {aka 'char*'} [-fpermissive]
19:50:42   172 |   if (madvise(address, size, MADV_DONTNEED) != 0) return false;
19:50:42       |               ^~~~~~~
19:50:42       |               |
19:50:42       |               void*
19:50:42 In file included from ../deps/v8/src/base/platform/platform-aix.cc:22:
19:50:42 /usr/include/sys/mman.h:189:41: note:   initializing argument 1 of 'int madvise(caddr_t, size_t, int)'
19:50:42   189 |         extern int              madvise(caddr_t, size_t, int);
19:50:42       |                                         ^~~~~~~
19:50:42 make[1]: *** [tools/v8_gypfiles/v8_libbase.target.mk:234: 

So with a reinterpret_cast:

diff --git a/deps/v8/src/base/platform/platform-aix.cc b/deps/v8/src/base/platform/platform-aix.cc
index ed4b5913387541..190acb000bb02b 100644
--- a/deps/v8/src/base/platform/platform-aix.cc
+++ b/deps/v8/src/base/platform/platform-aix.cc
@@ -166,39 +166,10 @@ Stack::StackSlot Stack::ObtainCurrentThreadStackStart() {
 
 // static
 bool OS::DecommitPages(void* address, size_t size) {
-  // The difference between this implementation and the alternative under
-  // platform-posix.cc is that on AIX, calling mmap on a pre-designated address
-  // with MAP_FIXED will fail and return -1 unless the application has requested
-  // SPEC1170 compliant behaviour:
-  // https://www.ibm.com/docs/en/aix/7.3?topic=m-mmap-mmap64-subroutine
-  // Therefore in case if failure we need to unmap the address before trying to
-  // map it again. The downside is another thread could place another mapping at
-  // the same address after the munmap but before the mmap, therefore a CHECK is
-  // also added to assure the address is mapped successfully. Refer to the
-  // comments under https://crrev.com/c/3010195 for more details.
-#define MMAP() \
-  mmap(address, size, PROT_NONE, MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)
   DCHECK_EQ(0, reinterpret_cast<uintptr_t>(address) % CommitPageSize());
   DCHECK_EQ(0, size % CommitPageSize());
-  void* ptr;
-  // Try without mapping first.
-  ptr = MMAP();
-  if (ptr != address) {
-    DCHECK_EQ(ptr, MAP_FAILED);
-    // Returns 0 when successful.
-    if (munmap(address, size)) {
-      return false;
-    }
-    // Try again after unmap.
-    ptr = MMAP();
-    // If this check fails it's most likely due to a racing condition where
-    // another thread has mapped the same address right before we do.
-    // Since this could cause hard-to-debug issues, potentially with security
-    // impact, and we can't recover from this, the best we can do is abort the
-    // process.
-    CHECK_EQ(ptr, address);
-  }
-#undef MMAP
+  if (mprotect(address, size, PROT_NONE) != 0) return false;
+  if (madvise(reinterpret_cast<caddr_t>(address), size, MADV_DONTNEED) != 0) return false;
   return true;
 }

we have 0 failures out of 1000 runs of wpt/test-wasm-jsapi: https://ci.nodejs.org/job/node-stress-single-test/684/nodes=aix72-power9/console
compared to 224 failures out of 1000 runs of wpt/test-wasm-jsapi without the above patch: https://ci.nodejs.org/job/node-stress-single-test/685/nodes=aix72-power9/console

Since this is in V8 we'll need to submit the change upstream to V8 and cherry-pick across to Node.js.

[kadler]

FYI madvise is a no-op on AIX: https://www.ibm.com/docs/en/aix/7.3.0?topic=m-madvise-subroutine

The madvise subroutine has no functionality and is supported for compatibility only.

This means that the contents of the mapping will be left as-is and if the mapping is made readable again, they can be read again. For mapped files, this also keeps the file mapping active which could have other implications, I'm not sure. Both the posix and the previous AIX version of the code replace the mapping with a new anonymous mapping which is zero-filled upon first read. I'm not sure whether this behavior change could cause problems or not, however.

[miladfarca]

If madvise is a no-op then you might run out memory eventually. You will need to find the best way to to get rid of the memory while keeping the address space on AIX, maybe disclaim64 ?

[kadler]

disclaim64 requires the mapping be writable or it will give EFAULT and the address cannot be mapped to a file. I am not sure this will work here.

[kadler]

It's too bad AIX does not provide a flag like MMAP_SPEC1170 which could be used to get exactly what we want without having to use the XPG_SUS_ENV environment variable, which changes other behavior too.

[abmusse]

Its unfortunate but it seems like our best option is to accept the limitations on AIX. The system could potentially run out of memory eventually but I don't see a clean way for us to handle it ourselves. When memory pressusre starts the OS may start to reclaim the physical memory.

Opened a CL with this change: https://chromium-review.googlesource.com/c/v8/v8/+/7780464/1/src/base/platform/platform-aix.cc#185

[miladfarca]

What about making the pages writable first with mprotect(address, size, PROT_READ | PROT_WRITE) before using disclaim64?