Implement `-C link-self-contained=(+/-)sanitizers`

[chriswailes]

This PR adds support for enabling/disabling self-contained sanitizer runtimes.

This is an alternative to https://github.com/rust-lang/rust/pull/121207.

r? @michaelwoerister

[rust-log-analyzer]

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:b4ffde65f46336ab88eb53be808477a3936bae11)
Download action repository 'actions/upload-artifact@v3' (SHA:a8a3f3ad30e3422c9c7b888a15615d19a852ae32)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=chriswailes
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_aa7a942f-9eff-4ab4-87dc-c8885fadda11
GITHUB_EVENT_NAME=pull_request
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=e5ac3e1d7749f3bbe94de4655c4ca6bdc542fbd7
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_aa7a942f-9eff-4ab4-87dc-c8885fadda11
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_aa7a942f-9eff-4ab4-87dc-c8885fadda11
GITHUB_TRIGGERING_ACTOR=chriswailes
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/121420/merge
GITHUB_WORKFLOW_SHA=e5ac3e1d7749f3bbe94de4655c4ca6bdc542fbd7
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
COPY scripts/sccache.sh /scripts/
RUN sh /scripts/sccache.sh

COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt \
    && pip3 install virtualenv
COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/

# NOTE: intentionally uses python2 for x.py so we can test it still works.
# NOTE: intentionally uses python2 for x.py so we can test it still works.
# validate-toolstate only runs in our CI, so it's ok for it to only support python3.
ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test \
           --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---

#10 [5/8] COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
#10 DONE 0.0s

#11 [6/8] RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt     && pip3 install virtualenv
#11 0.783   Downloading binaryornot-0.4.4-py2.py3-none-any.whl (9.0 kB)
#11 0.799 Collecting boolean-py==4.0
#11 0.806   Downloading boolean.py-4.0-py3-none-any.whl (25 kB)
#11 0.824 Collecting chardet==5.1.0
---
#11 3.983 Building wheels for collected packages: reuse
#11 3.983   Building wheel for reuse (pyproject.toml): started
#11 4.312   Building wheel for reuse (pyproject.toml): finished with status 'done'
#11 4.313   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#11 4.314   Stored in directory: /tmp/pip-ephem-wheel-cache-tui4n63f/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#11 4.316 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#11 4.338   Attempting uninstall: setuptools
#11 4.338     Found existing installation: setuptools 59.6.0
#11 4.339     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
#11 4.339     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
#11 4.340     Can't uninstall 'setuptools'. No files were found to uninstall.
#11 5.012 Successfully installed binaryornot-0.4.4 boolean-py-4.0 chardet-5.1.0 jinja2-3.1.2 license-expression-30.0.0 markupsafe-2.1.1 python-debian-0.1.49 reuse-1.1.0 setuptools-66.0.0
#11 5.012 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#11 5.526 Collecting virtualenv
#11 5.573   Downloading virtualenv-20.25.1-py3-none-any.whl (3.8 MB)
#11 5.713      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.8/3.8 MB 27.6 MB/s eta 0:00:00
#11 5.763 Collecting platformdirs<5,>=3.9.1
#11 5.771   Downloading platformdirs-4.2.0-py3-none-any.whl (17 kB)
#11 5.791 Collecting distlib<1,>=0.3.7
#11 5.814   Downloading distlib-0.3.8-py2.py3-none-any.whl (468 kB)
#11 5.857 Collecting filelock<4,>=3.12.2
#11 5.864   Downloading filelock-3.13.1-py3-none-any.whl (11 kB)
#11 5.864   Downloading filelock-3.13.1-py3-none-any.whl (11 kB)
#11 5.948 Installing collected packages: distlib, platformdirs, filelock, virtualenv
#11 6.111 Successfully installed distlib-0.3.8 filelock-3.13.1 platformdirs-4.2.0 virtualenv-20.25.1
#11 DONE 6.2s

#12 [7/8] COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
#12 DONE 0.0s
---
DirectMap4k:      194496 kB
DirectMap2M:     8194048 kB
DirectMap1G:    10485760 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
    Finished dev [unoptimized] target(s) in 0.03s
##[endgroup]
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/f62f490fd410c06031a57915b4e5580ccbd7a30f/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/llvm-f62f490fd410c06031a57915b4e5580ccbd7a30f-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 28.23s
##[endgroup]
fmt check
##[error]Diff in /checkout/compiler/rustc_codegen_ssa/src/back/link.rs at line 1217:
     }
 
 
