Format fat pointers as (addr, meta) for better insight. #84927

vojtechkral · 2021-05-04T21:10:21Z

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 · 2021-05-04T21:10:25Z

r? @joshtriplett

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

vojtechkral · 2021-05-05T11:14:42Z

brb trying to reproduce that CI failure locally...

vojtechkral · 2021-05-07T17:01:28Z

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

vojtechkral · 2021-05-08T13:44:53Z

@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.

SimonSapin · 2021-05-08T22:07:05Z

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

rust/compiler/rustc_mir/src/monomorphize/collector.rs

Line 968 in 4bac69d

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 · 2021-05-08T22:10:11Z

@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()

dtolnay

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 · 2021-05-13T23:21:37Z

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 · 2021-05-14T08:06:34Z

@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 · 2021-05-17T18:41:01Z

@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 · 2021-06-02T22:48:21Z

@vojtechkral , any luck tracking down that ICE?

vojtechkral · 2021-06-03T13:47:29Z

@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 · 2021-06-03T14:40:15Z

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 · 2021-06-13T18:01:16Z

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 · 2021-07-04T04:36:36Z

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

vojtechkral · 2021-07-05T21:56:00Z

@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 · 2022-02-01T12:34:41Z

superseded by #93544

rust-highfive assigned joshtriplett May 4, 2021

rust-highfive added the S-waiting-on-review label May 4, 2021

This comment has been minimized.

Sign in to view

vojtechkral force-pushed the fmt-fat-ptrs branch from 23bbffe to fc7035d Compare May 5, 2021 08:44

This comment has been minimized.

Sign in to view

vojtechkral force-pushed the fmt-fat-ptrs branch from e55eddc to 76e7b1b Compare May 8, 2021 13:41

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

dtolnay reviewed May 11, 2021

View reviewed changes

bstrie added S-waiting-on-author and removed S-waiting-on-review labels Jun 2, 2021

fmt: Format fat pointers as (addr, meta) for better insight

0bca351

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.

vojtechkral force-pushed the fmt-fat-ptrs branch from 9b08851 to 0bca351 Compare June 13, 2021 17:56

vojtechkral closed this Jul 5, 2021

vojtechkral mentioned this pull request Feb 1, 2022

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

Open

dtolnay assigned dtolnay and unassigned joshtriplett Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format fat pointers as (addr, meta) for better insight. #84927

Format fat pointers as (addr, meta) for better insight. #84927

vojtechkral commented May 4, 2021 •

edited

Loading

rust-highfive commented May 4, 2021

This comment has been minimized.

This comment has been minimized.

vojtechkral commented May 5, 2021

This comment has been minimized.

vojtechkral commented May 7, 2021 •

edited

Loading

vojtechkral commented May 8, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented May 8, 2021

vojtechkral commented May 8, 2021 •

edited

Loading

dtolnay left a comment

yaahc commented May 13, 2021 •

edited

Loading

vojtechkral commented May 14, 2021

yaahc commented May 17, 2021 •

edited

Loading

bstrie commented Jun 2, 2021

vojtechkral commented Jun 3, 2021

m-ou-se commented Jun 3, 2021

rust-log-analyzer commented Jun 13, 2021

crlf0710 commented Jul 4, 2021

vojtechkral commented Jul 5, 2021 •

edited

Loading

vojtechkral commented Feb 1, 2022

Format fat pointers as (addr, meta) for better insight. #84927

Format fat pointers as (addr, meta) for better insight. #84927

Conversation

vojtechkral commented May 4, 2021 • edited Loading

rust-highfive commented May 4, 2021

This comment has been minimized.

This comment has been minimized.

vojtechkral commented May 5, 2021

This comment has been minimized.

vojtechkral commented May 7, 2021 • edited Loading

vojtechkral commented May 8, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented May 8, 2021

vojtechkral commented May 8, 2021 • edited Loading

dtolnay left a comment

Choose a reason for hiding this comment

yaahc commented May 13, 2021 • edited Loading

vojtechkral commented May 14, 2021

yaahc commented May 17, 2021 • edited Loading

bstrie commented Jun 2, 2021

vojtechkral commented Jun 3, 2021

m-ou-se commented Jun 3, 2021

rust-log-analyzer commented Jun 13, 2021

crlf0710 commented Jul 4, 2021

vojtechkral commented Jul 5, 2021 • edited Loading

vojtechkral commented Feb 1, 2022

vojtechkral commented May 4, 2021 •

edited

Loading

vojtechkral commented May 7, 2021 •

edited

Loading

vojtechkral commented May 8, 2021 •

edited

Loading

vojtechkral commented May 8, 2021 •

edited

Loading

yaahc commented May 13, 2021 •

edited

Loading

yaahc commented May 17, 2021 •

edited

Loading

vojtechkral commented Jul 5, 2021 •

edited

Loading