Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upIntroduce Rust symbol mangling scheme. #57967
+5,017
−2,834
Conversation
rust-highfive
assigned
nikomatsakis
Jan 29, 2019
rust-highfive
added
the
S-waiting-on-review
label
Jan 29, 2019
This was referenced Jan 29, 2019
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
added a commit
that referenced
this pull request
Jan 29, 2019
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
So that's probably because the checking is quite thorough (therefore a bit slow) - I don't want to compromise on that, is there any way to temporarily raise the limit? |
This comment has been minimized.
This comment has been minimized.
|
@michaelwoerister @alexcrichton The
If we add the actual input/output types, it will significantly inflate symbol name sizes, whereas a hash would be a constant overhead (but @michaelwoerister's RFC tries to avoid hashes for anything other than crate roots). IMO ideally the entire dylib would be versioned in some way, so a new build would be incompatible - IIRC we have something (SVH?) for detecting this when EDIT: one hacky way would be holding pointers to dylib dependency statics (each containing SVH or equivalent in their symbol name) in a static of each crate. |
This comment has been minimized.
This comment has been minimized.
|
Do you mean only for this try build or for a while? We'd have to ask Travis Support either way. |
This comment has been minimized.
This comment has been minimized.
|
@pietroalbini Only for this build, mostly for running crater. I suppose I can disable some of the work not needed to check symbols, but the roundtrip check is probably the most expensive thing anyway. I guess I'll see how long it takes for the PR build to fail again, with as much as possible disabled. |
eddyb
force-pushed the
eddyb:rmangle
branch
from
bfb78ea
to
d4a9b81
Jan 29, 2019
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
|
eddyb
added some commits
Dec 4, 2018
eddyb
added some commits
Jan 29, 2019
eddyb
force-pushed the
eddyb:rmangle
branch
from
d4a9b81
to
b506029
Jan 29, 2019
This comment has been minimized.
This comment has been minimized.
Time-to-fail is not as useful as I hoped, so I turned off the failing test (#57967 (comment)). |
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
added a commit
that referenced
this pull request
Jan 29, 2019
petrochenkov
referenced this pull request
Jan 29, 2019
Open
rustc: remove unnecessary extern_prelude logic from ty::item_path. #56655
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@craterbot run start=master#c1c3c4e95b69dfeaca5c5db6c622d7f90ad30a54 end=try#7fea0229842e79db1159ea58ad9a653b288efee9 |
This comment has been minimized.
This comment has been minimized.
|
|
craterbot
added
S-waiting-on-crater
and removed
S-waiting-on-review
labels
Jan 30, 2019
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Thanks, @eddyb! That's an epic amount of refactorings It might make sense to split the |
This comment has been minimized.
This comment has been minimized.
|
I don't know yet what to do about the failing test. I believe that the same thing would be caught if things went through cargo, because then the two crates would have a different disambiguator. Or at least they probably should have a different disambiguator. |
This comment has been minimized.
This comment has been minimized.
|
r? @michaelwoerister -- I feel like mw is the more obvious choice here |
rust-highfive
assigned
michaelwoerister
and unassigned
nikomatsakis
Jan 31, 2019
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis Maybe I'll assign you to the "erase ppaux from history" PR if/when I split this up. |
This comment has been minimized.
This comment has been minimized.
|
|
craterbot
added
S-waiting-on-review
and removed
S-waiting-on-crater
labels
Feb 1, 2019
This comment has been minimized.
This comment has been minimized.
|
According to this link that's quite the mouthful! |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton @michaelwoerister I think we should pass such symbols through |
This comment has been minimized.
This comment has been minimized.
|
All the other build failures are also caused by actix-web, other than the one in rusturn: Most of the test failures seem unrelated (unless I managed to cause a series of freak accidents?), except this one (which contains a new symbol name, or at least part of it, in an assertion failure). What I'm glad I'm not seeing is a roundtrip failure (which means the mangling scheme works as designed, and we can probably take the check out even if we were to do more testing). The "symbol |
This comment has been minimized.
This comment has been minimized.
|
@rust-timer build 7fea022 |
This comment has been minimized.
This comment has been minimized.
rust-timer
commented
Feb 2, 2019
|
Success: Queued 7fea022 with parent c1c3c4e, comparison URL. |
This comment has been minimized.
This comment has been minimized.
|
Minimal reproduction (for the fn size_of_val<T>(_: &T) -> usize {
std::mem::size_of::<T>()
}
struct Foo(i64);
pub fn size_of_foo() -> usize {
size_of_val(&Foo(0))
}
pub fn size_of_foo_ctor() -> usize {
size_of_val(&Foo)
}Surprised we don't have a test for this! So the problem is that I'm stripping a I think we should just encode it in some way (the same way closures are?) and have the demangler ignore it (if we even want it to be ignored). |
This comment has been minimized.
This comment has been minimized.
rust-timer
commented
Feb 2, 2019
|
Finished benchmarking try commit 7fea022 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
eddyb commentedJan 29, 2019
•
edited
This is an implementation of a "feature-complete" Rust mangling scheme, in the vein of rust-lang/rfcs#2603 - but with some differences, see rust-lang/rfcs#2603 (comment) for details.
The demangling implementation PR is alexcrichton/rustc-demangle#23
(this PR already uses it via a git dependency, to allow testing).
Discussion of the design of the mangling scheme should still happen on the RFC, but this PR's specific implementation details can be reviewed in parallel.
Notes for reviewers:
rustc-demangleto the compiler's pretty-printing, adjusted slightly to produce the same output), that I would like to try on crater-Zflag for nowr? @nikomatsakis / @michaelwoerister cc @rust-lang/compiler