Should -Cforce-frame-pointers favor the target or CLI?

[workingjubilee]

This PR exists to essentially document that we (that is, the compiler reviewer) implicitly made a decision in #86652 that defies the expectations of some programmers. Some programmers believe -Cforce-frame-pointers=false should obey the programmer in all cases, removing all frame pointers in the current session. I am forced to note that "remove frame pointers" is actually an optimization in terms of register allocation and the like, thus one that depends on the compiler to implement, but that's part of the problem:

• It can be argued that our CLI contract should be that we faithfully make this request of the codegen backend, without picking a different behavior, even if we know the target requires that different behavior.
• It can also be argued that it is a requirement of the ABI of some targets, and that we should prefer to obey that ABI.

All of this is further complicated by the simple fact that accurate backtraces and profiling can require these to get correct addresses. The included test includes some commentary where I try to enumerate all of the various problems I could find. A common bug seems to be our backtrace implementations stumbling into infinite loops due to a system API or unwinding info no longer behaving as expected.

Obviously, I personally believe the decision was correct, but it was a choice that some people can be surprised or even upset about. As it changed a highly user-facing behavior in ways that might break with user expectations, it perhaps should have a team decision behind it with an FCP and all that. Thus I submit this for review and discussion by T-compiler so we can figure out what we actually want, here, as it will become particularly relevant to have a decision about if rust-lang/compiler-team#872 is accepted.

The code itself has a TODO and thus is not fit for merging yet, indeed it really includes no changes at all, though it is precisely this TODO that one might wish to read.

r? @ghost

[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)

info: removing rustup binaries
info: rustup is uninstalled
##[group]Image checksum input
mingw-check-tidy
# We use the ghcr base image because ghcr doesn't have a rate limit
# and the mingw-check-tidy job doesn't cache docker images in CI.
FROM ghcr.io/rust-lang/ubuntu:22.04

ARG DEBIAN_FRONTEND=noninteractive
RUN apt-get update && apt-get install -y --no-install-recommends \
  g++ \
---

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.
# 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,cpp
#
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---
#12 2.895 Building wheels for collected packages: reuse
#12 2.897   Building wheel for reuse (pyproject.toml): started
#12 3.110   Building wheel for reuse (pyproject.toml): finished with status 'done'
#12 3.111   Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132719 sha256=d2a2565e7037ad3883fb9337653f2e25bbb588534fbef3697286cbc26d1bf634
#12 3.111   Stored in directory: /tmp/pip-ephem-wheel-cache-_uus6bow/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#12 3.113 Successfully built reuse
#12 3.114 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#12 3.519 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#12 3.519 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
#12 4.066 Collecting virtualenv
#12 4.121   Downloading virtualenv-20.31.1-py3-none-any.whl (6.1 MB)
#12 4.353      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 6.1/6.1 MB 26.5 MB/s eta 0:00:00
#12 4.420 Collecting filelock<4,>=3.12.2
#12 4.428   Downloading filelock-3.18.0-py3-none-any.whl (16 kB)
#12 4.465 Collecting platformdirs<5,>=3.9.1
#12 4.473   Downloading platformdirs-4.3.7-py3-none-any.whl (18 kB)
#12 4.495 Collecting distlib<1,>=0.3.7
#12 4.502   Downloading distlib-0.3.9-py2.py3-none-any.whl (468 kB)
#12 4.514      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 469.0/469.0 KB 47.8 MB/s eta 0:00:00
#12 4.596 Installing collected packages: distlib, platformdirs, filelock, virtualenv
#12 4.790 Successfully installed distlib-0.3.9 filelock-3.18.0 platformdirs-4.3.7 virtualenv-20.31.1
#12 4.790 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
#12 DONE 4.9s

#13 [7/8] COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
#13 DONE 0.0s
---
DirectMap4k:      157632 kB
DirectMap2M:     8230912 kB
DirectMap1G:    10485760 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py,cpp
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py,cpp
##[group]Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.05s
##[endgroup]
WARN: currently no CI rustc builds have rustc debug assertions enabled. Please either set `rust.debug-assertions` to `false` if you want to use download CI rustc or set `rust.download-rustc` to `false`.
WARN: `rust.debug-assertions = true` will prevent downloading CI rustc as alt CI rustc is not currently built with debug assertions.
---
fmt check
fmt: checked 6007 files
tidy check
tidy: Skipping binary file check, read-only filesystem
##[error]tidy error: /checkout/tests/codegen/frame-pointer-cli-control.rs:20: line longer than 100 chars
##[error]tidy error: /checkout/tests/codegen/frame-pointer-cli-control.rs:22: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/tests/codegen/frame-pointer-cli-control.rs:30: line longer than 100 chars
##[error]tidy error: /checkout/tests/codegen/frame-pointer-cli-control.rs:48: line longer than 100 chars
##[error]tidy error: /checkout/tests/codegen/frame-pointer-cli-control.rs:50: line longer than 100 chars
##[error]tidy error: /checkout/tests/codegen/frame-pointer-cli-control.rs:54: line longer than 100 chars
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'venv'
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'virtualenv'
Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (25.1.1)
linting python files
All checks passed!
checking python file formatting
26 files already formatted
checking C++ file formatting
some tidy checks failed
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:47
  local time: Wed May  7 21:57:32 UTC 2025
  network time: Wed, 07 May 2025 21:57:32 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bjorn3]

The current behavior of being a ratchet makes most sense to me. Enabling force-frame-pointers forces frame pointers to be used, disabling it is equivalent to not passing it at all and causes the default to be used. This matches the behavior of force-unwind-tables (for which the current behavior is required. disabling unwind tables when they are enabled by default is unsound and picking the target default even when force-unwind-tables is enabled makes the cli option useless)

[Noratrieb]

I agree and think the naming conveys this as well. The absence of force should not force the opposite, it just leaves it up to the compiler to choose.

In fact, this is already clearly documented: https://doc.rust-lang.org/rustc/codegen-options/index.html?highlight=force-frame#force-frame-pointers

n, no, off or false: do not force-enable frame pointers. This does not necessarily mean frame pointers will be removed.

And this documentation has been there basically forever: #65136

[workingjubilee]

Yes. This is essentially me looking for a clarification that yes, we intended to head in this direction, to make sure I have something authoritative to point to if it comes up again and help inform any near-future decisions about frame-pointer-related things.