This is an automated email from the ASF dual-hosted git repository. adar pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 458e6ced2f7c092c403d5d6ef3502fe94d0d98cc Author: Adar Dembo <[email protected]> AuthorDate: Wed Mar 18 14:45:22 2020 -0700 iwyu: standardize on libc++ A common IWYU pain point is that the set of recommendations you get for C++ standard library headers differs depending on the system you run on. Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit one must run IWYU on an Ubuntu 14.10 VM. Instead, let's standardize on libc++ for C++ standard library headers. Even though Kudu itself is built against the system's libstdc++, the standard library classes we use should be available in the same "public" headers regardless of implementation. And this way, all runs of IWYU will produce the same recommendations. A more comprehensive solution would be to standardize _all_ of Kudu on clang and libc++. That's a larger piece of work with its own issues (e.g. is it safe to statically link libc++ into the C++ client library?). This patch also introduces mappings for libc++. Some are from the IWYU repo while others are new for Kudu. I also included a patch to fix a bug in IWYU dealing with false <new> recommendations when processing a codebase using -fsized-deallocation[1]. Finally, I removed almost all of the "muted" files from iwyu.py; the few remaining instances trip up IWYU without recourse. Note: strictly speaking we don't need to _build_ libc++, but it's a quick build and this is the easiest way to get the appropriate set of headers into thirdparty/installed/<prefix>/include. 1. https://github.com/include-what-you-use/include-what-you-use/issues/777 Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5 Reviewed-on: http://gerrit.cloudera.org:8080/15492 Reviewed-by: Alexey Serbin <[email protected]> Tested-by: Kudu Jenkins --- build-support/iwyu.py | 40 ++----- build-support/iwyu/iwyu_tool.py | 39 +++++++ build-support/iwyu/mappings/libcxx-extra.imp | 27 +++++ build-support/iwyu/mappings/libcxx.imp | 60 +++++++++++ thirdparty/build-definitions.sh | 16 ++- thirdparty/build-thirdparty.sh | 25 +++-- thirdparty/download-thirdparty.sh | 5 +- .../patches/llvm-iwyu-sized-deallocation.patch | 116 +++++++++++++++++++++ 8 files changed, 283 insertions(+), 45 deletions(-) diff --git a/build-support/iwyu.py b/build-support/iwyu.py index bef4296..ce965d5 100755 --- a/build-support/iwyu.py +++ b/build-support/iwyu.py @@ -54,37 +54,9 @@ _RE_SOURCE_FILE = re.compile(r'\.(c|cc|h)$') _RE_CLANG_ERROR = re.compile(r'^.+?:\d+:\d+:\s*' r'(fatal )?error:', re.MULTILINE) -# Files that we don't want to ever run IWYU on, because they aren't clean yet. +# Files that we don't want to ever run IWYU on because it doesn't handle them properly. _MUTED_FILES = set([ - "src/kudu/cfile/cfile_reader.h", - "src/kudu/cfile/cfile_writer.h", - "src/kudu/client/client-internal.h", - "src/kudu/client/client-test.cc", - "src/kudu/common/encoded_key-test.cc", - "src/kudu/common/schema.h", - "src/kudu/experiments/rwlock-perf.cc", - "src/kudu/rpc/reactor.cc", - "src/kudu/rpc/reactor.h", - "src/kudu/security/ca/cert_management.cc", - "src/kudu/security/ca/cert_management.h", - "src/kudu/security/cert-test.cc", - "src/kudu/security/cert.cc", - "src/kudu/security/cert.h", - "src/kudu/security/openssl_util.cc", - "src/kudu/security/openssl_util.h", - "src/kudu/security/tls_context.cc", - "src/kudu/security/tls_handshake.cc", - "src/kudu/security/tls_socket.h", - "src/kudu/security/x509_check_host.cc", - "src/kudu/server/default-path-handlers.cc", - "src/kudu/server/webserver.cc", - "src/kudu/util/bit-util-test.cc", - "src/kudu/util/group_varint-test.cc", "src/kudu/util/metrics.h", - "src/kudu/util/minidump.cc", - "src/kudu/util/mt-metrics-test.cc", - "src/kudu/util/process_memory.cc", - "src/kudu/util/rle-test.cc" ]) # Flags to pass to iwyu/fix_includes.py for Kudu-specific style. @@ -95,7 +67,7 @@ _FIX_INCLUDES_STYLE_FLAGS = [ '--reorder' ] -# Directory containin the compilation database +# Directory containing the compilation database. _BUILD_DIR = os.path.join(ROOT, 'build/latest') def _get_file_list_from_git(): @@ -109,12 +81,14 @@ def _get_paths_from_compilation_db(): compilation_db = json.load(fileobj) return [entry['file'] for entry in compilation_db] -def _run_iwyu_tool(paths): +def _run_iwyu_tool(verbose, paths): iwyu_args = ['--max_line_length=256'] for m in glob.glob(os.path.join(_MAPPINGS_DIR, "*.imp")): iwyu_args.append("--mapping_file=%s" % os.path.abspath(m)) cmdline = [_IWYU_TOOL, '-p', _BUILD_DIR] + if verbose: + cmdline.append('--verbose') cmdline.extend(paths) cmdline.append('--') cmdline.extend(iwyu_args) @@ -172,7 +146,7 @@ def _get_fixer_flags(flags): def _do_iwyu(flags, paths): - iwyu_output = _run_iwyu_tool(paths) + iwyu_output = _run_iwyu_tool(flags.verbose, paths) if flags.dump_iwyu_output: logging.info("Dumping iwyu output to %s", flags.dump_iwyu_output) with open(flags.dump_iwyu_output, "w") as f: @@ -220,6 +194,8 @@ def main(argv): help=('Just sort #includes of files listed on cmdline;' ' do not add or remove any #includes')) + parser.add_option('--verbose', action='store_true', + help=('Run iwyu_tool.py in verbose mode. Useful for debugging')) parser.add_option('--dump-iwyu-output', type='str', help=('A path to dump the raw IWYU output to. This can be useful for ' 'debugging this tool.')) diff --git a/build-support/iwyu/iwyu_tool.py b/build-support/iwyu/iwyu_tool.py index 7d74900..d2a15ea 100755 --- a/build-support/iwyu/iwyu_tool.py +++ b/build-support/iwyu/iwyu_tool.py @@ -76,6 +76,7 @@ See iwyu_tool.py -h for more details on command-line arguments. """ import argparse +import glob import json import multiprocessing from multiprocessing.pool import ThreadPool @@ -210,6 +211,42 @@ class RelativeIncludeTest(unittest.TestCase): self.assertEquals(f('/foo/bar', 'gcc -I/abs/dir'), 'gcc -I/abs/dir') +def workaround_add_libcpp(build_dir, command): + """ + Modify 'command' to include C++ standard library headers from the libc++ + found in Kudu's thirdparty tree. + + By default, IWYU will use the system's libstdc++ for standard library + headers. This is problematic as every system has a different version of + libstdc++, leading to differing recommendations depending on the host system. + + If we standardize on libc++ from Kudu's thirdparty tree, we can ensure the + same recommendations regardless of system, though those recommendations will + change when libc++ is upgraded. + """ + nostdinc = "-nostdinc++" + if nostdinc not in command: + command += " " + nostdinc + + # Check for and add any dynamic (path-based) flags. + # + # Some sanitizer builds (like TSAN) already include libc++; if the command + # already has a flag including libc++, we don't want to add it again. + path_to_libcpp_prefix = os.path.join(build_dir, "..", "..", "thirdparty", "installed") + path_to_libcpp_suffix = os.path.join("include", "c++", "v1") + tp_pattern = os.path.join(path_to_libcpp_prefix, "*", path_to_libcpp_suffix) + found = False + for path in glob.glob(tp_pattern): + search_string = "-isystem " + path + if search_string in command: + found = True + break + if not found: + path_to_libcpp = os.path.join(path_to_libcpp_prefix, "uninstrumented", + path_to_libcpp_suffix) + command += " -isystem " + os.path.abspath(path_to_libcpp) + + return command def main(compilation_db_path, source_files, verbose, formatter, iwyu_args): """ Entry point. """ @@ -249,10 +286,12 @@ def main(compilation_db_path, source_files, verbose, formatter, iwyu_args): source) # Run analysis + build_dir = os.path.dirname(compilation_db_path) def run_iwyu_task(entry): cwd, compile_command = entry['directory'], entry['command'] compile_command = workaround_parent_dir_relative_includes( cwd, compile_command) + compile_command = workaround_add_libcpp(build_dir, compile_command) return run_iwyu(cwd, compile_command, iwyu_args, verbose) pool = ThreadPool(multiprocessing.cpu_count()) try: diff --git a/build-support/iwyu/mappings/libcxx-extra.imp b/build-support/iwyu/mappings/libcxx-extra.imp new file mode 100644 index 0000000..16f20cc --- /dev/null +++ b/build-support/iwyu/mappings/libcxx-extra.imp @@ -0,0 +1,27 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# More libc++ mappings not already present in include-what-you-use. +[ + { include: ["<__bit_reference>", private, "<vector>", public ] }, + { include: ["<__threading_support>", private, "<thread>", public ] }, + { include: ["<__tree>", private, "<map>", public ] }, + { include: ["<__tree>", private, "<set>", public ] }, + { include: ["<__tuple>", private, "<tuple>", public ] }, + { include: ["<__hash_table>", private, "<unordered_set>", public ] }, + { include: ["<__hash_table>", private, "<unordered_map>", public ] }, +] diff --git a/build-support/iwyu/mappings/libcxx.imp b/build-support/iwyu/mappings/libcxx.imp new file mode 100644 index 0000000..687e97f --- /dev/null +++ b/build-support/iwyu/mappings/libcxx.imp @@ -0,0 +1,60 @@ +# This file has been imported into the Kudu source tree from the IWYU source +# tree as of version 0.13: +# https://github.com/include-what-you-use/include-what-you-use/blob/master/libcxx.imp +# and corresponding license has been added: +# https://github.com/include-what-you-use/include-what-you-use/blob/master/LICENSE.TXT +# +# ============================================================================== +# LLVM Release License +# ============================================================================== +# University of Illinois/NCSA +# Open Source License +# +# Copyright (c) 2003-2010 University of Illinois at Urbana-Champaign. +# All rights reserved. +# +# Developed by: +# +# LLVM Team +# +# University of Illinois at Urbana-Champaign +# +# http://llvm.org +# +# Permission is hereby granted, free of charge, to any person obtaining a copy of +# this software and associated documentation files (the "Software"), to deal with +# the Software without restriction, including without limitation the rights to +# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +# of the Software, and to permit persons to whom the Software is furnished to do +# so, subject to the following conditions: +# +# * Redistributions of source code must retain the above copyright notice, +# this list of conditions and the following disclaimers. +# +# * Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimers in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the names of the LLVM Team, University of Illinois at +# Urbana-Champaign, nor the names of its contributors may be used to +# endorse or promote products derived from this Software without specific +# prior written permission. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS +# FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# CONTRIBUTORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS WITH THE +# SOFTWARE. + +# libc++ headers +[ + { include: ["<__functional_base>", private, "<functional>", public ] }, + { include: ["<__mutex_base>", private, "<mutex>", public ] }, + { symbol: [ "std::declval", private, "<utility>", public ] }, + { symbol: [ "std::forward", private, "<utility>", public ] }, + { symbol: [ "std::move", private, "<utility>", public ] }, + { symbol: [ "std::nullptr_t", private, "<cstddef>", public ] }, + { symbol: [ "std::string", private, "<string>", public ] }, +] diff --git a/thirdparty/build-definitions.sh b/thirdparty/build-definitions.sh index 0f88333..b28e7c5 100644 --- a/thirdparty/build-definitions.sh +++ b/thirdparty/build-definitions.sh @@ -107,7 +107,10 @@ build_libcxxabi() { mkdir -p $LIBCXXABI_BDIR pushd $LIBCXXABI_BDIR rm -Rf CMakeCache.txt CMakeFiles/ - cmake \ + + # libcxxabi requires gcc5 or newer. Since we can't guarantee that universally, + # let's always build it with clang. + CC="$CLANG" CXX="$CLANGXX" cmake \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_INSTALL_PREFIX=$PREFIX \ -DCMAKE_CXX_FLAGS="$EXTRA_CXXFLAGS $EXTRA_LDFLAGS" \ @@ -121,8 +124,11 @@ build_libcxxabi() { build_libcxx() { local BUILD_TYPE=$1 case $BUILD_TYPE in + "normal") + SANITIZER_ARG= + ;; "tsan") - SANITIZER_TYPE=Thread + SANITIZER_ARG="-DLLVM_USE_SANITIZER=Thread" ;; *) echo "Unknown build type: $BUILD_TYPE" @@ -134,7 +140,9 @@ build_libcxx() { mkdir -p $LIBCXX_BDIR pushd $LIBCXX_BDIR rm -Rf CMakeCache.txt CMakeFiles/ - cmake \ + # Since libcxxabi requires gcc5 or newer, we build it with clang. As libcxx is + # a dependency, let's also always use clang to build it. + CC="$CLANG" CXX="$CLANGXX" cmake \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_INSTALL_PREFIX=$PREFIX \ -DCMAKE_CXX_FLAGS="$EXTRA_CXXFLAGS" \ @@ -145,7 +153,7 @@ build_libcxx() { -DLIBCXX_CXX_ABI=libcxxabi \ -DLIBCXX_CXX_ABI_INCLUDE_PATHS=$LLVM_SOURCE/projects/libcxxabi/include \ -DLIBCXX_CXX_ABI_LIBRARY_PATH=$PREFIX/lib \ - -DLLVM_USE_SANITIZER=$SANITIZER_TYPE \ + $SANITIZER_ARG \ $EXTRA_CMAKE_FLAGS \ $LLVM_SOURCE/projects/libcxx ${NINJA:-make} -j$PARALLEL $EXTRA_MAKEFLAGS install diff --git a/thirdparty/build-thirdparty.sh b/thirdparty/build-thirdparty.sh index 6f3ab98..404da46 100755 --- a/thirdparty/build-thirdparty.sh +++ b/thirdparty/build-thirdparty.sh @@ -347,6 +347,15 @@ if [ -n "$F_COMMON" -o -n "$F_LLVM" ]; then build_llvm normal fi +# From this point forward, clang is available for us to use if needed. +if which ccache >/dev/null ; then + CLANG="$TP_DIR/../build-support/ccache-clang/clang" + CLANGXX="$TP_DIR/../build-support/ccache-clang/clang++" +else + CLANG="$TP_DIR/clang-toolchain/bin/clang" + CLANGXX="$TP_DIR/clang-toolchain/bin/clang++" +fi + save_env # Enable debug symbols so that stacktraces and linenumbers are available at @@ -355,6 +364,15 @@ save_env EXTRA_CFLAGS="-g $EXTRA_CFLAGS" EXTRA_CXXFLAGS="-g $EXTRA_CXXFLAGS" +# Build libc++abi first as it is a dependency for libc++. +if [ -n "$F_UNINSTRUMENTED" -o -n "$F_LLVM" ]; then + build_libcxxabi +fi + +if [ -n "$F_UNINSTRUMENTED" -o -n "$F_LLVM" ]; then + build_libcxx normal +fi + if [ -n "$F_UNINSTRUMENTED" -o -n "$F_GFLAGS" ]; then build_gflags fi @@ -447,13 +465,6 @@ fi # * -Wl,-rpath,... - Add instrumented libc++ location to the rpath so that it # can be found at runtime. -if which ccache >/dev/null ; then - CLANG="$TP_DIR/../build-support/ccache-clang/clang" - CLANGXX="$TP_DIR/../build-support/ccache-clang/clang++" -else - CLANG="$TP_DIR/clang-toolchain/bin/clang" - CLANGXX="$TP_DIR/clang-toolchain/bin/clang++" -fi export CC=$CLANG export CXX=$CLANGXX diff --git a/thirdparty/download-thirdparty.sh b/thirdparty/download-thirdparty.sh index 45f8649..244d012 100755 --- a/thirdparty/download-thirdparty.sh +++ b/thirdparty/download-thirdparty.sh @@ -320,13 +320,14 @@ fetch_and_patch \ $PYTHON_SOURCE \ $PYTHON_PATCHLEVEL -LLVM_PATCHLEVEL=4 +LLVM_PATCHLEVEL=5 fetch_and_patch \ llvm-${LLVM_VERSION}-iwyu-${IWYU_VERSION}.src.tar.gz \ $LLVM_SOURCE \ $LLVM_PATCHLEVEL \ "patch -p1 < $TP_DIR/patches/llvm-add-iwyu.patch" \ - "patch -p1 < $TP_DIR/patches/llvm-iwyu-include-picker.patch" + "patch -p1 < $TP_DIR/patches/llvm-iwyu-include-picker.patch" \ + "patch -p0 < $TP_DIR/patches/llvm-iwyu-sized-deallocation.patch" LZ4_PATCHLEVEL=0 fetch_and_patch \ diff --git a/thirdparty/patches/llvm-iwyu-sized-deallocation.patch b/thirdparty/patches/llvm-iwyu-sized-deallocation.patch new file mode 100644 index 0000000..5af6398 --- /dev/null +++ b/thirdparty/patches/llvm-iwyu-sized-deallocation.patch @@ -0,0 +1,116 @@ +--- tools/clang/tools/include-what-you-use/iwyu_ast_util.cc.orig 2020-03-23 14:03:01.060932783 -0700 ++++ tools/clang/tools/include-what-you-use/iwyu_ast_util.cc 2020-03-23 14:04:37.056235116 -0700 +@@ -47,6 +47,7 @@ + class FileEntry; + } // namespace clang + ++using clang::ASTContext; + using clang::BlockPointerType; + using clang::CXXConstructExpr; + using clang::CXXConstructorDecl; +@@ -78,6 +79,7 @@ + using clang::FullSourceLoc; + using clang::FunctionDecl; + using clang::FunctionType; ++using clang::IdentifierInfo; + using clang::ImplicitCastExpr; + using clang::InjectedClassNameType; + using clang::LValueReferenceType; +@@ -929,13 +931,81 @@ + !StartsWith(decl_name, "operator delete")) + return false; + +- // Placement-new/delete has 2 args, second is void*. The only other +- // 2-arg overloads of new/delete in <new> take a const nothrow_t&. +- if (decl->getNumParams() == 2 && +- !decl->getParamDecl(1)->getType().isConstQualified()) +- return false; +- +- return true; ++ // The following variants of operator new[1] are implicitly defined in every ++ // translation unit and should not require including <new>. ++ // ++ // void* operator new ( std::size_t count ); ++ // void* operator new[]( std::size_t count ); ++ // void* operator new ( std::size_t count, std::align_val_t al ); (since C++17) ++ // void* operator new[]( std::size_t count, std::align_val_t al ); (since C++17) ++ // ++ // Likewise, the following variants of operator delete[2] are implicitly ++ // defined in every translation unit and should not require including <new>. ++ // ++ // void operator delete ( void* ptr ) throw(); (until C++11) ++ // void operator delete ( void* ptr ) noexcept; (since C++11) ++ // void operator delete[]( void* ptr ) throw(); (until C++11) ++ // void operator delete[]( void* ptr ) noexcept; (since C++11) ++ // void operator delete ( void* ptr, std::align_val_t al ) noexcept; (since C++17) ++ // void operator delete[]( void* ptr, std::align_val_t al ) noexcept; (since C++17) ++ // void operator delete ( void* ptr, std::size_t sz ) noexcept; (since C++14) ++ // void operator delete[]( void* ptr, std::size_t sz ) noexcept; (since C++14) ++ // void operator delete ( void* ptr, std::size_t sz, ++ // std::align_val_t al ) noexcept; (since C++17) ++ // void operator delete[]( void* ptr, std::size_t sz, ++ // std::align_val_t al ) noexcept; (since C++17) ++ // void operator delete ( void* ptr, const std::nothrow_t& tag ) throw(); (until C++11) ++ // void operator delete ( void* ptr, const std::nothrow_t& tag ) noexcept; (since C++11) ++ // void operator delete[]( void* ptr, const std::nothrow_t& tag ) throw(); (until C++11) ++ // void operator delete[]( void* ptr, const std::nothrow_t& tag ) noexcept; (since C++11) ++ // ++ // The below code attempts to return true for these variants while returning ++ // false for all others. FunctionDecl::isReplaceableGlobalAllocationFunction ++ // comes very very close, but returns true for nothrow new, which is not ++ // implicitly defined. ++ // ++ // 1. https://en.cppreference.com/w/cpp/memory/new/operator_new ++ // 2. https://en.cppreference.com/w/cpp/memory/new/operator_delete ++ switch (decl->getNumParams()) { ++ case 1: ++ // All 1-arg variants are implicitly declared. ++ return true; ++ case 2: { ++ // Amongst 2-arg variants, aligned (C++17) new/delete, sized delete (C++14), and ++ // nothrow delete are implicitly declared. ++ ASTContext& ctx = decl->getASTContext(); ++ QualType t = decl->getParamDecl(1)->getType(); ++ if (t->isAlignValT() || // aligned new/delete ++ ctx.hasSameType(t, ctx.getSizeType())) // sized delete ++ return true; ++ // We have to work a bit harder to figure out if it's a nothrow delete. ++ // ++ // This cribs from FunctionDecl::isReplaceableGlobalAllocationFunction. ++ if (StartsWith(decl_name, "operator delete") && t->isReferenceType()) { ++ t = t->getPointeeType(); ++ if (t.isConstQualified()) { ++ const CXXRecordDecl* recordDecl = t->getAsCXXRecordDecl(); ++ if (recordDecl) { ++ const IdentifierInfo* iInfo = recordDecl->getIdentifier(); ++ if (iInfo && iInfo->isStr("nothrow_t") && recordDecl->isInStdNamespace()) ++ return true; ++ } ++ } ++ } ++ return false; ++ } ++ case 3: { ++ // Amongst 3-arg variants, only sized aligned delete (C++17) is implicitly ++ // declared. ++ ASTContext& ctx = decl->getASTContext(); ++ QualType t = decl->getParamDecl(1)->getType(); ++ return ctx.hasSameType(t, ctx.getSizeType()) && ++ decl->getParamDecl(2)->getType()->isAlignValT(); ++ } ++ default: ++ return false; ++ return true; ++ } + } + + bool IsFriendDecl(const Decl* decl) { +@@ -1082,7 +1152,7 @@ + + bool IsBuiltinFunction(const clang::NamedDecl* decl, + const std::string& symbol_name) { +- if (const clang::IdentifierInfo* iden = decl->getIdentifier()) { ++ if (const IdentifierInfo* iden = decl->getIdentifier()) { + return iden->getBuiltinID() != 0 && + !clang::Builtin::Context::isBuiltinFunc(symbol_name.c_str()); + }
