Add ASYNCIFY_PROPAGATE_ADD setting

[curiousdannii]

Adds a ASYNCIFY_PROPAGATE_ADD setting to control whether the ASYNCIFY_ADD list propagates. Closes #13150

I set this to be on by default, which does change the behaviour of ASYNCIFY_ADD. I think having it off by default is unsafe, and also greatly reduces its usefulness. For example, if you worked out the add-list in release mode, then switched back to debug mode, then it's likely your app would stop working because previously inlined functions would now need to be instrumented.

There are few examples in public code of people actually using it, probably because it requires you to list everything above it in the call tree. Here's one of the few examples I could find, which could probably be substantially simplified by this: https://github.com/Siv3D/OpenSiv3D/blob/c81400c5a39405d66843c07ce1cf90f46e01a86a/Web/App/CMakeLists.txt#L29
Thanks to @nokotan for getting the code working in Binaryen!

Note: I don't know if the Binaryen changes are in Emscripten yet.

[kripken]

Thanks! In general this looks good, though I still lean towards having this off by default. However, I don't feel strongly, so if most people want it on that sounds fine. Maybe let's wait a few days to see if people have opinions here.

If we do want it on then perhaps the changelog entry could be enlarged a little to say something like "as a result of this change, you may see an increase in the size of ASYNCIFY builds if you use ASYNCIFY_ADD (but correctness is not affected). To undo that and return to the old behavior, set -sNO_ASYNCIFY_PROPAGATE_ADD." (that is, to concretely explain what users might see changes)

[curiousdannii]

as a result of this change, you may see an increase in the size of ASYNCIFY builds if you use ASYNCIFY_ADD (but correctness is not affected)

I was trying to think of how it could end up bigger, because previously you would have to have still listed the functions above in ASYNCIFY_ADD. But suppose you have these functions:

void a() { c(); }
void b() { c(); }
void c() {  }

If you put c in ASYNCIFY_ADD, then both a and b will be added. But you might know that while a results in an async import being called, b's use of c never does. But the compiler can't work that out, so it has to add b. Previously you had to add a manually to be safe. Now you can manually add b to ASYNCIFY_REMOVE. So propagation by default is safer, but can result in bigger builds. Optimal use may still need some functions to be added (to REMOVE instead of ADD), but this is much safer, as, for example, different inlining decisions leading to a function not being instrumented could be fatal, but inlining decisions leading to a function being instrumented won't be.

I'll edit the changelog to explain this a little better:

Added the ASYNCIFY_PROPAGATE_ADD setting, to control whether the ASYNCIFY_ADD
list propagates or not. By default this is enabled; as a result you may see larger
ASYNCIFY builds as more of the function tree may be instrumented than you were
previously manually specifying in ASYNCIFY_ADD. To stop propagation you can
specify functions in the ASYNCIFY_REMOVE list, or to return to the previous
behaviour, disable this setting (set -sNO_ASYNCIFY_PROPAGATE_ADD.) (#21672)

[kripken]

Changelog looks good to me, thanks.

The CI error suggests to me that you need to run tools/maint/update_settings_docs.py to update a file, but I'm not sure.

[curiousdannii]

I don't like checked in autogenerated docs... I edited it manually haha.

There's now a different failing test which doesn't seem relevant to this PR...

[kripken]

That does look like a random test error, I restarted it.

Let's give it another day or so to see if people have thoughts on the default here. Otherwise I'm ok to land it as is, my preference the other way is minor.

[curiousdannii]

Has the changes from Binaryen been imported here yet? It wouldn't make sense to merge this until that happens..

[kripken]

Ah, yes, we need to wait for that, and while I pressed auto-merge it looks like that hasn't finished yet. But it should finish soon and the update then reaches emsdk tot in a matter of an hour or so.

[kripken]

I realized it's simple to test this, so I suggest we add that in this PR before landing. This should work, I think:

diff --git a/test/test_core.py b/test/test_core.py
index a6b437a0b..5f96694b9 100644
--- a/test/test_core.py
+++ b/test/test_core.py
@@ -8215,7 +8215,10 @@ Module.onRuntimeInitialized = () => {
   @parameterized({
     'normal': ([], True),
     'ignoreindirect': (['-sASYNCIFY_IGNORE_INDIRECT'], False),
-    'add': (['-sASYNCIFY_IGNORE_INDIRECT', '-sASYNCIFY_ADD=["__original_main","main","virt()"]'], True),
+    'add': (['-sASYNCIFY_IGNORE_INDIRECT', '-sASYNCIFY_ADD=["virt()"]'], True),
+    # If ASYNCIFY_PROPAGATE_ADD is disabled then we must specify the callers of
+    # virt() manually, rather than have them inferred automatically.
+    'add_no_prop': (['-sASYNCIFY_IGNORE_INDIRECT', '-sASYNCIFY_ADD=["__original_main","main","virt()"]', '-sASYNCIFY_PROPAGATE_ADD=0'], True),
   })
   def test_asyncify_indirect_lists(self, args, should_pass):
     self.set_setting('ASYNCIFY')

The Binaryen change has rolled in, but it takes a bit for new builds of tip of tree to be uploaded. Probably in an hour or two from now that should work.

[sbc100]

It looks like this change causes thelto2.test_asyncify_indirect_lists_add test to fail.

[kripken]

I think we just need to disable that test in LTO mode, I opened #21765 now.