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.

Sign up for GitHub

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

Jump to bottom

Special-case .llvm in mangler #61195

Merged
merged 1 commit into from
May 29, 2019
Merged

Special-case .llvm in mangler #61195

merged 1 commit into from
May 29, 2019

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented May 25, 2019 •
edited
Loading

Fixes #60925 and fixes #53912.

r? @michaelwoerister
cc @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2019
@davidtwco
Copy link
Member Author

I believe this is what @eddyb suggested. I wasn't able to work out how to write a test for this, so any thoughts appreciated.

@michaelwoerister
Copy link
Member

Thanks for looking into it, @davidtwco! That should fix the issue.

@bors r+

@bors
Copy link
Contributor

bors commented May 27, 2019

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove WIP from this PR's title when it is ready for review.

@michaelwoerister
Copy link
Member

Good call, bors... Regarding a test: You need to write some code with a module or function named llvm, I'd say, and then test that the symbol name does not contain the string .llvm. [ui/symbol-names](https://github.com/rust-lang/rust/tree/master/src/test/ui/symbol-names] show one way of checking symbol names. A codegen would be another option.

@eddyb
Copy link
Member

eddyb commented May 28, 2019

#53912 has a reduction (I guess it's tricky to cause it because the segfault is in LTO).

@davidtwco davidtwco force-pushed the seg-fault-mangler branch from 1803781 to 1c42a76 Compare May 28, 2019 19:22
@davidtwco
Special-case .llvm in mangler to fix segfaults
9c34473 
This commit special cases `.llvm` in the mangler to print `.llvm$6d$`
instead. This will avoid segfaults when names in a user's Rust code are
`llvm`.
@davidtwco davidtwco force-pushed the seg-fault-mangler branch from 1c42a76 to 9c34473 Compare May 28, 2019 19:27
@davidtwco davidtwco changed the title WIP: Special-case .llvm in mangler Special-case .llvm in mangler May 28, 2019
@davidtwco
Copy link
Member Author

Thanks @eddyb and @michaelwoerister. I've added two tests, one that checks the symbol name with the reproduction from #53912 and another that checks that the code compiles successfully (since I wasn't sure that the symbol check attribute didn't exit early).

@eddyb
Copy link
Member

eddyb commented May 28, 2019

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2019

📌 Commit 9c34473 has been approved by eddyb

@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 May 28, 2019
Centril added a commit to Centril/rust that referenced this pull request May 28, 2019
@Centril
Rollup merge of rust-lang#61195 - davidtwco:seg-fault-mangler, r=eddyb
dae331d 
Special-case `.llvm` in mangler

Fixes rust-lang#60925 and fixes rust-lang#53912.

r? @michaelwoerister
cc @eddyb
@Centril Centril mentioned this pull request May 28, 2019
Merged
bors added a commit that referenced this pull request May 28, 2019
@bors
Auto merge of #61296 - Centril:rollup-5ad68b0, r=Centril
4b9d803 
Rollup of 9 pull requests

Successful merges:

 - #60742 (Allow const parameters in array sizes to be unified)
 - #60756 (Add better tests for hidden lifetimes in impl trait)
 - #60928 (Changes the type `mir::Mir` into `mir::Body`)
 - #61024 (tests: Centralize proc macros commonly used for testing)
 - #61157 (BufReader: In Seek impl, remove extra discard_buffer call)
 - #61195 (Special-case `.llvm` in mangler)
 - #61202 (Print PermissionExt::mode() in octal in Documentation Examples)
 - #61259 (Mailmap fixes)
 - #61273 (mention that MaybeUninit is a bit like Option)

Failed merges:

r? @ghost
@bors bors merged commit 9c34473 into rust-lang:master May 29, 2019
@davidtwco davidtwco deleted the seg-fault-mangler branch May 29, 2019 07:11
eddyb
eddyb reviewed Jun 1, 2019
View reviewed changes
src/librustc_codegen_utils/symbol_names.rs
@@ -613,6 +613,9 @@ impl fmt::Write for SymbolPrinter<'_, '_> {
// for ':' and '-'
'-' | ':' => self.path.temp_buf.push('.'),

// Avoid segmentation fault on some platforms, see #60925.
'm' if self.path.temp_buf.ends_with(".llv") => self.path.temp_buf.push_str("$6d$"),
Copy link
Member

Choose a reason for hiding this comment

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

Ooops, turns out this should be $u6d$ (i.e. note the u).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've submitted #61423 that changes this.

davidtwco added a commit to davidtwco/rust that referenced this pull request Jun 1, 2019
@davidtwco
codegen: change $6d$ to $u6d$
e71f466 
This changes a mistake introduced in rust-lang#61195 where the mangling
workaround used was incorrect.
@davidtwco davidtwco mentioned this pull request Jun 1, 2019
Merged
Centril added a commit to Centril/rust that referenced this pull request Jun 2, 2019
@Centril
Rollup merge of rust-lang#61423 - davidtwco:correct-symbol-mangling, …
5599985 
…r=eddyb

codegen: change `$6d$` to `$u6d$`

This changes a mistake introduced in rust-lang#61195 where the mangling
workaround used was incorrect, resolving [this comment](rust-lang#61195 (comment)) from @eddyb.

r? @eddyb
@Zalathar Zalathar mentioned this pull request Feb 6, 2025
Merged
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 6, 2025
@workingjubilee
Rollup merge of rust-lang#136611 - Zalathar:llvm-underscore, r=workin…
c549268 
…gjubilee

cg_llvm: Remove the `mod llvm_` hack, which should no longer be necessary

This re-export was introduced in rust-lang@c76fc3d, as a workaround for rust-lang#53912.

In short, there was/is an assumption in some LLVM LTO code that symbol names would not contain `.llvm.`, but legacy symbol mangling would naturally produce that sequence for symbols in a module named `llvm`.

This was later “fixed” by adding a special case to the legacy symbol mangler in rust-lang#61195, which detects the sequence `llvm` and emits the `m` in an escaped form. As a result, there should no longer be any need to avoid the module name `llvm` in the compiler itself.

(Symbol mangling v0 avoids this problem by not using `.` in the first place, outside of the “vendor-specific suffix”.)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2025
@rust-timer
Unrolled build for rust-lang#136611
f499a9f 
Rollup merge of rust-lang#136611 - Zalathar:llvm-underscore, r=workingjubilee

cg_llvm: Remove the `mod llvm_` hack, which should no longer be necessary

This re-export was introduced in rust-lang@c76fc3d, as a workaround for rust-lang#53912.

In short, there was/is an assumption in some LLVM LTO code that symbol names would not contain `.llvm.`, but legacy symbol mangling would naturally produce that sequence for symbols in a module named `llvm`.

This was later “fixed” by adding a special case to the legacy symbol mangler in rust-lang#61195, which detects the sequence `llvm` and emits the `m` in an escaped form. As a result, there should no longer be any need to avoid the module name `llvm` in the compiler itself.

(Symbol mangling v0 avoids this problem by not using `.` in the first place, outside of the “vendor-specific suffix”.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
5 participants
@davidtwco @michaelwoerister @bors @eddyb @rust-highfive