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

Deduplicate pretty printing of constants #67133

Merged
merged 18 commits into from
Mar 16, 2020
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 8, 2019

r? @eddyb for the pretty printing logic
cc @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2019
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@@ -1425,6 +1492,19 @@ impl<F: fmt::Write> PrettyPrinter<'tcx> for FmtPrinter<'_, 'tcx, F> {
ty::ReClosureBound(_) => true,
}
}

fn pretty_print_const_pointer(
Copy link
Member

Choose a reason for hiding this comment

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

So is this used both for MIR dumps and something user-visible? Or why do we only sometimes print the AllocId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the pretty printer is used for both MIR dumps and error messages, and the "show alloc id" flag is only set by MIR dumps

Copy link
Member

Choose a reason for hiding this comment

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

But there's also something it is not used for and that goes through the other pretty_print_const_pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the SymbolPrinter and the AbsolutePathPrinter. The latter is used for type_name, which we probably don't want to make users see the alloc ids.

SymbolPrinter... May actually need the ids. cc @eddyb

Copy link
Member

Choose a reason for hiding this comment

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

SymbolPrinter... May actually need the ids. cc @eddyb

Which one? legacy symbols don't matter much, the hash contains all of the information, they should just be consistent across crates. You don't have to show the const's value if you can't do it consistently cross-crate.

v0 ones are trickier, and I feel like the push for #[structural_match] in const generics is going to force us to implement some sort of general ADT representation for constants in v0 symbols, sooner than later.
(it'd likely be similar to how all values of #[structural_match] types can technically be turned into some sort of pattern)

cc @michaelwoerister @varkor @yodaldevoid

Copy link
Member

Choose a reason for hiding this comment

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

For reference, the dedicated issue for the last comment is #61486.

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 could set it to print just integer types for now and bug! out for other types so we don't silently introduce bugs

src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
Comment on lines 993 to 1003
(Scalar::Raw { data: 0, .. }, ty::RawPtr(_)) => p!(write("{{null pointer}}")),
// This is UB, but we still print it
(Scalar::Raw { data: 0, ..}, ty::Ref(_, ty, _))
=> p!(write("{{null reference to "), print(ty), write("}}")),
(Scalar::Raw { data, .. }, ty::Ref(..)) |
(Scalar::Raw { data, .. }, ty::RawPtr(_)) => {
let pointer_width = self.tcx().data_layout.pointer_size.bytes();
p!(write("0x{:01$x}", data, pointer_width as usize * 2))
},
(Scalar::Ptr(p), ty::Ref(..)) |
(Scalar::Ptr(p), ty::RawPtr(_)) => self = self.pretty_print_const_pointer(p)?,
Copy link
Member

Choose a reason for hiding this comment

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

So here we (mostly) do not print types. I am fine with that, I think, but it seems somewhat inconsistent as we are otherwise so careful to have type information (e.g. with the integer types).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is inconsistent, it just seemed very noisy considering that there are no size differences that can be relevant like with the integer types.

src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@oli-obk oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2020
@eddyb
Copy link
Member

eddyb commented Mar 11, 2020

I can't easily check old comments, but I think #67133 (comment) and #67133 (comment) are the only remaining ones, and after that you can r=me.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 13, 2020

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Mar 13, 2020

📌 Commit 6ca65bd has been approved by eddyb

@bors
Copy link
Contributor

bors commented Mar 13, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2020
bors added a commit that referenced this pull request Mar 13, 2020
Add support for LLVM globals corresponding to miri allocations should be named alloc123

Adds support for this request from @eddyb in #69134:

> That is, if -Zfewer-names is false (usually only because of --emit=llvm-ir), we should use the same name for LLVM globals we generate out of miri allocs as #67133 does in MIR output (allocN).
>
>This way, we can easily see the mapping between MIR and LLVM IR (and it shouldn't be any costlier for regular compilation, which would continue to use unnamed globals).

r? @eddyb
cc @oli-obk
@bors
Copy link
Contributor

bors commented Mar 16, 2020

⌛ Testing commit 6ca65bd with merge dd67187...

@bors
Copy link
Contributor

bors commented Mar 16, 2020

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing dd67187 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2020
@bors bors merged commit dd67187 into rust-lang:master Mar 16, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #67133!

Tested on commit dd67187.
Direct link to PR: #67133

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Mar 16, 2020
Tested on commit rust-lang/rust@dd67187.
Direct link to PR: <rust-lang/rust#67133>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
@oli-obk oli-obk deleted the it_must_be_a_sign branch March 16, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants