From 99a84c83169a4443d844836bf5d892ec55130448 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 2 Jun 2016 16:04:20 -0700 Subject: [PATCH 01/10] Remove __cachectl. bionic doesn't have an implementation for this function, but neither does the kernel. cachectl has existed in the kernel as a stub that returns ENOSYS for over a decade. Bug: http://b/28178111 Change-Id: Id35f85fd143c5ea0d45d04b1021893cf5c0c749d --- libc/include/sys/cachectl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/libc/include/sys/cachectl.h b/libc/include/sys/cachectl.h index a302ff806..08302932d 100644 --- a/libc/include/sys/cachectl.h +++ b/libc/include/sys/cachectl.h @@ -30,6 +30,5 @@ #ifdef __mips__ #include -extern int __cachectl (void *addr, __const int nbytes, __const int op); #endif #endif /* sys/cachectl.h */ From 62aaf8f8fe123e34ec0f66b0cd51ec24e304c9b5 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 2 Jun 2016 14:27:21 -0700 Subject: [PATCH 02/10] versioner: improve usage messages. Don't spew all of usage when called improperly, and add a -h option that exits cleanly. Change-Id: I1a4517edce75afe0f9a80bc8d6c81353d6c12e99 --- tools/versioner/src/versioner.cpp | 43 +++++++++++++++++++------------ 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/tools/versioner/src/versioner.cpp b/tools/versioner/src/versioner.cpp index 1612ed000..5068a4e80 100644 --- a/tools/versioner/src/versioner.cpp +++ b/tools/versioner/src/versioner.cpp @@ -470,21 +470,28 @@ static bool checkVersions(const std::set& types, return !failed; } -static void usage() { - fprintf(stderr, "Usage: versioner [OPTION]... HEADER_PATH [DEPS_PATH]\n"); - fprintf(stderr, "Version headers at HEADER_PATH, with DEPS_PATH/ARCH/* on the include path\n"); - fprintf(stderr, "\n"); - fprintf(stderr, "Target specification (defaults to all):\n"); - fprintf(stderr, " -a API_LEVEL\tbuild with specified API level (can be repeated)\n"); - fprintf(stderr, " \t\tvalid levels are %s\n", Join(supported_levels).c_str()); - fprintf(stderr, " -r ARCH\tbuild with specified architecture (can be repeated)\n"); - fprintf(stderr, " \t\tvalid architectures are %s\n", Join(supported_archs).c_str()); - fprintf(stderr, "\n"); - fprintf(stderr, "Validation:\n"); - fprintf(stderr, " -p PATH\tcompare against NDK platform at PATH\n"); - fprintf(stderr, " -d\t\tdump symbol availability in libraries\n"); - fprintf(stderr, " -v\t\tenable verbose warnings\n"); - exit(1); +static void usage(bool help = false) { + fprintf(stderr, "Usage: versioner [OPTION]... [HEADER_PATH] [DEPS_PATH]\n"); + if (!help) { + printf("Try 'versioner -h' for more information.\n"); + exit(1); + } else { + fprintf(stderr, "Version headers at HEADER_PATH, with DEPS_PATH/ARCH/* on the include path\n"); + fprintf(stderr, "\n"); + fprintf(stderr, "Target specification (defaults to all):\n"); + fprintf(stderr, " -a API_LEVEL\tbuild with specified API level (can be repeated)\n"); + fprintf(stderr, " \t\tvalid levels are %s\n", Join(supported_levels).c_str()); + fprintf(stderr, " -r ARCH\tbuild with specified architecture (can be repeated)\n"); + fprintf(stderr, " \t\tvalid architectures are %s\n", Join(supported_archs).c_str()); + fprintf(stderr, "\n"); + fprintf(stderr, "Validation:\n"); + fprintf(stderr, " -p PATH\tcompare against NDK platform at PATH\n"); + fprintf(stderr, " -v\t\tenable verbose warnings\n"); + fprintf(stderr, "\n"); + fprintf(stderr, "Miscellaneous:\n"); + fprintf(stderr, " -h\t\tdisplay this message\n"); + exit(0); + } } int main(int argc, char** argv) { @@ -495,7 +502,7 @@ int main(int argc, char** argv) { std::set selected_levels; int c; - while ((c = getopt(argc, argv, "a:r:p:n:duv")) != -1) { + while ((c = getopt(argc, argv, "a:r:p:vh")) != -1) { default_args = false; switch (c) { case 'a': { @@ -542,6 +549,10 @@ int main(int argc, char** argv) { verbose = true; break; + case 'h': + usage(true); + break; + default: usage(); break; From 9b5af7ad5e5b36d751f152382971b034161755e5 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 2 Jun 2016 14:29:13 -0700 Subject: [PATCH 03/10] versioner: autodetect paths when no specified. Search for the header/dependency/platform directories in a hard-coded path relative to $ANDROID_BUILD_TOP when they're not specified. Change-Id: I476385cfc0247e3b2009348ec37c1810a0e9a7f7 --- tools/versioner/src/versioner.cpp | 38 +++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/tools/versioner/src/versioner.cpp b/tools/versioner/src/versioner.cpp index 5068a4e80..49cbc9f41 100644 --- a/tools/versioner/src/versioner.cpp +++ b/tools/versioner/src/versioner.cpp @@ -477,6 +477,7 @@ static void usage(bool help = false) { exit(1); } else { fprintf(stderr, "Version headers at HEADER_PATH, with DEPS_PATH/ARCH/* on the include path\n"); + fprintf(stderr, "Autodetects paths if HEADER_PATH and DEPS_PATH are not specified\n"); fprintf(stderr, "\n"); fprintf(stderr, "Target specification (defaults to all):\n"); fprintf(stderr, " -a API_LEVEL\tbuild with specified API level (can be repeated)\n"); @@ -559,10 +560,35 @@ int main(int argc, char** argv) { } } - if (argc - optind > 2 || optind >= argc) { + if (argc - optind > 2 || optind > argc) { usage(); } + std::string header_dir; + std::string dependency_dir; + + if (optind == argc) { + // Neither HEADER_PATH nor DEPS_PATH were specified, so try to figure them out. + const char* top = getenv("ANDROID_BUILD_TOP"); + if (!top) { + fprintf(stderr, "versioner: failed to autodetect bionic paths. Is ANDROID_BUILD_TOP set?\n"); + usage(); + } + + std::string versioner_dir = std::to_string(top) + "/bionic/tools/versioner"; + header_dir = versioner_dir + "/current"; + dependency_dir = versioner_dir + "/dependencies"; + if (platform_dir.empty()) { + platform_dir = versioner_dir + "/platforms"; + } + } else { + header_dir = argv[optind]; + + if (argc - optind == 2) { + dependency_dir = argv[optind + 1]; + } + } + if (selected_levels.empty()) { selected_levels = supported_levels; } @@ -571,14 +597,12 @@ int main(int argc, char** argv) { selected_architectures = supported_archs; } - std::string dependencies = (argc - optind == 2) ? argv[optind + 1] : ""; - const char* header_dir = argv[optind]; struct stat st; - if (stat(header_dir, &st) != 0) { - err(1, "failed to stat '%s'", header_dir); + if (stat(header_dir.c_str(), &st) != 0) { + err(1, "failed to stat '%s'", header_dir.c_str()); } else if (!S_ISDIR(st.st_mode)) { - errx(1, "'%s' is not a directory", header_dir); + errx(1, "'%s' is not a directory", header_dir.c_str()); } std::set compilation_types; @@ -594,7 +618,7 @@ int main(int argc, char** argv) { } bool failed = false; - declaration_database = compileHeaders(compilation_types, header_dir, dependencies, &failed); + declaration_database = compileHeaders(compilation_types, header_dir, dependency_dir, &failed); if (!sanityCheck(compilation_types, declaration_database)) { printf("versioner: sanity check failed\n"); From d67dbf003e6b203a4ec41d01081190d3e40f00e2 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 2 Jun 2016 15:21:14 -0700 Subject: [PATCH 04/10] versioner: ignore functions that are __INTRODUCED_IN_FUTURE. Bug: http://b/28178111 Change-Id: I8026181e08ed8f2d59b31a37adcf8b469fb6bdaf --- tools/versioner/src/versioner.cpp | 27 +++++++++++++++---- tools/versioner/tests/future/headers/foo.h | 1 + .../arch-arm/symbols/libc.so.functions.txt | 0 tools/versioner/tests/future/run.sh | 1 + .../versioner/tests/future_arch/headers/foo.h | 5 ++++ .../arch-arm/symbols/libc.so.functions.txt | 1 + .../arch-x86/symbols/libc.so.functions.txt | 0 tools/versioner/tests/future_arch/run.sh | 1 + 8 files changed, 31 insertions(+), 5 deletions(-) create mode 100644 tools/versioner/tests/future/headers/foo.h create mode 100644 tools/versioner/tests/future/platforms/android-9/arch-arm/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/future/run.sh create mode 100644 tools/versioner/tests/future_arch/headers/foo.h create mode 100644 tools/versioner/tests/future_arch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/future_arch/platforms/android-9/arch-x86/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/future_arch/run.sh diff --git a/tools/versioner/src/versioner.cpp b/tools/versioner/src/versioner.cpp index 49cbc9f41..91482edb1 100644 --- a/tools/versioner/src/versioner.cpp +++ b/tools/versioner/src/versioner.cpp @@ -341,17 +341,15 @@ static bool checkVersions(const std::set& types, arch_types[type.arch].insert(type); } + std::set completely_unavailable; + for (const auto& outer : declaration_database) { const std::string& symbol_name = outer.first; const auto& compilations = outer.second; auto platform_availability_it = symbol_database.find(symbol_name); if (platform_availability_it == symbol_database.end()) { - // This currently has lots of false positives (__INTRODUCED_IN_FUTURE, __errordecl, functions - // that come from crtbegin, etc.). Only print them with verbose, because of this. - if (verbose) { - printf("%s: not available in any platform\n", symbol_name.c_str()); - } + completely_unavailable.insert(symbol_name); continue; } @@ -467,6 +465,25 @@ static bool checkVersions(const std::set& types, } } + for (const std::string& symbol_name : completely_unavailable) { + // This currently has some false positives (mostly functions that come from crtbegin). + // Therefore, only report these declarations when running with verbose for now. + if (!verbose) { + break; + } + + // Check to see if the symbol is tagged with __INTRODUCED_IN_FUTURE. + auto symbol_it = declaration_database.find(symbol_name); + const Declaration& declaration = symbol_it->second.begin()->second; + DeclarationAvailability availability = declaration.locations.begin()->availability; + if (availability.introduced >= 10000) { + continue; + } + + printf("%s: not available in any platform\n", symbol_name.c_str()); + failed = true; + } + return !failed; } diff --git a/tools/versioner/tests/future/headers/foo.h b/tools/versioner/tests/future/headers/foo.h new file mode 100644 index 000000000..b5113f4bd --- /dev/null +++ b/tools/versioner/tests/future/headers/foo.h @@ -0,0 +1 @@ +int foo() __attribute__((availability(android, introduced = 10000))); diff --git a/tools/versioner/tests/future/platforms/android-9/arch-arm/symbols/libc.so.functions.txt b/tools/versioner/tests/future/platforms/android-9/arch-arm/symbols/libc.so.functions.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tools/versioner/tests/future/run.sh b/tools/versioner/tests/future/run.sh new file mode 100644 index 000000000..0dea98ff7 --- /dev/null +++ b/tools/versioner/tests/future/run.sh @@ -0,0 +1 @@ +versioner -v headers -p platforms -r arm -a 9 diff --git a/tools/versioner/tests/future_arch/headers/foo.h b/tools/versioner/tests/future_arch/headers/foo.h new file mode 100644 index 000000000..674097578 --- /dev/null +++ b/tools/versioner/tests/future_arch/headers/foo.h @@ -0,0 +1,5 @@ +#if defined(__arm__) +int foo() __attribute__((availability(android, introduced = 9))); +#else +int foo() __attribute__((availability(android, introduced = 10000))); +#endif diff --git a/tools/versioner/tests/future_arch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt b/tools/versioner/tests/future_arch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt new file mode 100644 index 000000000..257cc5642 --- /dev/null +++ b/tools/versioner/tests/future_arch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt @@ -0,0 +1 @@ +foo diff --git a/tools/versioner/tests/future_arch/platforms/android-9/arch-x86/symbols/libc.so.functions.txt b/tools/versioner/tests/future_arch/platforms/android-9/arch-x86/symbols/libc.so.functions.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tools/versioner/tests/future_arch/run.sh b/tools/versioner/tests/future_arch/run.sh new file mode 100644 index 000000000..36846daa5 --- /dev/null +++ b/tools/versioner/tests/future_arch/run.sh @@ -0,0 +1 @@ +versioner -v headers -p platforms -r arm -r x86 -a 9 From 80d909bbfb0ffb4955caebd80acf98120cb1b130 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 2 Jun 2016 15:59:32 -0700 Subject: [PATCH 05/10] versioner: clean up tests, test runner. Git doesn't track empty directories, so most of the tests would fail on a fresh checkout because of dependencies/common being missing. Remove the use of dependencies from all of the non-dependency related tests. Change-Id: I09cc5765aae1576914c1c5d7dfa3fb666eab4a3f --- .../tests/arch_specific/dependencies/common/foo/foodep.h | 1 - tools/versioner/tests/arch_specific/run.sh | 2 +- tools/versioner/tests/compilation_error/run.sh | 2 +- tools/versioner/tests/errordecl/headers/foo.h | 1 + .../platforms/android-9/arch-arm/symbols/libc.so.functions.txt | 0 tools/versioner/tests/errordecl/run.sh | 1 + tools/versioner/tests/inline/run.sh | 2 +- tools/versioner/tests/inline_version_mismatch/run.sh | 2 +- tools/versioner/tests/missing_api/run.sh | 2 +- .../tests/missing_arch/dependencies/common/foo/foodep.h | 1 - tools/versioner/tests/missing_arch/run.sh | 2 +- tools/versioner/tests/multiple_decl/run.sh | 2 +- tools/versioner/tests/multiple_decl_mismatch/run.sh | 2 +- tools/versioner/tests/obsoleted/run.sh | 2 +- tools/versioner/tests/smoke/run.sh | 2 +- 15 files changed, 12 insertions(+), 12 deletions(-) delete mode 100644 tools/versioner/tests/arch_specific/dependencies/common/foo/foodep.h create mode 100644 tools/versioner/tests/errordecl/headers/foo.h create mode 100644 tools/versioner/tests/errordecl/platforms/android-9/arch-arm/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/errordecl/run.sh delete mode 100644 tools/versioner/tests/missing_arch/dependencies/common/foo/foodep.h diff --git a/tools/versioner/tests/arch_specific/dependencies/common/foo/foodep.h b/tools/versioner/tests/arch_specific/dependencies/common/foo/foodep.h deleted file mode 100644 index 9feeb6c4c..000000000 --- a/tools/versioner/tests/arch_specific/dependencies/common/foo/foodep.h +++ /dev/null @@ -1 +0,0 @@ -typedef int foo_t; diff --git a/tools/versioner/tests/arch_specific/run.sh b/tools/versioner/tests/arch_specific/run.sh index 3a3dda8b8..6d97fb005 100644 --- a/tools/versioner/tests/arch_specific/run.sh +++ b/tools/versioner/tests/arch_specific/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -r x86 -a 9 +versioner headers -p platforms -r arm -r x86 -a 9 diff --git a/tools/versioner/tests/compilation_error/run.sh b/tools/versioner/tests/compilation_error/run.sh index d26ab7088..8babb73a4 100644 --- a/tools/versioner/tests/compilation_error/run.sh +++ b/tools/versioner/tests/compilation_error/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 +versioner headers -p platforms -r arm -a 9 diff --git a/tools/versioner/tests/errordecl/headers/foo.h b/tools/versioner/tests/errordecl/headers/foo.h new file mode 100644 index 000000000..c4664202e --- /dev/null +++ b/tools/versioner/tests/errordecl/headers/foo.h @@ -0,0 +1 @@ +int foo() __attribute__((unavailable)); diff --git a/tools/versioner/tests/errordecl/platforms/android-9/arch-arm/symbols/libc.so.functions.txt b/tools/versioner/tests/errordecl/platforms/android-9/arch-arm/symbols/libc.so.functions.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tools/versioner/tests/errordecl/run.sh b/tools/versioner/tests/errordecl/run.sh new file mode 100644 index 000000000..0dea98ff7 --- /dev/null +++ b/tools/versioner/tests/errordecl/run.sh @@ -0,0 +1 @@ +versioner -v headers -p platforms -r arm -a 9 diff --git a/tools/versioner/tests/inline/run.sh b/tools/versioner/tests/inline/run.sh index 027712386..914c55d32 100644 --- a/tools/versioner/tests/inline/run.sh +++ b/tools/versioner/tests/inline/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 -a 12 +versioner headers -p platforms -r arm -a 9 -a 12 diff --git a/tools/versioner/tests/inline_version_mismatch/run.sh b/tools/versioner/tests/inline_version_mismatch/run.sh index 027712386..914c55d32 100644 --- a/tools/versioner/tests/inline_version_mismatch/run.sh +++ b/tools/versioner/tests/inline_version_mismatch/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 -a 12 +versioner headers -p platforms -r arm -a 9 -a 12 diff --git a/tools/versioner/tests/missing_api/run.sh b/tools/versioner/tests/missing_api/run.sh index 027712386..914c55d32 100644 --- a/tools/versioner/tests/missing_api/run.sh +++ b/tools/versioner/tests/missing_api/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 -a 12 +versioner headers -p platforms -r arm -a 9 -a 12 diff --git a/tools/versioner/tests/missing_arch/dependencies/common/foo/foodep.h b/tools/versioner/tests/missing_arch/dependencies/common/foo/foodep.h deleted file mode 100644 index 9feeb6c4c..000000000 --- a/tools/versioner/tests/missing_arch/dependencies/common/foo/foodep.h +++ /dev/null @@ -1 +0,0 @@ -typedef int foo_t; diff --git a/tools/versioner/tests/missing_arch/run.sh b/tools/versioner/tests/missing_arch/run.sh index 3a3dda8b8..6d97fb005 100644 --- a/tools/versioner/tests/missing_arch/run.sh +++ b/tools/versioner/tests/missing_arch/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -r x86 -a 9 +versioner headers -p platforms -r arm -r x86 -a 9 diff --git a/tools/versioner/tests/multiple_decl/run.sh b/tools/versioner/tests/multiple_decl/run.sh index d26ab7088..8babb73a4 100644 --- a/tools/versioner/tests/multiple_decl/run.sh +++ b/tools/versioner/tests/multiple_decl/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 +versioner headers -p platforms -r arm -a 9 diff --git a/tools/versioner/tests/multiple_decl_mismatch/run.sh b/tools/versioner/tests/multiple_decl_mismatch/run.sh index d26ab7088..8babb73a4 100644 --- a/tools/versioner/tests/multiple_decl_mismatch/run.sh +++ b/tools/versioner/tests/multiple_decl_mismatch/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 +versioner headers -p platforms -r arm -a 9 diff --git a/tools/versioner/tests/obsoleted/run.sh b/tools/versioner/tests/obsoleted/run.sh index 027712386..914c55d32 100644 --- a/tools/versioner/tests/obsoleted/run.sh +++ b/tools/versioner/tests/obsoleted/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 -a 12 +versioner headers -p platforms -r arm -a 9 -a 12 diff --git a/tools/versioner/tests/smoke/run.sh b/tools/versioner/tests/smoke/run.sh index d26ab7088..8babb73a4 100644 --- a/tools/versioner/tests/smoke/run.sh +++ b/tools/versioner/tests/smoke/run.sh @@ -1 +1 @@ -versioner headers dependencies -p platforms -r arm -a 9 +versioner headers -p platforms -r arm -a 9 From 658dbd920d8e8d17723e014d886e43615107a59a Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 2 Jun 2016 15:59:44 -0700 Subject: [PATCH 06/10] versioner: merge stdout and stderr in the test runner. Some of the error messages emitted by versioner (the ones where it was invoked incorrectly) go to stderr, which meant that the test runner ignored them. Merge stdout and stderr, and switch from testing for exact equality to endswith, because of the compilation errors test. Change-Id: I0e2c25bcc9dea4c12ea82a6a05b29e561a61a902 --- tools/versioner/run_tests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) mode change 100644 => 100755 tools/versioner/run_tests.py diff --git a/tools/versioner/run_tests.py b/tools/versioner/run_tests.py old mode 100644 new mode 100755 index f5a31f2aa..18b2aa9a3 --- a/tools/versioner/run_tests.py +++ b/tools/versioner/run_tests.py @@ -20,13 +20,13 @@ def indent(text, spaces=4): def run_test(test_name, path): os.chdir(path) process = subprocess.Popen( - ["/bin/sh", "run.sh"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - (output, error) = process.communicate() + ["/bin/sh", "run.sh"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + (output, _) = process.communicate() if os.path.exists("expected_fail"): with open("expected_fail") as f: expected_output = f.read() - if output != expected_output: + if not output.endswith(expected_output): print("{} {}: expected output mismatch".format( prefix_fail, test_name)) print("") From 173e7c07539af0910d170cd17385cba6762cfbc8 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 3 Jun 2016 13:38:00 -0700 Subject: [PATCH 07/10] versioner: improve error output slightly. Print [introduced = 9, deprecated = 10, obsoleted = 11] instead of [9,10,11]. Change-Id: Ifb8a66abbcec92aa13086d220af7ee6fa17b0897 --- tools/versioner/src/DeclarationDatabase.h | 91 ++++++++++--------- tools/versioner/src/versioner.cpp | 2 +- .../inline_version_mismatch/expected_fail | 2 +- 3 files changed, 50 insertions(+), 45 deletions(-) diff --git a/tools/versioner/src/DeclarationDatabase.h b/tools/versioner/src/DeclarationDatabase.h index 057e4164c..2b462bda5 100644 --- a/tools/versioner/src/DeclarationDatabase.h +++ b/tools/versioner/src/DeclarationDatabase.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -72,6 +73,31 @@ struct DeclarationAvailability { int obsoleted = 0; void dump(std::ostream& out = std::cout) const { + out << describe(); + } + + bool empty() const { + return !(introduced || deprecated || obsoleted); + } + + auto tie() const { + return std::tie(introduced, deprecated, obsoleted); + } + + bool operator==(const DeclarationAvailability& rhs) const { + return this->tie() == rhs.tie(); + } + + bool operator!=(const DeclarationAvailability& rhs) const { + return !(*this == rhs); + } + + std::string describe() const { + if (!(introduced || deprecated || obsoleted)) { + return "no availability"; + } + + std::stringstream out; bool need_comma = false; auto comma = [&out, &need_comma]() { if (!need_comma) { @@ -93,27 +119,8 @@ struct DeclarationAvailability { comma(); out << "obsoleted = " << obsoleted; } - } - bool empty() const { - return !(introduced || deprecated || obsoleted); - } - - auto tie() const { - return std::tie(introduced, deprecated, obsoleted); - } - - bool operator==(const DeclarationAvailability& rhs) const { - return this->tie() == rhs.tie(); - } - - bool operator!=(const DeclarationAvailability& rhs) const { - return !(*this == rhs); - } - - std::string describe() const { - return std::string("[") + std::to_string(introduced) + "," + std::to_string(deprecated) + "," + - std::to_string(obsoleted) + "]"; + return out.str(); } }; @@ -137,6 +144,26 @@ struct DeclarationLocation { bool operator==(const DeclarationLocation& other) const { return tie() == other.tie(); } + + void dump(const std::string& base_path = "", std::ostream& out = std::cout) const { + const char* var_type = declarationTypeName(type); + const char* declaration_type = is_definition ? "definition" : "declaration"; + const char* linkage = is_extern ? "extern" : "static"; + + std::string stripped_path; + if (llvm::StringRef(filename).startswith(base_path)) { + stripped_path = filename.substr(base_path.size()); + } else { + stripped_path = filename; + } + + out << " " << linkage << " " << var_type << " " << declaration_type << " @ " + << stripped_path << ":" << line_number << ":" << column; + + out << "\t["; + availability.dump(out); + out << "]\n"; + } }; struct Declaration { @@ -165,29 +192,7 @@ struct Declaration { void dump(const std::string& base_path = "", std::ostream& out = std::cout) const { out << " " << name << " declared in " << locations.size() << " locations:\n"; for (const DeclarationLocation& location : locations) { - const char* var_type = declarationTypeName(location.type); - const char* declaration_type = location.is_definition ? "definition" : "declaration"; - const char* linkage = location.is_extern ? "extern" : "static"; - - std::string filename; - if (llvm::StringRef(location.filename).startswith(base_path)) { - filename = location.filename.substr(base_path.size()); - } else { - filename = location.filename; - } - - out << " " << linkage << " " << var_type << " " << declaration_type << " @ " - << filename << ":" << location.line_number << ":" << location.column; - - if (!location.availability.empty()) { - out << "\t["; - location.availability.dump(out); - out << "]"; - } else { - out << "\t[no availability]"; - } - - out << "\n"; + location.dump(base_path, out); } } }; diff --git a/tools/versioner/src/versioner.cpp b/tools/versioner/src/versioner.cpp index 91482edb1..fcef6b113 100644 --- a/tools/versioner/src/versioner.cpp +++ b/tools/versioner/src/versioner.cpp @@ -318,7 +318,7 @@ static bool sanityCheck(const std::set& types, // Make sure that availability declarations are consistent across API levels for a given arch. if (last_availability != current_availability) { error = true; - printf("%s: availability mismatch between %s and %s: %s before, %s after\n", + printf("%s: availability mismatch between %s and %s: [%s] before, [%s] after\n", symbol_name.c_str(), last_type.describe().c_str(), type.describe().c_str(), last_availability.describe().c_str(), current_availability.describe().c_str()); } diff --git a/tools/versioner/tests/inline_version_mismatch/expected_fail b/tools/versioner/tests/inline_version_mismatch/expected_fail index 2894499bd..7f0709c86 100644 --- a/tools/versioner/tests/inline_version_mismatch/expected_fail +++ b/tools/versioner/tests/inline_version_mismatch/expected_fail @@ -1,2 +1,2 @@ -foo: availability mismatch between arm-9 and arm-12: [9,0,0] before, [10,0,0] after +foo: availability mismatch between arm-9 and arm-12: [introduced = 9] before, [introduced = 10] after versioner: sanity check failed From 958f3b31c49e1975117898a54d6e7c1fe2b386ec Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 3 Jun 2016 13:44:00 -0700 Subject: [PATCH 08/10] versioner: fix false positive with functions only available as inlines. Change-Id: I09cc335b4006c6ceafcbd1bec9e50161f8262942 --- tools/versioner/src/versioner.cpp | 55 +++++++++++++++---- .../tests/inline_unavailable/headers/foo.h | 3 + .../arch-arm/symbols/libc.so.functions.txt | 0 .../versioner/tests/inline_unavailable/run.sh | 1 + 4 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 tools/versioner/tests/inline_unavailable/headers/foo.h create mode 100644 tools/versioner/tests/inline_unavailable/platforms/android-9/arch-arm/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/inline_unavailable/run.sh diff --git a/tools/versioner/src/versioner.cpp b/tools/versioner/src/versioner.cpp index fcef6b113..bfee6b995 100644 --- a/tools/versioner/src/versioner.cpp +++ b/tools/versioner/src/versioner.cpp @@ -272,6 +272,8 @@ static DeclarationDatabase compileHeaders(const std::set& types static bool sanityCheck(const std::set& types, const DeclarationDatabase& database) { bool error = false; + std::string cwd = getWorkingDir() + "/"; + for (auto outer : database) { const std::string& symbol_name = outer.first; CompilationType last_type; @@ -290,7 +292,7 @@ static bool sanityCheck(const std::set& types, bool availability_mismatch = false; DeclarationAvailability current_availability; - // Make sure that all of the availability declarations for this symbol match. + // Ensure that all of the availability declarations for this symbol match. for (const DeclarationLocation& location : declaration.locations) { if (!found_availability) { found_availability = true; @@ -306,7 +308,7 @@ static bool sanityCheck(const std::set& types, if (availability_mismatch) { printf("%s: availability mismatch for %s\n", symbol_name.c_str(), type.describe().c_str()); - declaration.dump(getWorkingDir() + "/"); + declaration.dump(cwd); } if (type.arch != last_type.arch) { @@ -315,7 +317,7 @@ static bool sanityCheck(const std::set& types, continue; } - // Make sure that availability declarations are consistent across API levels for a given arch. + // Ensure that availability declarations are consistent across API levels for a given arch. if (last_availability != current_availability) { error = true; printf("%s: availability mismatch between %s and %s: [%s] before, [%s] after\n", @@ -323,6 +325,23 @@ static bool sanityCheck(const std::set& types, last_availability.describe().c_str(), current_availability.describe().c_str()); } + // Ensure that at most one inline definition of a function exists. + std::set inline_definitions; + + for (const DeclarationLocation& location : declaration.locations) { + if (location.is_definition) { + inline_definitions.insert(location); + } + } + + if (inline_definitions.size() > 1) { + error = true; + printf("%s: multiple inline definitions found:\n", symbol_name.c_str()); + for (const DeclarationLocation& location : declaration.locations) { + location.dump(cwd); + } + } + last_type = type; } } @@ -378,13 +397,13 @@ static bool checkVersions(const std::set& types, bool symbol_available = symbol_availability_it != platform_availability.end(); if (decl_available) { if (!symbol_available) { - // Make sure that either it exists in the platform, or an inline definition is visible. + // Ensure that either it exists in the platform, or an inline definition is visible. if (!declaration.hasDefinition()) { missing_symbol.insert(type); continue; } } else { - // Make sure that symbols declared as functions/variables actually are. + // Ensure that symbols declared as functions/variables actually are. switch (declaration.type()) { case DeclarationType::inconsistent: printf("%s: inconsistent declaration type\n", symbol_name.c_str()); @@ -409,7 +428,7 @@ static bool checkVersions(const std::set& types, } } } else { - // Make sure it's not available in the platform. + // Ensure that it's not available in the platform. if (symbol_availability_it != platform_availability.end()) { printf("%s: symbol should be unavailable in %s (declared with availability %s)\n", symbol_name.c_str(), type.describe().c_str(), availability.describe().c_str()); @@ -472,11 +491,27 @@ static bool checkVersions(const std::set& types, break; } - // Check to see if the symbol is tagged with __INTRODUCED_IN_FUTURE. + bool found_inline_definition = false; + bool future = false; + auto symbol_it = declaration_database.find(symbol_name); - const Declaration& declaration = symbol_it->second.begin()->second; - DeclarationAvailability availability = declaration.locations.begin()->availability; - if (availability.introduced >= 10000) { + + // Ignore inline functions and functions that are tagged as __INTRODUCED_IN_FUTURE. + // Ensure that all of the declarations of that function satisfy that. + for (const auto& declaration_pair : symbol_it->second) { + const Declaration& declaration = declaration_pair.second; + DeclarationAvailability availability = declaration.locations.begin()->availability; + + if (availability.introduced >= 10000) { + future = true; + } + + if (declaration.hasDefinition()) { + found_inline_definition = true; + } + } + + if (future || found_inline_definition) { continue; } diff --git a/tools/versioner/tests/inline_unavailable/headers/foo.h b/tools/versioner/tests/inline_unavailable/headers/foo.h new file mode 100644 index 000000000..6800dd0da --- /dev/null +++ b/tools/versioner/tests/inline_unavailable/headers/foo.h @@ -0,0 +1,3 @@ +static int foo() __attribute__((availability(android, introduced = 9))) { + return 0; +} diff --git a/tools/versioner/tests/inline_unavailable/platforms/android-9/arch-arm/symbols/libc.so.functions.txt b/tools/versioner/tests/inline_unavailable/platforms/android-9/arch-arm/symbols/libc.so.functions.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tools/versioner/tests/inline_unavailable/run.sh b/tools/versioner/tests/inline_unavailable/run.sh new file mode 100644 index 000000000..0dea98ff7 --- /dev/null +++ b/tools/versioner/tests/inline_unavailable/run.sh @@ -0,0 +1 @@ +versioner -v headers -p platforms -r arm -a 9 From 4af829acb7bedbc2d08dfde0d099c658a1aa7567 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 3 Jun 2016 13:46:06 -0700 Subject: [PATCH 09/10] versioner: add missing test. Change-Id: I1dc9a708b53dbb46af9e4b8ab69bf8ed46ab045f --- tools/versioner/tests/version_mismatch/expected_fail | 2 ++ tools/versioner/tests/version_mismatch/headers/foo.h | 5 +++++ .../android-12/arch-arm/symbols/libc.so.functions.txt | 1 + .../android-9/arch-arm/symbols/libc.so.functions.txt | 1 + tools/versioner/tests/version_mismatch/run.sh | 1 + 5 files changed, 10 insertions(+) create mode 100644 tools/versioner/tests/version_mismatch/expected_fail create mode 100644 tools/versioner/tests/version_mismatch/headers/foo.h create mode 100644 tools/versioner/tests/version_mismatch/platforms/android-12/arch-arm/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/version_mismatch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt create mode 100644 tools/versioner/tests/version_mismatch/run.sh diff --git a/tools/versioner/tests/version_mismatch/expected_fail b/tools/versioner/tests/version_mismatch/expected_fail new file mode 100644 index 000000000..7f0709c86 --- /dev/null +++ b/tools/versioner/tests/version_mismatch/expected_fail @@ -0,0 +1,2 @@ +foo: availability mismatch between arm-9 and arm-12: [introduced = 9] before, [introduced = 10] after +versioner: sanity check failed diff --git a/tools/versioner/tests/version_mismatch/headers/foo.h b/tools/versioner/tests/version_mismatch/headers/foo.h new file mode 100644 index 000000000..4d23417e2 --- /dev/null +++ b/tools/versioner/tests/version_mismatch/headers/foo.h @@ -0,0 +1,5 @@ +#if __ANDROID_API__ <= 9 +int foo() __attribute__((availability(android, introduced = 9))); +#else +int foo() __attribute__((availability(android, introduced = 10))); +#endif diff --git a/tools/versioner/tests/version_mismatch/platforms/android-12/arch-arm/symbols/libc.so.functions.txt b/tools/versioner/tests/version_mismatch/platforms/android-12/arch-arm/symbols/libc.so.functions.txt new file mode 100644 index 000000000..257cc5642 --- /dev/null +++ b/tools/versioner/tests/version_mismatch/platforms/android-12/arch-arm/symbols/libc.so.functions.txt @@ -0,0 +1 @@ +foo diff --git a/tools/versioner/tests/version_mismatch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt b/tools/versioner/tests/version_mismatch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt new file mode 100644 index 000000000..257cc5642 --- /dev/null +++ b/tools/versioner/tests/version_mismatch/platforms/android-9/arch-arm/symbols/libc.so.functions.txt @@ -0,0 +1 @@ +foo diff --git a/tools/versioner/tests/version_mismatch/run.sh b/tools/versioner/tests/version_mismatch/run.sh new file mode 100644 index 000000000..914c55d32 --- /dev/null +++ b/tools/versioner/tests/version_mismatch/run.sh @@ -0,0 +1 @@ +versioner headers -p platforms -r arm -a 9 -a 12 From d8c77257ea0b0b3a4f92f8ae20b230b545456fa5 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 3 Jun 2016 13:54:28 -0700 Subject: [PATCH 10/10] versioner: whitelist atexit, turn on symbol checking by default. Change-Id: I32e726c74ee618ace3a4329d46408a42732a8d9d --- tools/versioner/src/versioner.cpp | 10 ++++------ tools/versioner/src/versioner.h | 6 ++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tools/versioner/src/versioner.cpp b/tools/versioner/src/versioner.cpp index bfee6b995..7238a8cf1 100644 --- a/tools/versioner/src/versioner.cpp +++ b/tools/versioner/src/versioner.cpp @@ -485,12 +485,6 @@ static bool checkVersions(const std::set& types, } for (const std::string& symbol_name : completely_unavailable) { - // This currently has some false positives (mostly functions that come from crtbegin). - // Therefore, only report these declarations when running with verbose for now. - if (!verbose) { - break; - } - bool found_inline_definition = false; bool future = false; @@ -515,6 +509,10 @@ static bool checkVersions(const std::set& types, continue; } + if (missing_symbol_whitelist.count(symbol_name) != 0) { + continue; + } + printf("%s: not available in any platform\n", symbol_name.c_str()); failed = true; } diff --git a/tools/versioner/src/versioner.h b/tools/versioner/src/versioner.h index 95635bb1a..ced9b795c 100644 --- a/tools/versioner/src/versioner.h +++ b/tools/versioner/src/versioner.h @@ -20,6 +20,7 @@ #include #include #include +#include extern bool verbose; @@ -55,3 +56,8 @@ static const std::unordered_map> header_black // time64.h #errors when included on LP64 archs. { "time64.h", { "arm64", "mips64", "x86_64" } }, }; + +static const std::unordered_set missing_symbol_whitelist = { + // atexit comes from crtbegin. + "atexit", +};