-    if matches!(crate_type, CrateType::Dylib | CrateType::Cdylib | CrateType::ProcMacro) &&
-        (sess.target.is_like_osx || sess.target.is_like_msvc) {
-
+    if matches!(crate_type, CrateType::Dylib | CrateType::Cdylib | CrateType::ProcMacro)
+        && (sess.target.is_like_osx || sess.target.is_like_msvc)
         return;
     }
 
 
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_codegen_ssa/src/back/link.rs" "/checkout/compiler/rustc_codegen_ssa/src/back/archive.rs" "/checkout/compiler/rustc_errors/src/markdown/parse.rs" "/checkout/compiler/rustc_errors/src/markdown/term.rs" "/checkout/compiler/rustc_codegen_ssa/src/back/rpath/tests.rs" "/checkout/compiler/rustc_codegen_ssa/src/back/lto.rs" "/checkout/compiler/rustc_codegen_ssa/src/back/mod.rs" "/checkout/compiler/rustc_codegen_ssa/src/back/write.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
  local time: Thu Feb 22 19:38:09 UTC 2024
  network time: Thu, 22 Feb 2024 19:38:09 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=chriswailes
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_410ed8ff-e7bc-4f71-a72b-97aa7bb4b4b0
GITHUB_EVENT_NAME=pull_request
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=7a6366528eab265eb05785a773f63a2eefaeef35
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_410ed8ff-e7bc-4f71-a72b-97aa7bb4b4b0
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_410ed8ff-e7bc-4f71-a72b-97aa7bb4b4b0
GITHUB_TRIGGERING_ACTOR=chriswailes
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/121420/merge
GITHUB_WORKFLOW_SHA=7a6366528eab265eb05785a773f63a2eefaeef35
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
#13 3.609 Building wheels for collected packages: reuse
#13 3.610   Building wheel for reuse (pyproject.toml): started
#13 3.937   Building wheel for reuse (pyproject.toml): finished with status 'done'
#13 3.938   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#13 3.938   Stored in directory: /tmp/pip-ephem-wheel-cache-f4tgvw_e/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#13 3.941 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#13 3.963   Attempting uninstall: setuptools
#13 3.963     Found existing installation: setuptools 59.6.0
#13 3.964     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
    Checking rustc_transmute v0.0.0 (/checkout/compiler/rustc_transmute)
error: no rules expected the token `crate_type`
    --> compiler/rustc_codegen_ssa/src/back/link.rs:1216:46
     |
