Skip to content

rustc: Remove __rust_crate_map_toplevel alias #12588

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.

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

Closed
wants to merge 1 commit into from

Conversation

klutzy
Copy link
Contributor

@klutzy klutzy commented Feb 27, 2014

It was a hack to workaround llvm bug: when
_rust_crate_map_toplevel is dll-exported, llvm generated
-export:__rust_crate_map_toplevel,data where extra underscore
is due to unnecessary mangling. Then linker tries to find
__rust_crate_map_toplevel symbol, but it doesn't exist.
Therefore alias was added to prevent the link failure.
(See http://llvm.org/bugs/show_bug.cgi?id=10174 for details.)

It also caused LTO issue: llvm's LTO pass removes
__rust_crate_map_toplevel alias for some reason I don't know,
so build failed at link time.

Fixes #12471

@klutzy
Copy link
Contributor Author

klutzy commented Feb 27, 2014

It was a hack to workaround llvm bug: when
`_rust_crate_map_toplevel` is dll-exported, llvm generated
`-export:__rust_crate_map_toplevel,data` where extra underscore
is due to unnecessary mangling. Then linker tries to find
`__rust_crate_map_toplevel` symbol, but it doesn't exist.
Therefore alias was added to prevent the link failure.
(See http://llvm.org/bugs/show_bug.cgi?id=10174 for details.)

It also caused LTO issue: llvm's LTO pass removes
`__rust_crate_map_toplevel` alias for some reason I don't know,
so build failed at link time.
@alexcrichton
Copy link
Member

Does this require an patch to LLVM itself? If so, have you tried to upstream it first?

@klutzy
Copy link
Contributor Author

klutzy commented Feb 27, 2014

Yes, llvm patch is needed. I haven't tried to upstream yet but I'll try it soon.

@alexcrichton
Copy link
Member

I'm in the process of upstreaming the bulk of the rest of our patches, so I'd prefer to not merge this before seeing what upstream thinks about the LLVM patch.

That being said, thank you so much for finding this! I think LLVM will be quite receptive to your patch.

@@ -1 +1 @@
Subproject commit b015ecddd3129490fa26e5278a1acee79f2ee5ef
Subproject commit 5fe5e1c63994a2510dc264a37dfb3071f9e3cea0
Copy link
Contributor

Choose a reason for hiding this comment

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

This submodule points towards rust-lang/llvm (it would not work with a commit hash from one of your repos).

@luqmana
Copy link
Member

luqmana commented Feb 28, 2014

@klutzy yay! thanks for getting rid of that annoying wart

@alexcrichton
Copy link
Member

Would it be possible to land the removal of the aliasing before the upgrade to LLVM?

@klutzy
Copy link
Contributor Author

klutzy commented Mar 10, 2014

@alexcrichton Yep, it seems easier to use .def file which is standard way to dllexport symbols: prepare some .def file and give it to ld.

Anyway I found my llvm patch is wrong (e.g. it breaks msvc-compatibility behavior) so I'm closing this until new fix is done.

@klutzy klutzy closed this Mar 10, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
internal: More completion reorganizing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-Z lto fails to compile in Windows
4 participants