diff --git a/.ci/generate_test_report_lib.py b/.ci/generate_test_report_lib.py index cc19c6922628f..b79624fdecdb6 100644 --- a/.ci/generate_test_report_lib.py +++ b/.ci/generate_test_report_lib.py @@ -355,10 +355,15 @@ def generate_report_from_files(title, return_code, build_log_files): def compute_platform_title() -> str: - logo = ":window:" if platform.system() == "Windows" else ":penguin:" + logo = { + "Windows": ":window:", + "Linux": ":penguin:", + "Darwin": ":green_apple:", + }.get(platform.system()) + # On Linux the machine value is x86_64 on Windows it is AMD64. if platform.machine() == "x86_64" or platform.machine() == "AMD64": arch = "x64" else: arch = platform.machine() - return f"{logo} {platform.system()} {arch} Test Results" + return f"{logo + ' ' if logo is not None else ''}{platform.system()} {arch} Test Results" diff --git a/.github/workflows/libc-fullbuild-tests.yml b/.github/workflows/libc-fullbuild-tests.yml index 03b73cb242b57..c910abd607f6a 100644 --- a/.github/workflows/libc-fullbuild-tests.yml +++ b/.github/workflows/libc-fullbuild-tests.yml @@ -88,13 +88,20 @@ jobs: cpp_compiler: clang++-23 target: armv7em-none-eabi testing: SKIP - - name: baremetal-armv8m + - name: baremetal-armv8m-softfp os: ubuntu-24.04 build_type: MinSizeRel c_compiler: clang-23 cpp_compiler: clang++-23 target: armv8m.main-none-eabi testing: SKIP + - name: baremetal-armv8m-hard + os: ubuntu-24.04 + build_type: MinSizeRel + c_compiler: clang-23 + cpp_compiler: clang++-23 + target: armv8m.main-none-eabihf + testing: SKIP - name: baremetal-armv8.1m os: ubuntu-24.04 build_type: MinSizeRel diff --git a/.github/workflows/libcxx-benchmark-commit.yml b/.github/workflows/libcxx-benchmark-commit.yml index 4e925758e0692..66dcd60008806 100644 --- a/.github/workflows/libcxx-benchmark-commit.yml +++ b/.github/workflows/libcxx-benchmark-commit.yml @@ -3,7 +3,9 @@ # it requires several inputs that allow customizing its behavior. name: "[libc++] Run benchmark suite against commit" -run-name: "[libc++] Run benchmark suite against ${{ inputs.commit }} on ${{ inputs.lnt-machine }}" + +# Keep in sync with libcxx/utils/ci/lnt/dispatch-benchmarks +run-name: "[libc++] Run benchmark suite against ${{ inputs.commit }} on ${{ inputs.lnt-machine }}${{ !inputs.submit-lnt && ' (dry run)' || '' }}" permissions: contents: read @@ -18,15 +20,15 @@ on: lnt-machine: description: 'The LNT machine to run the benchmarks on' required: true - type: choice - options: - - macos-26.5-arm64 - - linux-x86_64 - benchmark-suite-version: - description: 'The version of the benchmark suite to use (a LLVM monorepo SHA)' + type: string + benchmark-suite-override: + description: | + Override the version of the benchmark suite to use (a LLVM monorepo SHA). By default, the version pinned + for this machine in machines.json is used. This override can be used to dry-run benchmarks with arbitrary + commits of the benchmark suite, but only the version pinned in machines.json can be used when actually + submitting to LNT. required: false type: string - default: 0eefb2682bf8c04954c46e91916b5164d8424702 filter: description: 'An optional filter to determine which benchmarks to run' required: false @@ -49,37 +51,50 @@ jobs: outputs: matrix: ${{ steps.select.outputs.matrix }} steps: + - name: Checkout the machine definitions + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + persist-credentials: false + # Disabling cone mode allows checking out exactly the single file we need. + sparse-checkout: libcxx/utils/ci/lnt/machines.json + sparse-checkout-cone-mode: false + - name: Select the configuration to benchmark on id: select uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: LNT_MACHINE: ${{ inputs.lnt-machine }} + BENCHMARK_SUITE_OVERRIDE: ${{ inputs.benchmark-suite-override }} + SUBMIT_LNT: ${{ inputs.submit-lnt }} with: script: | - const MACHINES = [ - { - 'lnt-machine': 'macos-26.5-arm64', - runner: ['self-hosted', 'macOS', 'ARM64', '26', '26.5'], - cxx: 'clang++', - 'running-on': 'macos', - 'xcode-version': '26.5', - }, - { - 'lnt-machine': 'linux-x86_64', - runner: 'llvm-premerge-libcxx-runners', - cxx: 'clang++-22', - 'running-on': 'linux', - }, - ]; + const config = JSON.parse(require('fs').readFileSync('libcxx/utils/ci/lnt/machines.json', 'utf8')); const requested = process.env.LNT_MACHINE; - const selected = MACHINES.filter(m => m['lnt-machine'] === requested); + const selected = config.filter(cfg => cfg['lnt-machine'] === requested); if (selected.length === 0) { - const known = MACHINES.map(m => m['lnt-machine']).join(', '); + const known = config.map(cfg => cfg['lnt-machine']).join(', '); core.setFailed(`Unknown LNT machine '${requested}' (known machines: ${known})`); return; } + // Honor benchmark suite version override and make sure we don't submit if an incorrect + // override is provided. + const version_override = (process.env.BENCHMARK_SUITE_OVERRIDE || '').trim().toLowerCase(); + for (const cfg of selected) { + const pinned = cfg['benchmark-suite-version']; + if (process.env.SUBMIT_LNT === 'true' && version_override && version_override !== pinned) { + core.setFailed(`Refusing to submit results for ${cfg['lnt-machine']}, since the benchmark suite was ` + + `overridden to version ${version_override}, which is different from the version ` + + `pinned in machines.json (${pinned}).`); + return; + } + + if (version_override) { + cfg['benchmark-suite-version'] = version_override; + } + } + core.setOutput('matrix', JSON.stringify(selected)); run-benchmarks: @@ -134,7 +149,7 @@ jobs: - name: Run the benchmarks env: COMMIT: ${{ inputs.commit }} - BENCHMARK_SUITE_VERSION: ${{ inputs.benchmark-suite-version }} + BENCHMARK_SUITE_VERSION: ${{ matrix.benchmark-suite-version }} FILTER: ${{ inputs.filter }} LNT_MACHINE: ${{ matrix.lnt-machine }} run: | diff --git a/.github/workflows/libcxx-benchmark-cron.yml b/.github/workflows/libcxx-benchmark-cron.yml new file mode 100644 index 0000000000000..1bc224eb7fc74 --- /dev/null +++ b/.github/workflows/libcxx-benchmark-cron.yml @@ -0,0 +1,137 @@ +# This file defines a workflow that periodically requests benchmark runs for the commits +# that should have historical performance data but don't yet. +# +# The workflow keeps no state: on every invocation, the commits that should be benchmarked +# are recomputed from Git and the ones that have been benchmarked are recomputed from LNT +# and from the Github Actions API. It then dispatches libcxx-benchmark-commit.yml for the +# difference, within a budget. This makes the system converge towards having data for all +# the desired commits, without necessarily ever reaching that state (as new commits land). +# +# The actual configuration driving this job is in libcxx/utils/ci/lnt/machines.json. + +name: "[libc++] Request historical benchmark data" + +permissions: + contents: read + +on: + schedule: + # Trigger every hour, off the hour boundary to avoid popular times. + - cron: '37 * * * *' + workflow_dispatch: + inputs: + dry-run: + description: 'Report what would be requested, but request nothing' + required: false + type: boolean + default: false + allow-missing-machine: + description: 'Plan for machines that have no data in LNT yet. Needed to seed a new machine.' + required: false + type: boolean + default: false + +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + +jobs: + # Turn the machine definitions stored in libcxx/utils/ci/lnt/machines.json into actual + # dispatch jobs. + select-machines: + if: github.repository_owner == 'llvm' + runs-on: ubuntu-24.04 + outputs: + matrix: ${{ steps.select.outputs.matrix }} + steps: + - name: Checkout the machine definitions + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + persist-credentials: false + # Disabling cone mode allows checking out exactly the single file we need. + sparse-checkout: libcxx/utils/ci/lnt/machines.json + sparse-checkout-cone-mode: false + + - name: Select the machines to request runs for + id: select + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const config = JSON.parse(require('fs').readFileSync('libcxx/utils/ci/lnt/machines.json', 'utf8')); + core.setOutput('matrix', JSON.stringify(config.map(cfg => ({ + machine: cfg['lnt-machine'], + ...cfg.coverage, + })))); + + request-runs: + permissions: + contents: read + actions: write # to dispatch libcxx-benchmark-commit.yml + needs: + - select-machines + strategy: + matrix: + include: ${{ fromJSON(needs.select-machines.outputs.matrix) }} + fail-fast: false + name: "Request ${{ matrix.machine }}" + runs-on: ubuntu-24.04 + timeout-minutes: 30 + env: + MACHINE: ${{ matrix.machine }} + SINCE: ${{ matrix.since }} + EVERY: ${{ matrix.every }} + SAMPLES: ${{ matrix.samples }} + MAX_IN_FLIGHT: ${{ matrix.max-in-flight }} + LNT_URL: ${{ matrix.lnt-url }} + + ALLOW_MISSING_MACHINE: ${{ inputs.allow-missing-machine || 'false' }} + DRY_RUN: ${{ inputs.dry-run || 'false' }} + steps: + - name: Checkout the LLVM monorepo + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + persist-credentials: false + # Selecting anchor commits requires full Git history, but not the blob content. + fetch-depth: 0 + filter: blob:none + + - name: Install Python + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 + with: + python-version: '3.14' + cache: pip + cache-dependency-path: libcxx/utils/requirements.txt + + - name: Install dependencies + run: pip install -r libcxx/utils/requirements.txt + + - name: Determine which commits should have benchmark data + run: | + libcxx/utils/ci/lnt/select-anchor-commits --since "${SINCE}" --every "${EVERY}" --output anchors.txt + # Anchor commits go from oldest to newest. Reverse them to prioritize newer commits first. + tac anchors.txt > tmp.txt + mv tmp.txt anchors.txt + + - name: Determine which of them are missing from LNT + run: | + allow_missing=() + if [ "${ALLOW_MISSING_MACHINE}" = "true" ]; then + allow_missing=(--allow-missing-machine) + fi + + libcxx/utils/ci/lnt/plan-benchmarks --commit-list anchors.txt --lnt-url "${LNT_URL}" --test-suite libcxx \ + --machine "${MACHINE}" --samples "${SAMPLES}" "${allow_missing[@]}" \ + --output plan.jsonl + + - name: Request the missing runs + env: + GITHUB_TOKEN: ${{ github.token }} + run: | + dry_run=() + if [ "${DRY_RUN}" = "true" ]; then + dry_run=(--dry-run) + fi + + libcxx/utils/ci/lnt/dispatch-benchmarks --work-items plan.jsonl --lnt-url "${LNT_URL}" \ + --max-in-flight "${MAX_IN_FLIGHT}" \ + --output dispatched.jsonl "${dry_run[@]}" diff --git a/.github/workflows/libcxx-pr-benchmark.yml b/.github/workflows/libcxx-pr-benchmark.yml index ef1bdc21d33a5..9b5bdb5adb834 100644 --- a/.github/workflows/libcxx-pr-benchmark.yml +++ b/.github/workflows/libcxx-pr-benchmark.yml @@ -48,6 +48,14 @@ jobs: permissions: pull-requests: write steps: + - name: Checkout the LNT configurations + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + persist-credentials: false + # Disabling cone mode allows checking out exactly the single file we need. + sparse-checkout: libcxx/utils/ci/lnt/machines.json + sparse-checkout-cone-mode: false + - name: Extract information from the PR id: vars uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 @@ -63,6 +71,10 @@ jobs: const match = context.payload.comment.body.match(/\/libcxx-bot benchmark (.+)/); core.setOutput('benchmarks', match ? match[1] : ''); + // Benchmark on the same configurations that we track performance on. + const config = JSON.parse(require('fs').readFileSync('libcxx/utils/ci/lnt/machines.json', 'utf8')); + core.setOutput('matrix', JSON.stringify(config.map(({coverage, ...machine}) => machine))); + - name: Update comment with link to the run uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: @@ -84,20 +96,12 @@ jobs: pr_base: ${{ steps.vars.outputs.pr_base }} pr_head: ${{ steps.vars.outputs.pr_head }} benchmarks: ${{ steps.vars.outputs.benchmarks }} + matrix: ${{ steps.vars.outputs.matrix }} run-benchmarks: strategy: matrix: - include: - - platform: macOS 26.5 arm64 - runner: ["self-hosted", "macOS", "ARM64", "26", "26.5"] - cxx: clang++ - running-on: macos - xcode-version: '26.5' - - platform: Linux x86_64 - runner: llvm-premerge-libcxx-runners - cxx: clang++-22 - running-on: linux + include: ${{ fromJSON(needs.extract-info.outputs.matrix) }} fail-fast: false permissions: pull-requests: write @@ -107,7 +111,7 @@ jobs: env: BENCHMARKS: ${{ needs.extract-info.outputs.benchmarks }} COMPILER: ${{ matrix.cxx }} - PLATFORM: ${{ matrix.platform }} + LNT_MACHINE: ${{ matrix.lnt-machine }} PR_HEAD: ${{ needs.extract-info.outputs.pr_head }} PR_BASE: ${{ needs.extract-info.outputs.pr_base }} steps: @@ -158,21 +162,14 @@ jobs: - name: Run baseline and candidate interleaved run: | source .venv/bin/activate - # Run 3 times so we can pick the median, and interleave to mitigate the impact of environmental noise - ./libcxx/utils/test-at-commit --libcxx-installation install/baseline -B benchmarks/baseline --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" - ./libcxx/utils/consolidate-benchmarks benchmarks/baseline | tee baseline.lnt - ./libcxx/utils/test-at-commit --libcxx-installation install/candidate -B benchmarks/candidate --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" - ./libcxx/utils/consolidate-benchmarks benchmarks/candidate | tee candidate.lnt - - ./libcxx/utils/test-at-commit --libcxx-installation install/baseline -B benchmarks/baseline --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" - ./libcxx/utils/consolidate-benchmarks benchmarks/baseline | tee -a baseline.lnt - ./libcxx/utils/test-at-commit --libcxx-installation install/candidate -B benchmarks/candidate --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" - ./libcxx/utils/consolidate-benchmarks benchmarks/candidate | tee -a candidate.lnt - - ./libcxx/utils/test-at-commit --libcxx-installation install/baseline -B benchmarks/baseline --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" - ./libcxx/utils/consolidate-benchmarks benchmarks/baseline | tee -a baseline.lnt - ./libcxx/utils/test-at-commit --libcxx-installation install/candidate -B benchmarks/candidate --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" - ./libcxx/utils/consolidate-benchmarks benchmarks/candidate | tee -a candidate.lnt + # Run 5 times so we can pick the median, and interleave baseline and candidate to mitigate the impact of + # environmental noise + for _ in $(seq 1 5); do + ./libcxx/utils/test-at-commit --libcxx-installation install/baseline -B benchmarks/baseline --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" + ./libcxx/utils/consolidate-benchmarks benchmarks/baseline | tee -a baseline.lnt + ./libcxx/utils/test-at-commit --libcxx-installation install/candidate -B benchmarks/candidate --compiler "${COMPILER}" -- -sv -j1 --param optimization=speed "$BENCHMARKS" + ./libcxx/utils/consolidate-benchmarks benchmarks/candidate | tee -a candidate.lnt + done - name: Compare baseline and candidate runs run: | @@ -193,7 +190,7 @@ jobs: const details = [ '
', '', - `Benchmark results for ${process.env.PLATFORM}:`, + `Benchmark results for ${process.env.LNT_MACHINE}:`, '', '', '```', @@ -220,7 +217,7 @@ jobs: comment_id: context.payload.comment.id, }); const run_url = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; - const note = `> _:x: Benchmarks for ${process.env.PLATFORM} failed. See ${run_url} for details._`; + const note = `> _:x: Benchmarks for ${process.env.LNT_MACHINE} failed. See ${run_url} for details._`; await github.rest.issues.updateComment({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml index f2f50710cc0a9..36c04523fda06 100644 --- a/.github/workflows/release-binaries.yml +++ b/.github/workflows/release-binaries.yml @@ -90,6 +90,7 @@ jobs: id: vars env: LLVM_VERSION_FROM_SOURCE: ${{ steps.version-from-source.outputs.full-no-suffix }} + LLVM_VERSION_MAJOR: ${{ steps.version-from-source.outputs.major }} INPUTS_RUNS_ON: ${{ inputs.runs-on }} shell: bash # In order for the test-release.sh script to run correctly, the LLVM @@ -113,6 +114,14 @@ jobs: fi ref="$GITHUB_SHA" fi + + if [ "$RUNNER_OS" = "Windows" ] && grep -q 'rc' <<< "$release_version"; then + # The Wix installer generator does not support strings in the version number, + # so we need to fixup the version number for release candidates. + # For Example: 23.1.0-rc2 will become 23.0.0.2 + release_version="$LLVM_VERSION_MAJOR.0.0.$(cut -d c -f2 <<< $release_version)" + fi + if [ -n "${{ inputs.upload }}" ]; then upload="${{ inputs.upload }}" else @@ -296,7 +305,7 @@ jobs: echo "windows-installer-filename=$(Split-Path -Path $installer -Leaf)" >> $env:GITHUB_OUTPUT - name: Dump Wix logs - if: runner.os == 'Windows' + if: runner.os == 'Windows' && always() env: LLVM_VERSION: ${{ needs.prepare.outputs.release-version }} BUILD_DIR_SUFFIX: ${{ case(runner.arch == 'ARM64', 'arm64', 'amd64') }} @@ -375,7 +384,9 @@ jobs: - name: Upload Artifacts uses: $/.github/workflows/upload-release-artifact with: - release-version: ${{ needs.prepare.outputs.release-version }} + # We need to use the inputs.release-version here, because on Windows we + # need to fixup the rc version that's stored in needs.prepare.outputs.release-version + release-version: ${{ inputs.release-version || needs.prepare.outputs.release-version }} artifact-id: ${{ needs.build-release-package.outputs.artifact-id }} attestation-name: ${{ needs.prepare.outputs.attestation-name }} digest: ${{ needs.build-release-package.outputs.digest }} diff --git a/.github/workflows/release-documentation.yml b/.github/workflows/release-documentation.yml index d0b6a9994edb5..c19148017cfec 100644 --- a/.github/workflows/release-documentation.yml +++ b/.github/workflows/release-documentation.yml @@ -38,76 +38,43 @@ on: LLVM_TOKEN_GENERATOR_PRIVATE_KEY: description: "Private key for our GitHub App we use for generating access tokens." required: true + # Run on pull_requests for testing purposes. + pull_request: + paths: + - '.github/workflows/release-documentation.yml' + - 'llvm/utils/release/build-docs.sh' + types: + - opened + - synchronize + - reopened + # When a PR is closed, we still start this workflow, but then skip + # all the jobs, which makes it effectively a no-op. The reason to + # do this is that it allows us to take advantage of concurrency groups + # to cancel in progress CI jobs whenever the PR is closed. + - closed + +concurrency: + group: release-documentation-${{ inputs.release-version || github.event.pull_request.number }} + cancel-in-progress: true jobs: - # This job checks permissions and validates inputs to prevent potential - # malicious actions. Since the release-documentation job has contents: write - # permissions we need to be extra careful about who can run the job and what - # inputs can be provided. - release-man-pages-validate-input: - name: Release Man Pages Validate Input - runs-on: ubuntu-24.04 - environment: - name: release - deployment: false - permissions: - contents: read - steps: - - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 - with: - persist-credentials: false - sparse-checkout: | - .github/workflows/ - - - name: Check Permissions - uses: ./.github/workflows/require-team-membership - with: - team-slug: llvm-release-managers - LLVM_TOKEN_GENERATOR_CLIENT_ID: ${{ secrets.LLVM_TOKEN_GENERATOR_CLIENT_ID }} - LLVM_TOKEN_GENERATOR_PRIVATE_KEY: ${{ secrets.LLVM_TOKEN_GENERATOR_PRIVATE_KEY }} - - - name: Validate Input - uses: ./.github/workflows/validate-release-version - with: - release-version: ${{ inputs.release-version }} - release-documentation: name: Build and Upload Release Documentation and Man Pages runs-on: ubuntu-24.04 - needs: - - release-man-pages-validate-input + if: >- + github.repository_owner == 'llvm' && + github.event.action != 'closed' outputs: man-page-digest: ${{ steps.man-page-digest.outputs.man-page-digest }} man-page-artifact-id: ${{ steps.man-page-artifact-upload.outputs.artifact-id }} - - man-page-release-version: ${{ steps.vars.outputs.man-page-release-version }} - man-page-tarball-name: ${{ steps.vars.outputs.man-page-tarball-name }} - man-page-upload: ${{ steps.vars.outputs.man-page-upload }} - man-page-attestation-name: ${{ steps.vars.outputs.man-page-attestation-name }} - env: - upload: ${{ inputs.upload && !contains(inputs.release-version, 'rc') }} steps: - - name: Collect Variables - id: vars - env: - INPUTS_RELEASE_VERSION: ${{ inputs.release-version }} - UPLOAD_MAN_PAGES: ${{ inputs.upload }} - shell: bash - run: | - { - echo "man-page-release-version=$INPUTS_RELEASE_VERSION" - echo "man-page-tarball-name=llvm_man_pages-$INPUTS_RELEASE_VERSION.tar.xz" - echo "man-page-ref=llvmorg-$INPUTS_RELEASE_VERSION" - echo "man-page-upload=$UPLOAD_MAN_PAGES" - echo "man-page-attestation-name=$RUNNER_OS-$RUNNER_ARCH-release-man-page-attestation" - } >> "$GITHUB_OUTPUT" - - name: Checkout LLVM uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: persist-credentials: false - name: Validate Input + if: inputs.release-version uses: ./.github/workflows/validate-release-version with: release-version: ${{ inputs.release-version }} @@ -129,25 +96,30 @@ jobs: pip3 install --require-hashes --user -r ./llvm/docs/requirements.txt - name: Build Documentation + id: build env: GITHUB_TOKEN: ${{ github.token }} INPUTS_RELEASE_VERSION: ${{ inputs.release-version }} run: | - ./llvm/utils/release/build-docs.sh -release "$INPUTS_RELEASE_VERSION" -no-doxygen + ./llvm/utils/release/build-docs.sh \ + $(test -n "$INPUTS_RELEASE_VERSION" && echo -release "$INPUTS_RELEASE_VERSION" || echo -srcdir llvm) -no-doxygen + echo "man-page-tarball-name=$(basename $(find . -iname 'llvm_man_pages-*.tar.xz'))" >> "$GITHUB_OUTPUT" + - name: Generate sha256 digest for man page tarball id: man-page-digest shell: bash env: - TARBALL_NAME: ${{ steps.vars.outputs.man-page-tarball-name }} + TARBALL_NAME: ${{ steps.build.outputs.man-page-tarball-name }} run: | - echo "man-page-digest=$(cat "$TARBALL_NAME" | sha256sum | cut -d ' ' -f 1)" >> $GITHUB_OUTPUT + echo "man-page-digest=$(cat "$TARBALL_NAME" | sha256sum | cut -d ' ' -f 1)" >> "$GITHUB_OUTPUT" - id: man-page-artifact-upload uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: + name: man-pages path: | - ${{ steps.vars.outputs.man-page-tarball-name }} + ${{ steps.build.outputs.man-page-tarball-name }} - name: Create Release Notes Artifact uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -155,8 +127,23 @@ jobs: name: release-notes path: docs-build/html-export/ + + upload-release-notes: + name: "Upload Release Notes" + runs-on: ubuntu-24.04 + environment: + deployment: false + name: release + needs: + - release-documentation + if: >- + github.event_name != 'pull_request' && + inputs.upload && + !contains(inputs.release-version, 'rc') + permissions: + contents: read + steps: - name: Clone www-releases - if: env.upload uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: repository: ${{ github.repository_owner }}/www-releases @@ -165,15 +152,19 @@ jobs: path: www-releases persist-credentials: false + - name: Download Release Notes Artifact + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + id: download-artifact + with: + name: release-notes + path: ${{ github.workspace }}/www-releases/${{ inputs.release-version }} + - name: Upload Release Notes - if: env.upload env: PUSH_TOKEN: ${{ secrets.LLVMBOT_WWW_RELEASES_PUSH }} GH_TOKEN: ${{ secrets.WWW_RELEASES_TOKEN }} INPUTS_RELEASE_VERSION: ${{ inputs.release-version }} run: | - mkdir -p www-releases/$INPUTS_RELEASE_VERSION - mv ./docs-build/html-export/* www-releases/$INPUTS_RELEASE_VERSION cd www-releases git checkout -b $INPUTS_RELEASE_VERSION git add $INPUTS_RELEASE_VERSION @@ -203,10 +194,10 @@ jobs: id: man-page-artifact-upload uses: $/.github/workflows/upload-release-artifact with: - release-version: ${{ needs.release-documentation.outputs.man-page-release-version }} + release-version: ${{ inputs.release-version }} artifact-id: ${{ needs.release-documentation.outputs.man-page-artifact-id }} - attestation-name: ${{ needs.release-documentation.outputs.man-page-attestation-name }} + attestation-name: ${{ runner.os }}-${{ runner.arch }}-release-man-page-attestation digest: ${{ needs.release-documentation.outputs.man-page-digest }} - upload: ${{ needs.release-documentation.outputs.man-page-upload }} + upload: ${{ inputs.upload }} LLVM_TOKEN_GENERATOR_CLIENT_ID: ${{ secrets.LLVM_TOKEN_GENERATOR_CLIENT_ID }} LLVM_TOKEN_GENERATOR_PRIVATE_KEY: ${{ secrets.LLVM_TOKEN_GENERATOR_PRIVATE_KEY }} diff --git a/.github/workflows/release-tasks.yml b/.github/workflows/release-tasks.yml index 932a9ffbe8601..5b9cdc64d1b8d 100644 --- a/.github/workflows/release-tasks.yml +++ b/.github/workflows/release-tasks.yml @@ -103,6 +103,7 @@ jobs: id-token: write # Requred for pypi publishing needs: - validate-tag + if: ${{ !contains(needs.validate-tag.outputs.release-version, 'rc') }} environment: pypi steps: - name: Checkout LLVM @@ -143,12 +144,6 @@ jobs: path: | llvm/utils/lit/dist - - name: Upload lit to test.pypi.org - uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # v1.14.0 - with: - repository-url: https://test.pypi.org/legacy/ - packages-dir: llvm/utils/lit/dist/ - - name: Upload lit to pypi.org uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # v1.14.0 with: diff --git a/bolt/docs/CommandLineArgumentReference.md b/bolt/docs/CommandLineArgumentReference.md index bf706ba3a1cdc..f41b23ec59877 100644 --- a/bolt/docs/CommandLineArgumentReference.md +++ b/bolt/docs/CommandLineArgumentReference.md @@ -196,6 +196,11 @@ Write BOLT Address Translation tables +- `--fix-branches-with-liveness` + + Use liveness analysis to decide whether AArch64 branch inversion must + preserve condition flags + - `--force-data-relocations` Force relocations to data sections to always be processed diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h index 37f78ce1d6e91..14d7f9b5b5359 100644 --- a/bolt/include/bolt/Core/BinaryFunction.h +++ b/bolt/include/bolt/Core/BinaryFunction.h @@ -54,6 +54,7 @@ #include #include #include +#include #include #include #include @@ -66,6 +67,8 @@ class DWARFUnit; namespace bolt { +class BranchLivenessInfo; + using InputOffsetToAddressMapTy = std::unordered_multimap; /// Types of macro-fusion alignment corrections. @@ -403,11 +406,15 @@ class BinaryFunction { /// to avoid redundant processing. bool NeedBranchValidation{true}; - /// Name for the section this function code should reside in. - std::string CodeSectionName; + /// Name for the section this function code should reside in. When unset, the + /// default name is derived on demand from the function's name (see + /// getMainSectionName()). Deferring this avoids eagerly storing a copy of the + /// (potentially large, mangled) function name for every function, which is a + /// significant source of memory use on large binaries. + std::optional CodeSectionName; - /// Name for the corresponding cold code section. - std::string ColdCodeSectionName; + /// Name for the corresponding cold code section. See CodeSectionName. + std::optional ColdCodeSectionName; /// Parent function fragment for split function fragments. using FragmentsSetTy = SmallPtrSet; @@ -747,12 +754,24 @@ class BinaryFunction { static std::string buildColdCodeSectionName(StringRef Name, const BinaryContext &BC); + /// Return the name of the main code section, using the default derived from + /// the function's name when no name has been explicitly assigned. + std::string getMainSectionName() const { + return CodeSectionName ? *CodeSectionName + : buildCodeSectionName(getOneName(), BC); + } + + /// Return the name of the cold code section, using the default derived from + /// the function's name when no name has been explicitly assigned. + std::string getColdSectionName() const { + return ColdCodeSectionName ? *ColdCodeSectionName + : buildColdCodeSectionName(getOneName(), BC); + } + /// Creation should be handled by RewriteInstance or BinaryContext BinaryFunction(const std::string &Name, BinarySection &Section, uint64_t Address, uint64_t Size, BinaryContext &BC) : OriginSection(&Section), Address(Address), Size(Size), BC(BC), - CodeSectionName(buildCodeSectionName(Name, BC)), - ColdCodeSectionName(buildColdCodeSectionName(Name, BC)), FunctionNumber(++Count) { Symbols.push_back(BC.Ctx->getOrCreateSymbol(Name)); } @@ -1398,12 +1417,12 @@ class BinaryFunction { SmallString<32> getCodeSectionName(const FragmentNum Fragment = FragmentNum::main()) const { if (Fragment == FragmentNum::main()) - return SmallString<32>(CodeSectionName); + return SmallString<32>(getMainSectionName()); if (Fragment == FragmentNum::cold()) - return SmallString<32>(ColdCodeSectionName); + return SmallString<32>(getColdSectionName()); if (BC.HasWarmSection && Fragment == FragmentNum::warm()) return SmallString<32>(BC.getWarmCodeSectionName()); - return formatv("{0}.{1}", ColdCodeSectionName, Fragment.get() - 1); + return formatv("{0}.{1}", getColdSectionName(), Fragment.get() - 1); } /// Assign a code section name to the function. @@ -2504,7 +2523,7 @@ class BinaryFunction { /// while the second successor - false/fall-through branch. /// /// When we reverse the branch condition, the CFG is updated accordingly. - void fixBranches(); + void fixBranches(const BranchLivenessInfo *BLI = nullptr); /// Mark function as finalized. No further optimizations are permitted. void setFinalized() { CurrentState = State::CFG_Finalized; } diff --git a/bolt/include/bolt/Core/BranchLivenessInfo.h b/bolt/include/bolt/Core/BranchLivenessInfo.h new file mode 100644 index 0000000000000..7587edfa30d51 --- /dev/null +++ b/bolt/include/bolt/Core/BranchLivenessInfo.h @@ -0,0 +1,43 @@ +//===- bolt/Core/BranchLivenessInfo.h ---------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef BOLT_CORE_BRANCHLIVENESSINFO_H +#define BOLT_CORE_BRANCHLIVENESSINFO_H + +namespace llvm { +class MCInst; + +namespace bolt { +class BinaryFunction; + +class BranchLivenessInfo { + BinaryFunction *BF; + unsigned AnnotationIndex; + + void swap(BranchLivenessInfo &Other) noexcept; + +public: + explicit BranchLivenessInfo(BinaryFunction &BF); + ~BranchLivenessInfo(); + + // Copies would create multiple owners for removing the same annotations. + BranchLivenessInfo(const BranchLivenessInfo &) = delete; + BranchLivenessInfo &operator=(const BranchLivenessInfo &) = delete; + + BranchLivenessInfo(BranchLivenessInfo &&Other) noexcept; + BranchLivenessInfo &operator=(BranchLivenessInfo &&Other) noexcept; + + bool mustPreserveFlags(const MCInst &Inst) const; + void removeAnnotation(MCInst &Inst) const; + void setFlagsDead(MCInst &Inst); +}; + +} // namespace bolt +} // namespace llvm + +#endif diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index aae520d5afe54..be0d58af14fc4 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -475,7 +475,8 @@ class MCPlusBuilder { } /// Check whether this conditional branch can be reversed - virtual bool isReversibleBranch(const MCInst &Inst) const { + virtual bool isReversibleBranch(const MCInst &Inst, + bool MustPreserveFlags = true) const { assert(!isUnsupportedInstruction(Inst) && isConditionalBranch(Inst) && "Instruction is not known conditional branch"); @@ -2150,9 +2151,13 @@ class MCPlusBuilder { llvm_unreachable("not implemented"); } - /// Reverses the branch condition in Inst and update its taken target to TBB. - virtual void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB, - MCContext *Ctx) const { + /// Return the instruction sequence for the reversed branch condition of + /// \p Inst and update its taken target to \p TBB. Assumes that the branch is + /// reversible. It may replace Inst with a longer instruction sequence on some + /// targets. + virtual InstructionListType + reverseBranchCondition(MCInst Inst, const MCSymbol *TBB, MCContext *Ctx, + bool MustPreserveFlags = true) const { llvm_unreachable("not implemented"); } diff --git a/bolt/include/bolt/Passes/BranchLivenessUtils.h b/bolt/include/bolt/Passes/BranchLivenessUtils.h new file mode 100644 index 0000000000000..69955d0b6960a --- /dev/null +++ b/bolt/include/bolt/Passes/BranchLivenessUtils.h @@ -0,0 +1,28 @@ +//===- bolt/Passes/BranchLivenessUtils.h ------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef BOLT_PASSES_BRANCHLIVENESSUTILS_H +#define BOLT_PASSES_BRANCHLIVENESSUTILS_H + +#include "bolt/Core/BranchLivenessInfo.h" + +namespace llvm { +namespace bolt { +class BinaryFunction; +class RegAnalysis; + +/// Return true if \p BF needs liveness info for branch transformations. +bool needsBranchLiveness(BinaryFunction &BF); + +/// Return liveness info required for branch transformations. +BranchLivenessInfo computeBranchLiveness(BinaryFunction &BF, RegAnalysis &RA); + +} // namespace bolt +} // namespace llvm + +#endif diff --git a/bolt/include/bolt/Passes/LongJmp.h b/bolt/include/bolt/Passes/LongJmp.h index 4633d30104d43..4a288d3bde4e3 100644 --- a/bolt/include/bolt/Passes/LongJmp.h +++ b/bolt/include/bolt/Passes/LongJmp.h @@ -14,6 +14,8 @@ namespace llvm { namespace bolt { +class BranchLivenessInfo; + /// LongJmp is veneer-insertion pass originally written for AArch64 that /// compensates for its short-range branches, typically done during linking. We /// pull this pass inside BOLT because here we can do a better job at stub @@ -73,8 +75,10 @@ class LongJmpPass : public BinaryFunctionPass { /// Relax all internal function branches including those between fragments. /// Assume that fragments are placed in different sections but are within - /// 128MB of each other. - void relaxLocalBranches(BinaryFunction &BF); + /// 128MB of each other. Return false and report an error if a branch cannot + /// be relaxed. + bool relaxLocalBranches(BinaryFunction &BF, + const BranchLivenessInfo *BLI = nullptr); /// -- Layout estimation methods -- /// Try to do layout before running the emitter, by looking at BinaryFunctions diff --git a/bolt/include/bolt/Profile/Heatmap.h b/bolt/include/bolt/Profile/Heatmap.h index 268b02c7d093c..6584999315615 100644 --- a/bolt/include/bolt/Profile/Heatmap.h +++ b/bolt/include/bolt/Profile/Heatmap.h @@ -76,9 +76,9 @@ class Heatmap { void print(raw_ostream &OS) const; - void printCDF(StringRef FileName) const; + void printCDF(StringRef FileName, StringRef Label) const; - void printCDF(raw_ostream &OS) const; + void printCDF(raw_ostream &OS, StringRef Label) const; void printSectionHotness(StringRef Filename) const; diff --git a/bolt/include/bolt/Utils/CommandLineOpts.h b/bolt/include/bolt/Utils/CommandLineOpts.h index e11b18d3489cf..80eeacbfd6ae7 100644 --- a/bolt/include/bolt/Utils/CommandLineOpts.h +++ b/bolt/include/bolt/Utils/CommandLineOpts.h @@ -48,7 +48,13 @@ enum SplitFunctionsStrategy : char { All }; -using HeatmapBlockSizes = std::vector; +/// A bucket size and how it was spelled on the command line, so output can +/// echo "64K" rather than reformatting the value. +struct HeatmapBlockSize { + unsigned Value = 0; + std::string Spec; +}; +using HeatmapBlockSizes = std::vector; struct HeatmapBlockSpecParser : public llvm::cl::parser { explicit HeatmapBlockSpecParser(llvm::cl::Option &O) : llvm::cl::parser(O) {} @@ -94,6 +100,7 @@ extern llvm::cl::opt HeatmapBlock; extern llvm::cl::opt HeatmapMaxAddress; extern llvm::cl::opt HeatmapMinAddress; +extern llvm::cl::opt HeatmapCdfPct; extern llvm::cl::opt HeatmapPrintMappings; extern llvm::cl::opt HeatmapOutput; extern llvm::cl::opt HotData; @@ -132,6 +139,10 @@ extern llvm::cl::opt UpdateDebugSections; // dbgs() for output within DEBUG(). extern llvm::cl::opt Verbosity; +// Option to control whether liveness analysis should be used by +// FixupBranches and LongJmpPass. Needed for branch inversion on AArch64. +extern llvm::cl::opt FixBranchesWithLiveness; + /// Return true if we should process all functions in the binary. bool processAllFunctions(); diff --git a/bolt/include/bolt/Utils/NameResolver.h b/bolt/include/bolt/Utils/NameResolver.h index 9719ce1297a7f..34b91f0bf401b 100644 --- a/bolt/include/bolt/Utils/NameResolver.h +++ b/bolt/include/bolt/Utils/NameResolver.h @@ -13,26 +13,39 @@ #ifndef BOLT_UTILS_NAME_RESOLVER_H #define BOLT_UTILS_NAME_RESOLVER_H -#include "llvm/ADT/StringMap.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/StringRef.h" #include "llvm/ADT/Twine.h" +#include "llvm/Support/xxhash.h" namespace llvm { namespace bolt { class NameResolver { - /// Track the number of duplicate names. - StringMap Counters; + /// Track the number of duplicate names, keyed by a 128-bit hash of the name + /// rather than by the name itself. Storing hashes instead of the full strings + /// avoids duplicating potentially large (mangled) symbol names, which is a + /// significant source of memory use while processing the symbol table. Using + /// a 128-bit hash makes collisions effectively impossible, so the counts (and + /// therefore the generated unique names) are identical to a string-keyed map + /// and remain reproducible to match profile (fdata) names. + DenseMap, uint64_t> Counters; /// Character guaranteed not to be used by any "native" name passed to /// uniquify() function. static constexpr char Sep = '/'; + /// Return the map key used to track occurrences of \p Name. + static std::pair getKey(StringRef Name) { + const XXH128_hash_t Hash = llvm::xxh3_128bits( + reinterpret_cast(Name.data()), Name.size()); + return {Hash.low64, Hash.high64}; + } + public: /// Return the number of uniquified versions of a given \p Name. uint64_t getUniquifiedNameCount(StringRef Name) const { - if (Counters.contains(Name)) - return Counters.at(Name); - return 0; + return Counters.lookup(getKey(Name)); } /// Return unique version of the \p Name in the form "Name". @@ -43,10 +56,14 @@ class NameResolver { /// Register new version of \p Name and return unique version in the form /// "Name". std::string uniquify(StringRef Name) { - const uint64_t ID = ++Counters[Name]; + const uint64_t ID = ++Counters[getKey(Name)]; return getUniqueName(Name, ID); } + /// Release the memory used to track name occurrences. Call once no more names + /// need to be uniquified (e.g. after file object discovery is complete). + void clear() { Counters.clear(); } + /// For uniquified \p Name, return the original form (that may no longer be /// unique). static StringRef restore(StringRef Name) { diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index a6722389e5d50..a81fa2f45c206 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -12,6 +12,7 @@ #include "bolt/Core/BinaryFunction.h" #include "bolt/Core/BinaryBasicBlock.h" +#include "bolt/Core/BranchLivenessInfo.h" #include "bolt/Core/DynoStats.h" #include "bolt/Core/HashUtilities.h" #include "bolt/Core/MCPlusBuilder.h" @@ -3676,7 +3677,7 @@ bool BinaryFunction::validateCFG() const { return true; } -void BinaryFunction::fixBranches() { +void BinaryFunction::fixBranches(const BranchLivenessInfo *BLI) { assert(isSimple() && "Expected function with valid CFG."); auto &MIB = BC.MIB; @@ -3735,7 +3736,8 @@ void BinaryFunction::fixBranches() { // Reverse branch condition and swap successors. auto swapSuccessors = [&]() { - if (!MIB->isReversibleBranch(*CondBranch)) { + bool PreserveFlags = BLI ? BLI->mustPreserveFlags(*CondBranch) : true; + if (!MIB->isReversibleBranch(*CondBranch, PreserveFlags)) { if (opts::Verbosity) { BC.outs() << "BOLT-INFO: unable to swap successors in " << *this << '\n'; @@ -3745,7 +3747,11 @@ void BinaryFunction::fixBranches() { std::swap(TSuccessor, FSuccessor); BB->swapConditionalSuccessors(); auto L = BC.scopeLock(); - MIB->reverseBranchCondition(*CondBranch, TSuccessor->getLabel(), Ctx); + if (BLI) + BLI->removeAnnotation(*CondBranch); + InstructionListType Code = MIB->reverseBranchCondition( + *CondBranch, TSuccessor->getLabel(), Ctx, PreserveFlags); + BB->replaceInstruction(BB->findInstruction(CondBranch), Code); return true; }; diff --git a/bolt/lib/Core/BranchLivenessInfo.cpp b/bolt/lib/Core/BranchLivenessInfo.cpp new file mode 100644 index 0000000000000..608821c69652e --- /dev/null +++ b/bolt/lib/Core/BranchLivenessInfo.cpp @@ -0,0 +1,75 @@ +//===- bolt/Core/BranchLivenessInfo.cpp ----------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "bolt/Core/BranchLivenessInfo.h" +#include "bolt/Core/BinaryBasicBlock.h" +#include "bolt/Core/BinaryContext.h" +#include "bolt/Core/BinaryFunction.h" +#include "bolt/Core/MCPlusBuilder.h" +#include "llvm/MC/MCInst.h" +#include +#include + +namespace llvm { +namespace bolt { + +BranchLivenessInfo::BranchLivenessInfo(BinaryFunction &BF) + : BF(&BF), + AnnotationIndex( + BF.getBinaryContext().MIB->getOrCreateAnnotationIndex("DeadFlags")) {} + +BranchLivenessInfo::~BranchLivenessInfo() { + if (!BF) + return; + + MCPlusBuilder &MIB = *BF->getBinaryContext().MIB; + for (BinaryBasicBlock &BB : *BF) + for (MCInst &Inst : BB) + MIB.removeAnnotation(Inst, AnnotationIndex); +} + +BranchLivenessInfo::BranchLivenessInfo(BranchLivenessInfo &&Other) noexcept + : BF(nullptr), AnnotationIndex(0) { + swap(Other); +} + +BranchLivenessInfo & +BranchLivenessInfo::operator=(BranchLivenessInfo &&Other) noexcept { + BranchLivenessInfo Tmp(std::move(Other)); + swap(Tmp); + return *this; +} + +void BranchLivenessInfo::swap(BranchLivenessInfo &Other) noexcept { + std::swap(BF, Other.BF); + std::swap(AnnotationIndex, Other.AnnotationIndex); +} + +bool BranchLivenessInfo::mustPreserveFlags(const MCInst &Inst) const { + if (!BF) + return true; + + return !BF->getBinaryContext().MIB->hasAnnotation(Inst, AnnotationIndex); +} + +void BranchLivenessInfo::removeAnnotation(MCInst &Inst) const { + assert(BF && "branch liveness info is not initialized"); + + BF->getBinaryContext().MIB->removeAnnotation(Inst, AnnotationIndex); +} + +void BranchLivenessInfo::setFlagsDead(MCInst &Inst) { + assert(BF && "branch liveness info is not initialized"); + + MCPlusBuilder &MIB = *BF->getBinaryContext().MIB; + if (!MIB.hasAnnotation(Inst, AnnotationIndex)) + MIB.addAnnotation(Inst, AnnotationIndex, true); +} + +} // namespace bolt +} // namespace llvm diff --git a/bolt/lib/Core/CMakeLists.txt b/bolt/lib/Core/CMakeLists.txt index 58cfcab370f16..430a1c93a9f0c 100644 --- a/bolt/lib/Core/CMakeLists.txt +++ b/bolt/lib/Core/CMakeLists.txt @@ -21,6 +21,7 @@ add_llvm_library(LLVMBOLTCore BinaryFunctionCallGraph.cpp BinaryFunctionProfile.cpp BinarySection.cpp + BranchLivenessInfo.cpp CallGraph.cpp CallGraphWalker.cpp DebugData.cpp diff --git a/bolt/lib/Core/Relocation.cpp b/bolt/lib/Core/Relocation.cpp index 6663abcffc7e8..b0f6b6ce0eddc 100644 --- a/bolt/lib/Core/Relocation.cpp +++ b/bolt/lib/Core/Relocation.cpp @@ -71,6 +71,8 @@ static bool isSupportedAArch64(uint32_t Type) { case ELF::R_AARCH64_LDST16_ABS_LO12_NC: case ELF::R_AARCH64_LDST8_ABS_LO12_NC: case ELF::R_AARCH64_ADR_GOT_PAGE: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC: @@ -183,6 +185,8 @@ static size_t getSizeForTypeAArch64(uint32_t Type) { case ELF::R_AARCH64_LDST16_ABS_LO12_NC: case ELF::R_AARCH64_LDST8_ABS_LO12_NC: case ELF::R_AARCH64_ADR_GOT_PAGE: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC: @@ -385,6 +389,7 @@ static uint64_t extractValueAArch64(uint32_t Type, uint64_t Contents, Contents &= ~0xffffffffff00001fULL; return static_cast(PC) + SignExtend64<21>(Contents >> 3); case ELF::R_AARCH64_ADR_GOT_PAGE: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21: @@ -417,6 +422,7 @@ static uint64_t extractValueAArch64(uint32_t Type, uint64_t Contents, } case ELF::R_AARCH64_TLSLE_ADD_TPREL_HI12: case ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12_NC: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSDESC_ADD_LO12: case ELF::R_AARCH64_ADD_ABS_LO12_NC: { // Immediate goes in bits 21:10 of ADD instruction @@ -557,6 +563,8 @@ static bool isGOTAArch64(uint32_t Type) { case ELF::R_AARCH64_LD64_GOT_LO12_NC: case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC: case ELF::R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_TLSDESC_LD64_LO12: @@ -591,6 +599,8 @@ static bool isTLSAArch64(uint32_t Type) { switch (Type) { default: return false; + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC: @@ -661,6 +671,7 @@ static bool isPCRelativeAArch64(uint32_t Type) { case ELF::R_AARCH64_LDST16_ABS_LO12_NC: case ELF::R_AARCH64_LDST8_ABS_LO12_NC: case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSLE_ADD_TPREL_HI12: case ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12_NC: case ELF::R_AARCH64_TLSLE_MOVW_TPREL_G0: @@ -685,6 +696,7 @@ static bool isPCRelativeAArch64(uint32_t Type) { case ELF::R_AARCH64_ADR_PREL_PG_HI21_NC: case ELF::R_AARCH64_ADR_GOT_PAGE: case ELF::R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_PREL16: diff --git a/bolt/lib/Passes/AArch64RelaxationPass.cpp b/bolt/lib/Passes/AArch64RelaxationPass.cpp index 2b7384dc848dd..51260ceadf918 100644 --- a/bolt/lib/Passes/AArch64RelaxationPass.cpp +++ b/bolt/lib/Passes/AArch64RelaxationPass.cpp @@ -59,10 +59,12 @@ void AArch64RelaxationPass::runOnFunction(BinaryFunction &BF) { continue; } - // Don't relax ADR/LDR if it points to the same function and is in the - // main fragment and BF initial size is < 1MB. + // The layout of a non-simple function is preserved, so references within + // the same fragment retain their original in-range displacement. For + // simple functions, basic blocks can move, but an initial size below 1MiB + // guarantees that internal references remain in range after reordering. const unsigned OneMB = 0x100000; - if (BF.getSize() < OneMB) { + if (!BF.isSimple() || BF.getSize() < OneMB) { BinaryFunction *TargetBF = BC.getFunctionForSymbol(Symbol); if (TargetBF == &BF && !BB.isSplit()) continue; diff --git a/bolt/lib/Passes/BinaryPasses.cpp b/bolt/lib/Passes/BinaryPasses.cpp index fc0a6e6ee9c00..d24295e5c2c32 100644 --- a/bolt/lib/Passes/BinaryPasses.cpp +++ b/bolt/lib/Passes/BinaryPasses.cpp @@ -13,14 +13,18 @@ #include "bolt/Passes/BinaryPasses.h" #include "bolt/Core/FunctionLayout.h" #include "bolt/Core/ParallelUtilities.h" +#include "bolt/Passes/BranchLivenessUtils.h" +#include "bolt/Passes/RegAnalysis.h" #include "bolt/Passes/ReorderAlgorithm.h" #include "bolt/Passes/ReorderFunctions.h" #include "bolt/Utils/CommandLineOpts.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/Support/CommandLine.h" #include #include #include #include +#include #include #define DEBUG_TYPE "bolt-opts" @@ -553,12 +557,29 @@ bool ReorderBasicBlocks::modifyFunctionLayout(BinaryFunction &BF, } Error FixupBranches::runOnFunctions(BinaryContext &BC) { + const bool ShouldRunRegisterAnalysis = + opts::FixBranchesWithLiveness && + llvm::any_of(BC.getBinaryFunctions(), [&](auto &It) { + BinaryFunction &BF = It.second; + return BC.shouldEmit(BF) && BF.isSimple() && needsBranchLiveness(BF); + }); + + std::optional RA; + if (ShouldRunRegisterAnalysis) + RA.emplace(BC, nullptr, nullptr); + for (auto &It : BC.getBinaryFunctions()) { - BinaryFunction &Function = It.second; - if (!BC.shouldEmit(Function) || !Function.isSimple()) + BinaryFunction &BF = It.second; + if (!BC.shouldEmit(BF) || !BF.isSimple()) continue; - Function.fixBranches(); + if (!RA || !needsBranchLiveness(BF)) { + BF.fixBranches(); + continue; + } + + BranchLivenessInfo BLI = computeBranchLiveness(BF, *RA); + BF.fixBranches(&BLI); } return Error::success(); } @@ -969,7 +990,11 @@ uint64_t SimplifyConditionalTailCalls::fixTailCalls(BinaryFunction &BF) { uint64_t Count = 0; if (CondSucc != BB) { // Patch the new target address into the conditional branch. - MIB->reverseBranchCondition(*CondBranch, CalleeSymbol, Ctx); + InstructionListType Code = + MIB->reverseBranchCondition(*CondBranch, CalleeSymbol, Ctx); + auto II = PredBB->replaceInstruction( + PredBB->findInstruction(CondBranch), Code); + CondBranch = &*(II + Code.size() - 1); // Since we reversed the condition on the branch we need to change // the target for the unconditional branch or add a unconditional // branch to the old target. This has to be done manually since diff --git a/bolt/lib/Passes/BranchLivenessUtils.cpp b/bolt/lib/Passes/BranchLivenessUtils.cpp new file mode 100644 index 0000000000000..a187ca3c98757 --- /dev/null +++ b/bolt/lib/Passes/BranchLivenessUtils.cpp @@ -0,0 +1,59 @@ +//===- bolt/Passes/BranchLivenessUtils.cpp -------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "bolt/Passes/BranchLivenessUtils.h" +#include "bolt/Core/BinaryContext.h" +#include "bolt/Core/BinaryFunction.h" +#include "bolt/Core/MCPlusBuilder.h" +#include "bolt/Passes/DataflowInfoManager.h" +#include "bolt/Passes/RegAnalysis.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/MC/MCRegister.h" + +namespace llvm { +namespace bolt { + +bool needsBranchLiveness(BinaryFunction &BF) { + BinaryContext &BC = BF.getBinaryContext(); + if (!BC.isAArch64()) + return false; + + return llvm::any_of(BF, [&](BinaryBasicBlock &BB) { + return llvm::any_of(BB, [&](MCInst &Inst) { + return BC.MIB->isShortRangeBranch(Inst) && + !BC.MIB->isReversibleBranch(Inst); + }); + }); +} + +BranchLivenessInfo computeBranchLiveness(BinaryFunction &BF, RegAnalysis &RA) { + BinaryContext &BC = BF.getBinaryContext(); + SmallVector Insts; + if (BC.isAArch64()) + for (BinaryBasicBlock &BB : BF) + for (MCInst &Inst : BB) + if (BC.MIB->isShortRangeBranch(Inst) && + !BC.MIB->isReversibleBranch(Inst)) + Insts.push_back(&Inst); + + BranchLivenessInfo BLI(BF); + if (Insts.empty()) + return BLI; + + DataflowInfoManager DIM(BF, &RA, nullptr); + LivenessAnalysis &LA = DIM.getLivenessAnalysis(); + const MCPhysReg FlagsReg = BC.MIB->getFlagsReg(); + for (MCInst *Inst : Insts) + if (!LA.getLiveIn(*Inst).test(FlagsReg)) + BLI.setFlagsDead(*Inst); + return BLI; +} + +} // namespace bolt +} // namespace llvm diff --git a/bolt/lib/Passes/CMakeLists.txt b/bolt/lib/Passes/CMakeLists.txt index ec012f05cc498..686dee6987a73 100644 --- a/bolt/lib/Passes/CMakeLists.txt +++ b/bolt/lib/Passes/CMakeLists.txt @@ -4,6 +4,7 @@ add_llvm_library(LLVMBOLTPasses AllocCombiner.cpp AsmDump.cpp BinaryPasses.cpp + BranchLivenessUtils.cpp CMOVConversion.cpp CacheMetrics.cpp DataflowAnalysis.cpp diff --git a/bolt/lib/Passes/LongJmp.cpp b/bolt/lib/Passes/LongJmp.cpp index b771e6a8b120a..38ad4ed52f339 100644 --- a/bolt/lib/Passes/LongJmp.cpp +++ b/bolt/lib/Passes/LongJmp.cpp @@ -12,7 +12,10 @@ #include "bolt/Passes/LongJmp.h" #include "bolt/Core/ParallelUtilities.h" +#include "bolt/Passes/BranchLivenessUtils.h" +#include "bolt/Passes/RegAnalysis.h" #include "bolt/Utils/CommandLineOpts.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/Support/MathExtras.h" #define DEBUG_TYPE "longjmp" @@ -662,13 +665,16 @@ Error LongJmpPass::relax(BinaryFunction &Func, bool &Modified) { return Error::success(); } -void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { +bool LongJmpPass::relaxLocalBranches(BinaryFunction &BF, + const BranchLivenessInfo *BLI) { BinaryContext &BC = BF.getBinaryContext(); auto &MIB = BC.MIB; - // Quick path. - if (!BF.isSplit() && BF.estimateSize() < ShortestJumpSpan) - return; + // Quick path. Only valid for simple functions, where all branch targets are + // basic blocks of the function itself. A non-simple function may branch to a + // symbol outside of it that ends up out of range. + if (BF.isSimple() && !BF.isSplit() && BF.estimateSize() < ShortestJumpSpan) + return true; auto isBranchOffsetInRange = [&](const MCInst &Inst, int64_t Offset) { const unsigned Bits = MIB->getPCRelEncodingSize(Inst); @@ -708,22 +714,32 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { DenseMap FragmentTrampolines; // Create a trampoline code after \p BB or at the end of the fragment if BB - // is nullptr. If \p UpdateOffsets is true, update FragmentSize and offsets - // for basic blocks affected by the insertion of the trampoline. + // is nullptr. The trampoline branches to \p TargetSym. If \p TargetBB is + // set, it is added as a successor and registered in FragmentTrampolines. + // \p Offset reflects the size delta of BB caused by splitting unconditional + // branches, or replacing a branch with a longer instruction sequence. It is + // used to update the output addresses of basic blocks following the + // trampoline. auto addTrampolineAfter = [&](BinaryBasicBlock *BB, + const MCSymbol *TargetSym, BinaryBasicBlock *TargetBB, uint64_t Count, - bool UpdateOffsets = true) { + uint64_t Offset = 0) { FunctionTrampolines.emplace_back(BB ? BB : FF.back(), BF.createBasicBlock()); BinaryBasicBlock *TrampolineBB = FunctionTrampolines.back().second.get(); + const uint64_t OldBBEnd = BB ? BB->getOutputEndAddress() : 0; + if (BB && Offset) + BB->setOutputEndAddress(OldBBEnd + Offset); + Offset += TrampolineSize; MCInst Inst; { auto L = BC.scopeLock(); - MIB->createUncondBranch(Inst, TargetBB->getLabel(), BC.Ctx.get()); + MIB->createUncondBranch(Inst, TargetSym, BC.Ctx.get()); } TrampolineBB->addInstruction(Inst); - TrampolineBB->addSuccessor(TargetBB, Count); + if (TargetBB) + TrampolineBB->addSuccessor(TargetBB, Count); TrampolineBB->setExecutionCount(Count); const uint64_t TrampolineAddress = BB ? BB->getOutputEndAddress() : FragmentSize; @@ -731,13 +747,23 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { TrampolineBB->setOutputEndAddress(TrampolineAddress + TrampolineSize); TrampolineBB->setFragmentNum(FF.getFragmentNum()); - if (!FragmentTrampolines.lookup(TargetBB)) + // Shift the fragment-local output address range for blocks at or after + // the old end address. + auto adjustBasicBlockAddress = [](BinaryBasicBlock *BB, uint64_t Address, + uint64_t Offset) { + if (BB->getOutputStartAddress() < Address) + return; + BB->setOutputStartAddress(BB->getOutputStartAddress() + Offset); + BB->setOutputEndAddress(BB->getOutputEndAddress() + Offset); + }; + + if (TargetBB && !FragmentTrampolines.lookup(TargetBB)) FragmentTrampolines[TargetBB] = TrampolineBB; - if (!UpdateOffsets) + if (!Offset) return TrampolineBB; - FragmentSize += TrampolineSize; + FragmentSize += Offset; // If the trampoline was added at the end of the fragment, offsets of // other fragments should stay intact. @@ -745,13 +771,8 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { return TrampolineBB; // Update offsets for blocks after BB. - for (BinaryBasicBlock *IBB : FF) { - if (IBB->getOutputStartAddress() >= TrampolineAddress) { - IBB->setOutputStartAddress(IBB->getOutputStartAddress() + - TrampolineSize); - IBB->setOutputEndAddress(IBB->getOutputEndAddress() + TrampolineSize); - } - } + for (BinaryBasicBlock *IBB : FF) + adjustBasicBlockAddress(IBB, OldBBEnd, Offset); // Update offsets for trampolines in this fragment that are placed after // the new trampoline. Note that trampoline blocks are not part of the @@ -763,33 +784,34 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { continue; if (IBB == TrampolineBB) continue; - if (IBB->getOutputStartAddress() >= TrampolineAddress) { - IBB->setOutputStartAddress(IBB->getOutputStartAddress() + - TrampolineSize); - IBB->setOutputEndAddress(IBB->getOutputEndAddress() + TrampolineSize); - } + adjustBasicBlockAddress(IBB, OldBBEnd, Offset); } return TrampolineBB; }; // Pre-populate trampolines by splitting unconditional branches from the - // containing basic block. - for (BinaryBasicBlock *BB : FF) { - MCInst *Inst = BB->getLastNonPseudoInstr(); - if (!Inst || !MIB->isUnconditionalBranch(*Inst)) - continue; + // containing basic block. Skip for non-simple functions: this creates + // trampolines for targets inside the function, while in a non-simple + // function we only relax branches to targets outside of it. + if (BF.isSimple()) { + for (BinaryBasicBlock *BB : FF) { + MCInst *Inst = BB->getLastNonPseudoInstr(); + if (!Inst || !MIB->isUnconditionalBranch(*Inst)) + continue; - const MCSymbol *TargetSymbol = MIB->getTargetSymbol(*Inst); - BB->eraseInstruction(BB->findInstruction(Inst)); - BB->setOutputEndAddress(BB->getOutputEndAddress() - TrampolineSize); + const MCSymbol *TargetSymbol = MIB->getTargetSymbol(*Inst); + BB->eraseInstruction(BB->findInstruction(Inst)); - BinaryBasicBlock::BinaryBranchInfo BI; - BinaryBasicBlock *TargetBB = BB->getSuccessor(TargetSymbol, BI); + BinaryBasicBlock::BinaryBranchInfo BI; + BinaryBasicBlock *TargetBB = BB->getSuccessor(TargetSymbol, BI); - BinaryBasicBlock *TrampolineBB = - addTrampolineAfter(BB, TargetBB, BI.Count, /*UpdateOffsets*/ false); - BB->replaceSuccessor(TargetBB, TrampolineBB, BI.Count); + // Erasing the unconditional branch shrinks BB by one instruction. + BinaryBasicBlock *TrampolineBB = + addTrampolineAfter(BB, TargetBB->getLabel(), TargetBB, BI.Count, + /*Offset=*/-4); + BB->replaceSuccessor(TargetBB, TrampolineBB, BI.Count); + } } /// Relax the branch \p Inst in basic block \p BB that targets \p TargetBB. @@ -821,7 +843,8 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { // case we will need further relaxation. const int64_t OffsetToEnd = FragmentSize - InstAddress; if (Count == 0 && isBranchOffsetInRange(Inst, OffsetToEnd)) { - TrampolineBB = addTrampolineAfter(nullptr, TargetBB, Count); + TrampolineBB = + addTrampolineAfter(nullptr, TargetBB->getLabel(), TargetBB, Count); BB->replaceSuccessor(TargetBB, TrampolineBB, Count); auto L = BC.scopeLock(); MIB->replaceBranchTarget(Inst, TrampolineBB->getLabel(), BC.Ctx.get()); @@ -832,7 +855,8 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { // If the other successor is a fall-through, invert the condition code. BinaryBasicBlock *NextBB = BF->getLayout().getBasicBlockAfter(BB, /*IgnoreSplits*/ false); - bool IsReversibleBranch = MIB->isReversibleBranch(Inst); + bool PreserveFlags = BLI ? BLI->mustPreserveFlags(Inst) : true; + bool IsReversibleBranch = MIB->isReversibleBranch(Inst, PreserveFlags); bool ShouldReverseBranch = BB->getConditionalSuccessor(false) == NextBB; // Create a trampoline basic block for the fall-through target of the @@ -840,24 +864,40 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { if (ShouldReverseBranch && !IsReversibleBranch) { const uint64_t NextCount = BB->getBranchInfo(*NextBB).Count; BinaryBasicBlock *FallThrough = - addTrampolineAfter(BB, NextBB, NextCount); + addTrampolineAfter(BB, NextBB->getLabel(), NextBB, NextCount); BB->replaceSuccessor(NextBB, FallThrough, NextCount); } - // Create a trampoline basic block for the taken target of the branch. - TrampolineBB = addTrampolineAfter(BB, TargetBB, Count); - if (ShouldReverseBranch && IsReversibleBranch) { + const uint64_t OldBBSize = BB->estimateSize(); BB->swapConditionalSuccessors(); - auto L = BC.scopeLock(); - MIB->reverseBranchCondition(Inst, NextBB->getLabel(), BC.Ctx.get()); + { + auto L = BC.scopeLock(); + if (BLI) + BLI->removeAnnotation(Inst); + InstructionListType Code = MIB->reverseBranchCondition( + Inst, NextBB->getLabel(), BC.Ctx.get(), PreserveFlags); + BB->replaceInstruction(BB->findInstruction(&Inst), Code); + } + const uint64_t NewBBSize = BB->estimateSize(); + + // Create a trampoline basic block for the original taken target. + TrampolineBB = addTrampolineAfter(BB, TargetBB->getLabel(), TargetBB, + Count, NewBBSize - OldBBSize); } else { + // Create a trampoline basic block for the taken target of the branch. + TrampolineBB = + addTrampolineAfter(BB, TargetBB->getLabel(), TargetBB, Count); auto L = BC.scopeLock(); MIB->replaceBranchTarget(Inst, TrampolineBB->getLabel(), BC.Ctx.get()); } BB->replaceSuccessor(TargetBB, TrampolineBB, Count); }; + // For non-simple functions, branch targets may be different functions, + // so we track trampolines by symbol rather than by basic block. + DenseMap SymbolTrampolines; + bool MayNeedRelaxation; uint64_t NumIterations = 0; do { @@ -866,7 +906,10 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { for (auto BBI = FF.begin(); BBI != FF.end(); ++BBI) { BinaryBasicBlock *BB = *BBI; uint64_t NextInstOffset = BB->getOutputStartAddress(); - for (MCInst &Inst : *BB) { + // Branch reversal may replace the current instruction with a sequence. + // Use an index so the next instruction is reloaded after the mutation. + for (size_t I = 0; I < BB->size(); ++I) { + MCInst &Inst = *(BB->begin() + I); const size_t InstAddress = NextInstOffset; if (!MIB->isPseudo(Inst)) NextInstOffset += 4; @@ -881,18 +924,58 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { continue; const MCSymbol *TargetSymbol = MIB->getTargetSymbol(Inst); - BinaryBasicBlock *TargetBB = BB->getSuccessor(TargetSymbol); - assert(TargetBB && - "Basic block target expected for conditional branch."); - - // Check if the relaxation is needed. - if (TargetBB->getFragmentNum() == FF.getFragmentNum() && - isBlockInRange(Inst, InstAddress, *TargetBB)) - continue; - relaxBranch(BB, Inst, InstAddress, TargetBB); - - MayNeedRelaxation = true; + if (BF.isSimple()) { + BinaryBasicBlock *TargetBB = BB->getSuccessor(TargetSymbol); + assert(TargetBB && + "Basic block target expected for conditional branch."); + + // Check if the relaxation is needed. + if (TargetBB->getFragmentNum() == FF.getFragmentNum() && + isBlockInRange(Inst, InstAddress, *TargetBB)) + continue; + + relaxBranch(BB, Inst, InstAddress, TargetBB); + MayNeedRelaxation = true; + } else { + // Skip if the target is within this function. + if (BF.getBasicBlockForLabel(TargetSymbol)) + continue; + + // Try to reuse an existing trampoline for this symbol. + BinaryBasicBlock *TrampolineBB = + SymbolTrampolines.lookup(TargetSymbol); + if (TrampolineBB && + isBlockInRange(Inst, InstAddress, *TrampolineBB)) { + auto L = BC.scopeLock(); + MIB->replaceBranchTarget(Inst, TrampolineBB->getLabel(), + BC.Ctx.get()); + continue; + } + + // Create a trampoline at the end of the function. Since the layout + // of a non-simple function has to be preserved, the end of the + // function is the only place where we can put it. + const int64_t OffsetToEnd = FragmentSize - InstAddress; + if (!isBranchOffsetInRange(Inst, OffsetToEnd)) { + auto L = BC.scopeLock(); + BC.errs() << "BOLT-ERROR: cannot relax branch in non-simple " + "function " + << BF << ": a trampoline at the end of the function is " + << OffsetToEnd << " bytes away, out of reach for a " + << BitsAvailable << "-bit branch\n"; + BC.printInstruction(BC.errs(), Inst); + return false; + } + + TrampolineBB = addTrampolineAfter(/*BB=*/nullptr, TargetSymbol, + /*TargetBB=*/nullptr, + /*Count=*/0); + SymbolTrampolines[TargetSymbol] = TrampolineBB; + auto L = BC.scopeLock(); + MIB->replaceBranchTarget(Inst, TrampolineBB->getLabel(), + BC.Ctx.get()); + } } } @@ -927,6 +1010,8 @@ void LongJmpPass::relaxLocalBranches(BinaryFunction &BF) { /*UpdateLayout*/ true, /*UpdateCFI*/ true, /*RecomputeLPs*/ false); } + + return true; } Error LongJmpPass::runOnFunctions(BinaryContext &BC) { @@ -935,23 +1020,48 @@ Error LongJmpPass::runOnFunctions(BinaryContext &BC) { opts::SplitStrategy != opts::SplitFunctionsStrategy::CDSplit) && "LongJmp cannot work with functions split in more than two fragments"); + DenseMap BranchLiveness; + if (opts::FixBranchesWithLiveness) { + SmallVector Candidates; + for (auto &It : BC.getBinaryFunctions()) { + BinaryFunction &BF = It.second; + if (BC.shouldEmit(BF) && BF.isSimple() && needsBranchLiveness(BF)) + Candidates.push_back(&BF); + } + if (!Candidates.empty()) { + RegAnalysis RA(BC, nullptr, nullptr); + for (BinaryFunction *BF : Candidates) + BranchLiveness.try_emplace(BF, computeBranchLiveness(*BF, RA)); + } + } + auto getBranchLiveness = [&](BinaryFunction &BF) { + auto It = BranchLiveness.find(&BF); + return It == BranchLiveness.end() ? nullptr : &It->second; + }; + if (opts::CompactCodeModel) { BC.outs() << "BOLT-INFO: relaxing branches for compact code model (<128MB)\n"; + std::atomic HasFatal{false}; ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) { - relaxLocalBranches(BF); + if (HasFatal) + return; + if (!relaxLocalBranches(BF, getBranchLiveness(BF))) + HasFatal = true; }; ParallelUtilities::PredicateTy SkipPredicate = - [&](const BinaryFunction &BF) { - return !BC.shouldEmit(BF) || !BF.isSimple(); - }; + [&](const BinaryFunction &BF) { return !BC.shouldEmit(BF); }; ParallelUtilities::runOnEachFunction( BC, ParallelUtilities::SchedulingPolicy::SP_INST_LINEAR, WorkFun, SkipPredicate, "RelaxLocalBranches"); + // The error has already been reported by relaxLocalBranches(). + if (HasFatal) + return createFatalBOLTError("branch relaxation failure"); + return Error::success(); } @@ -970,7 +1080,7 @@ Error LongJmpPass::runOnFunctions(BinaryContext &BC) { // Don't ruin non-simple functions, they can't afford to have the layout // changed. if (Modified && Func->isSimple()) - Func->fixBranches(); + Func->fixBranches(getBranchLiveness(*Func)); } } while (Modified); BC.outs() << "BOLT-INFO: Inserted " << NumHotStubs diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp index abf35964240f3..a1eba32e7fbbe 100644 --- a/bolt/lib/Profile/DataAggregator.cpp +++ b/bolt/lib/Profile/DataAggregator.cpp @@ -84,12 +84,11 @@ static cl::opt ParseMemProfile( "on by default unless `--itrace` is set."), cl::init(true), cl::cat(AggregatorCategory)); -static cl::opt -FilterPID("pid", - cl::desc("only use samples from process with specified PID"), - cl::init(0), - cl::Optional, - cl::cat(AggregatorCategory)); +static cl::list + FilterPID("pid", + cl::desc("only use samples from process with specified PID(s) " + "(comma-separated)"), + cl::CommaSeparated, cl::ZeroOrMore, cl::cat(AggregatorCategory)); static cl::opt ImputeTraceFallthrough( "impute-trace-fall-through", @@ -622,18 +621,27 @@ Error DataAggregator::generatePerfScriptData() { } Error DataAggregator::filterBinaryMMapInfo() { - if (opts::FilterPID) { - auto MMapInfoIter = BinaryMMapInfo.find(opts::FilterPID); - if (MMapInfoIter != BinaryMMapInfo.end()) { - MMapInfo MMap = MMapInfoIter->second; - BinaryMMapInfo.clear(); - BinaryMMapInfo.insert(std::make_pair(MMap.PID, MMap)); - } else { + if (!opts::FilterPID.empty()) { + std::unordered_map FilteredMMapInfo; + for (unsigned long long PID : opts::FilterPID) { + auto MMapInfoIter = BinaryMMapInfo.find(PID); + if (MMapInfoIter != BinaryMMapInfo.end()) + FilteredMMapInfo.insert(*MMapInfoIter); + } + if (FilteredMMapInfo.empty()) { if (errs().has_colors()) errs().changeColor(raw_ostream::RED); - errs() << "PERF2BOLT-ERROR: could not find a profile matching PID \"" - << opts::FilterPID << "\"" - << " for binary \"" << BC->getFilename() << "\"."; + errs() << "PERF2BOLT-ERROR: could not find a profile matching "; + if (opts::FilterPID.size() == 1) { + errs() << "PID \"" << opts::FilterPID[0] << "\""; + } else { + errs() << "any requested PID(s) \""; + for (size_t I = 0; I < opts::FilterPID.size(); ++I) + errs() << opts::FilterPID[I] + << (I == opts::FilterPID.size() - 1 ? "" : ","); + errs() << "\""; + } + errs() << " for binary \"" << BC->getFilename() << "\"."; assert(!BinaryMMapInfo.empty() && "No memory map for matching binary"); errs() << " Profile for the following process is available:\n"; for (std::pair &MMI : BinaryMMapInfo) @@ -646,6 +654,7 @@ Error DataAggregator::filterBinaryMMapInfo() { return createStringError(std::errc::not_supported, "could not find a profile matching PID"); } + BinaryMMapInfo = std::move(FilteredMMapInfo); } return Error::success(); } @@ -1750,7 +1759,7 @@ std::error_code DataAggregator::printLBRHeatMap() { opts::HeatmapMinAddress = KernelBaseAddr; } opts::HeatmapBlockSizes &HMBS = opts::HeatmapBlock; - Heatmap HM(HMBS[0], opts::HeatmapMinAddress, opts::HeatmapMaxAddress, + Heatmap HM(HMBS[0].Value, opts::HeatmapMinAddress, opts::HeatmapMaxAddress, getTextSections(BC)); auto getSymbolValue = [&](const MCSymbol *Symbol) -> uint64_t { if (Symbol) @@ -1792,19 +1801,21 @@ std::error_code DataAggregator::printLBRHeatMap() { HM.print(opts::HeatmapOutput); if (opts::HeatmapOutput == "-") { - HM.printCDF(opts::HeatmapOutput); + HM.printCDF(opts::HeatmapOutput, HMBS.front().Spec); HM.printSectionHotness(opts::HeatmapOutput); } else { - HM.printCDF(opts::HeatmapOutput + ".csv"); + HM.printCDF(opts::HeatmapOutput + ".csv", HMBS.front().Spec); HM.printSectionHotness(opts::HeatmapOutput + "-section-hotness.csv"); } // Provide coarse-grained heatmaps if requested via zoom-out scales - for (const uint64_t NewBucketSize : ArrayRef(HMBS).drop_front()) { + for (const auto &[NewBucketSize, Label] : ArrayRef(HMBS).drop_front()) { HM.resizeBucket(NewBucketSize); if (opts::HeatmapOutput == "-") HM.print(opts::HeatmapOutput); else HM.print(formatv("{0}-{1}", opts::HeatmapOutput, NewBucketSize).str()); + // Working set only; the table is emitted once, at the finest granularity. + HM.printCDF(nulls(), Label); } return std::error_code(); diff --git a/bolt/lib/Profile/Heatmap.cpp b/bolt/lib/Profile/Heatmap.cpp index deed04d0f982c..ceaa1dd5673e9 100644 --- a/bolt/lib/Profile/Heatmap.cpp +++ b/bolt/lib/Profile/Heatmap.cpp @@ -251,17 +251,17 @@ void Heatmap::print(raw_ostream &OS) const { } } -void Heatmap::printCDF(StringRef FileName) const { +void Heatmap::printCDF(StringRef FileName, StringRef Label) const { std::error_code EC; raw_fd_ostream OS(FileName, EC, sys::fs::OpenFlags::OF_None); if (EC) { errs() << "error opening output file: " << EC.message() << '\n'; exit(1); } - printCDF(OS); + printCDF(OS, Label); } -void Heatmap::printCDF(raw_ostream &OS) const { +void Heatmap::printCDF(raw_ostream &OS, StringRef Label) const { uint64_t NumTotalCounts = 0; std::vector Counts; @@ -278,15 +278,25 @@ void Heatmap::printCDF(raw_ostream &OS) const { double RatioRightInPercent = 100.0 / NumTotalCounts; uint64_t RunningCount = 0; + // Buckets covering the cutoff share of the samples. + const uint64_t CutOff = opts::HeatmapCdfPct; + assert(CutOff <= 1000000 && "cutoff must be at most 1000000"); + const uint64_t Target = (NumTotalCounts * CutOff) / 1000000; + uint64_t NumBuckets = 0; + OS << "Bucket counts, Size (KB), CDF (%)\n"; for (uint64_t I = 0; I < Counts.size(); I++) { RunningCount += Counts[I]; + if (!NumBuckets && RunningCount >= Target) + NumBuckets = I + 1; OS << format("%llu", (I + 1)) << ", " << format("%.4f", RatioLeftInKB * (I + 1)) << ", " << format("%.4f", RatioRightInPercent * (RunningCount)) << "\n"; } - Counts.clear(); + outs() << "HEATMAP: working set @ bucket size " << Label << " p" + << format("%g", CutOff / 10000.0) << "/total: " << NumBuckets << "/" + << Counts.size() << '\n'; } void Heatmap::printSectionHotness(StringRef FileName) const { diff --git a/bolt/lib/Rewrite/BuildIDRewriter.cpp b/bolt/lib/Rewrite/BuildIDRewriter.cpp index 86524746c490f..2e6a679118aa0 100644 --- a/bolt/lib/Rewrite/BuildIDRewriter.cpp +++ b/bolt/lib/Rewrite/BuildIDRewriter.cpp @@ -96,10 +96,11 @@ Error BuildIDRewriter::postEmitFinalizer() { if (!BuildIDSection || !BuildIDOffset) return Error::success(); - const uint8_t LastByte = BuildID[BuildID.size() - 1]; - SmallVector Patch = {static_cast(LastByte ^ 1)}; - BuildIDSection->addPatch(*BuildIDOffset + BuildID.size() - 1, Patch); - BC.outs() << "BOLT-INFO: patched build-id (flipped last bit)\n"; + SmallVector Patch(BuildID.begin(), BuildID.end()); + Patch.front() ^= 0x80; + Patch.back() ^= 0x01; + BuildIDSection->addPatch(*BuildIDOffset, Patch); + BC.outs() << "BOLT-INFO: patched build-id (flipped first and last bits)\n"; return Error::success(); } diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp index 05ea606bdad7b..a5de2b5733355 100644 --- a/bolt/lib/Rewrite/RewriteInstance.cpp +++ b/bolt/lib/Rewrite/RewriteInstance.cpp @@ -1451,6 +1451,10 @@ void RewriteInstance::discoverFileObjects() { FileSymRefs.clear(); discoverBOLTReserved(); + + // The name resolver is only needed while discovering and disambiguating file + // objects. Release its memory now that all names have been uniquified. + NR.clear(); } void RewriteInstance::discoverBOLTReserved() { @@ -3314,8 +3318,13 @@ void RewriteInstance::handleRelocation(const SectionRef &RelocatedSection, // Occasionally we may see a reference past the last byte of the function // typically as a result of __builtin_unreachable(). Check it here. - BinaryFunction *ReferencedBF = BC->getBinaryFunctionContainingAddress( - Address, /*CheckPastEnd*/ true, /*UseMaxSize*/ IsAArch64); + // + // Only look for a referenced function when the symbol itself denotes code + // or it is a section relocation. + BinaryFunction *ReferencedBF = nullptr; + if (IsToCode || IsSectionRelocation) + ReferencedBF = BC->getBinaryFunctionContainingAddress( + Address, /*CheckPastEnd*/ true, /*UseMaxSize*/ IsAArch64); if (!IsSectionRelocation) { if (BinaryFunction *BF = diff --git a/bolt/lib/RuntimeLibs/RuntimeLibrary.cpp b/bolt/lib/RuntimeLibs/RuntimeLibrary.cpp index 98852ee691ceb..46c42953eef0c 100644 --- a/bolt/lib/RuntimeLibs/RuntimeLibrary.cpp +++ b/bolt/lib/RuntimeLibs/RuntimeLibrary.cpp @@ -95,9 +95,10 @@ void RuntimeLibrary::loadLibrary(StringRef LibPath, BOLTLinker &Linker, file_magic Magic = identify_magic(B->getBuffer()); if (Magic == file_magic::archive) { - Error Err = Error::success(); - object::Archive Archive(B->getMemBufferRef(), Err); - for (const object::Archive::Child &C : Archive.children(Err)) { + std::unique_ptr Archive; + Error Err = object::Archive::create(B->getMemBufferRef()).moveInto(Archive); + check_error(std::move(Err), B->getBufferIdentifier()); + for (const object::Archive::Child &C : Archive->children(Err)) { std::unique_ptr Bin = cantFail(C.getAsBinary()); if (object::ObjectFile *Obj = dyn_cast(&*Bin)) Linker.loadObject(Obj->getMemoryBufferRef(), MapSections); diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp index 26004c94acdd0..8d7fee733b9c3 100644 --- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp +++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp @@ -1433,6 +1433,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { return MCSpecifierExpr::create(Expr, AArch64::S_ABS, Ctx); } else if (isADRP(Inst) || RelType == ELF::R_AARCH64_ADR_PREL_PG_HI21 || RelType == ELF::R_AARCH64_ADR_PREL_PG_HI21_NC || + RelType == ELF::R_AARCH64_TLSGD_ADR_PAGE21 || RelType == ELF::R_AARCH64_TLSDESC_ADR_PAGE21 || RelType == ELF::R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21 || RelType == ELF::R_AARCH64_ADR_GOT_PAGE) { @@ -1448,6 +1449,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { case ELF::R_AARCH64_LDST32_ABS_LO12_NC: case ELF::R_AARCH64_LDST64_ABS_LO12_NC: case ELF::R_AARCH64_LDST128_ABS_LO12_NC: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: case ELF::R_AARCH64_TLSDESC_ADD_LO12: case ELF::R_AARCH64_TLSDESC_LD64_LO12: case ELF::R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC: @@ -2045,6 +2047,25 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { exit(1); } + unsigned getInvertedCC(unsigned Opcode) const { + // clang-format off + switch (Opcode) { + default: + llvm_unreachable("Failed to invert condition code"); + return Opcode; + // Compare register with immediate and branch. + case AArch64::CBGTWri: return AArch64CC::LE; + case AArch64::CBGTXri: return AArch64CC::LE; + case AArch64::CBLTWri: return AArch64CC::GE; + case AArch64::CBLTXri: return AArch64CC::GE; + case AArch64::CBHIWri: return AArch64CC::LS; + case AArch64::CBHIXri: return AArch64CC::LS; + case AArch64::CBLOWri: return AArch64CC::HS; + case AArch64::CBLOXri: return AArch64CC::HS; + } + // clang-format on + } + unsigned getInvertedBranchOpcode(unsigned Opcode) const { // clang-format off switch (Opcode) { @@ -2171,38 +2192,75 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { } } - bool isReversibleBranch(const MCInst &Inst) const override { + bool isReversibleBranch(const MCInst &Inst, + bool MustPreserveFlags = true) const override { if (isCompAndBranch(Inst)) { unsigned InvertedOpcode = getInvertedBranchOpcode(Inst.getOpcode()); - if (needsImmDec(InvertedOpcode) && Inst.getOperand(1).getImm() == 0) + if (needsImmDec(InvertedOpcode) && Inst.getOperand(1).getImm() == 0 && + MustPreserveFlags) return false; - if (needsImmInc(InvertedOpcode) && Inst.getOperand(1).getImm() == 63) + if (needsImmInc(InvertedOpcode) && Inst.getOperand(1).getImm() == 63 && + MustPreserveFlags) return false; } return MCPlusBuilder::isReversibleBranch(Inst); } - void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB, - MCContext *Ctx) const override { - if (!isReversibleBranch(Inst)) { - errs() << "BOLT-ERROR: Cannot reverse branch " << Inst << "\n"; - exit(1); - } + InstructionListType + reverseBranchCondition(MCInst Inst, const MCSymbol *TBB, MCContext *Ctx, + bool MustPreserveFlags = true) const override { + assert(isReversibleBranch(Inst, MustPreserveFlags) && + "Irreversible branch"); if (isTB(Inst) || isCB(Inst) || isCompAndBranch(Inst)) { + bool ImmediateOutOfBounds = false; unsigned InvertedOpcode = getInvertedBranchOpcode(Inst.getOpcode()); - Inst.setOpcode(InvertedOpcode); - assert(Inst.getOpcode() != 0 && "Invalid branch instruction"); + assert(InvertedOpcode != 0 && "Invalid branch instruction"); // The FEAT_CMPBR compare-and-branch instructions cannot encode all // the possible condition codes, therefore we either have to adjust // the immediate value by +-1, or to swap the register operands // when reversing the branch condition. if (needsRegSwap(InvertedOpcode)) std::swap(Inst.getOperand(0), Inst.getOperand(1)); - else if (needsImmDec(InvertedOpcode)) - Inst.getOperand(1).setImm(Inst.getOperand(1).getImm() - 1); - else if (needsImmInc(InvertedOpcode)) - Inst.getOperand(1).setImm(Inst.getOperand(1).getImm() + 1); + else if (needsImmDec(InvertedOpcode)) { + if (Inst.getOperand(1).getImm() == 0) + ImmediateOutOfBounds = true; + else + Inst.getOperand(1).setImm(Inst.getOperand(1).getImm() - 1); + } else if (needsImmInc(InvertedOpcode)) { + if (Inst.getOperand(1).getImm() == 63) + ImmediateOutOfBounds = true; + else + Inst.getOperand(1).setImm(Inst.getOperand(1).getImm() + 1); + } + if (ImmediateOutOfBounds) { + auto is32BitVariant = [](unsigned Opcode) { + switch (Opcode) { + default: + return false; + case AArch64::CBGTWri: + case AArch64::CBLTWri: + case AArch64::CBHIWri: + case AArch64::CBLOWri: + return true; + } + }; + InstructionListType Code; + MCInstBuilder Cmp = + is32BitVariant(InvertedOpcode) + ? MCInstBuilder(AArch64::SUBSWri).addReg(AArch64::WZR) + : MCInstBuilder(AArch64::SUBSXri).addReg(AArch64::XZR); + Cmp.addReg(Inst.getOperand(0).getReg()) + .addImm(Inst.getOperand(1).getImm()) + .addImm(0); + Code.emplace_back(std::move(Cmp)); + Code.emplace_back(MCInstBuilder(AArch64::Bcc) + .addImm(getInvertedCC(Inst.getOpcode())) + .addExpr(MCSymbolRefExpr::create(TBB, *Ctx))); + moveAnnotations(std::move(Inst), Code.back()); + return Code; + } + Inst.setOpcode(InvertedOpcode); } else if (Inst.getOpcode() == AArch64::Bcc) { Inst.getOperand(0).setImm(AArch64CC::getInvertedCondCode( static_cast(Inst.getOperand(0).getImm()))); @@ -2214,6 +2272,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { llvm_unreachable("Unrecognized branch instruction"); } replaceBranchTarget(Inst, TBB, Ctx); + return {Inst}; } int getPCRelEncodingSize(const MCInst &Inst) const override { @@ -2965,6 +3024,8 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { case ELF::R_AARCH64_LDST32_ABS_LO12_NC: case ELF::R_AARCH64_LDST64_ABS_LO12_NC: case ELF::R_AARCH64_LDST128_ABS_LO12_NC: + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: case ELF::R_AARCH64_TLSDESC_ADD_LO12: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: case ELF::R_AARCH64_TLSDESC_ADR_PREL21: diff --git a/bolt/lib/Target/AArch64/AArch64MCSymbolizer.cpp b/bolt/lib/Target/AArch64/AArch64MCSymbolizer.cpp index 7bbfb1429e37b..da469a9a5ab95 100644 --- a/bolt/lib/Target/AArch64/AArch64MCSymbolizer.cpp +++ b/bolt/lib/Target/AArch64/AArch64MCSymbolizer.cpp @@ -103,6 +103,8 @@ AArch64MCSymbolizer::adjustRelocation(const Relocation &Rel, switch (Rel.Type) { default: break; + case ELF::R_AARCH64_TLSGD_ADD_LO12_NC: + case ELF::R_AARCH64_TLSGD_ADR_PAGE21: case ELF::R_AARCH64_TLSDESC_LD64_LO12: case ELF::R_AARCH64_TLSDESC_ADD_LO12: case ELF::R_AARCH64_TLSDESC_ADR_PAGE21: diff --git a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp index d1a0572277874..03d3213fb07ed 100644 --- a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp +++ b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp @@ -162,11 +162,13 @@ class RISCVMCPlusBuilder : public MCPlusBuilder { } } - void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB, - MCContext *Ctx) const override { + InstructionListType + reverseBranchCondition(MCInst Inst, const MCSymbol *TBB, MCContext *Ctx, + bool MustPreserveFlags = true) const override { auto Opcode = getInvertedBranchOpcode(Inst.getOpcode()); Inst.setOpcode(Opcode); replaceBranchTarget(Inst, TBB, Ctx); + return {Inst}; } void replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB, @@ -557,8 +559,8 @@ class RISCVMCPlusBuilder : public MCPlusBuilder { MCPhysReg RegCnt) const { Inst = MCInstBuilder(atomicAddOpc()) .addReg(RegAtomic) - .addReg(RegTo) - .addReg(RegCnt); + .addReg(RegCnt) + .addReg(RegTo); } InstructionListType createRegCmpJE(MCPhysReg RegNo, const MCSymbol *Target, diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp index 9fd3cdb909ce6..684bedacde3e9 100644 --- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp +++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp @@ -2811,13 +2811,15 @@ class X86MCPlusBuilder : public MCPlusBuilder { Inst.addOperand(MCOperand::createImm(CC)); } - void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB, - MCContext *Ctx) const override { + InstructionListType + reverseBranchCondition(MCInst Inst, const MCSymbol *TBB, MCContext *Ctx, + bool MustPreserveFlags = true) const override { unsigned InvCC = getInvertedCondCode(getCondCode(Inst)); assert(InvCC != X86::COND_INVALID && "invalid branch instruction"); Inst.getOperand(Info->get(Inst.getOpcode()).NumOperands - 1).setImm(InvCC); Inst.getOperand(0) = MCOperand::createExpr(MCSymbolRefExpr::create(TBB, *Ctx)); + return {Inst}; } bool replaceBranchCondition(MCInst &Inst, const MCSymbol *TBB, MCContext *Ctx, diff --git a/bolt/lib/Utils/CommandLineOpts.cpp b/bolt/lib/Utils/CommandLineOpts.cpp index 20b24c3b4acc5..25b0710f3efda 100644 --- a/bolt/lib/Utils/CommandLineOpts.cpp +++ b/bolt/lib/Utils/CommandLineOpts.cpp @@ -192,7 +192,9 @@ bool HeatmapBlockSpecParser::parse(cl::Option &O, StringRef ArgName, unsigned PreviousSize = 0; for (StringRef Size : Sizes) { StringRef OrigSize = Size; - unsigned &SizeVal = Val.emplace_back(0); + HeatmapBlockSize &Block = Val.emplace_back(); + Block.Spec = OrigSize.str(); + unsigned &SizeVal = Block.Value; if (Size.consumeInteger(10, SizeVal)) { O.error("'" + OrigSize + "' value can't be parsed as an integer"); return true; @@ -216,10 +218,23 @@ cl::opt HeatmapBlock( "block-size", cl::value_desc("initial_size{,zoom-out_size,...}"), cl::desc("heatmap bucket size, optionally followed by zoom-out sizes " - "for coarse-grained heatmaps (default 64B, 4K, 256K)."), - cl::init(HeatmapBlockSizes{/*Initial*/ 64, /*Zoom-out*/ 4096, 262144}), + "for coarse-grained heatmaps (default 64, 4K, 16K, 64K, 2M)."), + // Cache line, then the page sizes x86-64 and AArch64 actually use + // (4K, and 16K/64K on AArch64), then the PMD hugepage above a 4K base + // page. + cl::init(HeatmapBlockSizes{/*Initial*/ {64, "64"}, + /*Zoom-out*/ {4096, "4K"}, + {16384, "16K"}, + {65536, "64K"}, + {2097152, "2M"}}), cl::cat(HeatmapCategory)); +cl::opt HeatmapCdfPct( + "heatmap-cdf-pct", cl::init(990000), + cl::desc("Sample CDF cutoff, in millionths, at which to report the working " + "set."), + cl::value_desc("n"), cl::cat(HeatmapCategory)); + cl::opt HeatmapMaxAddress( "max-address", cl::init(0xffffffff), cl::desc("maximum address considered valid for heatmap (default 4GB)"), @@ -365,6 +380,12 @@ cl::opt cl::init(0), cl::ZeroOrMore, cl::cat(BoltCategory), cl::sub(cl::SubCommand::getAll())); +cl::opt FixBranchesWithLiveness( + "fix-branches-with-liveness", + cl::desc("use liveness analysis during branch fixup " + "(needed for branch inversion on AArch64)"), + cl::init(false), cl::cat(BoltCategory)); + bool processAllFunctions() { if (opts::AggregateOnly) return false; diff --git a/bolt/test/AArch64/adr-relaxation-large-non-simple.s b/bolt/test/AArch64/adr-relaxation-large-non-simple.s new file mode 100644 index 0000000000000..babcdb5304b98 --- /dev/null +++ b/bolt/test/AArch64/adr-relaxation-large-non-simple.s @@ -0,0 +1,30 @@ +## Check that an ADR targeting the same fragment is not relaxed in a large +## non-simple function. BOLT preserves the layout of non-simple functions, so +## the ADR displacement cannot change even when the function is larger than the +## instruction's 1MiB range. + +# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o +# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q +# RUN: llvm-bolt %t.exe -o %t.bolt --lite=false + + .text + .globl _start + .type _start, %function +_start: + .cfi_startproc +.Ladr: + adr x1, .Ladr + br x0 + + // Make the function's code larger than 1MiB. The unknown indirect branch + // makes the function non-simple, while the self-referential ADR remains in + // range. + .rept 0x40000 + nop + .endr + ret + .cfi_endproc + .size _start, .-_start + + // Force BOLT's relocation mode. + .reloc 0, R_AARCH64_NONE diff --git a/bolt/test/AArch64/compact-code-model-nonsimple.s b/bolt/test/AArch64/compact-code-model-nonsimple.s new file mode 100644 index 0000000000000..eff53f8c0f307 --- /dev/null +++ b/bolt/test/AArch64/compact-code-model-nonsimple.s @@ -0,0 +1,87 @@ +## Check that llvm-bolt relaxes conditional tail calls in non-simple functions +## for compact code model. Without the relaxation, the branches below are out +## of range after reordering and llvm-bolt fails with JITLink error. + +# REQUIRES: system-linux + +# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o +# RUN: llvm-strip --strip-unneeded %t.o +# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -static +# RUN: echo nonsimple > %t.order +# RUN: echo large_function >> %t.order +# RUN: echo _start >> %t.order +# RUN: llvm-bolt %t.exe -o %t.bolt --compact-code-model --keep-nops \ +# RUN: --function-order=%t.order --print-cfg --print-only=nonsimple \ +# RUN: | FileCheck %s --check-prefix=CHECK-CFG +# RUN: llvm-objdump -d --disassemble-symbols=nonsimple %t.bolt | FileCheck %s + + .text + .globl _start + .type _start, %function +_start: + .cfi_startproc + bl nonsimple + ret x30 + .cfi_endproc +.size _start, .-_start + +## 64KB of code placed between "nonsimple" and "cold_target" by the order file, +## which puts "cold_target" beyond the +-32KB reach of the tbz below. + .globl large_function + .type large_function, %function +large_function: + .cfi_startproc + .rept 16000 + nop + .endr + ret x30 + .cfi_endproc +.size large_function, .-large_function + +## Non-simple function ("br x16" has unknown control flow) with two conditional +## tail calls to the same target. + .globl nonsimple + .type nonsimple, %function +nonsimple: + .cfi_startproc + cmp x0, #1 + b.eq cold_target + tbz x0, #0, cold_target + ldr x16, [sp] + br x16 + .cfi_endproc +.size nonsimple, .-nonsimple + + .globl cold_target + .type cold_target, %function +cold_target: + .cfi_startproc + mov x0, #1 + ret x30 + .cfi_endproc +.size cold_target, .-cold_target + +## Force relocation mode. + .reloc 0, R_AARCH64_NONE + +## Verify that the function under test is really non-simple. If this ever +## starts printing "IsSimple : 1", the test no longer covers the non-simple +## relaxation path. +# CHECK-CFG: Binary Function "nonsimple" after building cfg +# CHECK-CFG: IsSimple : 0 + +## Both branches should be retargeted to a single trampoline appended after the +## function body, which in turn branches to the original target. The layout of +## the function itself must be preserved. +## +## Note that the tbz reuses the trampoline created for the b.eq, as both target +## the same symbol: the address captured from the b.eq must match the one used +## by the tbz, and no second trampoline may be emitted. +# CHECK: : +# CHECK: cmp x0, #0x1 +# CHECK-NEXT: b.eq 0x[[TRAMP:[0-9a-f]+]] +# CHECK-NEXT: tbz {{.*}}, 0x[[TRAMP]] +# CHECK-NEXT: ldr x16, [sp] +# CHECK-NEXT: br x16 +# CHECK-NEXT: [[TRAMP]]: {{.*}} b 0x{{[0-9a-f]+}} +# CHECK-NOT: b 0x{{[0-9a-f]+}} diff --git a/bolt/test/AArch64/compare-and-branch-inversion.S b/bolt/test/AArch64/compare-and-branch-inversion.S index 28167416c31cb..871fce6c67f5f 100644 --- a/bolt/test/AArch64/compare-and-branch-inversion.S +++ b/bolt/test/AArch64/compare-and-branch-inversion.S @@ -1,18 +1,24 @@ # This test checks that branch inversion works when reordering blocks which # contain short range conditional branches. Handles edge cases, like when # the immediate value is the upper or lower allowed value in which case the -# transformation bails. +# transformation bails. If liveness analysis proves that the condition flags +# are dead we can replace the branch with cmp + b.cc # REQUIRES: system-linux, asserts # RUN: %clang %cflags -march=armv9-a+cmpbr -Wl,-q %s -o %t # RUN: link_fdata --no-lbr %s %t %t.fdata # RUN: llvm-strip --strip-unneeded %t +# # RUN: llvm-bolt -v=1 %t -o %t.bolt --data %t.fdata --reorder-blocks=ext-tsp --compact-code-model \ -# RUN: | FileCheck %s --check-prefix=BOLT-INFO -# RUN: llvm-objdump -d %t.bolt | FileCheck %s +# RUN: | FileCheck %s --check-prefix=BOLT-INFO-NO-LIVENESS +# RUN: llvm-objdump -d %t.bolt | FileCheck %s --check-prefix=COMMON --check-prefix=NO-LIVENESS +# +# RUN: llvm-bolt -v=1 %t -o %t.bolt --data %t.fdata --reorder-blocks=ext-tsp --compact-code-model \ +# RUN: --fix-branches-with-liveness | FileCheck %s --check-prefix=BOLT-INFO-LIVENESS +# RUN: llvm-objdump -d %t.bolt | FileCheck %s --check-prefix=COMMON --check-prefix=LIVENESS -# CHECK: Disassembly of section .text: +# COMMON: Disassembly of section .text: .globl immediate_increment .type immediate_increment, %function @@ -29,12 +35,12 @@ immediate_increment: mov x0, #2 ret -# CHECK: : -# CHECK-NEXT: {{.*}} cblt x0, #0x1, 0x[[ADDR0:[0-9a-f]+]] <{{.*}}> -# CHECK-NEXT: {{.*}} mov x0, #0x2 // =2 -# CHECK-NEXT: {{.*}} ret -# CHECK-NEXT: [[ADDR0]]: {{.*}} mov x0, #0x1 // =1 -# CHECK-NEXT: {{.*}} ret +# COMMON: : +# COMMON-NEXT: {{.*}} cblt x0, #0x1, 0x[[ADDR0:[0-9a-f]+]] <{{.*}}> +# COMMON-NEXT: {{.*}} mov x0, #0x2 // =2 +# COMMON-NEXT: {{.*}} ret +# COMMON-NEXT: [[ADDR0]]: {{.*}} mov x0, #0x1 // =1 +# COMMON-NEXT: {{.*}} ret .globl immediate_decrement .type immediate_decrement, %function @@ -51,12 +57,12 @@ immediate_decrement: mov x0, #2 ret -# CHECK: : -# CHECK-NEXT: {{.*}} cbhi x0, #0x0, 0x[[ADDR1:[0-9a-f]+]] <{{.*}}> -# CHECK-NEXT: {{.*}} mov x0, #0x2 // =2 -# CHECK-NEXT: {{.*}} ret -# CHECK-NEXT: [[ADDR1]]: {{.*}} mov x0, #0x1 // =1 -# CHECK-NEXT: {{.*}} ret +# COMMON: : +# COMMON-NEXT: {{.*}} cbhi x0, #0x0, 0x[[ADDR1:[0-9a-f]+]] <{{.*}}> +# COMMON-NEXT: {{.*}} mov x0, #0x2 // =2 +# COMMON-NEXT: {{.*}} ret +# COMMON-NEXT: [[ADDR1]]: {{.*}} mov x0, #0x1 // =1 +# COMMON-NEXT: {{.*}} ret .globl register_swap .type register_swap, %function @@ -73,37 +79,77 @@ register_swap: mov x0, #2 ret -# CHECK: : -# CHECK-NEXT: {{.*}} cbgt x1, x0, 0x[[ADDR2:[0-9a-f]+]] <{{.*}}> -# CHECK-NEXT: {{.*}} mov x0, #0x2 // =2 -# CHECK-NEXT: {{.*}} ret -# CHECK-NEXT: [[ADDR2]]: {{.*}} mov x0, #0x1 // =1 -# CHECK-NEXT: {{.*}} ret +# COMMON: : +# COMMON-NEXT: {{.*}} cbgt x1, x0, 0x[[ADDR2:[0-9a-f]+]] <{{.*}}> +# COMMON-NEXT: {{.*}} mov x0, #0x2 // =2 +# COMMON-NEXT: {{.*}} ret +# COMMON-NEXT: [[ADDR2]]: {{.*}} mov x0, #0x1 // =1 +# COMMON-NEXT: {{.*}} ret - .globl irreversible - .type irreversible, %function -irreversible: + .globl immediate_overflow + .type immediate_overflow, %function +immediate_overflow: .entry3: -# FDATA: 1 irreversible #.entry3# 10 +# FDATA: 1 immediate_overflow #.entry3# 10 cbgt x0, #63, .exit3 .cold3: -# FDATA: 1 irreversible #.cold3# 1 +# FDATA: 1 immediate_overflow #.cold3# 1 mov x0, #1 ret .exit3: -# FDATA: 1 irreversible #.exit3# 10 +# FDATA: 1 immediate_overflow #.exit3# 10 + mov x0, #2 + ret + +# BOLT-INFO-NO-LIVENESS: unable to swap successors in immediate_overflow +# +# Without liveness the blocks get reordered, but since the branch is +# irreversible an additional unconditional branch is emitted. +# This codegen is suboptimal yet correct. +# +# NO-LIVENESS: : +# NO-LIVENESS-NEXT: {{.*}} cbgt x0, #0x3f, 0x[[ADDR3:[0-9a-f]+]] <{{.*}}> +# NO-LIVENESS-NEXT: {{.*}} b 0x[[ADDR4:[0-9a-f]+]] <{{.*}}> +# NO-LIVENESS-NEXT: [[ADDR3]]: {{.*}} mov x0, #0x2 // =2 +# NO-LIVENESS-NEXT: {{.*}} ret +# NO-LIVENESS-NEXT: [[ADDR4]]: {{.*}} mov x0, #0x1 // =1 +# NO-LIVENESS-NEXT: {{.*}} ret + +# LIVENESS: : +# LIVENESS-NEXT: {{.*}} cmp x0, #0x3f +# LIVENESS-NEXT: {{.*}} b.le 0x[[ADDR5:[0-9a-f]+]] <{{.*}}> +# LIVENESS-NEXT: {{.*}} mov x0, #0x2 // =2 +# LIVENESS-NEXT: {{.*}} ret +# LIVENESS-NEXT: [[ADDR5]]: {{.*}} mov x0, #0x1 // =1 +# LIVENESS-NEXT: {{.*}} ret + + .globl irreversible + .type irreversible, %function +irreversible: +.entry4: +# FDATA: 1 irreversible #.entry4# 10 + cmp x0, #63 + cbgt x0, #63, .exit4 +.cold4: +# FDATA: 1 irreversible #.cold4# 1 + csel x0, x1, x2, le + ret +.exit4: +# FDATA: 1 irreversible #.exit4# 10 mov x0, #2 ret -# BOLT-INFO: unable to swap successors in irreversible +# BOLT-INFO-NO-LIVENESS: unable to swap successors in irreversible +# BOLT-INFO-LIVENESS: unable to swap successors in irreversible -# CHECK: : -# CHECK-NEXT: {{.*}} cbgt x0, #0x3f, 0x[[ADDR3:[0-9a-f]+]] <{{.*}}> -# CHECK-NEXT: {{.*}} b 0x[[ADDR4:[0-9a-f]+]] <{{.*}}> -# CHECK-NEXT: [[ADDR3]]: {{.*}} mov x0, #0x2 // =2 -# CHECK-NEXT: {{.*}} ret -# CHECK-NEXT: [[ADDR4]]: {{.*}} mov x0, #0x1 // =1 -# CHECK-NEXT: {{.*}} ret +# COMMON: : +# COMMON-NEXT: {{.*}} cmp x0, #0x3f +# COMMON-NEXT: {{.*}} cbgt x0, #0x3f, 0x[[ADDR6:[0-9a-f]+]] <{{.*}}> +# COMMON-NEXT: {{.*}} b 0x[[ADDR7:[0-9a-f]+]] <{{.*}}> +# COMMON-NEXT: [[ADDR6]]: {{.*}} mov x0, #0x2 // =2 +# COMMON-NEXT: {{.*}} ret +# COMMON-NEXT: [[ADDR7]]: {{.*}} csel x0, x1, x2, le +# COMMON-NEXT: {{.*}} ret ## Force relocation mode. .reloc 0, R_AARCH64_NONE diff --git a/bolt/test/AArch64/runtime-relocs.test b/bolt/test/AArch64/runtime-relocs.test index a8347b531c144..660959721305c 100644 --- a/bolt/test/AArch64/runtime-relocs.test +++ b/bolt/test/AArch64/runtime-relocs.test @@ -27,11 +27,21 @@ CHECKEXE: {{.*}} R_AARCH64_JUMP_SLOT {{.*}} inc + 0 // the initial binary was built with gcc and ld with -mtls-dialect=trad flag. RUN: yaml2obj %p/Inputs/tls-trad.yaml &> %t.trad.so -RUN: llvm-bolt %t.trad.so -o %t.trad.bolt.so --use-old-text=0 --lite=0 +RUN: llvm-bolt %t.trad.so -o %t.trad.bolt.so --use-old-text=0 --lite=0 2>&1 | \ +RUN: FileCheck %s --check-prefix=CHECKTRAD-BOLT RUN: llvm-readelf -rW %t.trad.so | FileCheck %s -check-prefix=CHECKTRAD +RUN: llvm-objdump -d --no-show-raw-insn --start-address=0x4000e0 \ +RUN: --stop-address=0x4000e8 %t.trad.bolt.so | \ +RUN: FileCheck %s --check-prefix=CHECKTRAD-TEXT +CHECKTRAD-BOLT: BOLT-INFO: enabling relocation mode +CHECKTRAD-BOLT-NOT: Failed to analyze CHECKTRAD: {{.*}} R_AARCH64_TLS_DTPMOD64 {{.*}} t1 + 0 CHECKTRAD: {{.*}} R_AARCH64_TLS_DTPREL64 {{.*}} t1 + 0 +CHECKTRAD: {{.*}} R_AARCH64_TLSGD_ADR_PAGE21 {{.*}} t1 + 0 +CHECKTRAD: {{.*}} R_AARCH64_TLSGD_ADD_LO12_NC {{.*}} t1 + 0 +CHECKTRAD-TEXT: adrp x0, 0x10000 +CHECKTRAD-TEXT-NEXT: add x0, x0, #0xfd0 // The ld linker emits R_AARCH64_TLSDESC to .rela.plt section, check that // it is emitted correctly. diff --git a/bolt/test/CMakeLists.txt b/bolt/test/CMakeLists.txt index 6e18b028bddfc..9cfa4e20ff785 100644 --- a/bolt/test/CMakeLists.txt +++ b/bolt/test/CMakeLists.txt @@ -35,6 +35,7 @@ list(APPEND BOLT_TEST_DEPS FileCheck llc lld + llvm-ar llvm-config llvm-bolt llvm-bolt-binary-analysis diff --git a/bolt/test/X86/heatmap-preagg.test b/bolt/test/X86/heatmap-preagg.test index c47083b6e04a6..e5e0419f2b6de 100644 --- a/bolt/test/X86/heatmap-preagg.test +++ b/bolt/test/X86/heatmap-preagg.test @@ -40,9 +40,13 @@ CHECK-HEATMAP-BAT-1K-NOT: HEATMAP: dumping heatmap with bucket size CHECK-HEATMAP: PERF2BOLT: read 81 aggregated brstack entries CHECK-HEATMAP: HEATMAP: invalid traces: 1 CHECK-HEATMAP: HEATMAP: dumping heatmap with bucket size 64 +CHECK-HEATMAP: HEATMAP: working set @ bucket size 64 p99/total: 28/43 CHECK-HEATMAP: HEATMAP: dumping heatmap with bucket size 128 +CHECK-HEATMAP: HEATMAP: working set @ bucket size 128 p99/total: 16/23 CHECK-HEATMAP: HEATMAP: dumping heatmap with bucket size 1024 +CHECK-HEATMAP: HEATMAP: working set @ bucket size 1K p99/total: 3/3 CHECK-HEATMAP-NOT: HEATMAP: dumping heatmap with bucket size +CHECK-HEATMAP-NOT: HEATMAP: working set @ bucket size CHECK-SEC-HOT: Section Name, Begin Address, End Address, Percentage Hotness, Utilization Pct, Partition Score CHECK-SEC-HOT-NEXT: .init, 0x401000, 0x40101b, 16.8545, 100.0000, 0.1685 diff --git a/bolt/test/X86/reloc-data-symbol-negative-addend.s b/bolt/test/X86/reloc-data-symbol-negative-addend.s new file mode 100644 index 0000000000000..1c3a5cf21bd8f --- /dev/null +++ b/bolt/test/X86/reloc-data-symbol-negative-addend.s @@ -0,0 +1,40 @@ +## Check that a relocation against a data symbol with a negative addend is not +## mistaken for a reference into a function when "symbol + addend" happens to +## resolve inside one. Compilers fold a bias into the displacement for indexed +## accesses, e.g. "mov sym-0x3fe00(,%rax,8)", and the unbiased address is never +## used on its own. + +# REQUIRES: system-linux + +# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o +# RUN: ld.lld %t.o -o %t.exe -q --nostdlib -e _start --image-base=0x200000 \ +# RUN: --section-start=.text=0x200000 --section-start=.mydata=0x400000 +# RUN: llvm-bolt %t.exe -o %t.bolt --relocs 2>&1 | FileCheck %s + +## The biased displacement resolves into the middle of an instruction in +## "target". BOLT used to report that as an external branch and ignore both +## functions. +# CHECK-NOT: corrupted control flow + + .text + .globl target + .type target, @function +target: +## 10-byte instruction at 0x200000, so 0x200003 is not an instruction boundary. + movabsq $0x1122334455667788, %rax + retq + .size target, .-target + + .globl _start + .type _start, @function +_start: +## datasym is at 0x400000, so datasym-0x1ffffd resolves to 0x200003, inside +## "target" above. + movq datasym-0x1ffffd(,%rax,8), %r14 + retq + .size _start, .-_start + + .section .mydata, "aw", @progbits + .globl datasym +datasym: + .quad 0 diff --git a/bolt/test/build-id-patch.c b/bolt/test/build-id-patch.c new file mode 100644 index 0000000000000..f78a8a3ca75f6 --- /dev/null +++ b/bolt/test/build-id-patch.c @@ -0,0 +1,17 @@ +// Check that BOLT patches the build ID of the output binary so that it cannot +// be mistaken for the input. The high bit of the first byte and the low bit of +// the last byte are flipped. +// +// REQUIRES: system-linux + +// RUN: %clang %cflags -Wl,-q %s -o %t.exe \ +// RUN: -Wl,--build-id=0x0123456789abcdef0123456789abcdef01234567 +// RUN: llvm-readelf -n %t.exe | FileCheck %s --check-prefix=CHECK-INPUT +// RUN: llvm-bolt %t.exe -o %t.bolt | FileCheck %s --check-prefix=CHECK-BOLT +// RUN: llvm-readelf -n %t.bolt | FileCheck %s --check-prefix=CHECK-OUTPUT + +// CHECK-INPUT: Build ID: 0123456789abcdef0123456789abcdef01234567 +// CHECK-BOLT: BOLT-INFO: patched build-id +// CHECK-OUTPUT: Build ID: 8123456789abcdef0123456789abcdef01234566 + +int main() { return 0; } diff --git a/bolt/test/lit.cfg.py b/bolt/test/lit.cfg.py index 218ea24867eb5..30cfc8fa54901 100644 --- a/bolt/test/lit.cfg.py +++ b/bolt/test/lit.cfg.py @@ -96,9 +96,11 @@ if config.libbolt_rt_instr: llvm_bolt_args.append(f"--runtime-instrumentation-lib={config.libbolt_rt_instr}") + config.substitutions.append(("%libbolt_rt_instr", config.libbolt_rt_instr)) if config.libbolt_rt_hugify: llvm_bolt_args.append(f"--runtime-hugify-lib={config.libbolt_rt_hugify}") + config.substitutions.append(("%libbolt_rt_hugify", config.libbolt_rt_hugify)) tools = [ ToolSubst("llc", unresolved="fatal"), @@ -114,6 +116,7 @@ ToolSubst("llvm-bat-dump", unresolved="fatal"), ToolSubst("perf2bolt", unresolved="fatal"), ToolSubst("yaml2obj", unresolved="fatal"), + ToolSubst("llvm-ar", unresolved="fatal"), ToolSubst("llvm-mc", unresolved="fatal"), ToolSubst("llvm-nm", unresolved="fatal"), ToolSubst("llvm-objdump", unresolved="fatal"), diff --git a/bolt/test/runtime/RISCV/basic-instrumentation.s b/bolt/test/runtime/RISCV/basic-instrumentation.s index e926f98cef43b..4b7b5189f741a 100644 --- a/bolt/test/runtime/RISCV/basic-instrumentation.s +++ b/bolt/test/runtime/RISCV/basic-instrumentation.s @@ -2,6 +2,9 @@ # RUN: %clang %cflags -Wl,-q -o %t.exe %s # RUN: llvm-bolt --instrument --instrumentation-file=%t.fdata -o %t.instr %t.exe +# RUN: llvm-objdump -d --no-show-raw-insn --disassemble-symbols=main %t.instr \ +# RUN: | FileCheck %s --check-prefix=INSTR +# INSTR: amoadd.d zero, a1, (a0) ## Run the profiled binary and check that the profile reports at least that `f` ## has been called. diff --git a/bolt/test/runtime/thin-archive.c b/bolt/test/runtime/thin-archive.c new file mode 100644 index 0000000000000..a033b8e0c7cab --- /dev/null +++ b/bolt/test/runtime/thin-archive.c @@ -0,0 +1,22 @@ +// Test that BOLT can consume thin archives for runtime libraries. + +// REQUIRES: system-linux,bolt-runtime + +// RUN: rm -rf %t && mkdir -p %t/objects + +// RUN: cd %t/objects && llvm-ar x %libbolt_rt_instr +// RUN: cd %t && llvm-ar rcT libbolt_rt_instr.a objects/* +// RUN: FileCheck --input-file %t/libbolt_rt_instr.a --check-prefix=THIN %s +// THIN: ! + +// RUN: %clang %cflags -no-pie -Wl,-q -o %t/exe %s +// RUN: llvm-bolt -o %t/exe.bolt %t/exe \ +// RUN: --instrument --instrumentation-file=%t/exe.fdata \ +// RUN: --runtime-instrumentation-lib=%t/libbolt_rt_instr.a +// RUN: %t/exe.bolt +// RUN: cat %t/exe.fdata | FileCheck %s +// CHECK: main 0 0 1 + +#include + +int main(int argc, char *argv[]) { puts("thin archive test"); } diff --git a/bolt/unittests/Core/MCPlusBuilder.cpp b/bolt/unittests/Core/MCPlusBuilder.cpp index e67460fe2a6a6..a692f45f551eb 100644 --- a/bolt/unittests/Core/MCPlusBuilder.cpp +++ b/bolt/unittests/Core/MCPlusBuilder.cpp @@ -233,10 +233,11 @@ TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch) { .addExpr(MCSymbolRefExpr::create( TargetBB->getLabel(), *BC->Ctx.get())); ASSERT_TRUE(BC->MIB->isReversibleBranch(NeedsImmInc)); - BC->MIB->reverseBranchCondition(NeedsImmInc, TargetBB->getLabel(), - BC->Ctx.get()); - ASSERT_EQ(NeedsImmInc.getOpcode(), AArch64::CBLTXri); - ASSERT_EQ(NeedsImmInc.getOperand(1).getImm(), 1); + auto NeedsImmIncCode = BC->MIB->reverseBranchCondition( + NeedsImmInc, TargetBB->getLabel(), BC->Ctx.get()); + ASSERT_EQ(NeedsImmIncCode.size(), 1u); + ASSERT_EQ(NeedsImmIncCode[0].getOpcode(), AArch64::CBLTXri); + ASSERT_EQ(NeedsImmIncCode[0].getOperand(1).getImm(), 1); // Compare register with immediate and branch. // Inversion requires decrementing the immediate value. @@ -247,10 +248,11 @@ TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch) { .addExpr(MCSymbolRefExpr::create( TargetBB->getLabel(), *BC->Ctx.get())); ASSERT_TRUE(BC->MIB->isReversibleBranch(NeedsImmDec)); - BC->MIB->reverseBranchCondition(NeedsImmDec, TargetBB->getLabel(), - BC->Ctx.get()); - ASSERT_EQ(NeedsImmDec.getOpcode(), AArch64::CBHIXri); - ASSERT_EQ(NeedsImmDec.getOperand(1).getImm(), 0); + auto NeedsImmDecCode = BC->MIB->reverseBranchCondition( + NeedsImmDec, TargetBB->getLabel(), BC->Ctx.get()); + ASSERT_EQ(NeedsImmDecCode.size(), 1u); + ASSERT_EQ(NeedsImmDecCode[0].getOpcode(), AArch64::CBHIXri); + ASSERT_EQ(NeedsImmDecCode[0].getOperand(1).getImm(), 0); // Compare registers and branch. // Inversion requires swapping registers. @@ -261,11 +263,12 @@ TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch) { .addExpr(MCSymbolRefExpr::create( TargetBB->getLabel(), *BC->Ctx.get())); ASSERT_TRUE(BC->MIB->isReversibleBranch(CompRegNeedsRegSwap)); - BC->MIB->reverseBranchCondition(CompRegNeedsRegSwap, TargetBB->getLabel(), - BC->Ctx.get()); - ASSERT_EQ(CompRegNeedsRegSwap.getOpcode(), AArch64::CBGTXrr); - ASSERT_EQ(CompRegNeedsRegSwap.getOperand(0).getReg(), AArch64::X1); - ASSERT_EQ(CompRegNeedsRegSwap.getOperand(1).getReg(), AArch64::X0); + auto CompRegCode = BC->MIB->reverseBranchCondition( + CompRegNeedsRegSwap, TargetBB->getLabel(), BC->Ctx.get()); + ASSERT_EQ(CompRegCode.size(), 1u); + ASSERT_EQ(CompRegCode[0].getOpcode(), AArch64::CBGTXrr); + ASSERT_EQ(CompRegCode[0].getOperand(0).getReg(), AArch64::X1); + ASSERT_EQ(CompRegCode[0].getOperand(1).getReg(), AArch64::X0); // Compare bytes and branch. // Inversion requires swapping registers. @@ -276,11 +279,12 @@ TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch) { .addExpr(MCSymbolRefExpr::create( TargetBB->getLabel(), *BC->Ctx.get())); ASSERT_TRUE(BC->MIB->isReversibleBranch(CompByteNeedsRegSwap)); - BC->MIB->reverseBranchCondition(CompByteNeedsRegSwap, TargetBB->getLabel(), - BC->Ctx.get()); - ASSERT_EQ(CompByteNeedsRegSwap.getOpcode(), AArch64::CBBHSWrr); - ASSERT_EQ(CompByteNeedsRegSwap.getOperand(0).getReg(), AArch64::W1); - ASSERT_EQ(CompByteNeedsRegSwap.getOperand(1).getReg(), AArch64::W0); + auto CompByteCode = BC->MIB->reverseBranchCondition( + CompByteNeedsRegSwap, TargetBB->getLabel(), BC->Ctx.get()); + ASSERT_EQ(CompByteCode.size(), 1u); + ASSERT_EQ(CompByteCode[0].getOpcode(), AArch64::CBBHSWrr); + ASSERT_EQ(CompByteCode[0].getOperand(0).getReg(), AArch64::W1); + ASSERT_EQ(CompByteCode[0].getOperand(1).getReg(), AArch64::W0); // Compare halfwords and branch. // Inversion requires swapping registers. @@ -291,11 +295,12 @@ TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch) { .addExpr(MCSymbolRefExpr::create( TargetBB->getLabel(), *BC->Ctx.get())); ASSERT_TRUE(BC->MIB->isReversibleBranch(CompHalfNeedsRegSwap)); - BC->MIB->reverseBranchCondition(CompHalfNeedsRegSwap, TargetBB->getLabel(), - BC->Ctx.get()); - ASSERT_EQ(CompHalfNeedsRegSwap.getOpcode(), AArch64::CBHHIWrr); - ASSERT_EQ(CompHalfNeedsRegSwap.getOperand(0).getReg(), AArch64::W1); - ASSERT_EQ(CompHalfNeedsRegSwap.getOperand(1).getReg(), AArch64::W0); + auto CompHalfCode = BC->MIB->reverseBranchCondition( + CompHalfNeedsRegSwap, TargetBB->getLabel(), BC->Ctx.get()); + ASSERT_EQ(CompHalfCode.size(), 1u); + ASSERT_EQ(CompHalfCode[0].getOpcode(), AArch64::CBHHIWrr); + ASSERT_EQ(CompHalfCode[0].getOperand(0).getReg(), AArch64::W1); + ASSERT_EQ(CompHalfCode[0].getOperand(1).getReg(), AArch64::W0); // Compare register with immediate and branch. // Inversion not possible, immediate value underflows. @@ -318,6 +323,109 @@ TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch) { ASSERT_FALSE(BC->MIB->isReversibleBranch(Overflows)); } +TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch_Underflows) { + if (GetParam() != Triple::aarch64) + GTEST_SKIP(); + + BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true); + BinaryBasicBlock *EntryBB = BF->addBasicBlock(); + BinaryBasicBlock *FallThroughBB = BF->addBasicBlock(); + BinaryBasicBlock *TargetBB = BF->addBasicBlock(); + BF->addEntryPoint(*EntryBB); + EntryBB->addSuccessor(TargetBB); + EntryBB->addSuccessor(FallThroughBB); + + // Inversion requires expansion, immediate value underflows. + // cblt x0, #0, target ~> cmp x0, #0 + // b.ge target + auto I = + EntryBB->addInstruction(MCInstBuilder(AArch64::CBLTXri) + .addReg(AArch64::X0) + .addImm(0) + .addExpr(MCSymbolRefExpr::create( + TargetBB->getLabel(), *BC->Ctx.get()))); + ASSERT_TRUE(BC->MIB->isReversibleBranch(*I, /*PreserveFlags=*/false)); + auto Code = BC->MIB->reverseBranchCondition( + *I, TargetBB->getLabel(), BC->Ctx.get(), /*PreserveFlags=*/false); + ASSERT_EQ(Code.size(), 2u); + ASSERT_EQ(Code[0].getOpcode(), AArch64::SUBSXri); + ASSERT_EQ(Code[0].getOperand(0).getReg(), AArch64::XZR); + ASSERT_EQ(Code[0].getOperand(1).getReg(), AArch64::X0); + ASSERT_EQ(Code[0].getOperand(2).getImm(), 0); + ASSERT_EQ(Code[0].getOperand(3).getImm(), 0); + ASSERT_EQ(Code[1].getOpcode(), AArch64::Bcc); + ASSERT_EQ(Code[1].getOperand(0).getImm(), AArch64CC::GE); +} + +TEST_P(MCPlusBuilderTester, AArch64_ReverseCompAndBranch_Overflows) { + if (GetParam() != Triple::aarch64) + GTEST_SKIP(); + + BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true); + BinaryBasicBlock *EntryBB = BF->addBasicBlock(); + BinaryBasicBlock *FallThroughBB = BF->addBasicBlock(); + BinaryBasicBlock *TargetBB = BF->addBasicBlock(); + BF->addEntryPoint(*EntryBB); + EntryBB->addSuccessor(TargetBB); + EntryBB->addSuccessor(FallThroughBB); + + // Inversion requires expansion, immediate value overflows. + // cbhi w0, #63, target ~> cmp w0, #63 + // b.ls target + auto I = + EntryBB->addInstruction(MCInstBuilder(AArch64::CBHIWri) + .addReg(AArch64::W0) + .addImm(63) + .addExpr(MCSymbolRefExpr::create( + TargetBB->getLabel(), *BC->Ctx.get()))); + ASSERT_TRUE(BC->MIB->isReversibleBranch(*I, /*PreserveFlags=*/false)); + auto Code = BC->MIB->reverseBranchCondition( + *I, TargetBB->getLabel(), BC->Ctx.get(), /*PreserveFlags=*/false); + ASSERT_EQ(Code.size(), 2u); + ASSERT_EQ(Code[0].getOpcode(), AArch64::SUBSWri); + ASSERT_EQ(Code[0].getOperand(0).getReg(), AArch64::WZR); + ASSERT_EQ(Code[0].getOperand(1).getReg(), AArch64::W0); + ASSERT_EQ(Code[0].getOperand(2).getImm(), 63); + ASSERT_EQ(Code[0].getOperand(3).getImm(), 0); + ASSERT_EQ(Code[1].getOpcode(), AArch64::Bcc); + ASSERT_EQ(Code[1].getOperand(0).getImm(), AArch64CC::LS); +} + +TEST_P(MCPlusBuilderTester, AArch64_IsReversibleBranch_LiveCondFlags) { + if (GetParam() != Triple::aarch64) + GTEST_SKIP(); + + BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true); + BinaryBasicBlock *EntryBB = BF->addBasicBlock(); + BinaryBasicBlock *FallThroughBB = BF->addBasicBlock(); + BinaryBasicBlock *TargetBB = BF->addBasicBlock(); + BF->addEntryPoint(*EntryBB); + EntryBB->addSuccessor(TargetBB); + EntryBB->addSuccessor(FallThroughBB); + + // cmp x0, #63 + EntryBB->addInstruction(MCInstBuilder(AArch64::SUBSXri) + .addReg(AArch64::XZR) + .addReg(AArch64::X0) + .addImm(63) + .addImm(0)); + // cbgt x0, #63, target + auto I = + EntryBB->addInstruction(MCInstBuilder(AArch64::CBGTXri) + .addReg(AArch64::X0) + .addImm(63) + .addExpr(MCSymbolRefExpr::create( + TargetBB->getLabel(), *BC->Ctx.get()))); + // csel x0, x1, x2, le + FallThroughBB->addInstruction(MCInstBuilder(AArch64::CSELXr) + .addReg(AArch64::X0) + .addReg(AArch64::X1) + .addReg(AArch64::X2) + .addImm(13)); + + ASSERT_FALSE(BC->MIB->isReversibleBranch(*I, /*PreserveFlags=*/true)); +} + TEST_P(MCPlusBuilderTester, AArch64_CmpJE) { if (GetParam() != Triple::aarch64) GTEST_SKIP(); diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt index 9ee9255fbe17b..270e7b37d5327 100644 --- a/clang-tools-extra/clang-tidy/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/CMakeLists.txt @@ -75,7 +75,6 @@ add_subdirectory(openmp) add_subdirectory(performance) add_subdirectory(portability) add_subdirectory(readability) -add_subdirectory(zircon) set(ALL_CLANG_TIDY_CHECKS clangTidyAndroidModule clangTidyAbseilModule @@ -98,7 +97,6 @@ set(ALL_CLANG_TIDY_CHECKS clangTidyPerformanceModule clangTidyPortabilityModule clangTidyReadabilityModule - clangTidyZirconModule ) if(CLANG_TIDY_ENABLE_QUERY_BASED_CUSTOM_CHECKS) diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 32e18645880e1..8135826ef660d 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -188,7 +188,7 @@ class ErrorReporter { } reportFix(Diag, Error.Message.Fix); } - for (const auto Fix : FixLocations) { + for (const auto &Fix : FixLocations) { Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied : diag::note_fixit_failed); } diff --git a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h index 2450384016e25..87f99e8aab610 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h +++ b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h @@ -132,11 +132,6 @@ extern volatile int ReadabilityModuleAnchorSource; [[maybe_unused]] static int ReadabilityModuleAnchorDestination = ReadabilityModuleAnchorSource; -// This anchor is used to force the linker to link the ZirconModule. -extern volatile int ZirconModuleAnchorSource; -[[maybe_unused]] static int ZirconModuleAnchorDestination = - ZirconModuleAnchorSource; - } // namespace clang::tidy #endif diff --git a/clang-tools-extra/clang-tidy/ClangTidyModuleRegistry.h b/clang-tools-extra/clang-tidy/ClangTidyModuleRegistry.h deleted file mode 100644 index 39aecd955ef73..0000000000000 --- a/clang-tools-extra/clang-tidy/ClangTidyModuleRegistry.h +++ /dev/null @@ -1,21 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULEREGISTRY_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULEREGISTRY_H - -// NOLINTBEGIN - -// TODO(LLVM 24) Delete this header. -#warning The ClangTidyModuleRegistry.h header is deprecated and will be removed in LLVM 24. All of the symbols it used to define have been moved into ClangTidyModule.h. - -#include "ClangTidyModule.h" - -// NOLINTEND - -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULEREGISTRY_H diff --git a/clang-tools-extra/clang-tidy/add_new_check.py b/clang-tools-extra/clang-tidy/add_new_check.py index 1c3ead6d4b1e5..36e3aa6106e55 100755 --- a/clang-tools-extra/clang-tidy/add_new_check.py +++ b/clang-tools-extra/clang-tidy/add_new_check.py @@ -257,15 +257,13 @@ def add_release_notes( ) ) check_name_dashes = f"{module}-{check_name}" - filename = os.path.normpath( - os.path.join(module_path, "../../docs/ReleaseNotes.rst") - ) + filename = os.path.normpath(os.path.join(module_path, "../../docs/ReleaseNotes.md")) with open(filename, "r", encoding="utf8") as f: lines = f.readlines() - lineMatcher = re.compile("New checks") - nextSectionMatcher = re.compile("New check aliases") - checkMatcher = re.compile("- New :doc:`(.*)") + lineMatcher = re.compile(r"#### New checks") + nextSectionMatcher = re.compile(r"#### New check aliases") + checkMatcher = re.compile(r"- New \{doc\}`(.*)") print(f"Updating {filename}...") with open(filename, "w", encoding="utf8", newline="\n") as f: @@ -288,21 +286,16 @@ def add_release_notes( f.write(line) continue - if line.startswith("^^^^"): - f.write(line) - continue - if header_found and add_note_here: - if not line.startswith("^^^^"): - f.write( - f"""- New :doc:`{check_name_dashes} + f.write( + f"""- New {{doc}}`{check_name_dashes} ` check. {wrapped_desc} """ - ) - note_added = True + ) + note_added = True f.write(line) diff --git a/clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp b/clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp index 62fc3b159241d..e892fc0ba70c1 100644 --- a/clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp +++ b/clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp @@ -134,9 +134,9 @@ bool UnrollLoopsCheck::hasKnownBounds(const Stmt *Statement, } } // If increment is unary and not one of ++ and --, loop bounds are unknown. - if (const auto *Op = dyn_cast(Increment)) - if (!Op->isIncrementDecrementOp()) - return false; + if (const auto *Op = dyn_cast(Increment); + Op && !Op->isIncrementDecrementOp()) + return false; if (const auto *BinaryOp = dyn_cast(Conditional)) { const Expr *LHS = BinaryOp->getLHS(); diff --git a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp index ba399efa4a8a6..397d6f8e87e83 100644 --- a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp +++ b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp @@ -67,12 +67,11 @@ void ComparisonInTempFailureRetryCheck::check( const SourceLocation Invocation = SM.getImmediateMacroCallerLoc(LocStart); Token Tok; if (!Lexer::getRawToken(SM.getSpellingLoc(Invocation), Tok, SM, Opts, - /*IgnoreWhiteSpace=*/true)) { - if (Tok.getKind() == tok::raw_identifier && - llvm::is_contained(RetryMacros, Tok.getRawIdentifier())) { - RetryMacroName = Tok.getRawIdentifier(); - break; - } + /*IgnoreWhiteSpace=*/true) && + Tok.getKind() == tok::raw_identifier && + llvm::is_contained(RetryMacros, Tok.getRawIdentifier())) { + RetryMacroName = Tok.getRawIdentifier(); + break; } LocStart = Invocation; diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 2f260c36155ff..783944cd1a5a1 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -21,9 +21,9 @@ namespace clang::tidy::bugprone { using utils::lexer::CommentToken; namespace { AST_MATCHER(Decl, isFromStdNamespaceOrSystemHeader) { - if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext()) - if (D->isStdNamespace()) - return true; + if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext(); + D && D->isStdNamespace()) + return true; if (Node.getLocation().isInvalid()) return false; return Node.getASTContext().getSourceManager().isInSystemHeader( @@ -184,10 +184,11 @@ static const CXXMethodDecl *findMockedMethod(const CXXMethodDecl *Method) { return nullptr; } if (const auto *Next = - dyn_cast_or_null(Method->getNextDeclInContext())) { - if (looksLikeExpectMethod(Next) && areMockAndExpectMethods(Method, Next)) - return Method; - } + dyn_cast_or_null(Method->getNextDeclInContext()); + Next && looksLikeExpectMethod(Next) && + areMockAndExpectMethods(Method, Next)) + return Method; + return nullptr; } @@ -326,16 +327,14 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, const IdentifierInfo *II = PVD->getIdentifier(); if (!II) continue; - if (FunctionDecl *Template = Callee->getTemplateInstantiationPattern()) { - // Don't warn on arguments for parameters instantiated from template - // parameter packs. If we find more arguments than the template - // definition has, it also means that they correspond to a parameter - // pack. - if (Template->getNumParams() <= I || - Template->getParamDecl(I)->isParameterPack()) { - continue; - } - } + // Don't warn on arguments for parameters instantiated from template + // parameter packs. If we find more arguments than the template + // definition has, it also means that they correspond to a parameter + // pack. + if (FunctionDecl *Template = Callee->getTemplateInstantiationPattern(); + Template && (Template->getNumParams() <= I || + Template->getParamDecl(I)->isParameterPack())) + continue; const CharSourceRange BeforeArgument = MakeFileCharRange(ArgBeginLoc, Args[I]->getBeginLoc()); diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp index b7c7a3196d787..f7c0024fc1fd1 100644 --- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp @@ -39,9 +39,9 @@ AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls, if (const auto *OpCallExpr = dyn_cast(E)) { if (const auto *MethodDecl = - dyn_cast_or_null(OpCallExpr->getDirectCallee())) - if (MethodDecl->isConst()) - return false; + dyn_cast_or_null(OpCallExpr->getDirectCallee()); + MethodDecl && MethodDecl->isConst()) + return false; const OverloadedOperatorKind OpKind = OpCallExpr->getOperator(); return OpKind == OO_Equal || OpKind == OO_PlusEqual || diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp index 1c70fb482aa2d..5df9aaf9fff22 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp @@ -106,10 +106,9 @@ void ChainedComparisonData::extract(const Expr *Op) { return; } - if (const auto *OverloadedOp = dyn_cast(Op)) { - if (OverloadedOp->getNumArgs() == 2U) - extract(OverloadedOp); - } + if (const auto *OverloadedOp = dyn_cast(Op); + OverloadedOp && OverloadedOp->getNumArgs() == 2U) + extract(OverloadedOp); } ChainedComparisonCheck::ChainedComparisonCheck(StringRef Name, diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp index 0e0f3b95fffdd..d1ddb186d3b18 100644 --- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp @@ -1563,14 +1563,13 @@ static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node) { }(); LLVM_DEBUG(llvm::dbgs() << "\tType name is '" << NodeTypeName << "'\n"); - if (!NodeTypeName.empty()) { - if (llvm::any_of(Check.IgnoredParameterTypeSuffixes, - [NodeTypeName](StringRef E) { - return !E.empty() && NodeTypeName.ends_with(E); - })) { - LLVM_DEBUG(llvm::dbgs() << "\tType suffix ignored.\n"); - return true; - } + if (!NodeTypeName.empty() && llvm::any_of(Check.IgnoredParameterTypeSuffixes, + [NodeTypeName](StringRef E) { + return !E.empty() && + NodeTypeName.ends_with(E); + })) { + LLVM_DEBUG(llvm::dbgs() << "\tType suffix ignored.\n"); + return true; } return false; @@ -1661,9 +1660,9 @@ class AppearsInSameExpr : public RecursiveASTVisitor { if (!CurrentExprOnlyTreeRoot) return true; - if (auto *PVD = dyn_cast(DRE->getDecl())) - if (llvm::find(FD->parameters(), PVD)) - ParentExprsForParamRefs[PVD].insert(CurrentExprOnlyTreeRoot); + if (auto *PVD = dyn_cast(DRE->getDecl()); + PVD && llvm::find(FD->parameters(), PVD)) + ParentExprsForParamRefs[PVD].insert(CurrentExprOnlyTreeRoot); return true; } diff --git a/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp index 9e2214a5c7c82..d082f12723b77 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp @@ -40,9 +40,9 @@ AST_MATCHER(QualType, isEnableIf) { return true; // Case: enable_if_t< >. if (const auto *TT = BaseType->getAs()) if (const NestedNameSpecifier Q = TT->getQualifier(); - Q.getKind() == NestedNameSpecifier::Kind::Type) - if (CheckTemplate(Q.getAsType()->getAs())) - return true; // Case: enable_if< >::type. + Q.getKind() == NestedNameSpecifier::Kind::Type && + CheckTemplate(Q.getAsType()->getAs())) + return true; // Case: enable_if< >::type. return false; } AST_MATCHER_P(TemplateTypeParmDecl, hasDefaultArgument, diff --git a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp index 072e80e20b0c5..88bd2b708d4cb 100644 --- a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp @@ -91,7 +91,7 @@ static bool isVarPossiblyChanged(const Decl *Func, const Stmt *LoopStmt, static bool isVarThatIsPossiblyChanged(const Decl *Func, const Stmt *LoopStmt, const Stmt *Cond, ASTContext *Context) { if (const auto *DRE = dyn_cast(Cond)) { - if (const auto *VD = dyn_cast(DRE->getDecl())) + if (const ValueDecl *VD = DRE->getDecl()) return isVarPossiblyChanged(Func, LoopStmt, VD, Context); } else if (isa(Cond)) { @@ -225,14 +225,14 @@ static bool overlap(ArrayRef SCC, /// returns true iff `Cond` involves at least one static local variable. static bool hasStaticLocalVariable(const Stmt *Cond) { if (const auto *DRE = dyn_cast(Cond)) { - if (const auto *VD = dyn_cast(DRE->getDecl())) - if (VD->isStaticLocal()) - return true; + if (const auto *VD = dyn_cast(DRE->getDecl()); + VD && VD->isStaticLocal()) + return true; if (const auto *BD = dyn_cast(DRE->getDecl())) - if (const auto *DD = dyn_cast(BD->getDecomposedDecl())) - if (DD->isStaticLocal()) - return true; + if (const auto *DD = dyn_cast(BD->getDecomposedDecl()); + DD && DD->isStaticLocal()) + return true; } return llvm::any_of(Cond->children(), [](const Stmt *Child) { diff --git a/clang-tools-extra/clang-tidy/bugprone/InvalidEnumDefaultInitializationCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InvalidEnumDefaultInitializationCheck.cpp index 09d84391d8ba3..abe27388a3f8c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/InvalidEnumDefaultInitializationCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/InvalidEnumDefaultInitializationCheck.cpp @@ -41,10 +41,10 @@ AST_MATCHER(EnumDecl, isCompleteAndHasNoZeroValue) { AST_MATCHER(Expr, isEmptyInit) { if (isa(&Node)) return true; - if (const auto *Init = dyn_cast(&Node)) { - if (Init->getNumInits() == 0) - return true; - } + if (const auto *Init = dyn_cast(&Node); + Init && Init->getNumInits() == 0) + return true; + return false; } diff --git a/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp index 051f0b569c66b..626237ea1e27c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp @@ -101,10 +101,10 @@ static std::optional getStandardEndText(ASTContext &Context, unsigned EndIdx = 1; const Expr *FirstArg = Call->getArg(0); if (const auto *Record = - FirstArg->getType().getNonReferenceType()->getAsCXXRecordDecl()) { - if (Record->getIdentifier() && Record->getName().ends_with("_policy")) - EndIdx = 2; - } + FirstArg->getType().getNonReferenceType()->getAsCXXRecordDecl(); + Record && Record->getIdentifier() && + Record->getName().ends_with("_policy")) + EndIdx = 2; if (Call->getNumArgs() <= EndIdx) return std::nullopt; diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp index dc09fabffed1e..9e462d33bfd0e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp @@ -67,14 +67,14 @@ static unsigned getLength(const Expr *E, E = E->IgnoreImpCasts(); if (const auto *LengthDRE = dyn_cast(E)) - if (const auto *LengthVD = dyn_cast(LengthDRE->getDecl())) - if (!isa(LengthVD)) - if (const Expr *LengthInit = LengthVD->getInit(); - LengthInit && !LengthInit->isValueDependent()) { - Expr::EvalResult Length; - if (LengthInit->EvaluateAsInt(Length, *Result.Context)) - return Length.Val.getInt().getZExtValue(); - } + if (const auto *LengthVD = dyn_cast(LengthDRE->getDecl()); + LengthVD && !isa(LengthVD)) + if (const Expr *LengthInit = LengthVD->getInit(); + LengthInit && !LengthInit->isValueDependent()) { + Expr::EvalResult Length; + if (LengthInit->EvaluateAsInt(Length, *Result.Context)) + return Length.Val.getInt().getZExtValue(); + } if (const auto *LengthIL = dyn_cast(E)) return LengthIL->getValue().getZExtValue(); @@ -107,9 +107,9 @@ static const CallExpr *getStrlenExpr(const MatchFinder::MatchResult &Result) { Result.Nodes.getNodeAs(WrongLengthExprName)) if (const Decl *D = StrlenExpr->getCalleeDecl()) if (const FunctionDecl *FD = D->getAsFunction()) - if (const IdentifierInfo *II = FD->getIdentifier()) - if (II->isStr("strlen") || II->isStr("wcslen")) - return StrlenExpr; + if (const IdentifierInfo *II = FD->getIdentifier(); + II && (II->isStr("strlen") || II->isStr("wcslen"))) + return StrlenExpr; return nullptr; } @@ -233,9 +233,9 @@ isGivenLengthEqualToSrcLength(const MatchFinder::MatchResult &Result) { if (GivenLength != 0 && SrcLength != 0 && GivenLength == SrcLength) return true; - if (const auto *LengthExpr = Result.Nodes.getNodeAs(LengthExprName)) - if (isa(LengthExpr->IgnoreParenImpCasts())) - return false; + if (const auto *LengthExpr = Result.Nodes.getNodeAs(LengthExprName); + LengthExpr && isa(LengthExpr->IgnoreParenImpCasts())) + return false; // Check the strlen()'s argument's 'VarDecl' is equal to the source 'VarDecl'. if (const CallExpr *StrlenCE = getStrlenExpr(Result)) @@ -324,21 +324,18 @@ static void lengthExprHandle(const Expr *LengthExpr, const Expr *LhsExpr = BO->getLHS()->IgnoreImpCasts(); const Expr *RhsExpr = BO->getRHS()->IgnoreImpCasts(); - if (const auto *LhsIL = dyn_cast(LhsExpr)) { - if (LhsIL->getValue().getZExtValue() == 1) { - Diag << FixItHint::CreateRemoval( - {LhsIL->getBeginLoc(), - RhsExpr->getBeginLoc().getLocWithOffset(-1)}); - return; - } + if (const auto *LhsIL = dyn_cast(LhsExpr); + LhsIL && LhsIL->getValue().getZExtValue() == 1) { + Diag << FixItHint::CreateRemoval( + {LhsIL->getBeginLoc(), RhsExpr->getBeginLoc().getLocWithOffset(-1)}); + return; } - if (const auto *RhsIL = dyn_cast(RhsExpr)) { - if (RhsIL->getValue().getZExtValue() == 1) { - Diag << FixItHint::CreateRemoval( - {LhsExpr->getEndLoc().getLocWithOffset(1), RhsIL->getEndLoc()}); - return; - } + if (const auto *RhsIL = dyn_cast(RhsExpr); + RhsIL && RhsIL->getValue().getZExtValue() == 1) { + Diag << FixItHint::CreateRemoval( + {LhsExpr->getEndLoc().getLocWithOffset(1), RhsIL->getEndLoc()}); + return; } } @@ -912,9 +909,9 @@ void NotNullTerminatedResultCheck::memcpySFix( void NotNullTerminatedResultCheck::memchrFix( StringRef Name, const MatchFinder::MatchResult &Result) { const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); - if (const auto *GivenCL = dyn_cast(FunctionExpr->getArg(1))) - if (GivenCL->getValue() != 0) - return; + if (const auto *GivenCL = dyn_cast(FunctionExpr->getArg(1)); + GivenCL && GivenCL->getValue() != 0) + return; const auto Diag = diag(FunctionExpr->getArg(2)->IgnoreParenCasts()->getBeginLoc(), diff --git a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp index dda687ff7ade5..1e3615f9ac971 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp @@ -90,9 +90,9 @@ static const Decl *findRVRefOverload(const FunctionDecl &FD, for (const Decl *Overload : LookupResult) { if (Overload == &FD) continue; - if (const auto *O = dyn_cast(Overload)) - if (hasSameParameterTypes(FD, *O, PD)) - return O; + if (const auto *O = dyn_cast(Overload); + O && hasSameParameterTypes(FD, *O, PD)) + return O; } return nullptr; } diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp index 12b5a5de55618..970cd0f39a9ce 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp @@ -375,15 +375,14 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) { const auto *SzOfExpr = Result.Nodes.getNodeAs("sizeof-expr"); - if (const auto *Type = dyn_cast(SizeofArgTy)) { - // check if the array element size is larger than one. If true, - // the size of the array is higher than the number of elements - if (!getSizeOfType(Ctx, Type->getElementType().getTypePtr()).isOne()) { - diag(SzOfExpr->getBeginLoc(), - "suspicious usage of 'sizeof' in the loop") - << SzOfExpr->getSourceRange(); - } - } + // check if the array element size is larger than one. If true, + // the size of the array is higher than the number of elements + if (const auto *Type = dyn_cast(SizeofArgTy); + Type && + !getSizeOfType(Ctx, Type->getElementType().getTypePtr()).isOne()) + diag(SzOfExpr->getBeginLoc(), "suspicious usage of 'sizeof' in the loop") + << SzOfExpr->getSourceRange(); + } else if (const auto *E = Result.Nodes.getNodeAs("sizeof-pointer")) { diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression " "of pointer type") diff --git a/clang-tools-extra/clang-tidy/bugprone/StdNamespaceModificationCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StdNamespaceModificationCheck.cpp index a623ed690697b..709de88a71f48 100644 --- a/clang-tools-extra/clang-tidy/bugprone/StdNamespaceModificationCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/StdNamespaceModificationCheck.cpp @@ -34,6 +34,19 @@ AST_POLYMORPHIC_MATCHER_P( Builder) != Args.end(); } +AST_MATCHER(Decl, isInStdOrPosixNamespace) { + for (const auto *DC = dyn_cast(&Node); DC; + DC = DC->getParent()) { + if (DC->isStdNamespace()) + return true; + + if (const auto *NS = dyn_cast(DC); + NS && NS->getName() == "posix" && NS->getParent()->isTranslationUnit()) + return true; + } + return false; +} + } // namespace namespace clang::tidy::bugprone { @@ -43,10 +56,11 @@ void StdNamespaceModificationCheck::registerMatchers(MatchFinder *Finder) { hasDeclContext(namespaceDecl(hasAnyName("std", "posix"), unless(hasParent(namespaceDecl()))) .bind("nmspc")); + // FIXME: Investigate why lambda closure declarations can be absent from the + // AST parent map. const auto UserDefinedDecl = namedDecl(anyOf(classTemplateDecl(), tagDecl()), - hasAncestor(namespaceDecl(hasAnyName("std", "posix"), - unless(hasParent(namespaceDecl()))))); + hasDeclContext(isInStdOrPosixNamespace())); const auto UserDefinedType = qualType(hasUnqualifiedDesugaredType(anyOf( tagType(unless(hasDeclaration(UserDefinedDecl))), templateSpecializationType(unless(hasDeclaration(UserDefinedDecl)))))); diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp index 7890afb41addb..4247c9050009d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp @@ -52,14 +52,13 @@ void SuspiciousMemoryComparisonCheck::check( if (PointeeType->isRecordType()) { if (const RecordDecl *RD = PointeeType->getAsRecordDecl()->getDefinition()) { - if (const auto *CXXDecl = dyn_cast(RD)) { - if (!CXXDecl->isStandardLayout()) { - diag(CE->getBeginLoc(), - "comparing object representation of non-standard-layout type " - "%0; consider using a comparison operator instead") - << PointeeQualifiedType; - break; - } + if (const auto *CXXDecl = dyn_cast(RD); + CXXDecl && !CXXDecl->isStandardLayout()) { + diag(CE->getBeginLoc(), + "comparing object representation of non-standard-layout type " + "%0; consider using a comparison operator instead") + << PointeeQualifiedType; + break; } } } diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousMissingCommaCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMissingCommaCheck.cpp index 1ada9e5eba86c..d9d1660f3b916 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SuspiciousMissingCommaCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMissingCommaCheck.cpp @@ -108,10 +108,9 @@ void SuspiciousMissingCommaCheck::check( unsigned int Count = 0; for (unsigned int I = 0; I < Size; ++I) { const Expr *Child = InitializerList->getInit(I)->IgnoreImpCasts(); - if (const auto *Literal = dyn_cast(Child)) { - if (Literal->getNumConcatenated() > 1) - ++Count; - } + if (const auto *Literal = dyn_cast(Child); + Literal && Literal->getNumConcatenated() > 1) + ++Count; } // Warn only when concatenation is not common in this initializer list. diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp index 03fc3c56f428c..e87d83bee3d4b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp @@ -81,9 +81,9 @@ class FindAssignToVarBefore bool VisitDeclStmt(const DeclStmt *S) { for (const Decl *D : S->getDeclGroup()) - if (const auto *LeftVar = dyn_cast(D)) - if (LeftVar->hasInit()) - return isAccessForVar(LeftVar->getInit()); + if (const auto *LeftVar = dyn_cast(D); + LeftVar && LeftVar->hasInit()) + return isAccessForVar(LeftVar->getInit()); return false; } bool VisitBinaryOperator(const BinaryOperator *S) { @@ -140,9 +140,10 @@ void SuspiciousReallocUsageCheck::check( dyn_cast(PtrInputExpr->IgnoreParenImpCasts())) if (const auto *Var = dyn_cast(DeclRef->getDecl())) if (const auto *Func = - Result.Nodes.getNodeAs("parent_function")) - if (FindAssignToVarBefore{Var, DeclRef, SM}.Visit(Func->getBody())) - return; + Result.Nodes.getNodeAs("parent_function"); + Func && + FindAssignToVarBefore{Var, DeclRef, SM}.Visit(Func->getBody())) + return; const StringRef CodeOfAssignedExpr = Lexer::getSourceText( CharSourceRange::getTokenRange(PtrResultExpr->getSourceRange()), SM, diff --git a/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp index 152c0cbd106f5..ba4c034a4b1a8 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp @@ -25,10 +25,10 @@ void SwappedArgumentsCheck::registerMatchers(MatchFinder *Finder) { /// implicit conversions that have no effect on the input but block our view for /// other implicit casts. static const Expr *ignoreNoOpCasts(const Expr *E) { - if (auto *Cast = dyn_cast(E)) - if (Cast->getCastKind() == CK_LValueToRValue || - Cast->getCastKind() == CK_NoOp) - return ignoreNoOpCasts(Cast->getSubExpr()); + if (auto *Cast = dyn_cast(E); + Cast && (Cast->getCastKind() == CK_LValueToRValue || + Cast->getCastKind() == CK_NoOp)) + return ignoreNoOpCasts(Cast->getSubExpr()); return E; } diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 3a2f07b376e86..b13367e25dcb2 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -424,13 +424,12 @@ void UseAfterMoveFinder::getDeclRefs( !MovedAs->hasMemberName(Member->getMemberDecl()->getIdentifier())) { continue; } - if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) { + if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block && + (Operator || !isSpecifiedAfterMove(DeclRef->getDecl()))) // Ignore uses of a standard smart pointer or classes annotated as // "null_after_move" (smart-pointer-like behavior) that don't // dereference the pointer. - if (Operator || !isSpecifiedAfterMove(DeclRef->getDecl())) - DeclRefs->insert(DeclRef); - } + DeclRefs->insert(DeclRef); } }; diff --git a/clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp index 67a44d155bd2b..07dc77b5dee0a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp @@ -241,23 +241,22 @@ void VirtualNearMissCheck::check(const MatchFinder::MatchResult &Result) { const unsigned EditDistance = BaseMD->getName().edit_distance( DerivedMD->getName(), EditDistanceThreshold); - if (EditDistance > 0 && EditDistance <= EditDistanceThreshold) { - if (checkOverrideWithoutName(Context, BaseMD, DerivedMD)) { - // A "virtual near miss" is found. - const auto Range = CharSourceRange::getTokenRange( - SourceRange(DerivedMD->getLocation())); - - const bool ApplyFix = !BaseMD->isTemplateInstantiation() && - !DerivedMD->isTemplateInstantiation(); - const auto Diag = - diag(DerivedMD->getBeginLoc(), - "method '%0' has a similar name and the same signature as " - "virtual method '%1'; did you mean to override it?") - << DerivedMD->getQualifiedNameAsString() - << BaseMD->getQualifiedNameAsString(); - if (ApplyFix) - Diag << FixItHint::CreateReplacement(Range, BaseMD->getName()); - } + if (EditDistance > 0 && EditDistance <= EditDistanceThreshold && + checkOverrideWithoutName(Context, BaseMD, DerivedMD)) { + // A "virtual near miss" is found. + const auto Range = CharSourceRange::getTokenRange( + SourceRange(DerivedMD->getLocation())); + + const bool ApplyFix = !BaseMD->isTemplateInstantiation() && + !DerivedMD->isTemplateInstantiation(); + const auto Diag = + diag(DerivedMD->getBeginLoc(), + "method '%0' has a similar name and the same signature as " + "virtual method '%1'; did you mean to override it?") + << DerivedMD->getQualifiedNameAsString() + << BaseMD->getQualifiedNameAsString(); + if (ApplyFix) + Diag << FixItHint::CreateReplacement(Range, BaseMD->getName()); } } } diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp index 74c47e23044f4..98faa3efe84b6 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp @@ -184,11 +184,10 @@ void PreferMemberInitializerCheck::check( if (isNoReturnCallStatement(S)) return; - if (const auto *CondOp = dyn_cast(S)) { - if (isNoReturnCallStatement(CondOp->getLHS()) || - isNoReturnCallStatement(CondOp->getRHS())) - return; - } + if (const auto *CondOp = dyn_cast(S); + CondOp && (isNoReturnCallStatement(CondOp->getLHS()) || + isNoReturnCallStatement(CondOp->getRHS()))) + return; std::optional AssignmentToMember = isAssignmentToMemberOf(Class, S, Ctor); diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp index fe95dbba68118..47aaf6cfa5189 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp @@ -89,12 +89,10 @@ void SlicingCheck::diagnoseSlicedOverriddenMethods( } } // Recursively process bases. - for (const auto &Base : DerivedDecl.bases()) { - if (const auto *BaseRecord = Base.getType()->getAsCXXRecordDecl()) { - if (BaseRecord->isCompleteDefinition()) - diagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl); - } - } + for (const auto &Base : DerivedDecl.bases()) + if (const auto *BaseRecord = Base.getType()->getAsCXXRecordDecl(); + BaseRecord && BaseRecord->isCompleteDefinition()) + diagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl); } void SlicingCheck::check(const MatchFinder::MatchResult &Result) { diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/UseEnumClassCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/UseEnumClassCheck.cpp index 84720d10c233e..0865148a2fad4 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/UseEnumClassCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/UseEnumClassCheck.cpp @@ -10,8 +10,15 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" using namespace clang::ast_matchers; +using namespace clang::ast_matchers::internal; namespace clang::tidy::cppcoreguidelines { +namespace { +// FIXME: The matcher 'hasName(Name)' asserts that its argument 'Name' is +// nonempty. Perhaps remove that assertion and replace 'isUnnamed()' with +// 'hasName("")'. +AST_MATCHER(EnumDecl, isUnnamed) { return Node.getName().empty(); } +} // namespace UseEnumClassCheck::UseEnumClassCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), @@ -26,10 +33,10 @@ void UseEnumClassCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { } void UseEnumClassCheck::registerMatchers(MatchFinder *Finder) { - const auto EnumDecl = - IgnoreUnscopedEnumsInClasses - ? enumDecl(unless(isScoped()), unless(hasParent(recordDecl()))) - : enumDecl(unless(isScoped())); + const auto EnumDecl = IgnoreUnscopedEnumsInClasses + ? enumDecl(unless(isScoped()), unless(isUnnamed()), + unless(hasParent(recordDecl()))) + : enumDecl(unless(isScoped()), unless(isUnnamed())); Finder->addMatcher(EnumDecl.bind("unscoped_enum"), this); } diff --git a/clang-tools-extra/clang-tidy/fuchsia/TemporaryObjectsCheck.cpp b/clang-tools-extra/clang-tidy/fuchsia/TemporaryObjectsCheck.cpp index 2fa83b41869ea..a29eda7289aa2 100644 --- a/clang-tools-extra/clang-tidy/fuchsia/TemporaryObjectsCheck.cpp +++ b/clang-tools-extra/clang-tidy/fuchsia/TemporaryObjectsCheck.cpp @@ -7,7 +7,6 @@ //===----------------------------------------------------------------------===// #include "TemporaryObjectsCheck.h" -#include "../utils/CheckUtils.h" #include "../utils/OptionsUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" @@ -20,9 +19,6 @@ namespace clang::tidy::fuchsia { namespace { -constexpr llvm::StringLiteral DeprecatedCheckName = "zircon-temporary-objects"; -constexpr llvm::StringLiteral CanonicalCheckName = "fuchsia-temporary-objects"; - AST_MATCHER_P(CXXRecordDecl, matchesAnyName, ArrayRef, Names) { const std::string QualifiedName = Node.getQualifiedNameAsString(); return llvm::is_contained(Names, QualifiedName); @@ -33,11 +29,7 @@ AST_MATCHER_P(CXXRecordDecl, matchesAnyName, ArrayRef, Names) { TemporaryObjectsCheck::TemporaryObjectsCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), - Names(utils::options::parseStringList(Options.get("Names", ""))) { - if (Name == DeprecatedCheckName) - utils::diagDeprecatedCheckAlias(*this, *Context, DeprecatedCheckName, - CanonicalCheckName); -} + Names(utils::options::parseStringList(Options.get("Names", ""))) {} void TemporaryObjectsCheck::registerMatchers(MatchFinder *Finder) { // Matcher for default constructors. diff --git a/clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp b/clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp index ee0e29b9c5d17..d068764d1fe69 100644 --- a/clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp +++ b/clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp @@ -34,23 +34,21 @@ void GlobalNamesInHeadersCheck::check(const MatchFinder::MatchResult &Result) { if (D->getBeginLoc().isMacroID()) return; - // Ignore if it comes from the "main" file ... + // Ignore if it comes from the "main" file unless that file is a header. if (Result.SourceManager->isInMainFile( - Result.SourceManager->getExpansionLoc(D->getBeginLoc()))) { - // unless that file is a header. - if (!utils::isSpellingLocInHeaderFile( - D->getBeginLoc(), *Result.SourceManager, getHeaderFileExtensions())) - return; - } + Result.SourceManager->getExpansionLoc(D->getBeginLoc())) && + !utils::isSpellingLocInHeaderFile(D->getBeginLoc(), *Result.SourceManager, + getHeaderFileExtensions())) + return; - if (const auto *UsingDirective = dyn_cast(D)) { - if (UsingDirective->getNominatedNamespace()->isAnonymousNamespace()) { - // Anonymous namespaces inject a using directive into the AST to import - // the names into the containing namespace. - // We should not have them in headers, but there is another warning for - // that. - return; - } + if (const auto *UsingDirective = dyn_cast(D); + UsingDirective && + UsingDirective->getNominatedNamespace()->isAnonymousNamespace()) { + // Anonymous namespaces inject a using directive into the AST to import + // the names into the containing namespace. + // We should not have them in headers, but there is another warning for + // that. + return; } diag(D->getBeginLoc(), diff --git a/clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp b/clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp index 60baee7fdba6a..4384a1067d581 100644 --- a/clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp @@ -37,10 +37,10 @@ void PreferRegisterOverUnsignedCheck::check( bool NeedsQualification = true; const DeclContext *Context = UserVarDecl->getDeclContext(); while (Context) { - if (const auto *Namespace = dyn_cast(Context)) - if (isa(Namespace->getDeclContext()) && - Namespace->getName() == "llvm") - NeedsQualification = false; + if (const auto *Namespace = dyn_cast(Context); + Namespace && isa(Namespace->getDeclContext()) && + Namespace->getName() == "llvm") + NeedsQualification = false; for (const auto *UsingDirective : Context->using_directives()) { const NamespaceDecl *Namespace = UsingDirective->getNominatedNamespace(); if (isa(Namespace->getDeclContext()) && diff --git a/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp index 59d821e29e75a..afaae90ef172e 100644 --- a/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp @@ -24,9 +24,9 @@ AST_MATCHER(VarDecl, isLocalVariable) { return Node.isLocalVarDecl(); } AST_MATCHER(Decl, isLexicallyInAnonymousNamespace) { for (const DeclContext *DC = Node.getLexicalDeclContext(); DC != nullptr; DC = DC->getLexicalParent()) { - if (const auto *ND = dyn_cast(DC)) - if (ND->isAnonymousNamespace()) - return true; + if (const auto *ND = dyn_cast(DC); + ND && ND->isAnonymousNamespace()) + return true; } return false; diff --git a/clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp b/clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp index 3120c5c6c86d5..231198a8ccd32 100644 --- a/clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp @@ -69,9 +69,9 @@ void InlineFunctionDeclCheck::check(const MatchFinder::MatchResult &Result) { return; // Ignore lambda functions as they are internal and implicit. - if (const auto *MethodDecl = dyn_cast(FuncDecl)) - if (MethodDecl->getParent()->isLambda()) - return; + if (const auto *MethodDecl = dyn_cast(FuncDecl); + MethodDecl && MethodDecl->getParent()->isLambda()) + return; // Check if decl starts with LIBC_INLINE const auto Loc = FullSourceLoc(Result.SourceManager->getFileLoc(SrcBegin), diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp index 2385808ff7a7e..1abe4db743a25 100644 --- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp @@ -255,14 +255,13 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) { VariableCategory VC = VariableCategory::Value; const QualType VT = Variable->getType(); - if (VT->isReferenceType()) { + if (VT->isReferenceType()) VC = VariableCategory::Reference; - } else if (VT->isPointerType()) { + else if (VT->isPointerType()) + VC = VariableCategory::Pointer; + else if (const auto *ArrayT = dyn_cast(VT); + ArrayT && ArrayT->getElementType()->isPointerType()) VC = VariableCategory::Pointer; - } else if (const auto *ArrayT = dyn_cast(VT)) { - if (ArrayT->getElementType()->isPointerType()) - VC = VariableCategory::Pointer; - } const auto CheckValue = [&]() { // Offload const-analysis to utility function. @@ -339,11 +338,11 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) { if (WarnPointersAsValues && !VT.isConstQualified()) CheckValue(); if (WarnPointersAsPointers) { - if (const auto *PT = dyn_cast(VT)) { - if (!PT->getPointeeType().isConstQualified() && - !PT->getPointeeType()->isFunctionType()) - CheckPointee(); - } + if (const auto *PT = dyn_cast(VT); + PT && !PT->getPointeeType().isConstQualified() && + !PT->getPointeeType()->isFunctionType()) + CheckPointee(); + if (const auto *AT = dyn_cast(VT)) { assert(AT->getElementType()->isPointerType()); if (!AT->getElementType()->getPointeeType().isConstQualified()) diff --git a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp index adbe8d75ba5aa..beb1c9bc8d3ea 100644 --- a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp @@ -67,12 +67,11 @@ AST_MATCHER(Decl, isFirstDecl) { return Node.isFirstDecl(); } AST_MATCHER(FunctionDecl, hasBody) { return Node.hasBody(); } AST_MATCHER(Decl, isInImportableModuleUnit) { - if (const Module *OwningModule = Node.getOwningModule()) - if (OwningModule->Kind == Module::ModuleInterfaceUnit || - OwningModule->Kind == Module::ModulePartitionInterface || - OwningModule->Kind == Module::ModulePartitionImplementation) - return true; - return false; + const Module *OwningModule = Node.getOwningModule(); + return OwningModule && + (OwningModule->Kind == Module::ModuleInterfaceUnit || + OwningModule->Kind == Module::ModulePartitionInterface || + OwningModule->Kind == Module::ModulePartitionImplementation); } AST_MATCHER_P(Decl, isAllRedeclsInMainFile, const FileExtensionsSet *, diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp index 551ae8b1110bc..3cf0173aaa002 100644 --- a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp @@ -181,10 +181,10 @@ initializeBindArgumentForCallExpr(const MatchFinder::MatchResult &Result, static bool anyDescendantIsLocal(const Stmt *Statement) { if (const auto *DeclRef = dyn_cast(Statement)) { const ValueDecl *Decl = DeclRef->getDecl(); - if (const auto *Var = dyn_cast_or_null(Decl)) { - if (Var->isLocalVarDeclOrParm()) - return true; - } + if (const auto *Var = dyn_cast_or_null(Decl); + Var && Var->isLocalVarDeclOrParm()) + return true; + } else if (isa(Statement)) { return true; } @@ -378,12 +378,10 @@ static void addFunctionCallArgs(ArrayRef Args, static bool isPlaceHolderIndexRepeated(const ArrayRef Args) { llvm::SmallSet PlaceHolderIndices; - for (const BindArgument &B : Args) { - if (B.PlaceHolderIndex) { - if (!PlaceHolderIndices.insert(B.PlaceHolderIndex).second) - return true; - } - } + for (const BindArgument &B : Args) + if (B.PlaceHolderIndex && + !PlaceHolderIndices.insert(B.PlaceHolderIndex).second) + return true; return false; } diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidCStyleCastCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidCStyleCastCheck.cpp index 98ca1a46be845..e519a1a60d47c 100644 --- a/clang-tools-extra/clang-tidy/modernize/AvoidCStyleCastCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/AvoidCStyleCastCheck.cpp @@ -169,16 +169,15 @@ void AvoidCStyleCastCheck::check(const MatchFinder::MatchResult &Result) { DestTypeAsWritten->isRecordType() && !DestTypeAsWritten->isElaboratedTypeSpecifier(); - if (CastExpr->getCastKind() == CK_NoOp && !FnToFnCast) { - // Function pointer/reference casts may be needed to resolve ambiguities in - // case of overloaded functions, so detection of redundant casts is trickier - // in this case. Don't emit "redundant cast" warnings for function - // pointer/reference types. - if (sameTypeAsWritten(SourceTypeAsWritten, DestTypeAsWritten)) { - diag(CastExpr->getBeginLoc(), "redundant cast to the same type") - << FixItHint::CreateRemoval(ReplaceRange); - return; - } + // Function pointer/reference casts may be needed to resolve ambiguities in + // case of overloaded functions, so detection of redundant casts is trickier + // in this case. Don't emit "redundant cast" warnings for function + // pointer/reference types. + if (CastExpr->getCastKind() == CK_NoOp && !FnToFnCast && + sameTypeAsWritten(SourceTypeAsWritten, DestTypeAsWritten)) { + diag(CastExpr->getBeginLoc(), "redundant cast to the same type") + << FixItHint::CreateRemoval(ReplaceRange); + return; } // The rest of this check is only relevant to C++. diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp index 6965569e6b87e..75bf2a7325900 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -501,13 +501,14 @@ static bool canBeModified(ASTContext *Context, const Expr *E) { const auto Parents = Context->getParents(*E); if (Parents.size() != 1) return true; - if (const auto *Cast = Parents[0].get()) { - if ((Cast->getCastKind() == CK_NoOp && - ASTContext::hasSameType(Cast->getType(), E->getType().withConst())) || - (Cast->getCastKind() == CK_LValueToRValue && - !Cast->getType().isNull() && Cast->getType()->isFundamentalType())) - return false; - } + if (const auto *Cast = Parents[0].get(); + Cast && + ((Cast->getCastKind() == CK_NoOp && + ASTContext::hasSameType(Cast->getType(), E->getType().withConst())) || + (Cast->getCastKind() == CK_LValueToRValue && !Cast->getType().isNull() && + Cast->getType()->isFundamentalType()))) + return false; + // FIXME: Make this function more generic. return true; } @@ -755,7 +756,8 @@ void LoopConvertCheck::doConversion( Parents[0].getSourceRange().getBegin()))) { Range = Paren->getSourceRange(); } - } else if (const auto *UOP = Parents[0].get()) { + } else if (const auto *UOP = Parents[0].get(); + UOP && UOP->getOpcode() == UO_AddrOf) { // If we are taking the address of the loop variable, then we must // not use a copy, as it would mean taking the address of the loop's // local index instead. @@ -763,8 +765,7 @@ void LoopConvertCheck::doConversion( // of the loop's body (for instance, in a function that got the // loop's index as a const reference parameter), or where we take // the address of a member (like "&Arr[i].A.B.C"). - if (UOP->getOpcode() == UO_AddrOf) - CanCopy = false; + CanCopy = false; } } } else { @@ -851,9 +852,9 @@ StringRef LoopConvertCheck::getContainerString(ASTContext *Context, } else { // For CXXOperatorCallExpr such as vector_ptr->size() we want the class // object vector_ptr, but for vector[2] we need the whole expression. - if (const auto *E = dyn_cast(ContainerExpr)) - if (E->getOperator() != OO_Subscript) - ContainerExpr = E->getArg(0); + if (const auto *E = dyn_cast(ContainerExpr); + E && E->getOperator() != OO_Subscript) + ContainerExpr = E->getArg(0); ContainerString = getStringFromRange(Context->getSourceManager(), Context->getLangOpts(), ContainerExpr->getSourceRange()); @@ -991,11 +992,11 @@ bool LoopConvertCheck::isConvertible(ASTContext *Context, return false; } else if (FixerKind == LFK_PseudoArray) { - if (const auto *EndCall = Nodes.getNodeAs(EndCallName)) { + if (const auto *EndCall = Nodes.getNodeAs(EndCallName); + EndCall && !isa(EndCall->getCallee())) // This call is required to obtain the container. - if (!isa(EndCall->getCallee())) - return false; - } + return false; + return Nodes.getNodeAs(EndCallName) != nullptr; } return true; diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp index da86ecf6395ae..f173de2c52bc0 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp @@ -121,7 +121,7 @@ bool DeclFinderASTVisitor::VisitNamedDecl(NamedDecl *D) { /// Forward any declaration references to the actual check on the /// referenced declaration. bool DeclFinderASTVisitor::VisitDeclRefExpr(DeclRefExpr *DeclRef) { - if (auto *D = dyn_cast(DeclRef->getDecl())) + if (ValueDecl *D = DeclRef->getDecl()) return VisitNamedDecl(D); return true; } @@ -139,12 +139,12 @@ bool DeclFinderASTVisitor::VisitTypeLoc(TypeLoc TL) { // Check for base type conflicts. For example, when a struct is being // referenced in the body of the loop, the above getAsString() will return the // whole type (ex. "struct s"), but will be caught here. - if (const IdentifierInfo *Ident = QType.getBaseTypeIdentifier()) { - if (Ident->getName() == Name) { - Found = true; - return false; - } + if (const IdentifierInfo *Ident = QType.getBaseTypeIdentifier(); + Ident && Ident->getName() == Name) { + Found = true; + return false; } + return true; } @@ -179,9 +179,9 @@ const Expr *digThroughConstructorsConversions(const Expr *E) { } // If this is a conversion (as iterators commonly convert into their const // iterator counterparts), dig through that as well. - if (const auto *ME = dyn_cast(E)) - if (isa(ME->getMethodDecl())) - return digThroughConstructorsConversions(ME->getImplicitObjectArgument()); + if (const auto *ME = dyn_cast(E); + ME && isa(ME->getMethodDecl())) + return digThroughConstructorsConversions(ME->getImplicitObjectArgument()); return E; } @@ -289,10 +289,11 @@ static bool isIndexInSubscriptExpr(const ASTContext *Context, Obj->IgnoreParenImpCasts())) return true; - if (const Expr *InnerObj = getDereferenceOperand(Obj->IgnoreParenImpCasts())) - if (PermitDeref && areSameExpr(Context, SourceExpr->IgnoreParenImpCasts(), - InnerObj->IgnoreParenImpCasts())) - return true; + if (const Expr *InnerObj = getDereferenceOperand(Obj->IgnoreParenImpCasts()); + InnerObj && PermitDeref && + areSameExpr(Context, SourceExpr->IgnoreParenImpCasts(), + InnerObj->IgnoreParenImpCasts())) + return true; return false; } @@ -536,21 +537,21 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) { const Expr *ResultExpr = Member; QualType ExprType; if (const auto *Call = - dyn_cast(Base->IgnoreParenImpCasts())) { - // If operator->() is a MemberExpr containing a CXXOperatorCallExpr, then - // the MemberExpr does not have the expression we want. We therefore catch - // that instance here. - // For example, if vector::iterator defines operator->(), then the - // example `i->bar()` at the top of this function is a CXXMemberCallExpr - // referring to `i->` as the member function called. We want just `i`, so - // we take the argument to operator->() as the base object. - if (Call->getOperator() == OO_Arrow) { - assert(Call->getNumArgs() == 1 && - "Operator-> takes more than one argument"); - Obj = getDeclRef(Call->getArg(0)); - ResultExpr = Obj; - ExprType = Call->getCallReturnType(*Context); - } + dyn_cast(Base->IgnoreParenImpCasts()); + Call && Call->getOperator() == OO_Arrow) + // If operator->() is a MemberExpr containing a CXXOperatorCallExpr, then + // the MemberExpr does not have the expression we want. We therefore catch + // that instance here. + // For example, if vector::iterator defines operator->(), then the + // example `i->bar()` at the top of this function is a CXXMemberCallExpr + // referring to `i->` as the member function called. We want just `i`, so + // we take the argument to operator->() as the base object. + { + assert(Call->getNumArgs() == 1 && + "Operator-> takes more than one argument"); + Obj = getDeclRef(Call->getArg(0)); + ResultExpr = Obj; + ExprType = Call->getCallReturnType(*Context); } if (Obj && exprReferencesVariable(IndexVar, Obj)) { @@ -600,13 +601,12 @@ bool ForLoopIndexUseVisitor::TraverseCXXMemberCallExpr( // this is restricted to pseudo-arrays by requiring a single, integer // argument. const IdentifierInfo *Ident = Member->getMemberDecl()->getIdentifier(); - if (Ident && Ident->isStr("at") && MemberCall->getNumArgs() == 1) { - if (isIndexInSubscriptExpr(Context, MemberCall->getArg(0), IndexVar, - Member->getBase(), ContainerExpr, - ContainerNeedsDereference)) { - addUsage(Usage(MemberCall)); - return true; - } + if (Ident && Ident->isStr("at") && MemberCall->getNumArgs() == 1 && + isIndexInSubscriptExpr(Context, MemberCall->getArg(0), IndexVar, + Member->getBase(), ContainerExpr, + ContainerNeedsDereference)) { + addUsage(Usage(MemberCall)); + return true; } if (containsExpr(Context, &DependentExprs, Member->getBase())) @@ -828,12 +828,12 @@ bool ForLoopIndexUseVisitor::TraverseStmt(Stmt *S) { // traversal so that we don't end up diagnosing the contained DeclRefExpr as // inconsistent usage. No need to record the usage here -- this is done in // TraverseLambdaCapture(). - if (const auto *LE = dyn_cast_or_null(NextStmtParent)) { + if (const auto *LE = dyn_cast_or_null(NextStmtParent); + LE && S != LE->getBody()) // Any child of a LambdaExpr that isn't the body is an initialization // expression. - if (S != LE->getBody()) - return true; - } + return true; + return traverseStmtImpl(S); } diff --git a/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp index 3c25dd7bd3aa2..bef2e3cca57a7 100644 --- a/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp @@ -542,11 +542,11 @@ void MacroToEnumCheck::check( return; SourceRange Range = TLDecl->getSourceRange(); - if (auto *TemplateFn = Result.Nodes.getNodeAs("top")) { - if (TemplateFn->isThisDeclarationADefinition() && TemplateFn->hasBody()) - Range = SourceRange{TemplateFn->getBeginLoc(), - TemplateFn->getUnderlyingDecl()->getBodyRBrace()}; - } + if (auto *TemplateFn = Result.Nodes.getNodeAs("top"); + TemplateFn && TemplateFn->isThisDeclarationADefinition() && + TemplateFn->hasBody()) + Range = SourceRange{TemplateFn->getBeginLoc(), + TemplateFn->getUnderlyingDecl()->getBodyRBrace()}; if (isValid(Range) && !empty(Range)) PPCallback->invalidateRange(Range); diff --git a/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp index a2a98111be3f8..c05df597a28a4 100644 --- a/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp @@ -347,10 +347,10 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag, // std::make_smart_ptr(std::initializer_list({}), 1); // std::make_smart_ptr(std::vector({1})); // std::make_smart_ptr(S2{1, 2}, 3); - if (const auto *CE = New->getConstructExpr()) { - if (HasListInitializedArgument(CE)) - return false; - } + if (const auto *CE = New->getConstructExpr(); + CE && HasListInitializedArgument(CE)) + return false; + if (ArraySizeExpr.empty()) { const SourceRange InitRange = New->getDirectInitRange(); Diag << FixItHint::CreateRemoval( @@ -406,14 +406,14 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag, // Pair. If we found any invisible or deleted copy/move constructor, we // stop generating fixes -- as the C++ rule is complicated and we are less // certain about the correct fixes. - if (const CXXRecordDecl *RD = New->getType()->getPointeeCXXRecordDecl()) { - if (llvm::any_of(RD->ctors(), [](const CXXConstructorDecl *Ctor) { - return Ctor->isCopyOrMoveConstructor() && - (Ctor->isDeleted() || Ctor->getAccess() == AS_private); - })) { - return false; - } + if (const CXXRecordDecl *RD = New->getType()->getPointeeCXXRecordDecl(); + RD && llvm::any_of(RD->ctors(), [](const CXXConstructorDecl *Ctor) { + return Ctor->isCopyOrMoveConstructor() && + (Ctor->isDeleted() || Ctor->getAccess() == AS_private); + })) { + return false; } + InitRange = SourceRange( New->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(), New->getInitializer()->getSourceRange().getEnd()); diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp index d1f0b16c26468..20166f419aa4e 100644 --- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp @@ -115,15 +115,15 @@ static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor, /// /// Stops the AST traversal if more than one usage is found. bool VisitDeclRefExpr(DeclRefExpr *D) { - if (const ParmVarDecl *To = dyn_cast(D->getDecl())) { - if (To == ParamDecl) { - ++Count; - if (Count > 1U) { - // No need to look further, used more than once. - return false; - } + if (const ParmVarDecl *To = dyn_cast(D->getDecl()); + To && To == ParamDecl) { + ++Count; + if (Count > 1U) { + // No need to look further, used more than once. + return false; } } + return true; } diff --git a/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.cpp index aefaab3e45e5c..9dd0e00f56988 100644 --- a/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ReturnBracedInitListCheck.cpp @@ -116,8 +116,8 @@ void ReturnBracedInitListCheck::check(const MatchFinder::MatchResult &Result) { // Make sure that the ctor arguments match the declaration. for (unsigned I = 0, NumParams = MatchedConstructExpr->getNumArgs(); I < NumParams; ++I) { - if (const auto *VD = dyn_cast( - MatchedConstructExpr->getConstructor()->getParamDecl(I))) { + if (const ParmVarDecl *VD = + MatchedConstructExpr->getConstructor()->getParamDecl(I)) { const auto ArgType = MatchedConstructExpr->getArg(I)->getType(); const auto ParamType = VD->getType().getNonReferenceType(); if (ArgType.getCanonicalType().getUnqualifiedType() != diff --git a/clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp index af6f108006d16..49e21e8e57ce0 100644 --- a/clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp @@ -292,10 +292,10 @@ void TypeTraitsCheck::check(const MatchFinder::MatchResult &Result) { if (!DRE->hasQualifier()) return; if (const auto *CTSD = dyn_cast_if_present( - DRE->getQualifier().getAsRecordDecl())) { - if (isNamedDeclInStdTraitsSet(CTSD, ValueTraits)) - EmitValueWarning(DRE->getQualifierLoc(), DRE->getEndLoc()); - } + DRE->getQualifier().getAsRecordDecl()); + CTSD && isNamedDeclInStdTraitsSet(CTSD, ValueTraits)) + EmitValueWarning(DRE->getQualifierLoc(), DRE->getEndLoc()); + return; } @@ -303,11 +303,11 @@ void TypeTraitsCheck::check(const MatchFinder::MatchResult &Result) { const NestedNameSpecifierLoc QualLoc = TL->getQualifierLoc(); const NestedNameSpecifier NNS = QualLoc.getNestedNameSpecifier(); if (const auto *CTSD = dyn_cast_if_present( - NNS.getAsRecordDecl())) { - if (isNamedDeclInStdTraitsSet(CTSD, TypeTraits)) - EmitTypeWarning(TL->getQualifierLoc(), TL->getEndLoc(), - TL->getElaboratedKeywordLoc()); - } + NNS.getAsRecordDecl()); + CTSD && isNamedDeclInStdTraitsSet(CTSD, TypeTraits)) + EmitTypeWarning(TL->getQualifierLoc(), TL->getEndLoc(), + TL->getElaboratedKeywordLoc()); + return; } diff --git a/clang-tools-extra/clang-tidy/modernize/UseAutoCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseAutoCheck.cpp index ca93f01e2ef9c..6c873e3f80004 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseAutoCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseAutoCheck.cpp @@ -308,14 +308,15 @@ void UseAutoCheck::replaceIterators(const DeclStmt *D, ASTContext *Context) { return; } - if (const auto *NestedConstruct = dyn_cast(E)) { + if (const auto *NestedConstruct = dyn_cast(E); + NestedConstruct && + NestedConstruct->getConstructor()->isConvertingConstructor(false)) { // If we ran into an implicit conversion constructor, can't convert. // // FIXME: The following only checks if the constructor can be used // implicitly, not if it actually was. Cases where the converting // constructor was used explicitly won't get converted. - if (NestedConstruct->getConstructor()->isConvertingConstructor(false)) - return; + return; } if (!ASTContext::hasSameType(V->getType(), E->getType())) return; diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp index 4dc78904f8bb5..78a0c6d150804 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp @@ -68,8 +68,8 @@ matchEnableIfSpecializationImplTypename(TypeLoc TheType) { if (const auto SpecializationLoc = TheType.getAs()) { - const auto *Specialization = - dyn_cast(SpecializationLoc.getTypePtr()); + const TemplateSpecializationType *Specialization = + SpecializationLoc.getTypePtr(); if (!Specialization) return std::nullopt; @@ -98,8 +98,8 @@ static std::optional matchEnableIfSpecializationImplTrait(TypeLoc TheType) { if (const auto SpecializationLoc = TheType.getAs()) { - const auto *Specialization = - dyn_cast(SpecializationLoc.getTypePtr()); + const TemplateSpecializationType *Specialization = + SpecializationLoc.getTypePtr(); if (!Specialization) return std::nullopt; @@ -186,16 +186,13 @@ matchTrailingTemplateParam(const FunctionTemplateDecl *FunctionTemplate) { LastTemplateParam->getTypeSourceInfo()->getTypeLoc()), LastTemplateParam}; } - if (const auto *LastTemplateParam = - dyn_cast(LastParam)) { - if (LastTemplateParam->hasDefaultArgument() && - LastTemplateParam->getIdentifier() == nullptr) { - return { - matchEnableIfSpecialization(LastTemplateParam->getDefaultArgument() - .getTypeSourceInfo() - ->getTypeLoc()), - LastTemplateParam}; - } + if (const auto *LastTemplateParam = dyn_cast(LastParam); + LastTemplateParam && LastTemplateParam->hasDefaultArgument() && + LastTemplateParam->getIdentifier() == nullptr) { + return {matchEnableIfSpecialization(LastTemplateParam->getDefaultArgument() + .getTypeSourceInfo() + ->getTypeLoc()), + LastTemplateParam}; } return {}; } diff --git a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp index 572b9fa225a94..872f4d359c8f6 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp @@ -78,11 +78,11 @@ static const DeclRefExpr *findFirstNonVisibleDeclRef(const Stmt *S, if (!S) return nullptr; - if (const auto *DRE = dyn_cast(S)) { - if (!isVisibleFromDefaultMemberInitializer(DRE->getDecl(), Field, SM) || - !isVisibleFromDefaultMemberInitializer(DRE->getFoundDecl(), Field, SM)) - return DRE; - } + if (const auto *DRE = dyn_cast(S); + DRE && + (!isVisibleFromDefaultMemberInitializer(DRE->getDecl(), Field, SM) || + !isVisibleFromDefaultMemberInitializer(DRE->getFoundDecl(), Field, SM))) + return DRE; for (const Stmt *Child : S->children()) if (const auto *DRE = findFirstNonVisibleDeclRef(Child, Field, SM)) diff --git a/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp index f0466852ef5c3..651fb98101ff2 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp @@ -21,10 +21,10 @@ AST_MATCHER(FunctionDecl, hasAnyDefinition) { Node.isDeleted()) return true; - if (const FunctionDecl *Definition = Node.getDefinition()) - if (Definition->hasBody() || Definition->isPureVirtual() || - Definition->isDefaulted() || Definition->isDeleted()) - return true; + if (const FunctionDecl *Definition = Node.getDefinition(); + Definition && (Definition->hasBody() || Definition->isPureVirtual() || + Definition->isDefaulted() || Definition->isDeleted())) + return true; return false; } diff --git a/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp index f1f42aac25e2a..4f561a1f10204 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp @@ -470,12 +470,11 @@ class CastSequenceVisitor : public RecursiveASTVisitor { // TypeLoc and NestedNameSpecifierLoc are members of the parent map. Skip // them and keep going up. - if (Loc.isValid()) { - if (!expandsFrom(Loc, MacroLoc)) { - Result = Parent; - return true; - } + if (Loc.isValid() && !expandsFrom(Loc, MacroLoc)) { + Result = Parent; + return true; } + Start = Parent; } diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp index d8ab0fedfc112..61335d680eace 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp @@ -69,8 +69,7 @@ struct NotLengthExprForStringNode { return true; } - if (const auto *OnNode = - dyn_cast(MemberCallNode->getImplicitObjectArgument())) { + if (const Expr *OnNode = MemberCallNode->getImplicitObjectArgument()) { return !utils::areStatementsIdentical(OnNode->IgnoreParenImpCasts(), ExprNode->IgnoreParenImpCasts(), *Context); diff --git a/clang-tools-extra/clang-tidy/objc/NSDateFormatterCheck.cpp b/clang-tools-extra/clang-tidy/objc/NSDateFormatterCheck.cpp index 7551afea1c0c6..5be5d6daca23e 100644 --- a/clang-tools-extra/clang-tidy/objc/NSDateFormatterCheck.cpp +++ b/clang-tools-extra/clang-tidy/objc/NSDateFormatterCheck.cpp @@ -48,7 +48,7 @@ static bool isValidDatePattern(StringRef Pattern) { void NSDateFormatterCheck::check(const MatchFinder::MatchResult &Result) { // Callback implementation. const auto *StrExpr = Result.Nodes.getNodeAs("str_lit"); - const StringLiteral *SL = cast(StrExpr)->getString(); + const StringLiteral *SL = StrExpr->getString(); const StringRef SR = SL->getString(); if (!isValidDatePattern(SR)) diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp index be6a4f30e610e..e74620739d94f 100644 --- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp @@ -88,11 +88,10 @@ static bool isRValueReferenceParam(const Expr *Invocation, return true; if (const auto *ConstructCallExpr = dyn_cast(Invocation)) { - if (const auto *ConstructorDecl = ConstructCallExpr->getConstructor()) { - if (!ConstructorDecl->isCopyOrMoveConstructor() && - !ConstructorDecl->isDefaultConstructor()) - return true; - } + if (const auto *ConstructorDecl = ConstructCallExpr->getConstructor(); + ConstructorDecl && !ConstructorDecl->isCopyOrMoveConstructor() && + !ConstructorDecl->isDefaultConstructor()) + return true; } } return false; diff --git a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp index e2ea0cd112a3f..e887a9862ca5f 100644 --- a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp @@ -175,10 +175,8 @@ void UseStdMoveCheck::check(const MatchFinder::MatchResult &Result) { if (!S.isReachable()) continue; auto &W = CFGState.find(&*S)->second; - if (W.Ready) { - if (--W.RemainingSuccessors == 0) - WorkList.push_back(&*S); - } + if (W.Ready && --W.RemainingSuccessors == 0) + WorkList.push_back(&*S); } } } diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp index c5fbd3022756d..2db5385052843 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp @@ -339,13 +339,12 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) { return; // Always true/false, no warnings for that. - if (Value == 0) { - if ((OpCode == BinaryOperatorKind::BO_GT && !ContainerIsLHS) || - (OpCode == BinaryOperatorKind::BO_LT && ContainerIsLHS) || - (OpCode == BinaryOperatorKind::BO_LE && !ContainerIsLHS) || - (OpCode == BinaryOperatorKind::BO_GE && ContainerIsLHS)) - return; - } + if (Value == 0 && + ((OpCode == BinaryOperatorKind::BO_GT && !ContainerIsLHS) || + (OpCode == BinaryOperatorKind::BO_LT && ContainerIsLHS) || + (OpCode == BinaryOperatorKind::BO_LE && !ContainerIsLHS) || + (OpCode == BinaryOperatorKind::BO_GE && ContainerIsLHS))) + return; // Do not warn for size > 1, 1 < size, size <= 1, 1 >= size. if (Value == 1) { diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp index 86cc399611a83..4ac23948c5e01 100644 --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -416,6 +416,9 @@ IdentifierNamingCheck::~IdentifierNamingCheck() = default; bool IdentifierNamingCheck::HungarianNotation::checkOptionValid( int StyleKindIndex) const { + if (StyleKindIndex == SK_Default) + return true; + if ((StyleKindIndex >= SK_EnumConstant) && (StyleKindIndex <= SK_ConstantParameter)) return true; @@ -643,7 +646,7 @@ StringRef IdentifierNamingCheck::HungarianNotation::getClassPrefix( !isOptionEnabled("TreatStructAsClass", HNOption.General)) return {}; - return CRD->isAbstract() ? "I" : "C"; + return CRD->hasDefinition() && CRD->isAbstract() ? "I" : "C"; } std::string IdentifierNamingCheck::HungarianNotation::getEnumPrefix( @@ -1241,9 +1244,9 @@ StyleKind IdentifierNamingCheck::findStyleKind( // C++17 structured bindings: treat each binding as if it were a variable // with the same storage and qualifiers as the parent DecompositionDecl. if (const auto *BD = dyn_cast(D)) { - if (const auto *Decomp = dyn_cast_or_null(BD->getDecomposedDecl())) - if (!BD->getType().isNull()) - return findStyleKindForVar(Decomp, BD->getType(), NamingStyles); + if (const auto *Decomp = dyn_cast_or_null(BD->getDecomposedDecl()); + Decomp && !BD->getType().isNull()) + return findStyleKindForVar(Decomp, BD->getType(), NamingStyles); return SK_Invalid; } @@ -1255,9 +1258,9 @@ StyleKind IdentifierNamingCheck::findStyleKind( // If this method has the same name as any base method, this is likely // necessary even if it's not an override. e.g. CRTP. for (const CXXBaseSpecifier &Base : Decl->getParent()->bases()) - if (const auto *RD = Base.getType()->getAsCXXRecordDecl()) - if (RD->hasMemberName(Decl->getDeclName())) - return SK_Invalid; + if (const auto *RD = Base.getType()->getAsCXXRecordDecl(); + RD && RD->hasMemberName(Decl->getDeclName())) + return SK_Invalid; if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod]) return SK_ConstexprMethod; diff --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp index acf9503f265fe..c0b11b7fc1afb 100644 --- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp @@ -178,9 +178,10 @@ bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result, // Don't warn on string user defined literals: // std::string s = "Hello World"s; - if (const auto *UDL = Parent.get()) - if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String) - return true; + if (const auto *UDL = Parent.get(); + UDL && + UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String) + return true; return false; }); diff --git a/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp b/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp index 967c63db51dd7..e33a045095147 100644 --- a/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp @@ -177,10 +177,9 @@ class FindUsageOfThis : public RecursiveASTVisitor { const auto *Parent = getParentExprIgnoreParens(E); // Look through deref of this. - if (const auto *UnOp = dyn_cast_or_null(Parent)) { - if (UnOp->getOpcode() == UO_Deref) - Parent = getParentExprIgnoreParens(UnOp); - } + if (const auto *UnOp = dyn_cast_or_null(Parent); + UnOp && UnOp->getOpcode() == UO_Deref) + Parent = getParentExprIgnoreParens(UnOp); // It's okay to // return (const S*)this; @@ -195,9 +194,9 @@ class FindUsageOfThis : public RecursiveASTVisitor { // (const T)(S->t) // (LValueToRValue)(S->t) // when 't' is either of builtin type or a public member. - } else if (const auto *Member = dyn_cast_or_null(Parent)) { - if (visitUser(Member, /*OnConstObject=*/false)) - return true; + } else if (const auto *Member = dyn_cast_or_null(Parent); + Member && visitUser(Member, /*OnConstObject=*/false)) { + return true; } // Unknown user of this. diff --git a/clang-tools-extra/clang-tidy/readability/NamedParameterCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamedParameterCheck.cpp index a7bd42e7b39f0..07d1bbeccbc16 100644 --- a/clang-tools-extra/clang-tidy/readability/NamedParameterCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/NamedParameterCheck.cpp @@ -100,9 +100,10 @@ void NamedParameterCheck::check(const MatchFinder::MatchResult &Result) { continue; // Skip gmock testing::Unused parameters. - if (const auto *Typedef = Parm->getType()->getAs()) - if (Typedef->getDecl()->getQualifiedNameAsString() == "testing::Unused") - continue; + if (const auto *Typedef = Parm->getType()->getAs(); + Typedef && + Typedef->getDecl()->getQualifiedNameAsString() == "testing::Unused") + continue; // Skip std::nullptr_t. if (Parm->getType().getCanonicalType()->isNullPtrType()) diff --git a/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp b/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp index 4cc72bb14917a..12113fa3b570a 100644 --- a/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp @@ -75,10 +75,9 @@ void NonConstParameterCheck::registerMatchers(MatchFinder *Finder) { void NonConstParameterCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *Parm = Result.Nodes.getNodeAs("Parm")) { if (const DeclContext *D = Parm->getParentFunctionOrMethod()) { - if (const auto *M = dyn_cast(D)) { - if (M->isVirtual() || M->size_overridden_methods() != 0) - return; - } + if (const auto *M = dyn_cast(D); + M && (M->isVirtual() || M->size_overridden_methods() != 0)) + return; } addParm(Parm); } else if (const auto *Ctor = @@ -283,7 +282,7 @@ void NonConstParameterCheck::markCanNotBeConst(const Expr *E, } else if (const auto *Constr = dyn_cast(E)) { for (const auto *Arg : Constr->arguments()) if (const auto *M = dyn_cast(Arg)) - markCanNotBeConst(cast(M->getSubExpr()), CanNotBeConst); + markCanNotBeConst(M->getSubExpr(), CanNotBeConst); else markCanNotBeConst(Arg, CanNotBeConst); } else if (const auto *CE = dyn_cast(E)) { diff --git a/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp index 069350599d270..9e55613f0b969 100644 --- a/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp @@ -136,18 +136,18 @@ void RedundantStringInitCheck::check(const MatchFinder::MatchResult &Result) { } if (const auto *CtorInit = Result.Nodes.getNodeAs("ctorInit")) { - if (const FieldDecl *Member = CtorInit->getMember()) { - if (!Member->hasInClassInitializer() || - Result.Nodes.getNodeAs("empty_init")) { - // The String isn't declared in the class with an initializer or its - // declared with a redundant initializer, which will be removed. Either - // way the string will be default initialized, therefore we can remove - // the constructor initializer entirely. - diag(CtorInit->getMemberLocation(), "redundant string initialization") - << FixItHint::CreateRemoval(CtorInit->getSourceRange()); - return; - } + if (const FieldDecl *Member = CtorInit->getMember(); + Member && (!Member->hasInClassInitializer() || + Result.Nodes.getNodeAs("empty_init"))) { + // The String isn't declared in the class with an initializer or its + // declared with a redundant initializer, which will be removed. Either + // way the string will be default initialized, therefore we can remove + // the constructor initializer entirely. + diag(CtorInit->getMemberLocation(), "redundant string initialization") + << FixItHint::CreateRemoval(CtorInit->getSourceRange()); + return; } + const CXXConstructExpr *Construct = getConstructExpr(*CtorInit); if (!Construct) return; diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp index cc82e052d58da..3a63b6f84eed3 100644 --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -116,17 +116,15 @@ static bool needsZeroComparison(const Expr *E) { } static bool needsStaticCast(const Expr *E) { - if (const auto *ImpCast = dyn_cast(E)) { - if (ImpCast->getCastKind() == CK_UserDefinedConversion && - ImpCast->getSubExpr()->getType()->isBooleanType()) { - if (const auto *MemCall = - dyn_cast(ImpCast->getSubExpr())) { - if (const auto *MemDecl = - dyn_cast(MemCall->getMethodDecl())) { - if (MemDecl->isExplicit()) - return true; - } - } + if (const auto *ImpCast = dyn_cast(E); + ImpCast && ImpCast->getCastKind() == CK_UserDefinedConversion && + ImpCast->getSubExpr()->getType()->isBooleanType()) { + if (const auto *MemCall = + dyn_cast(ImpCast->getSubExpr())) { + if (const auto *MemDecl = + dyn_cast(MemCall->getMethodDecl()); + MemDecl && MemDecl->isExplicit()) + return true; } } @@ -165,16 +163,15 @@ static std::string replacementExpression(const ASTContext &Context, const bool NeedsStaticCast = Context.getLangOpts().CPlusPlus && needsStaticCast(E); if (Negated) { - if (const auto *UnOp = dyn_cast(E)) { - if (UnOp->getOpcode() == UO_LNot) { - if (needsNullPtrComparison(UnOp->getSubExpr())) - return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), true); + if (const auto *UnOp = dyn_cast(E); + UnOp && UnOp->getOpcode() == UO_LNot) { + if (needsNullPtrComparison(UnOp->getSubExpr())) + return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), true); - if (needsZeroComparison(UnOp->getSubExpr())) - return compareExpressionToZero(Context, UnOp->getSubExpr(), true); + if (needsZeroComparison(UnOp->getSubExpr())) + return compareExpressionToZero(Context, UnOp->getSubExpr(), true); - return replacementExpression(Context, false, UnOp->getSubExpr()); - } + return replacementExpression(Context, false, UnOp->getSubExpr()); } if (needsNullPtrComparison(E)) @@ -190,13 +187,13 @@ static std::string replacementExpression(const ASTContext &Context, NegatedOperator = negatedOperator(BinOp); LHS = BinOp->getLHS(); RHS = BinOp->getRHS(); - } else if (const auto *OpExpr = dyn_cast(E)) { - if (OpExpr->getNumArgs() == 2) { - NegatedOperator = negatedOperator(OpExpr); - LHS = OpExpr->getArg(0); - RHS = OpExpr->getArg(1); - } + } else if (const auto *OpExpr = dyn_cast(E); + OpExpr && OpExpr->getNumArgs() == 2) { + NegatedOperator = negatedOperator(OpExpr); + LHS = OpExpr->getArg(0); + RHS = OpExpr->getArg(1); } + if (!NegatedOperator.empty() && LHS && RHS) return (asBool((getText(Context, *LHS) + " " + NegatedOperator + " " + getText(Context, *RHS)) @@ -216,14 +213,13 @@ static std::string replacementExpression(const ASTContext &Context, return ("!" + asBool(Text, NeedsStaticCast)); } - if (const auto *UnOp = dyn_cast(E)) { - if (UnOp->getOpcode() == UO_LNot) { - if (needsNullPtrComparison(UnOp->getSubExpr())) - return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), false); + if (const auto *UnOp = dyn_cast(E); + UnOp && UnOp->getOpcode() == UO_LNot) { + if (needsNullPtrComparison(UnOp->getSubExpr())) + return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), false); - if (needsZeroComparison(UnOp->getSubExpr())) - return compareExpressionToZero(Context, UnOp->getSubExpr(), false); - } + if (needsZeroComparison(UnOp->getSubExpr())) + return compareExpressionToZero(Context, UnOp->getSubExpr(), false); } if (needsNullPtrComparison(E)) @@ -421,12 +417,11 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor { const DeclAndBool ElseAssignment = checkSingleStatement(If->getElse(), VarBoolAssignmentMatcher); if (ElseAssignment.Item == ThenAssignment.Item && - ElseAssignment.Bool != ThenAssignment.Bool) { - if (Check->ChainedConditionalAssignment || - !isa_and_nonnull(parent())) { - Check->replaceWithAssignment(Context, If, Var, Loc, - ElseAssignment.Bool); - } + ElseAssignment.Bool != ThenAssignment.Bool && + (Check->ChainedConditionalAssignment || + !isa_and_nonnull(parent()))) { + Check->replaceWithAssignment(Context, If, Var, Loc, + ElseAssignment.Bool); } } } @@ -563,19 +558,20 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor { if (!isExpectedBinaryOp(SubExpr)) return Base::TraverseUnaryOperator(Op); const auto *BinaryOp = cast(SubExpr); - if (Check->SimplifyDeMorganRelaxed || - checkEitherSide( - BinaryOp, - [this](const Expr *E) { return isExpectedUnaryLNot(E); }) || - checkEitherSide( - BinaryOp, [this](const Expr *E) { return nestedDemorgan(E, 1); })) { - if (Check->reportDeMorgan(Context, Op, BinaryOp, !IsProcessing, parent(), - Parens) && - !Check->areDiagsSelfContained()) { - const llvm::SaveAndRestore RAII(IsProcessing, true); - return Base::TraverseUnaryOperator(Op); - } + if ((Check->SimplifyDeMorganRelaxed || + checkEitherSide( + BinaryOp, + [this](const Expr *E) { return isExpectedUnaryLNot(E); }) || + checkEitherSide( + BinaryOp, + [this](const Expr *E) { return nestedDemorgan(E, 1); })) && + Check->reportDeMorgan(Context, Op, BinaryOp, !IsProcessing, parent(), + Parens) && + !Check->areDiagsSelfContained()) { + const llvm::SaveAndRestore RAII(IsProcessing, true); + return Base::TraverseUnaryOperator(Op); } + return Base::TraverseUnaryOperator(Op); } @@ -851,13 +847,12 @@ flipDemorganBinaryOperator(SmallVectorImpl &Fixes, constexpr bool LogicalOpParentheses = true; if (((*OuterBO == NewOp) || (!LogicalOpParentheses && (*OuterBO == BO_LOr && NewOp == BO_LAnd))) && - Parens) { - if (!Parens->getLParen().isMacroID() && - !Parens->getRParen().isMacroID()) { - Fixes.push_back(FixItHint::CreateRemoval(Parens->getLParen())); - Fixes.push_back(FixItHint::CreateRemoval(Parens->getRParen())); - } + Parens && !Parens->getLParen().isMacroID() && + !Parens->getRParen().isMacroID()) { + Fixes.push_back(FixItHint::CreateRemoval(Parens->getLParen())); + Fixes.push_back(FixItHint::CreateRemoval(Parens->getRParen())); } + if (*OuterBO == BO_LAnd && NewOp == BO_LOr && !Parens) { Fixes.push_back(FixItHint::CreateInsertion(BinOp->getBeginLoc(), "(")); Fixes.push_back(FixItHint::CreateInsertion( diff --git a/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp b/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp index 854bd1dae9e30..49c327ad77177 100644 --- a/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp @@ -726,12 +726,11 @@ void SuspiciousCallArgumentCheck::setArgNamesAndTypes( ArgNames.push_back(Var->getName()); continue; } - if (const auto *FCall = dyn_cast(ArgExpr->getDecl())) { - if (FCall->getNameInfo().getName().isIdentifier()) { - ArgTypes.push_back(FCall->getType()); - ArgNames.push_back(FCall->getName()); - continue; - } + if (const auto *FCall = dyn_cast(ArgExpr->getDecl()); + FCall && FCall->getNameInfo().getName().isIdentifier()) { + ArgTypes.push_back(FCall->getType()); + ArgNames.push_back(FCall->getName()); + continue; } } diff --git a/clang-tools-extra/clang-tidy/rename_check.py b/clang-tools-extra/clang-tidy/rename_check.py index 6bb274f873dad..18c998e839994 100755 --- a/clang-tools-extra/clang-tidy/rename_check.py +++ b/clang-tools-extra/clang-tidy/rename_check.py @@ -80,7 +80,7 @@ def deleteMatchingLines(fileName: str, pattern: str) -> bool: def getListOfFiles(clang_tidy_path: str) -> List[str]: files = glob.glob(os.path.join(clang_tidy_path, "**"), recursive=True) files += [ - os.path.normpath(os.path.join(clang_tidy_path, "../docs/ReleaseNotes.rst")) + os.path.normpath(os.path.join(clang_tidy_path, "../docs/ReleaseNotes.md")) ] files += glob.glob( os.path.join(clang_tidy_path, "..", "test", "clang-tidy", "checkers", "**"), @@ -183,13 +183,13 @@ def add_release_notes( clang_tidy_path: str, old_check_name: str, new_check_name: str ) -> None: filename = os.path.normpath( - os.path.join(clang_tidy_path, "../docs/ReleaseNotes.rst") + os.path.join(clang_tidy_path, "../docs/ReleaseNotes.md") ) with io.open(filename, "r", encoding="utf8") as f: lines = f.readlines() - lineMatcher = re.compile("Renamed checks") - nextSectionMatcher = re.compile("Improvements to include-fixer") + lineMatcher = re.compile(r"#### Renamed checks") + nextSectionMatcher = re.compile(r"### Improvements to include-fixer") checkMatcher = re.compile("- The '(.*)") print("Updating %s..." % filename) @@ -211,30 +211,29 @@ def add_release_notes( if match_next: add_note_here = True + # When inside the Renamed checks section and we reach any + # heading, insert before it (handles empty sections). + if header_found and line.startswith("#"): + add_note_here = True + if match: header_found = True f.write(line) continue - if line.startswith("^^^^"): - f.write(line) - continue - if header_found and add_note_here: - if not line.startswith("^^^^"): - f.write( - """- The '%s' check was renamed to :doc:`%s - ` - - """ - % ( - old_check_name, - new_check_name, - new_check_name.split("-", 1)[0], - "-".join(new_check_name.split("-")[1:]), - ) + f.write( + "- The '%s' check was renamed to {doc}`%s\n" + " `\n" + "\n" + % ( + old_check_name, + new_check_name, + new_check_name.split("-", 1)[0], + "-".join(new_check_name.split("-")[1:]), ) - note_added = True + ) + note_added = True f.write(line) diff --git a/clang-tools-extra/clang-tidy/tool/check_alphabetical_order.py b/clang-tools-extra/clang-tidy/tool/check_alphabetical_order.py index 56dc0fafa31b2..ab700e5115ead 100644 --- a/clang-tools-extra/clang-tidy/tool/check_alphabetical_order.py +++ b/clang-tools-extra/clang-tidy/tool/check_alphabetical_order.py @@ -17,7 +17,7 @@ Behavior: - Sort entries in docs/clang-tidy/checks/list.md Markdown tables. -- Sort key sections in docs/ReleaseNotes.rst. +- Sort key sections in docs/ReleaseNotes.md. - Detect duplicated entries in 'Changes in existing checks'. Flags: @@ -42,10 +42,10 @@ Tuple, ) -# Matches a :doc:`label ` or :doc:`label` reference anywhere in text and +# Matches a {doc}`label ` or {doc}`label` reference anywhere in text and # captures the label. Used to sort bullet items alphabetically in ReleaseNotes # items by their label. -DOC_LABEL_RN_RE: Final = re.compile(r":doc:`(?P