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.

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

Should raw-dylibs defined in windows-targets use import_name_type = "undecorated"? #3285

Closed
glandium opened this issue Sep 19, 2024 · 14 comments
Labels
question Further information is requested

Comments

@glandium
Copy link
Contributor

Summary

In a complex multi-language project, one may end up using both windows-target and the system libraries directly (that's the case of Firefox). When enabling windows-targets's support for raw-dylib, linking fails when using LLVM's lld-link (I haven't tried link.exe, it might actually work, but the result is probably suboptimal too). The cause for the link failure is possibly a bug in lld-link, but it is a manifestation of the fact that the fake import lib that rust creates with raw-dylib does not agree with the real system import lib, wrt symbol names: on 32-bits windows, the system import lib use the __imp__sym@n form, while the raw-dylib uses the __imp_sym form because of import_name_type = "undecorated".

See llvm/llvm-project#107371 (comment) for more details.

Removing import_name_type = "undecorated" solves the problem.

Crate manifest

No response

Crate code

No response

@glandium glandium added the bug Something isn't working label Sep 19, 2024
@glandium
Copy link
Contributor Author

I guess the discussion on the LLVM side points to the problem being on the rust side of raw-dylib.

@ChrisDenton
Copy link
Collaborator

Hm, do you know if older versions of Rust worked? I suspect this might be a rustc issue.

Looking at a Windows import library (Synchronization.lib because it's small), I see WakeByAddressSingle import defined as:

  Version      : 0
  Machine      : 14C (x86)
  TimeDateStamp: FFFFFFFF
  SizeOfData   : 00000038
  DLL name     : api-ms-win-core-synch-l1-2-0.dll
  Symbol name  : _WakeByAddressSingle@4
  Type         : code
  Name type    : undecorate
  Hint         : 59
  Name         : WakeByAddressSingle

And the linker members include:

6.api-ms-win-core-synch-l1-2-0.dll    _WakeByAddressSingle@4
6.api-ms-win-core-synch-l1-2-0.dll    __imp__WakeByAddressSingle@4

So the name type should be undecorate but the symbol it self should still be decorated. Not sure what's going wrong there.

I'm a bit puzzled why removing import_name_type = "undecorated" works because I'd naively expect that to try importing the decorated name from a DLL, which shouldn't be found. Unless it happens that the native lib takes precedence.

@kennykerr kennykerr added question Further information is requested and removed bug Something isn't working labels Sep 19, 2024
@kennykerr
Copy link
Collaborator

@dpaoliello thoughts?

@kennykerr
Copy link
Collaborator

Anyone know how we might add this...

env:
RUSTFLAGS: --cfg windows_raw_dylib

...to a yml strategy/matrix so I can more easily run raw-dylib tests across the board? Happy to expand test coverage if there's a gap here.

@ChrisDenton
Copy link
Collaborator

I did check with 1.71 and it has the same behaviour.

@kennykerr
Copy link
Collaborator

kennykerr commented Sep 19, 2024

#3287 passed meaning we have the same testing for raw-dylib as non raw-dylib.

@kennykerr
Copy link
Collaborator

Happy to test rust-lld / lld-link if someone can point me to how that is done.

I see its enabled by default on Linux but not Windows. Anyone know why?

https://blog.rust-lang.org/2024/05/17/enabling-rust-lld-on-linux.html

@ChrisDenton
Copy link
Collaborator

There have been compatibility issues in the past. I'm not exactly sure where we are at now but it seems rust-lang/rust#111480 is still open (though might just be that nobody has tested it recently).

@ChrisDenton
Copy link
Collaborator

Happy to test rust-lld / lld-link if someone can point me to how that is done.

You can put this in config.toml:

[target.x86_64-pc-windows-msvc]
linker = "rust-lld"

Or maybe use RUSTFLAGS with -C linker=rust-lld

@kennykerr
Copy link
Collaborator

Thanks, I've tested this and it seems to work fine.

@dpaoliello
Copy link
Collaborator

Seems like it's the same issue as rust-lang/rust#124958

Essentially, we should be setting symbol_name to the decorated name when creating the COFFShortExport, that way the import has both the decorated and undecorated name and should match the real import library.

@kennykerr
Copy link
Collaborator

kennykerr commented Sep 19, 2024

There have been compatibility issues in the past. I'm not exactly sure where we are at now but it seems rust-lang/rust#111480 is still open (though might just be that nobody has tested it recently).

I also ran rust-lld through the windows-rs build gauntlet and everything looks good: #3288

@glandium
Copy link
Contributor Author

I've tried with a tiny testcase, and it seems the side effects of the rust raw-dylib shortcoming is only really visible with crate-type = ["staticlib"], so it's not all that surprising that you don't see failures.

@kennykerr
Copy link
Collaborator

Well it sounds like this is unrelated to windows-rs so I'm closing this issue.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 7, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 7, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? ``@ChrisDenton``
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 8, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2024
Rollup merge of rust-lang#130586 - dpaoliello:fixrawdylib, r=wesleywiser

Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
mati865 pushed a commit to mati865/rust that referenced this issue Nov 12, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this issue Dec 11, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be #124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes #124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants