Skip to content

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented May 25, 2019

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
Collaborator

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
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
Collaborator

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
bors added a commit that referenced this pull request May 28, 2019
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
@@ -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
This changes a mistake introduced in rust-lang#61195 where the mangling
workaround used was incorrect.
Centril added a commit to Centril/rust that referenced this pull request Jun 2, 2019
…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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 6, 2025
…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
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
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
5 participants