Skip to content

Conversation

vojtechkral
Copy link
Contributor

@vojtechkral vojtechkral commented May 4, 2021

Now that we have ptr::metadata() I thought it would be nice to be able to see the meta part of fat ptrs in debug prints.
In past I'd run into a couple of situations where I needed to see the second part as well and this is curretly kind of cumbersome / requires unsafe transmutes IIRC Actually, with metadata() this is a lot easier now, but still, having this in Debug is IMO better...

cc #81513 - I've added the Debug constrain to Pointee::Metadata (impls already implement it).

Not sure if there's a backcompat concern?
I really do hope people don't rely on debug prints of pointers, but one never knows...

Let me know what you think 🙂

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(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 May 4, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@vojtechkral
Copy link
Contributor Author

brb trying to reproduce that CI failure locally...

@rust-log-analyzer

This comment has been minimized.

@vojtechkral
Copy link
Contributor Author

vojtechkral commented May 7, 2021

huh. I guess that's what I get for messing with lang items...

@vojtechkral
Copy link
Contributor Author

vojtechkral commented May 8, 2021

@SimonSapin can I bug you with help with this one? I must've upset the metadata/Pointee machinery somehow but I can't figure out what the problem is...

EDIT: Ok my best guess is when compiling the fmt code the relevant metadata types are not yet fully resolved.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@vojtechkral vojtechkral force-pushed the fmt-fat-ptrs branch 2 times, most recently from 9d04180 to 9b08851 Compare May 8, 2021 21:41
@rust-log-analyzer

This comment has been minimized.

@SimonSapin
Copy link
Contributor

Hmm I don’t know what to do about this ICE. I don’t find the Option::unwrap call near the given line number:

if type_has_metadata(inner_source) {

Can you reproduce with RUST_BACKTRACE=1?


Implementation bugs aside, @rust-lang/libs what do you think of this change? In particular:

  • The compatibility of changing the output of Debug for raw pointers
  • Adding a new bound to the Pointee::Metadata associated type

@vojtechkral
Copy link
Contributor Author

vojtechkral commented May 8, 2021

@SimonSapin I was unable to reproduce the ICE locally. But I found the line at the commit the beta compiler is based off and it's in create_mono_items_for_vtable_methods():

ty::Instance::resolve_for_vtable(
    tcx,
    ty::ParamEnv::reveal_all(),
    def_id,
    substs,
)
.unwrap()

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

what do you think of this change? In particular:

  • The compatibility of changing the output of Debug for raw pointers
  • Adding a new bound to the Pointee::Metadata associated type

I am on board with these.

I would consider a format like:

{0x0000560e5f2fe005, metadata: 4}

which gives some better guidance what the person is even looking at when seeing this in debug output, versus the current (0x0000560e5f2fe005, 4).

@yaahc
Copy link
Member

yaahc commented May 13, 2021

Implementation bugs aside, @rust-lang/libs what do you think of this change?

I love it

In particular:

* The compatibility of changing the output of `Debug` for raw pointers

I see no problems with it, given our stability policy for Debug.

* Adding a new bound to the `Pointee::Metadata` associated type

I also see no problems here. Since this is still an unstable API and like @vojtechkral said the existing types already all implemented Debug, seems likely that any custom Metadata types would already implement it too and even if they don't, adding a Debug impl isn't an onerous ask for nightly users.

I would consider a format like: {0x0000560e5f2fe005, metadata: 4}

seconding this

@vojtechkral
Copy link
Contributor Author

@yaahc I've also changed fmt::Pointer output, since Debug just delegates to that. Not sure if that one has more compat guarantees or not. They could be split if need be I suppose.

@yaahc
Copy link
Member

yaahc commented May 17, 2021

@yaahc I've also changed fmt::Pointer output, since Debug just delegates to that. Not sure if that one has more compat guarantees or not. They could be split if need be I suppose.

That should be fine, as far as I know that Debug stability policy applies to all types in the language and standard libraries.

@bstrie bstrie 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 Jun 2, 2021
@bstrie
Copy link
Contributor

bstrie commented Jun 2, 2021

@vojtechkral , any luck tracking down that ICE?

@vojtechkral
Copy link
Contributor Author

@bstrie No and honestly I haven't worked on this PR in a while. But I plan to come back to it an re-try 🙂

@m-ou-se
Copy link
Member

m-ou-se commented Jun 3, 2021

Changing Debug seems fine to me. But I'm not sure we can (or should) change Pointer.

Also, with the current implementation in this PR, {:#p} would enable the # flag on the debug_tuple formatting, which adds newlines and indentation. The # flag already has a different meaning for p: enable zero padding.

Use the new ptr::metadata() fn to improve debug prints of fat pointers.
Pointee::Metadata gets an additional Debug bound, impls already implemented it anyway.
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_codegen_llvm v0.0.0 (/checkout/compiler/rustc_codegen_llvm)
   Compiling rustc_ty_utils v0.0.0 (/checkout/compiler/rustc_ty_utils)
   Compiling rustc_lint v0.0.0 (/checkout/compiler/rustc_lint)
   Compiling rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', compiler/rustc_mir/src/monomorphize/collector.rs:1049:22

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.
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.53.0-beta.3 (82b862164 2021-05-22) 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 overflow-checks=off -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:
query stack during panic:
#0 [collect_and_partition_mono_items] collect_and_partition_mono_items
error: could not compile `rustc_traits`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...

@crlf0710
Copy link
Member

crlf0710 commented Jul 4, 2021

@vojtechkral Ping from triage, CI is still red, mind fixing it?

@vojtechkral
Copy link
Contributor Author

vojtechkral commented Jul 5, 2021

@crlf0710 Sorry, I'm not doing great in the free time departement at the moment :-(
I was also having a hard time reproducing & debugging that failure locally.
In any case, this PR isn't mergable as is, there's feedback by Mara to be addressed / code to be refactored.
I'll close this for now and reopen when I'm able to make progress on this again if that's ok...

@vojtechkral
Copy link
Contributor Author

superseded by #93544

@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants