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

rustc_symbol_mangling: support structural constants and &str in v0. #87194

Merged
merged 10 commits into from
Aug 26, 2021

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jul 16, 2021

This PR should unblock #85530 (except for float const generics, which AFAIK should've never worked).
(cc @tmiasko could the #85530 (comment) failures be retried with a quick crater "subset" run of this PR + changing the default to v0? Just to make sure I didn't miss anything other than the floats)

The encoding is the one suggested before in e.g. #61486 (comment), tho this PR won't by itself finish #61486, before closing that we'd likely want to move to @oli-obk's "valtrees" (i.e. #83234 and other associated work).


EDITs:

  1. switched unit/tuple/braced-with-named-fields <const-fields> prefixes from "u"/"T"/"" to "U"/"T"/"S" to avoid the ambiguity reported by @tmiasko in rustc_symbol_mangling: support structural constants and &str in v0. #87194 (comment).

  2. rustc-demangle PR: v0: demangle structural constants and &str. rustc-demangle#55

  3. RFC amendment PR: [RFC2603] Extend <const> to include str and structural constants. rfcs#3161

    • also removed the grammar changes included in that PR, from this description
  4. added tests (temporarily using my fork of rustc-demangle)


r? @michaelwoerister

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2021
@michaelwoerister
Copy link
Member

michaelwoerister commented Jul 19, 2021

Nice! Thanks for moving on that so quickly, @eddyb! For reference, here is a link to the current grammar: https://github.com/rust-lang/rfcs/blob/master/text/2603-rust-symbol-name-mangling-v0.md#syntax-of-mangled-names

@michaelwoerister
Copy link
Member

michaelwoerister commented Jul 19, 2021

And here is the relevant part of the grammar that this PR will supersede:

<const> = <type> <const-data>
        | "p" // placeholder, shown as _
        | <backref>

// The encoding of a constant depends on its type. Integers use their value,
// in base 16 (0-9a-f), not their memory representation. Negative integer
// values are preceded with "n". The bool value false is encoded as `0_`, true
// value as `1_`. The char constants are encoded using their Unicode scalar
// value.
<const-data> = ["n"] {<hex-digit>} "_"

@michaelwoerister
Copy link
Member

This looks great!

So now we just need support for this to land in rustc-demangle so that we can add test cases, right?

@tmiasko

This comment has been minimized.

@tmiasko
Copy link
Contributor

tmiasko commented Jul 21, 2021

I extended LLVM Rust demangler with support for proposed grammar. Apart from issue with u in <const-fields>, everything else seems good to me. Special casing &str to avoid printing &*".." was easy enough.

Since you are also updating the test cases here. I remember having to change the src/test/codegen/vec-shrink-panic.rs when enabling v0 by default. Is this no longer the case?

@eddyb

This comment has been minimized.

@michaelwoerister
Copy link
Member

The first and third production can start with u because of Punycode encoded identifiers.

Great catch, @tmiasko!

@tmiasko

This comment has been minimized.

@eddyb

This comment has been minimized.

@bors

This comment has been minimized.

@pcwalton
Copy link
Contributor

pcwalton commented Aug 4, 2021

What remains to be done here? Can I help move this along somehow?

@michaelwoerister
Copy link
Member

michaelwoerister commented Aug 9, 2021

If I understand correctly the sequence for making progress here is:

Does that sound right, @eddyb & @tmiasko?

EDIT: Only the first three steps are needed to merge this PR.

@eddyb
Copy link
Member Author

eddyb commented Aug 9, 2021

Apologies for not leaving a comment with the status, I've had an in-progress rustc-demangle branch (with necessary prerequsites - though I've landed some of them separately already - and the hardest parts like str demangling) for the past week or two, I've only not submitted yet because a few other things came up. I'll try to wrap it up soon.

@eddyb
Copy link
Member Author

eddyb commented Aug 10, 2021

Demangling PR up at rust-lang/rustc-demangle#55 - next step is submitting the RFC amendment, I'll try to get to that later today.

@eddyb
Copy link
Member Author

eddyb commented Aug 10, 2021

I've opened the RFC amendment PR (rust-lang/rfcs#3161 - should someone FCP it?) and just pushed a few commits with symbol-names tests.

We've already done a crater run through #85530, so I think we're only blocked on the RFC amendment (and reviews, I guess? if any are left).

@rust-log-analyzer

This comment has been minimized.

@eddyb

This comment has been minimized.

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

From my point of view this looks good to go! Maybe @oli-obk wants to have a closer look at the interactions with const-eval?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 26, 2021

@bors r=michaelwoerister,oli-obk

@bors
Copy link
Contributor

bors commented Aug 26, 2021

📌 Commit b4fcf1b has been approved by michaelwoerister,oli-obk

@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 Aug 26, 2021
@eddyb
Copy link
Member Author

eddyb commented Aug 26, 2021

@bors rollup=never

@bors
Copy link
Contributor

bors commented Aug 26, 2021

⌛ Testing commit b4fcf1b with merge 707a1c4d93cbf4ad2886d4327d8d9be3bae4a76a...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 26, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 26, 2021
@eddyb
Copy link
Member Author

eddyb commented Aug 26, 2021

Hopefully #87194 (comment) is fixed by the new commit to compiletest.

@bors r=michaelwoerister,oli-obk

@bors
Copy link
Contributor

bors commented Aug 26, 2021

📌 Commit 24526bb has been approved by michaelwoerister,oli-obk

@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 Aug 26, 2021
@bors
Copy link
Contributor

bors commented Aug 26, 2021

⌛ Testing commit 24526bb with merge ad02dc4...

@bors
Copy link
Contributor

bors commented Aug 26, 2021

☀️ Test successful - checks-actions
Approved by: michaelwoerister,oli-obk
Pushing ad02dc4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 26, 2021
@bors bors merged commit ad02dc4 into rust-lang:master Aug 26, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 26, 2021
@eddyb eddyb deleted the const-value-mangling branch August 26, 2021 22:41
@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.