From 6d777d4d7d473df98fc2fdb8033354f9318b91e3 Mon Sep 17 00:00:00 2001 From: Sylvain Beucler Date: Wed, 18 Sep 2019 12:18:42 +0200 Subject: [PATCH 1/5] asyncify: support *-matching in whitelist and blacklist --- src/passes/Asyncify.cpp | 44 +++++++++++++++++++++++--------------- src/support/string.h | 3 ++- test/unit/test_asyncify.py | 3 ++- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index bca9941ae86..e380913865b 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -229,7 +229,7 @@ // 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. // @@ -237,7 +237,7 @@ // // 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. // @@ -368,15 +368,12 @@ class ModuleAnalyzer { ModuleAnalyzer(Module& module, std::function canImportChangeState, bool canIndirectChangeState, - const String::Split& blacklistInput, - const String::Split& whitelistInput, + const String::Split& blacklist, + const String::Split& whitelist, bool asserts) : module(module), canIndirectChangeState(canIndirectChangeState), globals(module), asserts(asserts) { - blacklist.insert(blacklistInput.begin(), blacklistInput.end()); - whitelist.insert(whitelistInput.begin(), whitelistInput.end()); - // Scan to see which functions can directly change the state. // Also handle the asyncify imports, removing them (as we will implement // them later), and replace calls to them with calls to the later proper @@ -491,10 +488,18 @@ class ModuleAnalyzer { while (!work.empty()) { auto* func = work.pop(); for (auto* caller : map[func].calledBy) { - if (!map[caller].canChangeState && !map[caller].isBottomMostRuntime && - !blacklist.count(caller->name)) { - map[caller].canChangeState = true; - work.push(caller); + if (!map[caller].canChangeState && !map[caller].isBottomMostRuntime) { + bool matched = false; + for (auto& pattern : blacklist) { + if (String::wildcardMatch(pattern, std::string(caller->name.str))) { + matched = true; + break; + } + } + if (!matched) { + map[caller].canChangeState = true; + work.push(caller); + } } } } @@ -503,7 +508,12 @@ class ModuleAnalyzer { // 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; + for (auto& pattern : whitelist) { + if (String::wildcardMatch(pattern, std::string(func->name.str))) { + map[func.get()].canChangeState = true; + break; + } + } } } } @@ -566,8 +576,6 @@ class ModuleAnalyzer { } GlobalHelper globals; - std::set blacklist; - std::set whitelist; bool asserts; }; @@ -1145,9 +1153,11 @@ struct Asyncify : public Pass { 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"; + if (name.find('*') == std::string::npos) { + 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 " diff --git a/src/support/string.h b/src/support/string.h index b3d12c6aeb6..1495837795a 100644 --- a/src/support/string.h +++ b/src/support/string.h @@ -96,7 +96,8 @@ inline bool wildcardMatch(const std::string& pattern, return false; } if (pattern[i] == '*') { - return true; + return wildcardMatch(pattern.substr(i+1, std::string::npos), value.substr(i, std::string::npos)) + || wildcardMatch(pattern.substr(i, std::string::npos), value.substr(i+1, std::string::npos)); } if (pattern[i] != value[i]) { return false; diff --git a/test/unit/test_asyncify.py b/test/unit/test_asyncify.py index e2fe82af91a..4ed371f8455 100644 --- a/test/unit/test_asyncify.py +++ b/test/unit/test_asyncify.py @@ -36,7 +36,8 @@ 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@DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)', None), ]: print(arg, warning) From fb870e8c8e39a78a1ca1d89f2240ee7f96c4f9d2 Mon Sep 17 00:00:00 2001 From: Sylvain Beucler Date: Wed, 18 Sep 2019 16:00:13 +0200 Subject: [PATCH 2/5] clang-format --- src/support/string.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/support/string.h b/src/support/string.h index 1495837795a..639f9a571ac 100644 --- a/src/support/string.h +++ b/src/support/string.h @@ -96,8 +96,10 @@ inline bool wildcardMatch(const std::string& pattern, return false; } if (pattern[i] == '*') { - return wildcardMatch(pattern.substr(i+1, std::string::npos), value.substr(i, std::string::npos)) - || wildcardMatch(pattern.substr(i, std::string::npos), value.substr(i+1, std::string::npos)); + return wildcardMatch(pattern.substr(i + 1, std::string::npos), + value.substr(i, std::string::npos)) || + wildcardMatch(pattern.substr(i, std::string::npos), + value.substr(i + 1, std::string::npos)); } if (pattern[i] != value[i]) { return false; From 1673774edddf2e74f55bbdd1003f8e094900fd27 Mon Sep 17 00:00:00 2001 From: Sylvain Beucler Date: Thu, 19 Sep 2019 17:54:03 +0200 Subject: [PATCH 3/5] Better perfs, better reporting, fix empty wildcard matching --- src/passes/Asyncify.cpp | 108 +++++++++++++++++++++++++------------ src/support/string.h | 14 +++-- test/unit/test_asyncify.py | 4 ++ 3 files changed, 85 insertions(+), 41 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index e380913865b..3827763a1b4 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -452,8 +452,45 @@ class ModuleAnalyzer { map.swap(scanner.map); + std::set blacklist_names; + std::set whitelist_names; + std::set blacklist_patterns; + std::set whitelist_patterns; + std::set blacklist_patterns_matched; + std::set whitelist_patterns_matched; + + // The lists contain human-readable strings. Turn them into the internal + // escaped names for later comparisons + auto processList = [&module](const String::Split& list, + std::set& list_names, + std::set& list_patterns, + const std::string& which) { + for (auto& name : list) { + auto escaped = WasmBinaryBuilder::escape(name); + if (name.find('*') != std::string::npos) { + list_patterns.insert(std::string(escaped.str)); + } else { + 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'; + } + list_names.insert(escaped.str); + } + } + }; + processList(blacklist, blacklist_names, blacklist_patterns, "black"); + processList(whitelist, whitelist_names, whitelist_patterns, "white"); + // 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; } @@ -490,10 +527,17 @@ class ModuleAnalyzer { for (auto* caller : map[func].calledBy) { if (!map[caller].canChangeState && !map[caller].isBottomMostRuntime) { bool matched = false; - for (auto& pattern : blacklist) { - if (String::wildcardMatch(pattern, std::string(caller->name.str))) { - matched = true; - break; + if (blacklist_names.count(caller->name)) { + matched = true; + } + if (!matched) { + for (auto& pattern : blacklist_patterns) { + if (String::wildcardMatch(pattern, + std::string(caller->name.str))) { + matched = true; + blacklist_patterns_matched.insert(pattern); + break; + } } } if (!matched) { @@ -508,15 +552,37 @@ class ModuleAnalyzer { // Only the functions in the whitelist can change the state. for (auto& func : module.functions) { if (!func->imported()) { - for (auto& pattern : whitelist) { - if (String::wildcardMatch(pattern, std::string(func->name.str))) { - map[func.get()].canChangeState = true; - break; + map[func.get()].canChangeState = false; + if (whitelist_names.count(func->name) > 0) { + map[func.get()].canChangeState = true; + } else { + for (auto& pattern : whitelist_patterns) { + if (String::wildcardMatch(pattern, std::string(func->name.str))) { + map[func.get()].canChangeState = true; + whitelist_patterns_matched.insert(pattern); + break; + } } } } } } + + auto checkPatternsMatches = [](std::set patterns, + std::set patterns_matched, + const std::string& which) { + for (auto& pattern : patterns) { + if (patterns_matched.count(pattern) == 0) { + std::cerr << "warning: Asyncify " << which + << "list contained a non-matching pattern: " << pattern + << "\n"; + } + } + }; + checkPatternsMatches( + blacklist_patterns, blacklist_patterns_matched, "black"); + checkPatternsMatches( + whitelist_patterns, whitelist_patterns_matched, "white"); } bool needsInstrumentation(Function* func) { @@ -1146,30 +1212,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) { - if (name.find('*') == std::string::npos) { - 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."; diff --git a/src/support/string.h b/src/support/string.h index 639f9a571ac..b93d3b36367 100644 --- a/src/support/string.h +++ b/src/support/string.h @@ -87,20 +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; } - if (pattern[i] == '*') { - return wildcardMatch(pattern.substr(i + 1, std::string::npos), - value.substr(i, std::string::npos)) || - wildcardMatch(pattern.substr(i, std::string::npos), - value.substr(i + 1, std::string::npos)); - } if (pattern[i] != value[i]) { return false; } diff --git a/test/unit/test_asyncify.py b/test/unit/test_asyncify.py index 4ed371f8455..9b7c403c038 100644 --- a/test/unit/test_asyncify.py +++ b/test/unit/test_asyncify.py @@ -38,6 +38,10 @@ def test_asyncify_list_bad(self): ('--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'), ('--pass-arg=asyncify-whitelist@DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)', None), ]: print(arg, warning) From a686386f1c35c4647f14c5849022e492f15aefaa Mon Sep 17 00:00:00 2001 From: Sylvain Beucler Date: Thu, 19 Sep 2019 19:47:26 +0200 Subject: [PATCH 4/5] Naming convention --- src/passes/Asyncify.cpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index 3827763a1b4..d20b3ce2bfa 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -454,21 +454,21 @@ class ModuleAnalyzer { std::set blacklist_names; std::set whitelist_names; - std::set blacklist_patterns; - std::set whitelist_patterns; - std::set blacklist_patterns_matched; - std::set whitelist_patterns_matched; + std::set blacklistPatterns; + std::set whitelistPatterns; + std::set blacklistPatternsMatched; + std::set whitelistPatternsMatched; // The lists contain human-readable strings. Turn them into the internal // escaped names for later comparisons auto processList = [&module](const String::Split& list, std::set& list_names, - std::set& list_patterns, + std::set& listPatterns, const std::string& which) { for (auto& name : list) { auto escaped = WasmBinaryBuilder::escape(name); if (name.find('*') != std::string::npos) { - list_patterns.insert(std::string(escaped.str)); + listPatterns.insert(std::string(escaped.str)); } else { auto* func = module.getFunctionOrNull(escaped); if (!func) { @@ -486,8 +486,8 @@ class ModuleAnalyzer { } } }; - processList(blacklist, blacklist_names, blacklist_patterns, "black"); - processList(whitelist, whitelist_names, whitelist_patterns, "white"); + processList(blacklist, blacklist_names, blacklistPatterns, "black"); + processList(whitelist, whitelist_names, whitelistPatterns, "white"); // Functions in the blacklist are assumed to not change the state. for (auto& name : blacklist_names) { @@ -531,11 +531,11 @@ class ModuleAnalyzer { matched = true; } if (!matched) { - for (auto& pattern : blacklist_patterns) { + for (auto& pattern : blacklistPatterns) { if (String::wildcardMatch(pattern, std::string(caller->name.str))) { matched = true; - blacklist_patterns_matched.insert(pattern); + blacklistPatternsMatched.insert(pattern); break; } } @@ -556,10 +556,10 @@ class ModuleAnalyzer { if (whitelist_names.count(func->name) > 0) { map[func.get()].canChangeState = true; } else { - for (auto& pattern : whitelist_patterns) { + for (auto& pattern : whitelistPatterns) { if (String::wildcardMatch(pattern, std::string(func->name.str))) { map[func.get()].canChangeState = true; - whitelist_patterns_matched.insert(pattern); + whitelistPatternsMatched.insert(pattern); break; } } @@ -569,10 +569,10 @@ class ModuleAnalyzer { } auto checkPatternsMatches = [](std::set patterns, - std::set patterns_matched, + std::set patternsMatched, const std::string& which) { for (auto& pattern : patterns) { - if (patterns_matched.count(pattern) == 0) { + if (patternsMatched.count(pattern) == 0) { std::cerr << "warning: Asyncify " << which << "list contained a non-matching pattern: " << pattern << "\n"; @@ -580,9 +580,9 @@ class ModuleAnalyzer { } }; checkPatternsMatches( - blacklist_patterns, blacklist_patterns_matched, "black"); + blacklistPatterns, blacklistPatternsMatched, "black"); checkPatternsMatches( - whitelist_patterns, whitelist_patterns_matched, "white"); + whitelistPatterns, whitelistPatternsMatched, "white"); } bool needsInstrumentation(Function* func) { From 39f2b07a595f410e2b2c1b821b2a9ec7177678c3 Mon Sep 17 00:00:00 2001 From: Sylvain Beucler Date: Mon, 23 Sep 2019 20:32:11 +0200 Subject: [PATCH 5/5] Refactoring --- src/passes/Asyncify.cpp | 160 ++++++++++++++++++---------------------- 1 file changed, 73 insertions(+), 87 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index d20b3ce2bfa..d307ae31bb5 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -334,6 +334,65 @@ class GlobalHelper { std::map rev; }; +class PatternMatcher { +public: + std::string designation; + std::set names; + std::set patterns; + std::set 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. @@ -368,12 +427,15 @@ class ModuleAnalyzer { ModuleAnalyzer(Module& module, std::function canImportChangeState, bool canIndirectChangeState, - const String::Split& blacklist, - const String::Split& whitelist, + const String::Split& blacklistInput, + const String::Split& whitelistInput, bool asserts) : module(module), canIndirectChangeState(canIndirectChangeState), globals(module), asserts(asserts) { + 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 // them later), and replace calls to them with calls to the later proper @@ -452,45 +514,8 @@ class ModuleAnalyzer { map.swap(scanner.map); - std::set blacklist_names; - std::set whitelist_names; - std::set blacklistPatterns; - std::set whitelistPatterns; - std::set blacklistPatternsMatched; - std::set whitelistPatternsMatched; - - // The lists contain human-readable strings. Turn them into the internal - // escaped names for later comparisons - auto processList = [&module](const String::Split& list, - std::set& list_names, - std::set& listPatterns, - const std::string& which) { - for (auto& name : list) { - auto escaped = WasmBinaryBuilder::escape(name); - if (name.find('*') != std::string::npos) { - listPatterns.insert(std::string(escaped.str)); - } else { - 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'; - } - list_names.insert(escaped.str); - } - } - }; - processList(blacklist, blacklist_names, blacklistPatterns, "black"); - processList(whitelist, whitelist_names, whitelistPatterns, "white"); - // Functions in the blacklist are assumed to not change the state. - for (auto& name : blacklist_names) { + for (auto& name : blacklist.names) { if (auto* func = module.getFunctionOrNull(name)) { map[func].canChangeState = false; } @@ -525,64 +550,25 @@ class ModuleAnalyzer { while (!work.empty()) { auto* func = work.pop(); for (auto* caller : map[func].calledBy) { - if (!map[caller].canChangeState && !map[caller].isBottomMostRuntime) { - bool matched = false; - if (blacklist_names.count(caller->name)) { - matched = true; - } - if (!matched) { - for (auto& pattern : blacklistPatterns) { - if (String::wildcardMatch(pattern, - std::string(caller->name.str))) { - matched = true; - blacklistPatternsMatched.insert(pattern); - break; - } - } - } - if (!matched) { - map[caller].canChangeState = true; - work.push(caller); - } + if (!map[caller].canChangeState && !map[caller].isBottomMostRuntime && + !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 = false; - if (whitelist_names.count(func->name) > 0) { - map[func.get()].canChangeState = true; - } else { - for (auto& pattern : whitelistPatterns) { - if (String::wildcardMatch(pattern, std::string(func->name.str))) { - map[func.get()].canChangeState = true; - whitelistPatternsMatched.insert(pattern); - break; - } - } - } + map[func.get()].canChangeState = whitelist.match(func->name); } } } - auto checkPatternsMatches = [](std::set patterns, - std::set patternsMatched, - const std::string& which) { - for (auto& pattern : patterns) { - if (patternsMatched.count(pattern) == 0) { - std::cerr << "warning: Asyncify " << which - << "list contained a non-matching pattern: " << pattern - << "\n"; - } - } - }; - checkPatternsMatches( - blacklistPatterns, blacklistPatternsMatched, "black"); - checkPatternsMatches( - whitelistPatterns, whitelistPatternsMatched, "white"); + blacklist.checkPatternsMatches(); + whitelist.checkPatternsMatches(); } bool needsInstrumentation(Function* func) {