1216 |     if matches!(crate_type, CrateType::Rlib, crate_type == CrateType::StaticLib) {
     |                                              ^^^^^^^^^^ no rules expected this token in macro call
     = note: while trying to match sequence end

    Checking rustc_trait_selection v0.0.0 (/checkout/compiler/rustc_trait_selection)
error: could not compile `rustc_codegen_ssa` (lib) due to 1 previous error

[michaelwoerister]

This looks good to me. Thanks, @chriswailes!

@lqd, I think I'm a bit confused by the self_contained_components function. It seems to take the CLI arguments into account, but only if it is yes or no. Otherwise it will return the target defaults. Would it make sense for it to merge target defaults and the CLI options at that point?

[chriswailes]

Is there anything more you'd like me to do for this change?

[petrochenkov]

I didn't have a chance to look at the changes yet, I'll do it later this week, most likely.

[michaelwoerister]

I think we mainly need to clarify how this interacts with the defaults specified by the target (I'm not sure these are always "defaults", some parts of the code seem to treat that as restrictions on what options are allowed) -- or decide that we don't need to do this now because the feature isn't stable yet. In both cases, I don't want to approve this without feedback from @petrochenkov or @lqd, who have more ownership of the link-self-contained feature.

[lqd]

@lqd, I think I'm a bit confused by the self_contained_components function. It seems to take the CLI arguments into account, but only if it is yes or no. Otherwise it will return the target defaults. Would it make sense for it to merge target defaults and the CLI options at that point?

@michaelwoerister yeah that could eventually be part of the whole story

• the support for self-contained linking components is not finished of course, and I only implemented the linker component of Vadim's vision (and am not familiar with the other components). In this function here, we had a unique yes/no value (which still flows downstream e.g. in get_linker, and all this could need to handle components) so it mostly looks the same as the old -> bool one in order to limit regressions and keep the existing -Clink-self-contained=y stable behavior as-is. I feared our test coverage could be spotty for such uncommon feature and target combinations. I did break some targets in the process of implementing parts of this.
• I believe merging the CLI and target options is a small part of whether a component is actually enabled, which can also be very different per component. (Here's the self-contained linking linker component as an example, and this PR shows the sanitizers checks for comparison), so I'm not yet sure if it would help a lot. Once we have more components implemented, we could maybe pull out some of the common code between them at the cost of spreading their implementations around more.

Here are a couple questions I had reading this PR:

• do we expect -Clink-self-contained=+sanitizers with sess.target.link_self_contained.is_disabled() to throw an error about the target preventing self-contained linking?
• -Clink-self-contained=+sanitizers is a no-op here, right? Only -Clink-self-contained=-sanitizers bails out of adding the libraries, i.e. do we want to bail when !sess.opts.cg.link_self_contained.is_sanitizers_enabled()? And that begs the question, what is +sanitizers supposed to do without -Zsanitizer, and do we need some consistency checks between the two?
• do we want -Zsanitizer to imply -Clink-self-contained=+sanitizers, or to always require using both? Or similarly, whether we eventually want to check if the sanitizers component was enabled, at the various points in codegen where we take the active sanitizers into account?

I think we mainly need to clarify how this interacts with the defaults specified by the target (I'm not sure these are always "defaults", some parts of the code seem to treat that as restrictions on what options are allowed)

Exactly. I know this PR is only about CLI support, but some similar questions could be discussed about the target specs, e.g. they could enable the sanitizer component but I don't believe there's an easy -Zsanitizer equivalent today, what then? Tbh I'm not sure a target would want to require sanitizers (except maybe CFI), but I wonder if we generally want to have symmetry between the CLI and target specs. So in that sense, it seems fine to me to not take into account the target defaults yet, but I'd also like (probably from Vadim) opinions about what we expect from -Clink-self-contained=+sanitizers, and if/how it should impact -Zsanitizer?

[petrochenkov]

So it looks like -Zsanitizers does 2 things

• affects codegen
• affects linking (adds some libraries)

And the effect of both https://github.com/rust-lang/rust/pull/121420 and this PR is that the linking part is skipped entirely.

If the linking part is just skipped, then it doesn't look like -Clink-self-contained. With that option

• -Clink-self-contained=+sanitizers would pass either my-rust-toolchain/subdir/with/sanitizers/librustc_rt.msan.a (like it happens now by default) or something like -L my-rust-toolchain/subdir/with/sanitizers -l rustc_rt.msan to the linker.
• -Clink-self-contained=-sanitizers on the other hand would pass -l clang_rt.msan, so the sanitizer library is found somewhere on the system (the user may adjust where exactly by also passing -L options to the linker).

@chriswailes Do you plan to pass the sanitizer libraries manually in your scenario? By full path, by name? Will -Clink-self-contained=-sanitizers as described above work for you?

(Also is there some convergence on the names of sanitizer libraries, or they can be named differently?)

[chriswailes]

Do you plan to pass the sanitizer libraries manually in your scenario?

Yes, we link the libraries in explicitly with absolute paths.

-Clink-self-contained=-sanitizers on the other hand would pass -l clang_rt.msan, so the sanitizer library is found somewhere on the system (the user may adjust where exactly by also passing -L options to the linker).

The Android build system uses absolute paths in all possible cases when passing arguments to compilers. While adjusting the search path to find our desired libclang_rt.msan is possible, we'd prefer that no linker flag for the runtime was emitted and satisfying the requirement was left to whomever or whatever invoked the compiler.

(Also is there some convergence on the names of sanitizer libraries, or they can be named differently?)

I'm not sure if the naming of the sanitizer libraries is stabilized and being able to link in different sanitizer runtimes might be helpful to researchers and developers.

[petrochenkov]

It looks like this use-case is different from -Clink-self-contained after all.

Maybe something like -Zsanitizers=msan,nolink or a separate option like https://github.com/rust-lang/rust/pull/121207 would be more suitable.

[michaelwoerister]

Alright, given the above discussion I'll approve https://github.com/rust-lang/rust/pull/121207 and close this PR.