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

mangling_v0: Skip extern blocks during mangling #92316

Merged
merged 3 commits into from
Jan 20, 2022

Conversation

petrochenkov
Copy link
Contributor

There's no need to include the dummy Nt into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to #92032

(There's also a drive-by fix to the rust-demangler tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 27, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2021
@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov 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 Dec 28, 2021
@petrochenkov petrochenkov 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 Dec 28, 2021
@Mark-Simulacrum
Copy link
Member

Hm, let's r? @wesleywiser perhaps? I'm not sure what our stability implications for v0 mangling are, though in general this seems fine to me.

@petrochenkov
Copy link
Contributor Author

The mangling format itself doesn't seem to change, we should only drop some empty segments from it.

@wesleywiser
Copy link
Member

wesleywiser commented Jan 7, 2022

Yeah, this seems fine. I can't think of any reason we would want or need to have an empty segment for the extern block in the mangled name (or that there would be stability implications from this change).

@bors r+

@bors
Copy link
Contributor

bors commented Jan 7, 2022

📌 Commit 6bbbaf9 has been approved by wesleywiser

@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 Jan 7, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2022
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
@ehuss
Copy link
Contributor

ehuss commented Jan 8, 2022

@bors r-

#92659 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 8, 2022
@petrochenkov
Copy link
Contributor Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jan 8, 2022

📌 Commit 333a5cc has been approved by wesleywiser

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 9, 2022
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2022
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2022
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#92316 (mangling_v0: Skip extern blocks during mangling)
 - rust-lang#92630 (Change PhantomData type for `BuildHasherDefault` (and more))
 - rust-lang#92800 (Add manifest docs fallback.)
 - rust-lang#93005 (Move back templates into html folder)
 - rust-lang#93065 (Pretty printer algorithm revamp step 2)
 - rust-lang#93077 (remove `List::is_noop`)

Failed merges:

 - rust-lang#93068 (Fix spacing for `·` between stability and source)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 715cda2 into rust-lang:master Jan 20, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 20, 2022
@eddyb
Copy link
Member

eddyb commented May 13, 2022

@petrochenkov Isn't this a collision hazard?

If hygiene 2.0 can cause the same name to appear twice in a module, those two instances will have distinct DefPaths because the second one will have a disambiguator of 1 instead of 0.

But if one of them is extern { type }, then the DefPaths will be foo::Foo vs foo::{foreign module}::Foo (though printed as foo::Foo), both of which would have a disambiguator of 0.

So either the DefPath should not have the ForeignMod components (kind of like that old idea of desugaring extern "ABI" { fn foo(); } to something like extern "ABI" foreign_fn foo(); in HIR), or this PR needs to be reverted (ideally hygiene 2.0 lets us write tests for collisions solved only by disambiguators).

@petrochenkov
Copy link
Contributor Author

@eddyb
Yes, you are right, this can cause collisions and needs to be reverted.

Previously 't' with empty name happened to not collide with anything else in type namespace.
But maybe it would be better to reserve a separate letter for extern blocks (if there are enough free letters).

@eddyb
Copy link
Member

eddyb commented May 13, 2022

Oh yeah, I agree, t is not a good idea either - AFAICT all the lowercase namespaces (i.e. not guaranteed to mean anything in particular, and up for grabs for internal use) are declared here, and e.g. f is free:

DefPathData::TypeNs(_) => 't',
DefPathData::ValueNs(_) => 'v',
DefPathData::ClosureExpr => 'C',
DefPathData::Ctor => 'c',
DefPathData::AnonConst => 'k',
DefPathData::ImplTrait => 'i',

That is, the only other namespaces I could find are both uppercase (S for shims and I for impls).

In any case, namespaces (whether lowercase or uppercase) do not share encoding space with any other syntax, so most letters aren't used yet - this is intentional, to avoid running into issues if rustc decides to need a lot more namespaces for whatever reason (and if we somehow run out of all 52 of them, we can always shove extra information into the low bits of disambiguators, as unelegant as that is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants