Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asyncify: Instrument indirect calls from functions in add-list or only-list #2913

Merged
merged 8 commits into from
Jun 17, 2020

Conversation

kripken
Copy link
Member

@kripken kripken commented Jun 15, 2020

When doing manual tuning of calls using asyncify lists, we want it to
be possible to write out all the functions that can be on the stack when
pausing, and for that to work. This did not quite work right with the
ignore-indirect option: that would ignore all indirect calls all the
time, so that if foo() calls bar() indirectly, that indirect call was
not instrumented (we didn't check for a pause around it), even if
both foo() and bar() were listed. There was no way to make that
work (except for not ignoring indirect calls at all).

This PR makes the add-list and only-lists fully instrument the functions
mentioned in them: both themselves, and indirect calls from them.
(Note that direct calls need no special handling - we can just add
the direct call target to the add-list or only-list.)

This may add some overhead to existing users, but only in a function
that is instrumented anyhow, and also indirect calls are slow anyhow,
so it's probably fine. And it is simpler to do it this way instead of
adding another list for indirect call handling.

@kripken kripken requested a review from tlively June 16, 2020 18:41
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

src/passes/Asyncify.cpp Outdated Show resolved Hide resolved
Comment on lines 736 to 739
if (walker.hasIndirectCall &&
(canIndirectChangeState || map[func].addedFromList)) {
walker.canChangeState = true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this logic could be combined with the return below to be just one return statement. I think this might be simpler than the the current code.

Copy link
Member Author

@kripken kripken Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you suggesting they be combined?

I guess 738 could be return true. Or the 4 lines could be return walker.canChangeState || (walker.hasIndirectCall && (canIndirectChangeState || map[func].addedFromList)). But both seem less good to me?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the latter, but I think the former would also be an improvement. I just prefer to have to track less state and control flow. But this is very much a nit, so I leave it to you :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to explain why I prefer it as it is, combined with the lines above, this overall code has the form

compute result
// explain exception 1
if ..exception1.. set result to false
// explain exception 2
if ..complex exception2.. set result to true
return result

Actually re-reading it now, those two exceptions are in the wrong order anyhow, so this is moot... reordering, and then the condition before the return is very small so I agree it can be merged with the return, doing it that way.

@kripken kripken merged commit 251a68b into master Jun 17, 2020
@kripken kripken deleted the more branch June 17, 2020 15:06
kripken added a commit to emscripten-core/emscripten that referenced this pull request Jun 18, 2020
…nges (#11438)

Add support for the new ASYNCIFY_ADD_LIST, and update existing list names
following the updates in Binaryen, so that now we have ASYNCIFY_ADD_LIST to
add a function, ASYNCIFY_REMOVE_LIST to remove one (previously this was
called ASYNCIFY_BLACKLIST), and ASYNCIFY_ONLY_LIST to set a list of the
only functions to instrument and no others (previously this was called
ASYNCIFY_WHITELIST).

The updated lists also handle indirect calls properly, so that if you
use ASYNCIFY_IGNORE_INDIRECT and then add (using either the
add-list or the only-list) all the functions that are on the stack when
pausing, then things will work (for more, see
WebAssembly/binaryen#2913). A new test is
added to show this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants