Fix bugs in and improve name mangling #8875
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The test I added will not compile with today's rustc for a large number of reasons, and this pull request depends on #8843 |
|
Hmm... the "improved" name mangling aspect of this isn't working as I thought, I'm currently investigating... Regardless though the bug fixing changes are good to be looked at. |
|
Does this fix #6602? |
|
Aha, this should be good to go now. Here's a good before/after story for this code: #[no_std];
pub fn foo() -> int {
static foo_constant: int = 1;
return foo_constant;
}
struct A;
trait B {
fn bar(&self) -> int;
}
impl A {
fn foo(&self) -> int {
static foo_constant: int = 1;
return foo_constant + foo();
}
}
impl B for A {
fn bar(&self) -> int {
static bar_constant: int = 1;
return bar_constant + self.foo();
}
}
struct C<T>;
impl<T> C<T> {
fn baz(&self) -> int {
static baz_constant: int = 1;
return baz_constant;
}
}
#[start]
fn main(_: int, _: **u8, _: *u8) -> int {
let a = A;
a.bar();
let c = C::<()>;
c.baz();
3
}Before: After: |
|
That is wonderful! ⛵ |
|
What happens if (I could imagine this could be important if one had |
|
So here's a few things that I'm finding with this.
Note the lack of |
|
That doesn't sound too bad. (Although, I do remember there were some issues with the android assembler and non-ascii characters in symbols.) |
|
If that's the case, perhaps we could perform some odd normalization on android but let other platforms have the benefit of better symbol generation. I'd love to look into why there's the bug in the first place on android though (for a later time). |
|
ok it's almost done with make check, I'm just gonna assume that it'll finish all the way now |
|
Looks like I'm about to learn why we did sanitization in the first place. I'll investigate this soon. |
Before, the path name for all items defined in methods of traits and impls never took into account the name of the method. This meant that if you had two statics of the same name in two different methods the statics would end up having the same symbol named (even after mangling) because the path components leading to the symbol were exactly the same (just __extensions__ and the static name). It turns out that if you add the symbol "A" twice to LLVM, it automatically makes the second one "A1" instead of "A". What this meant is that in local crate compilations we never found this bug. Even across crates, this was never a problem. The problem arises when you have generic methods that don't get generated at compile-time of a library. If the statics were re-added to LLVM by a client crate of a library in a different order, you would reference different constants (the integer suffixes wouldn't be guaranteed to be the same). This fixes the problem by adding the method name to symbol path when building the ast_map. In doing so, two symbols in two different methods are disambiguated against.
As with the previous commit, this is targeted at removing the possibility of collisions between statics. The main use case here is when there's a type-parametric function with an inner static that's compiled as a library. Before this commit, any impl would generate a path item of "__extensions__". This changes this identifier to be a "pretty name", which is either the last element of the path of the trait implemented or the last element of the type's path that's being implemented. That doesn't quite cut it though, so the (trait, type) pair is hashed and again used to append information to the symbol. Essentially, __extensions__ was removed for something nicer for debugging, and then some more information was added to symbol name by including a hash of the trait being implemented and type it's being implemented for. This should prevent colliding names for inner statics in regular functions with similar names.
|
Alright let's try this again. I've reverted back to C++ name-mangling so we can get pretty r? the last commit? A few things changed a bit substantially there. |
|
#7610 seems related too. (Would it be possible to say have |
Remove __extensions__ from method symbols as well as the meth_XXX. The XXX is now used to append a few characters at the end of the name of the symbol. Closes rust-lang#6602
|
perhaps, the problem is then you can run into collisions with absurdly named structs. I think that the type hash will cause it such that there isn't actually a naming collision. I figured we could try these out for a bit and then maybe move to that. It's not a bad idea, though, especially if the convention for struct names is CamelCase and not with_underscores. |
bors
added a commit
that referenced
this pull request
Sep 5, 2013
… r=huonw These commits fix bugs related to identically named statics in functions of implementations in various situations. The commit messages have most of the information about what bugs are being fixed and why. As a bonus, while I was messing around with name mangling, I improved the backtraces we'll get in gdb by removing `__extensions__` for the trait/type being implemented and by adding the method name as well. Yay!
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Jun 16, 2022
Fixes for `derive_partial_eq_without_eq` fixes rust-lang#8875 changelog: Don't lint `derive_partial_eq_without_eq` on non-public types changelog: Better handle generics in `derive_partial_eq_without_eq`
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.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These commits fix bugs related to identically named statics in functions of implementations in various situations. The commit messages have most of the information about what bugs are being fixed and why.
As a bonus, while I was messing around with name mangling, I improved the backtraces we'll get in gdb by removing
__extensions__for the trait/type being implemented and by adding the method name as well. Yay!