Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug-format fat pointers with their metadata for better insight #93544

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented Feb 1, 2022

New iteration of #84927 (since I can no longer reopen that one).

Basically, add metadata via ptr::metadata() to debug prints of fat pointers (ie. to slices and trait objects).
Thin ptr debug format as well as all {:p} should be unadffected.

Example prints:

(0x56486780e7e3, 5)                            // pointer to a slice
0x7f0abc000d20, DynMetadata(0x564ef9f9e938))   // pointer to a trait object

This time I managed to leverage specialization instead of size_of shennanigans.

Blocked on #81513

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@vojtechkral
Copy link
Contributor Author

Man, I get the weirdest compiler errors...

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just two small nits:

library/core/src/fmt/mod.rs Outdated Show resolved Hide resolved
library/core/src/fmt/mod.rs Outdated Show resolved Hide resolved
Use ptr::metadata() fn to improve debug prints of fat pointers.

Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
   Compiling rustc_plugin_impl v0.0.0 (/checkout/compiler/rustc_plugin_impl)
   Compiling rustc_typeck v0.0.0 (/checkout/compiler/rustc_typeck)
   Compiling rustc_borrowck v0.0.0 (/checkout/compiler/rustc_borrowck)
error: internal compiler error: compiler/rustc_codegen_llvm/src/context.rs:924:21: `fn_abi_of_instance(std::ptr::metadata::<chalk_ir::GenericArg<ChalkRustInterner>>, [])` failed: the type `<chalk_ir::GenericArg<ChalkRustInterner> as std::ptr::Pointee>::Metadata` has an unknown layout
  --> /checkout/library/core/src/ptr/metadata.rs:93:1
   |
93 | pub const fn metadata<T: ?Sized>(ptr: *const T) -> <T as Pointee>::Metadata {

thread 'rustc' panicked at 'Box<dyn Any>', /rustc/28c8a34e18fc05277c81328d1bbf5ed931f4d22e/compiler/rustc_errors/src/lib.rs:1115:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.59.0-beta.5 (28c8a34e1 2022-01-27) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z macro-backtrace -Z tls-model=initial-exec -Z unstable-options -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -C debug-assertions=on -C symbol-mangling-version=v0 -C link-args=-Wl,-z,origin -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 --crate-type lib
note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack

@oli-obk
Copy link
Contributor

oli-obk commented Feb 7, 2022

r? @oli-obk

I'll look at the failure tomorrow

@rust-highfive rust-highfive assigned oli-obk and unassigned m-ou-se Feb 7, 2022
@eddyb
Copy link
Member

eddyb commented Feb 7, 2022

The bug here was fixed by #92248, you should be able to use #[cfg(not(bootstrap))] on your additions to avoid applying them to the beta compiler that doesn't have the fix yet.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 8, 2022

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned oli-obk Feb 8, 2022
impl PtrMetadataFmt for usize {
#[inline]
fn fmt_ptr(&self, ptr: *const (), f: &mut Formatter<'_>) -> Result {
write!(f, "({:p}, {})", ptr, *self)
Copy link
Member

@eddyb eddyb Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(bikeshedding) I wonder how silly something like *[_] { data: 0x..., len: 123 } would be. (and have meta instead of len in the general case, I guess?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand, what's the *[_] part?

Comment on lines +2291 to +2292
// Regular pointer / default impl
impl<T> PtrMetadataFmt for T {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to improve this in one of two ways:

  • split the () case from the default:
    • () is "guaranteed simple" (but maybe specializing on the original pointee being Sized would be stronger)
    • the default would only be hit by custom DSTs (and unreachable today? could print some indication of metadata existing but not being understood, or just fit an unreachable!() in there)
  • add a new sealed trait, and a bound for it on Pointee::Metadata (just like it requires Copy, Eq, etc. today)
    • ("sealed" as in a pub trait in a private mod, not nameable outside core - for now, anyway)
    • the new trait could just be this PR's PtrMetadataFmt for now, even if we may want to change it in the future (see below)
    • no specialization should be needed, since Pointee would be claiming it's always implemented for valid metadata types
    • whenever we get custom DSTs, it would be a nice bonus to force the user to implement such a trait, effectively making them decide how raw pointers to their custom DST should be formatted
      • this is when we'd remove the "sealing"
      • maybe it should just be a fmt::Debug bound and not let the user see the data pointer or choose how it is printed?
      • but this part is really all a bikshed for the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know exactly what you mean, in fact, I've been meaning to propose in #81513 that Pointee::Metadata should maybe be its own trait (and yeah initially sealed by all means), which could enable attaching arbitrary things to it that might be needed in the future, be it debug printing or something else.

I initially did try the fmt::Debug bound approach but I think there was issues with it, altough I don't remember off the top of my head at the moment...

Copy link
Contributor Author

@vojtechkral vojtechkral Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially did try the fmt::Debug bound approach but I think there was issues with it, altough I don't remember off the top of my head at the moment...

Right, because Debug on Pointee::Metadata gets me the ability to debug-print the metadata, but in this case I need to format the whole pointer+metadata combo somehow, ideally preserving the simple "just hex number" format for simple pointers...

Edit: But I could use a custom sealed trait, sure, I'll try that later and wil update...

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2022
@vojtechkral
Copy link
Contributor Author

FYI I have paused work on this due to stuff not being resolved in #81513

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2022
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 19, 2022
@JohnCSimon JohnCSimon added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 18, 2022
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 23, 2022
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 11, 2022
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 22, 2022
@bors
Copy link
Contributor

bors commented Jul 29, 2024

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants