rust icon indicating copy to clipboard operation
rust copied to clipboard

Add nul-terminated filename for #[track_caller]

Open Darksonn opened this issue 1 year ago • 11 comments

ACP: Add nul-terminated version of core::panic::Location::file

When using #[track_caller] you can get the filename of the caller using Location::caller().file(). We would like to utilize this in the Linux kernel to implement a Rust equivalent of the following utility:

/**
 * might_sleep - annotation for functions that can sleep
 *
 * this macro will print a stack trace if it is executed in an atomic
 * context (spinlock, irq-handler, ...). Additional sections where blocking is
 * not allowed can be annotated with non_block_start() and non_block_end()
 * pairs.
 *
 * This is a useful debugging help to be able to catch problems early and not
 * be bitten later when the calling function happens to sleep when it is not
 * supposed to.
 */
#define might_sleep() do { __might_sleep(__FILE__, __LINE__); might_resched(); } while (0)

It's essentially an assertion that crashes the kernel if a function is used in the wrong context. The filename and line number is used in the error message when it fails. Unfortunately, the __might_sleep function requires the filename to be a nul-terminated string. Thus, extend Location with a function that returns the filename as a &CStr.

Note that unlike with things like the file!() macro, it's impossible for us to do this ourselves statically. Copying the filename into another string to nul-terminate it is not a great solution because we need to create the string even if the assertion doesn't fail, as the assertion happens on the C side.

For more context, please see zulip and the Linux kernel mailing list. This is one of RfL's wanted features in core.

cc @ojeda @Urgau

Darksonn avatar Oct 17 '24 10:10 Darksonn

r? @jhpratt

rustbot has assigned @jhpratt. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot avatar Oct 17 '24 10:10 rustbot

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)

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:lint,cpp:fmt
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---
#13 2.902 Building wheels for collected packages: reuse
#13 2.903   Building wheel for reuse (pyproject.toml): started
#13 3.166   Building wheel for reuse (pyproject.toml): finished with status 'done'
#13 3.167   Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132720 sha256=026f3bb0f1aa8090b861fd0a0939cb1a782396d84c8aab7875096557d637a0f6
#13 3.167   Stored in directory: /tmp/pip-ephem-wheel-cache-crkbx4az/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#13 3.170 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#13 3.583 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
#13 3.584 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
#13 4.152 Collecting virtualenv
#13 4.152 Collecting virtualenv
#13 4.207   Downloading virtualenv-20.26.6-py3-none-any.whl (6.0 MB)
#13 4.396      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 6.0/6.0 MB 32.1 MB/s eta 0:00:00
#13 4.457 Collecting platformdirs<5,>=3.9.1
#13 4.465   Downloading platformdirs-4.3.6-py3-none-any.whl (18 kB)
#13 4.487 Collecting distlib<1,>=0.3.7
#13 4.510      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 469.0/469.0 KB 36.1 MB/s eta 0:00:00
#13 4.550 Collecting filelock<4,>=3.12.2
#13 4.558   Downloading filelock-3.16.1-py3-none-any.whl (16 kB)
#13 4.558   Downloading filelock-3.16.1-py3-none-any.whl (16 kB)
#13 4.641 Installing collected packages: distlib, platformdirs, filelock, virtualenv
#13 4.833 Successfully installed distlib-0.3.9 filelock-3.16.1 platformdirs-4.3.6 virtualenv-20.26.6
#13 DONE 4.9s

#14 [7/8] COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
#14 DONE 0.0s
---
DirectMap4k:      233408 kB
DirectMap2M:     9203712 kB
DirectMap1G:     9437184 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
downloading https://static.rust-lang.org/dist/2024-10-16/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/2024-10-16/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt
---
fmt check
Diff in /checkout/library/core/src/panic/location.rs:1:
-use crate::fmt;
-
 #[cfg(not(bootstrap))]
+use crate::fmt;
 
 /// A struct containing information about the location of a panic.
 ///
 ///
