Default to EXIT_RUNTIME=1 when building with ASan #15083

sbc100 · 2021-09-21T16:02:33Z

We were already setting EXIT_RUNTIME=1 under LSan because without this
leaks are not reported. Since ASan includes LSan by default it makes
sense to do that same for ASan.

Because this change also effectivly enables leak detection in the asan
test suite it also includes a bunch of fixes for memory leaks.

The code in emcc.py that has been moved depend on EXIT_RUNTIME and
so must be run only after this setting has been finalized.

As part of this change I found a couple of issues related to
EXIT_RUNTIME when combined with other flags:

#15080
#15081

Regarding memory that leaked from JS and/or system libraries there in
some cases we want to ignore the leaks rather then adding code to free
the memory and in this case I've used emscripten_builtin_malloc.

kripken · 2021-09-21T16:59:44Z

Its not clear to be exactly when one method is preferable over another. The atexit approach I think is reasonable when its used to implement a feature that already has a high overhead (such as RETAIN_COMPILER_SETTINGS).

Good question. I'd say that if we know an allocation is ok to leak, then just calling emscripten_builtin_malloc is best. One benefit is simplicity, and the other is avoiding overhead as you said.

kripken · 2021-09-21T17:08:43Z

emscripten.py

@@ -156,7 +156,7 @@ def apply_static_code_hooks(forwarded_json, code):
  code = shared.do_replace(code, '<<< ATINITS >>>', str(forwarded_json['ATINITS']))
  if settings.HAS_MAIN:
    code = shared.do_replace(code, '<<< ATMAINS >>>', str(forwarded_json['ATMAINS']))
-  if settings.EXIT_RUNTIME:
+  if settings.EXIT_RUNTIME and (not settings.MINIMAL_RUNTIME or settings.HAS_MAIN):


I don't follow this part of the change?

This is need for asan.test_fs_no_main_minimal_runtime. Prior to this change this test (and many others) were not run with EXIT_RUNTIME enabled.

Without this part of the change the above test fails like:

$ ./tests/runner asan.test_fs_no_main_minimal_runtime --skip-slow Test suites: ['test_core'] Running test_core: (1 tests) (checking sanity from test runner) shared:INFO: (Emscripten: Running sanity checks) test_fs_no_main_minimal_runtime (test_core.asan) ... emcc: error: expected to find pattern in input JS: <<< ATEXITS >>>

src/library.js

tests/test_core.py

system/lib/libc/musl/src/env/__environ.c

kripken · 2021-09-21T21:55:10Z

tests/test_core.py

