diff --git a/.cppcheck.rules b/.cppcheck.rules new file mode 100644 index 000000000000..1bef35c98312 --- /dev/null +++ b/.cppcheck.rules @@ -0,0 +1,26 @@ + + + + wcwidth \( + + wcwidthForbidden + warning + Always use fish_wcwidth rather than wcwidth. + + + + + wcswidth \( + + wcswidthForbidden + warning + Always use fish_wcswidth rather than wcswidth. + + +<--!> +]]> diff --git a/.cppcheck.suppressions b/.cppcheck.suppressions new file mode 100644 index 000000000000..154eebed2e80 --- /dev/null +++ b/.cppcheck.suppressions @@ -0,0 +1,10 @@ +// suppress all instances of varFuncNullUB: "Passing NULL after the last typed +// argument to a variadic function leads to undefined behaviour." That's +// because all the places we do this are valid and won't cause problems even +// on a ILP64 platform because we're careful about using NULL rather than 0. +varFuncNullUB +// Suppress the warning about unmatched suppressions. At the moment these +// warnings are emitted even when removing the suppression comment results in +// the warning being suppressed. In other words this unmatchedSuppression +// warnings are false positives. +unmatchedSuppression diff --git a/.oclint b/.oclint new file mode 100644 index 000000000000..a28c1987423d --- /dev/null +++ b/.oclint @@ -0,0 +1,95 @@ +rules: +rule-configurations: + # + # This is the default value (as of the time I wrote this) but I'm making + # it explicit since it needs to agree with the value used by clang-format. + # Thus, if we ever change the style guide to allow longer or shorter lines + # this should be changed (as well as the corresponding .clang-format file). + # + - key: LONG_LINE + value: 100 + # + # The default limit for the length of variable names is 20. Long names are + # problematic but twenty chars results in way too many errors. So increase + # the limit to something more reasonable. + # + - key: LONG_VARIABLE_NAME + value: 30 + # + # This allows us to avoid peppering our code with inline comments such as + # + # scoped_lock locker(m_lock); //!OCLINT(side-effect) + # + # Specifically, this config key tells oclint that the named classes have + # RAII behavior so the local vars are actually used. + # + - key: RAII_CUSTOM_CLASSES + value: scoped_lock scoped_buffer_t builtin_commandline_scoped_transient_t scoped_push + + # We're slightly more persmissive regarding the total number of lines in a + # function. Default is 50. + - key: LONG_METHOD + value: 60 + + # We're slightly more persmissive regarding the number of non-comment + # lines in a function. Default is 30. + - key: NCSS_METHOD + value: 40 + + # We're willing to allow slighly more linearly independent paths through a + # function. Most of our code has a lot of `switch` blocks or consecutive + # `if` tests that are straightforward to interpret but which increase this + # metric. Default is 10. + - key: CYCLOMATIC_COMPLEXITY + value: 14 + + # We're willing to allow slighly more execution paths through a function. + # Default is 200. + - key: NPATH_COMPLEXITY + value: 300 + +disable-rules: + # + # A few instances of "useless parentheses" errors are meaningful. Mostly + # in the context of the `return` statement. Unfortunately the vast + # majority would result in removing parentheses that decreases + # readability. So we're going to ignore this warning and rely on humans to + # notice when the parentheses are truly not needed. + # + # Also, some macro expansions, such as FD_SET(), trigger this warning and + # we don't want to suppress each of those individually. + # + - UselessParentheses + # + # OCLint wants variable names to be at least three characters in length. + # Which would be fine if it supported a reasonable set of exceptions + # (e.g., "i", "j", "k") and allowed adding additional exceptions to match + # conventions employed by a project. Since it doesn't, and thus generates + # a lot of really annoying warnings, we're going to disable this rule. + # + - ShortVariableName + # + # This rule flags perfectly reasonable conditions like `if (!some_condition)` + # and is therefore just noise. Disable this rule. + # + - InvertedLogic + # + # The idea behind the "double negative" rule is sound since constructs + # like "!!(var & flag)" should be written as "static_cast(var & + # flag)". Unfortunately this rule has way too many false positives; + # especially in the context of assert statements. So disable this rule. + # + - DoubleNegative + # + # Avoiding bitwise operators in a conditional is a good idea with one + # exception: testing whether a bit flag is set. Which happens to be the + # only time you'll see something like `if (j->flags & JOB_CONSTRUCTED)` + # in project source. + # + - BitwiseOperatorInConditional + # + # I don't think I've ever seen a case where assigning a value to a + # parameter inside the function body was unclear, let along dangerour or + # an error. This rule is therefore just noise. Disable this rule. + # + - ParameterReassignment diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e7f22420f652..00a089b60cfc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,10 +75,12 @@ of cleanup is required to reach that goal. For now simply try to avoid introducing new lint. To make linting the code easy there is the `bin/lint` command. If you -pass it the magic string `all` it will lint all the code. If you pass -it a list of files it will lint those. If run with no arguments it will -lint any uncommitted source files. If there are no uncommitted files it -will lint the files in the most recent commit. +pass it the magic string `--all` it will lint all the *src/cmd/ksh93* +code. If you pass it a list of files it will lint those. The paths can be +directory names in which case all the source beneath that directory will +be linted. If run with no arguments it will lint any uncommitted source +files. If there are no uncommitted files it will lint the files in the +most recent commit. ### Dealing With Lint Warnings @@ -105,8 +107,17 @@ on the topic. ## Ensuring Your Changes Conform to the Style Guides The following sections discuss the specific rules for the style that -should be used when writing AST ksh code. To ensure your changes conform -to the style rules you simply need to run +should be used when writing AST ksh code. + +To make restyling the code easy there is the `bin/style` command. If you +pass it the magic string `--all` it will restyle all the *src/cmd/ksh93* +code. If you pass it a list of files it will restyle those. The paths can be +directory names in which case all the source beneath that directory will +be restyled. If run with no arguments it will restyle any uncommitted source +files. If there are no uncommitted files it will restyle the files in the +most recent commit. + +To ensure your changes conform to the style rules you simply need to run ```sh bin/style @@ -122,14 +133,6 @@ is correct. However, in that case it will run `clang-format` to ensure the entire file, not just the lines modified by the commit, conform to the style. -If you want to check the style of the entire code base run - -```sh -bin/style all -``` - -That command will refuse to restyle any files if you have uncommitted changes. - ### Configuring Your Editor #### ViM diff --git a/bin/hdrs.c b/bin/hdrs.c new file mode 100644 index 000000000000..2c33c6dc8092 --- /dev/null +++ b/bin/hdrs.c @@ -0,0 +1,4 @@ +// This is a trivial program used by the `lint` script to find out where the +// compiler thinks the system headers live. +#include +int main() { return 0; } diff --git a/bin/lint b/bin/lint new file mode 100755 index 000000000000..92e8ed2fe9bb --- /dev/null +++ b/bin/lint @@ -0,0 +1,170 @@ +#!/usr/bin/env fish +# +# TODO: Rewrite this as a ksh script. +# +# Usage: bin/lint [--all] [directory_or_filename]... +# +# Run the source through various lint detection tools. If invoked with `-all` then all the +# src/cmd/ksh93 source files will be linted. If invoked with one or more path names they +# will be linted. If the pathname is a directory all *.c files inside it will be linted. +# Otherwise any uncommitted source files are linted. If there is no uncommitted change +# then the files in the most recent commit are linted. +# +set all no +set cppchecks warning,performance,portability,information,missingInclude +set enable_global_analysis +set lint_args +set c_files +set files +set kernel_name (uname -s) +set machine_type (uname -m) + +if not test -d build +or not test -f build/compile_commands.json + echo "You need to run `meson` to configure the build before we can lint the source." >&2 + exit 1 +end + +# Deal with any CLI flags. +while set -q argv[1] + if test "$argv[1]" = "--all" + or test "$argv[1]" = "all" + set all yes + set -e argv[1] + else + break + end +end + +if test $all = yes +and set -q argv[1] + echo "Unexpected arguments: '$argv'" >&2 + exit 1 +end + +# Figure out which files to lint. +if test $all = yes + set files src/cmd/ksh93/**.c +else if set -q argv[1] + set files + for f in $argv + if test -f $argv + set files $files $f + else if test -d $argv + set files $files {$argv}**.c + end + end +else + # We haven't been asked to lint all the source or specific files. If there are uncommitted + # changes lint those, else lint the files in the most recent commit. + # Select (cached files) (modified but not cached, and untracked files). + set files (git diff-index --cached HEAD --name-only) + set files $files (git ls-files --exclude-standard --others --modified) + if not set -q files[1] + # No pending changes so lint the files in the most recent commit. + set files (git diff-tree --no-commit-id --name-only -r HEAD) + end +end + +# Filter out non C source files. +set c_files +for file in (string match -r '^.*\.c$' -- $files) + if test -f $file + set c_files $c_files ../$file + end +end + +cd build + +# We need to limit the source modules to those for which we have build rules. We also need to +# produce a version of the compile_commands.json file that only contains the files to be linted. +# Finally, we need the `-D` and `-I` flags from the build rule for the IWYU and cppcheck programs. +set project_file compile_commands_partial.json +set defs_file cppcheck_defs +set c_files (../bin/partition_compile_db compile_commands.json $project_file $defs_file \ + $c_files) +if not set -q c_files[1] + echo >&2 + echo 'WARNING: No C files to check' >&2 + echo >&2 + exit 1 +end + +# On some platforms (e.g., macOS) oclint can't find the system headers. So ask the real compiler +# to tell us where they are and pass that information to oclint. +# +# Passing this path via the compiler `-isystem` flag also keeps oclint from complaining about +# problems with the system headers. +# +# We also need this value for cppcheck to find some system headers again, on platforms like macOS, +# where the system headers aren't found at /usr/include. +set system_hdrs (clang -H -E ../bin/hdrs.c 2>&1 | head -1 | sed -e 's/^\. //' -e 's/\/[^/]*$//') + +# On macOS the system headers used by `clang` may not be rooted at /usr/include. +set lint_args $lint_args -I$system_hdrs + +# This is needed with clang on macOS. Without it `cppcheck` fails with +# `#error Unsupported architecture` from `#include `. +if test "$machine_type" = "x86_64" + set lint_args $lint_args -D__x86_64__ -D__LP64__ +end + +set lint_args $lint_args (cat $defs_file) + +if type -q include-what-you-use + echo + echo ======================================== + echo Running IWYU + echo ======================================== + for c_file in $c_files + include-what-you-use -Xiwyu --transitive_includes_only --std=c99 \ + -Wno-expansion-to-defined -Wno-nullability-completeness \ + $lint_args $c_file 2>&1 + end +end + +if type -q cppcheck + echo + echo ======================================== + echo Running cppcheck + echo ======================================== + # The stderr to stdout redirection is because cppcheck, incorrectly IMHO, writes its + # diagnostic messages to stderr. Anyone running this who wants to capture its output will + # expect those messages to be written to stdout. + set -l cn (set_color normal | string trim --chars \cO) + set -l cb (set_color --bold) + set -l cu (set_color --underline) + set -l cm (set_color magenta) + set -l cbrm (set_color brmagenta) + set -l template "[$cb$cu{file}$cn$cb:{line}$cn] $cbrm{severity}$cm ({id}):$cn\n {message}" + + # It should be possible to use --project=$project_file but cppcheck 1.82 doesn't correctly + # extract the -D and -I flags. So we do it ourselves and pass the flags on the cppcheck + # command line. + set cppcheck_args $lint_args -q --verbose --std=c99 --std=posix --language=c \ + --template $template --suppress=missingIncludeSystem --inline-suppr --enable=$cppchecks \ + --rule-file=../.cppcheck.rules --suppressions-list=../.cppcheck.suppressions + + cppcheck $cppcheck_args $c_files 2>&1 + + echo + echo ======================================== + echo 'Running `cppcheck --check-config` to identify missing includes and similar problems.' + echo 'Ignore unmatchedSuppression warnings as they are probably false positives we' + echo 'cannot suppress.' + echo ======================================== + cppcheck $cppcheck_args --check-config $c_files 2>&1 +end + +if type -q oclint + echo + echo ======================================== + echo Running oclint + echo ======================================== + # A copy of this config file has to be in the CWD (the Meson build dir). + test -f .ocling + or cp ../.oclint . + + oclint -p $PWD -enable-clang-static-analyzer $enable_global_analysis \ + -extra-arg="-isystem" -extra-arg="$system_hdrs" $c_files +end diff --git a/bin/oclint-json b/bin/oclint-json new file mode 100755 index 000000000000..8bfc7864314e --- /dev/null +++ b/bin/oclint-json @@ -0,0 +1,80 @@ +#!/usr/bin/env python3 + +import os +import platform +import json +import argparse +import re +import subprocess +import sys + +OCLINT_BIN = "oclint" + +arg_parser = argparse.ArgumentParser(description='OCLint for JSON Compilation Database (compile_commands.json)') +arg_parser.add_argument("-v", action="store_true", dest="invocation", help="show invocation command with arguments") +arg_parser.add_argument('-debug', '--debug', action="store_true", dest="debug", help="invoke OCLint in debug mode") +arg_parser.add_argument('-i', '-include', '--include', action='append', dest='includes', help="extract files matching pattern") +arg_parser.add_argument('-e', '-exclude', '--exclude', action='append', dest='excludes', help="remove files matching pattern") +arg_parser.add_argument('-p', action='store', metavar='build-path', dest='build_path', default=os.getcwd(), + help="specify the directory containing compile_commands.json") +arg_parser.add_argument('oclint_args', nargs='*', help="arguments that are passed to OCLint invocation") +args = arg_parser.parse_args() + +def get_source_path(file_attr, dir_attr): + if file_attr.startswith(os.sep): + return file_attr + elif dir_attr.endswith(os.sep): + return dir_attr + file_attr + else: + return dir_attr + os.sep + file_attr + +def source_exist_at(path): + return os.path.isfile(path) + +def source_list_inclusion_filter(source_list, inclusion_filter): + filtered_list = [] + for path in source_list: + if re.search(inclusion_filter, path): + filtered_list.append(path) + return filtered_list + +def source_list_exclusion_filter(source_list, exclusion_filter): + filtered_list = [] + for path in source_list: + if not re.search(exclusion_filter, path): + filtered_list.append(path) + return filtered_list + +json_compilation_database = os.path.join(args.build_path, 'compile_commands.json') +if not source_exist_at(json_compilation_database): + print("Error: compile_commands.json not found at %s." % args.build_path) + sys.exit(98) + +compilation_database = json.load(open(json_compilation_database)) +source_list = [] +for file_item in compilation_database: + file_path = file_item["file"] + file_path = get_source_path(file_item["file"], file_item["directory"]) + if source_exist_at(file_path) and not file_path in source_list: + source_list.append(file_path) +if args.includes: + matched_list = [] + for inclusion_filter in args.includes: + matched_list.extend( source_list_inclusion_filter(source_list, inclusion_filter) ) + source_list = matched_list +if args.excludes: + for exclusion_filter in args.excludes: + source_list = source_list_exclusion_filter(source_list, exclusion_filter) +oclint_arguments = [OCLINT_BIN, '-p', args.build_path] +if args.oclint_args: + oclint_arguments += args.oclint_args +if args.debug: + oclint_arguments.append('-debug') +oclint_arguments += source_list +if args.invocation: + print('------------------------------ OCLint ------------------------------') + print(subprocess.list2cmdline(oclint_arguments)) + print('--------------------------------------------------------------------') +print("WTF oclint_arguments %r" % (oclint_arguments,), file=sys.stderr) +exit_code = subprocess.call(oclint_arguments) +sys.exit(exit_code) diff --git a/bin/partition_compile_db b/bin/partition_compile_db new file mode 100755 index 000000000000..8819a6105cba --- /dev/null +++ b/bin/partition_compile_db @@ -0,0 +1,50 @@ +#!/usr/bin/env python3 +# +# Usage: partition_compile_commands input.json output.json defs source_file... +# +# This filters the contents of the input.json compile commands database to +# output.json including only those source modules listed on our command line. +# It also writes all the -D and -I flags from the build command of the first +# source file to the file named by the `defs` argument. +# +import json +import shlex +import sys + +input_build_db = sys.argv[1] +output_build_db = sys.argv[2] +defs_file = sys.argv[3] +files_to_lint = set(sys.argv[4:]) +source_files = [] + + +# This assumes all the `-D` and `-I` flags we care about have the value +# concatenated to the flag rather than separated by whitespace. If that +# assumption ever becomes invalid this will need to become more complicated. +def extract_defines(command_line): + for token in shlex.split(command_line): + if token.startswith("-D") or token.startswith("-I"): + yield token + + +input_json = json.load(open(input_build_db, mode='r')) +output_json = [] +for file_item in input_json: + if file_item["file"] in files_to_lint: + output_json.append(file_item) + source_files.append(file_item["file"]) + files_to_lint.remove(file_item["file"]) + if len(source_files) == 1: + with open(defs_file, mode="w") as fp: + for define in extract_defines(file_item["command"]): + print(define, file=fp) + +json.dump(output_json, open(output_build_db, mode='w'), indent=4) +if files_to_lint: + # There were source files for which we could not find a build rule. + for f in files_to_lint: + print("WARN: No build rule so ignoring file: {!r}".format(f), + file=sys.stderr) +for f in source_files: + print(f) +sys.exit(0) diff --git a/bin/style b/bin/style new file mode 100755 index 000000000000..6a0eadffefee --- /dev/null +++ b/bin/style @@ -0,0 +1,113 @@ +#!/usr/bin/env fish +# +# TODO: Rewrite this as a ksh script. +# +# Usage: bin/style [--all] [source_filename]... +# +# Run the source through `clang-format`. If invoked with `-all` then all the +# src/cmd/ksh93 source files will be linted. If invoked with one or more path names they +# will be linted. If the pathname is a directory all *.c files inside it will be linted. +# Otherwise any uncommitted source files are linted. If there is no uncommitted change +# then the files in the most recent commit are linted. +# +set all no +set git_clang_format no +set c_files +set files + +# Deal with any CLI flags. +while set -q argv[1] + if test "$argv[1]" = "--all" + or test "$argv[1]" = "all" + set all yes + set -e argv[1] + else + break + end +end + +if test $all = yes +and set -q argv[1] + echo "Unexpected arguments: '$argv'" >&2 + exit 1 +end + +if test $all = yes + set files (git status --porcelain --short --untracked-files=all | sed -e 's/^ *[^ ]* *//') + if set -q files[1] + echo >&2 + echo You have uncommited changes. Cowardly refusing to restyle the entire code base. >&2 + echo >&2 + exit 1 + end + set files src/cmd/ksh93/**.c +else if set -q argv[1] + set files + for f in $argv + if test -f $argv + set files $files $f + else if test -d $argv + set files $files {$argv}**.c {$argv}**.h + end + end +else + # We haven't been asked to reformat all the source or specific files. If there are + # uncommitted changes reformat those using `git clang-format`. Else reformat the + # files in the most recent commit. Select (cached files) (modified but not cached, + # and untracked files). + set files (git diff-index --cached HEAD --name-only) + set files $files (git ls-files --exclude-standard --others --modified) + if set -q files[1] + # Pending changes so restyle just the regions modified. + set git_clang_format yes + else + # No pending changes so restyle the files in the most recent commit. + set files (git diff-tree --no-commit-id --name-only -r HEAD) + end +end + +# Filter out non C source files. +set c_files +for file in (string match -r '^.*\.(?:c|h)$' -- $files) + if test -f $file + set c_files $c_files $file + end +end + +# Run the C reformatter if we have any C files. +if not set -q c_files[1] + echo + echo 'WARNING: No C files to restyle' + echo + exit 1 +end + +if test $git_clang_format = yes + if type -q git-clang-format + echo + echo ======================================== + echo Running git-clang-format + echo ======================================== + git add $c_files + git-clang-format + else + echo + echo 'WARNING: Cannot find git-clang-format command' + echo + end +else if type -q clang-format + echo + echo ======================================== + echo Running clang-format + echo ======================================== + for file in $c_files + cp $file $file.new # preserves mode bits + clang-format $file >$file.new + if cmp --quiet $file $file.new + rm $file.new + else + echo $file was NOT correctly formatted + mv $file.new $file + end + end +end