fmt error: Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/library/core/src/ascii.rs" "/checkout/library/core/src/lib.rs" "/checkout/library/core/src/tuple.rs" "/checkout/compiler/rustc_codegen_gcc/src/asm.rs" "/checkout/compiler/rustc_codegen_gcc/src/builder.rs" "/checkout/library/core/src/slice/raw.rs" "/checkout/library/core/src/slice/cmp.rs" "/checkout/library/core/src/slice/rotate.rs" "/checkout/library/core/src/slice/index.rs" "/checkout/compiler/rustc_codegen_gcc/src/back/lto.rs" "/checkout/compiler/rustc_codegen_gcc/src/back/write.rs" "/checkout/compiler/rustc_codegen_gcc/src/back/mod.rs" "/checkout/compiler/rustc_codegen_gcc/src/abi.rs" "/checkout/compiler/rustc_codegen_gcc/src/context.rs" "/checkout/compiler/rustc_codegen_gcc/src/debuginfo.rs" "/checkout/compiler/rustc_codegen_gcc/src/archive.rs" "/checkout/compiler/rustc_codegen_gcc/src/declare.rs" "/checkout/compiler/rustc_codegen_gcc/src/allocator.rs" "/checkout/compiler/rustc_codegen_gcc/src/type_.rs" "/checkout/compiler/rustc_codegen_gcc/src/coverageinfo.rs" "/checkout/compiler/rustc_codegen_gcc/src/errors.rs" "/checkout/compiler/rustc_codegen_gcc/src/attributes.rs" "/checkout/compiler/rustc_codegen_gcc/src/consts.rs" "/checkout/compiler/rustc_codegen_gcc/src/type_of.rs" "/checkout/compiler/rustc_codegen_gcc/src/lib.rs" "/checkout/compiler/rustc_codegen_gcc/src/int.rs" "/checkout/compiler/rustc_codegen_gcc/src/mono_item.rs" "/checkout/compiler/rustc_codegen_gcc/src/common.rs" "/checkout/compiler/rustc_codegen_gcc/src/base.rs" "/checkout/compiler/rustc_codegen_gcc/src/callee.rs" "/checkout/compiler/rustc_codegen_gcc/src/gcc_util.rs" "/checkout/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs" "/checkout/compiler/rustc_codegen_gcc/src/intrinsic/llvm.rs" "/checkout/compiler/rustc_codegen_gcc/src/intrinsic/mod.rs" "/checkout/library/core/src/slice/sort/shared/smallsort.rs" "/checkout/library/core/src/slice/sort/shared/pivot.rs" "/checkout/library/core/src/slice/sort/shared/mod.rs" "/checkout/library/core/src/slice/sort/select.rs" "/checkout/library/core/src/slice/sort/stable/drift.rs" "/checkout/library/core/src/slice/sort/stable/tiny.rs" "/checkout/library/core/src/slice/sort/stable/merge.rs" "/checkout/library/core/src/slice/sort/stable/quicksort.rs" "/checkout/library/core/src/slice/sort/stable/mod.rs" "/checkout/library/core/src/slice/sort/unstable/heapsort.rs" "/checkout/library/core/src/slice/sort/unstable/quicksort.rs" "/checkout/compiler/rustc_codegen_gcc/build_system/build_sysroot/lib.rs" "/checkout/library/core/src/slice/sort/unstable/mod.rs" "/checkout/library/core/src/slice/sort/mod.rs" "/checkout/library/core/src/slice/ascii.rs" "/checkout/library/core/src/slice/iter/macros.rs" "/checkout/library/core/src/slice/specialize.rs" "/checkout/library/core/src/slice/memchr.rs" "/checkout/library/core/src/slice/iter.rs" "/checkout/library/core/src/slice/mod.rs" "/checkout/library/core/src/option.rs" "/checkout/library/core/src/panic/unwind_safe.rs" "/checkout/library/core/src/panic/location.rs" "/checkout/library/core/src/panic/panic_info.rs" "/checkout/library/core/src/result.rs" "/checkout/library/core/src/panicking.rs" "/checkout/compiler/rustc_codegen_gcc/build_system/src/build.rs" "/checkout/compiler/rustc_codegen_gcc/build_system/src/prepare.rs" "/checkout/compiler/rustc_codegen_gcc/build_system/src/clone_gcc.rs" "/checkout/library/core/src/ops/mod.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 Oct 17 10:20:00 UTC 2024
  network time: Thu, 17 Oct 2024 10:20:00 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

rust-log-analyzer avatar Oct 17 '24 10:10 rust-log-analyzer

see #117431, this is a small binary size improvement if we then make use of the nul terminator (probably only worth it after the bootstrap bump)

Noratrieb avatar Oct 17 '24 10:10 Noratrieb

Yeah, getting rid of the length could reduce the size of Location. Not sure how to do that properly considering the bootstrapping complications, though.

Darksonn avatar Oct 17 '24 10:10 Darksonn

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:c32c805632780b5c1de330e3f44561b336c2efe163bc0990acb392390157a8e1d9f855d75914a239aa40c49d77f4a837247d05d2f8d46f554b98e1f46712a3e3:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---

