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

Mark functions created for raw-dylib on x86 with DllImport storage class #104511

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Mark functions created for raw-dylib on x86 with DllImport storage class #104511

merged 1 commit into from
Nov 21, 2022

Conversation

dpaoliello
Copy link
Contributor

Fix for #104453

Issue Details

On x86 Windows, LLVM uses 'L' as the prefix for any private global symbols (PrivateGlobalPrefix), so when the raw-dylib feature creates an undecorated function symbol that begins with an 'L' LLVM misinterprets that as a private global symbol that it created and so fails the compilation at a later stage since such a symbol must have a definition.

Fix Details

Mark the function we are creating for raw-dylib with DllImport storage class (this was already being done for MSVC at a later point for callee::get_fn but not for GNU (due to "backwards compatibility")): this will cause LLVM to prefix the name with __imp_ and so it won't mistake it for a private global symbol.

@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2022

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 16, 2022
@dpaoliello
Copy link
Contributor Author

r? @michaelwoerister

@rustbot rustbot assigned michaelwoerister and unassigned cjgillot Nov 16, 2022
@michaelwoerister
Copy link
Member

Thanks, @dpaoliello! Looks good to me.

@mati865, you added that "backwards compatibility" comment in #72049 -- do you think it is still up to date? I can't quite tell if that PR added a regression test for the issue this refers to or not.

@mati865
Copy link
Contributor

mati865 commented Nov 17, 2022

@michaelwoerister I cannot test it any time soon but I think thanks to that one can create DLL that "exports" symbols without dllexport (so only bare foo like functions are exported instead of __imp_foo) and use that foo in Rust code.
IIRC this might be unnecessary with LLD (in MinGW mode) because it would fallback to nondecorated symbol but I have no idea about ld.bfd.
If my first statement is false (being able to link undecorated symbols) or second statement is true (linker being able to fixup symbols) then we can remove "backwards compatibility" without issues. Otherwise I feel its deletion would need braded discussion if it's fine.

@michaelwoerister
Copy link
Member

Thanks for the info, @mati865! It sounds like we are generally lacking test coverage for windows-gnu here. It's sad that we are relying so much on heuristics when it comes to dllimport.

michaelwoerister
michaelwoerister approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, @dpaoliello!

I think we can merge it with a little more documentation (see below).

compiler/rustc_codegen_llvm/src/callee.rs Show resolved Hide resolved
@dpaoliello dpaoliello force-pushed the privateglobalworkaround branch from 6b17af0 to 67e746c Compare November 18, 2022 19:38
@michaelwoerister
Copy link
Member

Thanks, @dpaoliello!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 21, 2022

📌 Commit 67e746c has been approved by michaelwoerister

It is now in the queue for this repository.

@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 Nov 21, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 21, 2022
@matthiaskrgr
Rollup merge of rust-lang#104511 - dpaoliello:privateglobalworkaround…
4eb13ef 
…, r=michaelwoerister

Mark functions created for `raw-dylib` on x86 with DllImport storage class

Fix for rust-lang#104453

## Issue Details
On x86 Windows, LLVM uses 'L' as the prefix for any private global symbols (`PrivateGlobalPrefix`), so when the `raw-dylib` feature creates an undecorated function symbol that begins with an 'L' LLVM misinterprets that as a private global symbol that it created and so fails the compilation at a later stage since such a symbol must have a definition.

## Fix Details
Mark the function we are creating for `raw-dylib` with `DllImport` storage class (this was already being done for MSVC at a later point for `callee::get_fn` but not for GNU (due to "backwards compatibility")): this will cause LLVM to prefix the name with `__imp_` and so it won't mistake it for a private global symbol.
This was referenced Nov 21, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2022
@bors
Auto merge of rust-lang#104673 - matthiaskrgr:rollup-85f65ov, r=matth…
1cbc459 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104420 (Fix doc example for `wrapping_abs`)
 - rust-lang#104499 (rustdoc JSON: Use `Function` everywhere and remove `Method`)
 - rust-lang#104500 (`rustc_ast`: remove `ref` patterns)
 - rust-lang#104511 (Mark functions created for `raw-dylib` on x86 with DllImport storage class)
 - rust-lang#104595 (Add `PolyExistentialPredicate` type alias)
 - rust-lang#104605 (deduplicate constant evaluation in cranelift backend)
 - rust-lang#104628 (Revert "Update CI to use Android NDK r25b")
 - rust-lang#104662 (Streamline deriving on packed structs.)
 - rust-lang#104667 (Revert formatting changes of a test)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cc2397b into rust-lang:master Nov 21, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 21, 2022
@goffrie goffrie mentioned this pull request Jan 25, 2023
Closed
@dpaoliello dpaoliello deleted the privateglobalworkaround branch January 16, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister Awaiting requested review from michaelwoerister

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
6 participants
@dpaoliello @rustbot @michaelwoerister @mati865 @bors @cjgillot