-
Notifications
You must be signed in to change notification settings - Fork 50
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Demangle ThinLTO-generated symbols as well
- Loading branch information
1 parent
6c94e7f
commit 48646c6
Showing
1 changed file
with
26 additions
and
1 deletion.
There are no files selected for viewing
This file contains 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
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added an issue about this and other issues here rust-lang/rust#46552 and then randomly checked this repo for updates.
So this seems intentional; but why?
c++filt
no longer works out of the box :/ and now comments like this https://github.com//m4b/rust/blob/383e313d181eceb3155eb1089d448144f830ee23/src/librustc_trans/back/symbol_names.rs#L339-L351 are invalid/misleading, as well as very likely causing trouble in debugger backends...48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM generates them and the job of this crate is to "just work"? Tools like
c++filt
never were at 100% parity with our name mangling before anyway?48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never had any trouble with c++filt until recently.
I find it hard to believe llvm is just spitting out symbols willy nilly without any say in the matter. C++ also does LTO with LLVM and this format would break their name mangler as well and could potentially cause huge problems, so seems there has to be a way to get it to output well formed names...
Also my last two points are still valid I think, which makes the existence of these names troublesome.
I understand this crate is supposed to just work, and that’s good, but I guess:
Since this crate seems somewhat canonical wrt rust names it worried me a little bit that these names got implemented (and hence validated).
I dunno if that makes sense?
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er to be clear it's not rustc doing this mangling, it's llvm's ThinLTO passes.
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that llvm was emitting this was the understanding I was operating under, but I assumed the rustc compiler was incorrectly passing mangled names (but it seems its using the interface fine, iiuc).
After investigating this, I believe that this is a pretty serious bug. As expected, I am unable to emit C++ mangled names with
.llvm
and a hash (like above) appended with thin lto for clang 5.0. (doing so would break c++ symbol names, and if it were a global symbol, could cause serious linking errors, in addition to breaking all debug info)However, I was able to do this with clang 3.9 in some cases.
That rustc (or technically llvm) is emitting these names is a bug; in the cases it is emitted, all debug information either disappears from the binary for those symbols, or the debug information references a non existent symbol (since its not expecting
.llvm.<hash>
appended).A simple repro is:
You can then see the dwarf debug information does not match:
(i.e., the
linkage_name
field matches the properly mangled rust name in both cases)when I use my compiler patched with m4b/rust@383e313 the debug information isn't even omitted at all.
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be that upgrading to llvm 5.0 will magically fix these issues; I don't know, I hope so. Otherwise, this is a pretty big bug imho, and should probably be worked out before enabling rust's thinlto by default.
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like an excellent comment for upstream! (I'm sure others have thoughts on this as well...)
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean rust upstream or llvm upstream ?
If rust I already opened an issue regarding invalid symbol mangling in rust (linked above); if llvm upstream I suppose I could open a bug but if it’s not reproducible in 5.0 my guess is they won’t care ?
48646c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have an opinion, I just wanted to point out that ongoing discussion on this particular commit is probably not actually going to cause any change to happen.