---- [ui] tests/ui/panics/location-detail-panic-no-location-info.rs stdout ----
diff of run.stderr:

- thread 'main' panicked at <redacted>:0:0:
- note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+ thread 'main' panicked at panicked at $SRC_DIR_REAL/core/src/panic/location.rs:LL:COL:
+ filename is not nul terminated
+ thread panicked while processing panic. aborting.
+ thread panicked while processing panic. aborting.
4 


The actual run.stderr differed from the expected run.stderr.
Actual run.stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-panic-no-location-info/location-detail-panic-no-location-info.run.stderr

error: 1 errors occurred comparing run output.
status: signal: 6 (SIGABRT) (core dumped)
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-panic-no-location-info" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_BACKTRACE="0" RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-panic-no-location-info/a"
--- stderr -------------------------------
thread 'main' panicked at panicked at /checkout/library/core/src/panic/location.rs:148:13:
filename is not nul terminated
thread panicked while processing panic. aborting.
thread panicked while processing panic. aborting.
------------------------------------------


---- [ui] tests/ui/panics/location-detail-unwrap-no-file.rs stdout ----
diff of run.stderr:

- thread 'main' panicked at <redacted>:8:9:
- called `Option::unwrap()` on a `None` value
+ thread 'main' panicked at panicked at $SRC_DIR_REAL/core/src/panic/location.rs:LL:COL:
+ filename is not nul terminated
+ thread panicked while processing panic. aborting.
4 
4 


The actual run.stderr differed from the expected run.stderr.
Actual run.stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-unwrap-no-file/location-detail-unwrap-no-file.run.stderr

error: 1 errors occurred comparing run output.
status: signal: 6 (SIGABRT) (core dumped)
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-unwrap-no-file" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_BACKTRACE="0" RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-unwrap-no-file/a"
--- stderr -------------------------------
thread 'main' panicked at panicked at /checkout/library/core/src/panic/location.rs:148:13:
filename is not nul terminated
thread panicked while processing panic. aborting.
thread panicked while processing panic. aborting.
------------------------------------------


---- [ui] tests/ui/panics/location-detail-panic-no-file.rs stdout ----
diff of run.stderr:

- thread 'main' panicked at <redacted>:7:5:
- file-redacted
+ thread 'main' panicked at panicked at $SRC_DIR_REAL/core/src/panic/location.rs:LL:COL:
+ filename is not nul terminated
+ thread panicked while processing panic. aborting.
4 
4 


The actual run.stderr differed from the expected run.stderr.
Actual run.stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-panic-no-file/location-detail-panic-no-file.run.stderr

error: 1 errors occurred comparing run output.
status: signal: 6 (SIGABRT) (core dumped)
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-panic-no-file" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_BACKTRACE="0" RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panics/location-detail-panic-no-file/a"
--- stderr -------------------------------
thread 'main' panicked at panicked at /checkout/library/core/src/panic/location.rs:148:13:
filename is not nul terminated
thread panicked while processing panic. aborting.

rust-log-analyzer avatar Oct 17 '24 10:10 rust-log-analyzer

My PR implements the bootstrapping part correctly (including the place where the Location constant is created), but the whole byte allocation stuff is wrong. I recommend you copy the library part and Location creation from my PR and keep the string allocator from yours.

Noratrieb avatar Oct 17 '24 11:10 Noratrieb

Based on the above failure, it seems like my allocation stuff is also wrong? Does it get allocated in multiple places?

Darksonn avatar Oct 17 '24 11:10 Darksonn

hm, not sure. not understanding the problems with the allocation stuff is why I quit my PR, so I can't help you there sadly :3

Noratrieb avatar Oct 17 '24 11:10 Noratrieb

@Noratrieb Nevermind, looks like I was just missing a nul-terminator in the <redacted> string. Seems like the code works as-is. Is there any issue with the current bootstrapping approach?

Darksonn avatar Oct 17 '24 12:10 Darksonn

it seems correct to me. i guess it makes more sense to do the size reduction separately in the future. r? Noratrieb could you open an ACP for the new method with the implication of now always storing the extra nul? I think that would make sense here.

Noratrieb avatar Oct 17 '24 12:10 Noratrieb

This seems like a good idea!

nikomatsakis avatar Oct 17 '24 14:10 nikomatsakis

:umbrella: The latest upstream changes (presumably #134096) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Dec 10 '24 06:12 bors

Closing in favor of #135054 as this PR is outdated.

Darksonn avatar Jan 07 '25 17:01 Darksonn