-      ccall('floatf', 'number', null, null, { async: true }).then(console.log);
+      ccall('floatf', 'number', null, null, { async: true }).then(function(arg) {
+        console.log(arg);
+        maybeExit();


Is this still needed? I think it's part of the stuff you said you'd revert.

I still see maybeExit() here, weird. Is the github UI confusing me maybe?

Oops, actually reverted this now.

kripken · 2021-09-21T23:43:33Z

system/lib/libc/musl/src/env/__environ.c

 char **__environ = 0;
 weak_alias(__environ, ___environ);
 weak_alias(__environ, _environ);
 weak_alias(__environ, environ);

 #ifdef __EMSCRIPTEN__
+#include <stdlib.h>
+#include <wasi/api.h>
+#include <emscripten/emmalloc.h>


why does this include emmalloc?

That is where emscripten_builtin_malloc is defined.

Oh, really? That's quite odd, as that header is for the emmalloc replacement for dlmalloc, which we don't even use by default.

It looks like the function is declared also in dlmalloc.c, but not in a header. We should probably create a new header just for that method, I think? For now to not block you, how about a comment here with a TODO to fix this?

Yes it does seem a little odd. I think maybe the idea was that emmalloc.h contains all the malloc-related functions? emmalloc.h also defines malloc and free as well BTW... i think it made sense maybe to define all 3 variants in the same place?

I didn't originally put them there .. maybe there is a better place? emscripten/heap.h? For now I think its fine. Certainly unrelated to this PR. We already include emmalloc.h in pthread _create.c for this same purpose:

emscripten/system/lib/pthread/pthread_create.c

Line 16 in 6b53ded

#include <emscripten/emmalloc.h>

kripken · 2021-09-21T23:44:53Z

tests/test_core.py

-      ccall('floatf', 'number', null, null, { async: true }).then(console.log);
+      ccall('floatf', 'number', null, null, { async: true }).then(function(arg) {
+        console.log(arg);
+        maybeExit();


I still see maybeExit() here, weird. Is the github UI confusing me maybe?

kripken · 2021-09-22T00:30:51Z

system/lib/libc/musl/src/env/__environ.c

 char **__environ = 0;
 weak_alias(__environ, ___environ);
 weak_alias(__environ, _environ);
 weak_alias(__environ, environ);

 #ifdef __EMSCRIPTEN__
+#include <stdlib.h>
+#include <wasi/api.h>
+#include <emscripten/emmalloc.h>


Oh, really? That's quite odd, as that header is for the emmalloc replacement for dlmalloc, which we don't even use by default.

It looks like the function is declared also in dlmalloc.c, but not in a header. We should probably create a new header just for that method, I think? For now to not block you, how about a comment here with a TODO to fix this?

We were already setting `EXIT_RUNTIME=1` under LSan because without this leaks are not reported. Since ASan includes LSan by default it makes sense to do that same for ASan. Because this change also effectivly enables leak detection in the `asan` test suite it also includes a bunch of fixes for memory leaks. The code in `emcc.py` that has been moved depend on `EXIT_RUNTIME` and so must be run only after this setting has been finalized. As part of this change I found a couple of issues related to `EXIT_RUNTIME` when combined with other flags: #15080 #15081 Regarding memory that leaked from JS and/or system libraries there in some cases we want to ignore the leaks rather then adding code to free the memory and in this case I've used `emscripten_builtin_malloc`.

Not sure how I missed adding these.

Since #15083, asan now sets `EXIT_RUNTIME` by default and this test depends on being able to run a `postRun` handler. Currently `postRun` handlers are not compatible with `EXIT_RUNTIME` under node: #15080.

sbc100 changed the title ~~Default to EXIT_RUNTIME=1 when under ASan~~ Default to EXIT_RUNTIME=1 when building with ASan Sep 21, 2021

sbc100 force-pushed the asan_exit_runtime branch 2 times, most recently from 19a5f12 to 1e5568d Compare September 21, 2021 16:07

sbc100 requested review from kripken and kleisauke September 21, 2021 16:07

sbc100 force-pushed the asan_exit_runtime branch 2 times, most recently from 7dfafca to 745e1bb Compare September 21, 2021 16:09

kripken reviewed Sep 21, 2021

View reviewed changes

sbc100 force-pushed the asan_exit_runtime branch 3 times, most recently from e723c57 to 03f6a48 Compare September 21, 2021 18:34

kripken reviewed Sep 21, 2021

View reviewed changes

src/library.js Outdated Show resolved Hide resolved

tests/test_core.py Show resolved Hide resolved

sbc100 force-pushed the asan_exit_runtime branch 2 times, most recently from 27800c7 to bc805da Compare September 21, 2021 21:10

sbc100 changed the base branch from main to async_ccall_runtime_keepalive September 21, 2021 21:10

sbc100 force-pushed the asan_exit_runtime branch from bc805da to 60a5050 Compare September 21, 2021 21:14

kripken reviewed Sep 21, 2021

View reviewed changes

sbc100 force-pushed the async_ccall_runtime_keepalive branch from 4b45323 to 62d31dc Compare September 21, 2021 21:59

Base automatically changed from async_ccall_runtime_keepalive to main September 21, 2021 22:07

sbc100 force-pushed the asan_exit_runtime branch 2 times, most recently from f70f505 to 561d036 Compare September 21, 2021 22:43

kripken reviewed Sep 21, 2021

View reviewed changes

sbc100 force-pushed the asan_exit_runtime branch from 561d036 to c0866bc Compare September 22, 2021 00:17

kripken approved these changes Sep 22, 2021

View reviewed changes

sbc100 force-pushed the asan_exit_runtime branch from c0866bc to c51e8d6 Compare September 22, 2021 00:43

sbc100 enabled auto-merge (squash) September 22, 2021 00:44

sbc100 disabled auto-merge September 22, 2021 00:44

sbc100 enabled auto-merge (squash) September 22, 2021 00:44

sbc100 force-pushed the asan_exit_runtime branch from c51e8d6 to 0e247e8 Compare September 22, 2021 00:47

sbc100 force-pushed the asan_exit_runtime branch 2 times, most recently from d9167a1 to c951f1d Compare September 22, 2021 05:22

sbc100 force-pushed the asan_exit_runtime branch from c951f1d to 61ebb86 Compare September 22, 2021 14:00

sbc100 merged commit 34ff848 into main Sep 22, 2021

sbc100 deleted the asan_exit_runtime branch September 22, 2021 14:52

sbc100 added a commit that referenced this pull request Sep 22, 2021

Missing comments from #15083. NFC.

c9664b1

Not sure how I missed adding these.

sbc100 added a commit that referenced this pull request Sep 22, 2021

Missing comments from #15083. NFC. (#15092)

1f533bd

Not sure how I missed adding these.

sbc100 mentioned this pull request Sep 23, 2021

Fix asan.test_openjpeg #15112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Default to EXIT_RUNTIME=1 when building with ASan #15083

Default to EXIT_RUNTIME=1 when building with ASan #15083

Uh oh!

sbc100 commented Sep 21, 2021 •

edited

Loading

Uh oh!

kripken commented Sep 21, 2021

Uh oh!

kripken Sep 21, 2021

Uh oh!

sbc100 Sep 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken Sep 21, 2021

Uh oh!

sbc100 Sep 21, 2021

Uh oh!

kripken Sep 21, 2021

Uh oh!

sbc100 Sep 22, 2021

Uh oh!

kripken Sep 21, 2021

Uh oh!

sbc100 Sep 22, 2021

Uh oh!

kripken Sep 22, 2021

Uh oh!

sbc100 Sep 22, 2021

Uh oh!

kripken Sep 21, 2021

Uh oh!

kripken Sep 22, 2021

Uh oh!

Uh oh!

Default to EXIT_RUNTIME=1 when building with ASan #15083

Default to EXIT_RUNTIME=1 when building with ASan #15083

Uh oh!

Conversation

sbc100 commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Sep 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 commented Sep 21, 2021 •

edited

Loading