Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 70 additions & 32 deletions src/passes/Asyncify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,15 @@
// input might reach code paths you missed during testing, so it's hard
// to know you got this right), so this is not recommended unless you
// really know what are doing, and need to optimize every bit of speed
// and size.
// and size. '*' wildcard matching supported.
//
// As with --asyncify-imports, you can use a response file here.
//
// --pass-arg=asyncify-whitelist@name1,name2,name3
//
// If the whitelist is provided, then only the functions in the list
// will be instrumented. Like the blacklist, getting this wrong will
// break your application.
// break your application. '*' wildcard matching supported.
//
// As with --asyncify-imports, you can use a response file here.
//
Expand Down Expand Up @@ -334,6 +334,65 @@ class GlobalHelper {
std::map<Name, Type> rev;
};

class PatternMatcher {
public:
std::string designation;
std::set<Name> names;
std::set<std::string> patterns;
std::set<std::string> patternsMatched;

PatternMatcher(std::string designation,
Module& module,
const String::Split& list)
: designation(designation) {
// The lists contain human-readable strings. Turn them into the
// internal escaped names for later comparisons
for (auto& name : list) {
auto escaped = WasmBinaryBuilder::escape(name);
if (name.find('*') != std::string::npos) {
patterns.insert(std::string(escaped.str));
} else {
auto* func = module.getFunctionOrNull(escaped);
if (!func) {
std::cerr << "warning: Asyncify " << designation
<< "list contained a non-existing function name: " << name
<< " (" << escaped << ")\n";
} else if (func->imported()) {
Fatal() << "Asyncify " << designation
<< "list contained an imported function name (use the import "
"list for imports): "
<< name << '\n';
}
names.insert(escaped.str);
}
}
}

bool match(Name funcName) {
if (names.count(funcName) > 0) {
return true;
} else {
for (auto& pattern : patterns) {
if (String::wildcardMatch(pattern, funcName.str)) {
patternsMatched.insert(pattern);
return true;
}
}
}
return false;
}

void checkPatternsMatches() {
for (auto& pattern : patterns) {
if (patternsMatched.count(pattern) == 0) {
std::cerr << "warning: Asyncify " << designation
<< "list contained a non-matching pattern: " << pattern
<< "\n";
}
}
}
};

// Analyze the entire module to see which calls may change the state, that
// is, start an unwind or rewind), either in itself or in something called
// by it.
Expand Down Expand Up @@ -374,8 +433,8 @@ class ModuleAnalyzer {
: module(module), canIndirectChangeState(canIndirectChangeState),
globals(module), asserts(asserts) {

blacklist.insert(blacklistInput.begin(), blacklistInput.end());
whitelist.insert(whitelistInput.begin(), whitelistInput.end());
PatternMatcher blacklist("black", module, blacklistInput);
PatternMatcher whitelist("white", module, whitelistInput);

// Scan to see which functions can directly change the state.
// Also handle the asyncify imports, removing them (as we will implement
Expand Down Expand Up @@ -456,7 +515,7 @@ class ModuleAnalyzer {
map.swap(scanner.map);

// Functions in the blacklist are assumed to not change the state.
for (auto& name : blacklist) {
for (auto& name : blacklist.names) {
if (auto* func = module.getFunctionOrNull(name)) {
map[func].canChangeState = false;
}
Expand Down Expand Up @@ -492,21 +551,24 @@ class ModuleAnalyzer {
auto* func = work.pop();
for (auto* caller : map[func].calledBy) {
if (!map[caller].canChangeState && !map[caller].isBottomMostRuntime &&
!blacklist.count(caller->name)) {
!blacklist.match(caller->name)) {
map[caller].canChangeState = true;
work.push(caller);
}
}
}

if (!whitelist.empty()) {
if (!whitelistInput.empty()) {
// Only the functions in the whitelist can change the state.
for (auto& func : module.functions) {
if (!func->imported()) {
map[func.get()].canChangeState = whitelist.count(func->name) > 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the behavior might have changed - before we always assigned to canChangeState, but in this PR it becomes only if it we turn it true. Maybe this explains the test failure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exactly. I didn't realize asyncify would apply auto-detection only to be overwritten by the whitelist. Perharps this can be optimized out, unless there's something left to do with imported functions.

map[func.get()].canChangeState = whitelist.match(func->name);
}
}
}

blacklist.checkPatternsMatches();
whitelist.checkPatternsMatches();
}

bool needsInstrumentation(Function* func) {
Expand Down Expand Up @@ -566,8 +628,6 @@ class ModuleAnalyzer {
}

GlobalHelper globals;
std::set<Name> blacklist;
std::set<Name> whitelist;
bool asserts;
};

Expand Down Expand Up @@ -1138,28 +1198,6 @@ struct Asyncify : public Pass {
blacklist = handleBracketingOperators(blacklist);
whitelist = handleBracketingOperators(whitelist);

// The lists contain human-readable strings. Turn them into the internal
// escaped names for later comparisons
auto processList = [module](String::Split& list, const std::string& which) {
for (auto& name : list) {
auto escaped = WasmBinaryBuilder::escape(name);
auto* func = module->getFunctionOrNull(escaped);
if (!func) {
std::cerr << "warning: Asyncify " << which
<< "list contained a non-existing function name: " << name
<< " (" << escaped << ")\n";
} else if (func->imported()) {
Fatal() << "Asyncify " << which
<< "list contained an imported function name (use the import "
"list for imports): "
<< name << '\n';
}
name = escaped.str;
}
};
processList(blacklist, "black");
processList(whitelist, "white");

if (!blacklist.empty() && !whitelist.empty()) {
Fatal() << "It makes no sense to use both a blacklist and a whitelist "
"with asyncify.";
Expand Down
11 changes: 6 additions & 5 deletions src/support/string.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,18 @@ inline String::Split handleBracketingOperators(String::Split split) {
return ret;
}

// Does a simple wildcard match between a pattern and a value. Currently
// supports a '*' at the end of the pattern.
// Does a simple '*' wildcard match between a pattern and a value.
inline bool wildcardMatch(const std::string& pattern,
const std::string& value) {
for (size_t i = 0; i < pattern.size(); i++) {
if (pattern[i] == '*') {
return wildcardMatch(pattern.substr(i + 1), value.substr(i)) ||
(value.size() > 0 &&
wildcardMatch(pattern.substr(i), value.substr(i + 1)));
}
if (i >= value.size()) {
return false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment on line 90-91 is now obsolete, please update it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
if (pattern[i] == '*') {
return true;
}
if (pattern[i] != value[i]) {
return false;
}
Expand Down
7 changes: 6 additions & 1 deletion test/unit/test_asyncify.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,12 @@ def test_asyncify_list_bad(self):
('--pass-arg=asyncify-whitelist@nonexistent', 'nonexistent'),
('--pass-arg=asyncify-blacklist@main', None),
('--pass-arg=asyncify-whitelist@main', None),
('--pass-arg=asyncify-whitelist@main', None),
('--pass-arg=asyncify-blacklist@m*n', None),
('--pass-arg=asyncify-whitelist@m*n', None),
('--pass-arg=asyncify-whitelist@main*', None),
('--pass-arg=asyncify-whitelist@*main', None),
('--pass-arg=asyncify-blacklist@non*existent', 'non*existent'),
('--pass-arg=asyncify-whitelist@non*existent', 'non*existent'),
Comment thread
kripken marked this conversation as resolved.
('--pass-arg=asyncify-whitelist@DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)', None),
]:
print(arg, warning)
Expand Down