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

Implementation of import_name_type #100732

Merged
merged 1 commit into from
Aug 27, 2022
Merged

Conversation

dpaoliello
Copy link
Contributor

Fixes #96534 by implementing rust-lang/compiler-team#525

Symbols that are exported or imported from a binary on 32bit x86 Windows can be named in four separate ways, corresponding to the import name types from the PE-COFF spec. The exporting and importing binaries must use the same name encoding, otherwise mismatches can lead to link failures due to "missing symbols" or to 0xc0000139 (STATUS_ENTRYPOINT_NOT_FOUND) errors when the executable/library is loaded. For details, see the comments on the raw-dylib feature's #58713. To generate the correct import libraries for these DLLs, therefore, rustc must know the import name type for each extern function, and there is currently no way for users to provide this information.

This change adds a new MetaNameValueStr key to the #[link] attribute called import_name_type, and which accepts one of three values: decorated, noprefix, and undecorated.

A single DLL is likely to export all its functions using the same import type name, hence import_name_type is a parameter of #[link] rather than being its own attribute that is applied per-function. It is possible to have a single DLL that exports different functions using different import name types, but users could express such cases by providing multiple export blocks for the same DLL, each with a different import name type.

Note: there is a fourth import name type defined in the PE-COFF spec, IMPORT_ORDINAL. This case is already handled by the #[link_ordinal] attribute. While it could be merged into import_type_name, that would not make sense as #[link_ordinal] provides per-function information (namely the ordinal itself).

Design decisions (these match the MCP linked above):

  • For GNU, decorated matches the PE Spec and MSVC rather than the default behavior of dlltool (i.e., there will be a leading _ for stdcall).
  • If import_name_type is not present, we will keep our current behavior of matching the environment (MSVC vs GNU) default for decorating.
  • Using import_name_type on architectures other than 32bit x86 will result in an error.
  • Using import_name_type with link kinds other than "raw-dylib" will result in an error.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 18, 2022
@rust-highfive
Copy link
Collaborator

r? @TaKO8Ki

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2022
@dpaoliello
Copy link
Contributor Author

r? @wesleywiser @michaelwoerister

@rust-highfive rust-highfive assigned wesleywiser and unassigned TaKO8Ki Aug 18, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Overall, this looks pretty good, nice work!

compiler/rustc_metadata/src/native_libs.rs Outdated Show resolved Hide resolved
@dpaoliello dpaoliello force-pushed the import_name_type branch 2 times, most recently from 9abf76e to f847388 Compare August 22, 2022 19:08
@dpaoliello dpaoliello requested a review from wesleywiser August 23, 2022 17:22
@wesleywiser
Copy link
Member

Awesome, thanks @dpaoliello!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2022

📌 Commit f847388 has been approved by wesleywiser

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 Aug 24, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Aug 24, 2022

Thanks for working on this!

This looks good to me, with my lang team hat on.

A single DLL is likely to export all its functions using the same import type name, hence import_name_type is a parameter of #[link] rather than being its own attribute that is applied per-function. It is possible to have a single DLL that exports different functions using different import name types, but users could express such cases by providing multiple export blocks for the same DLL, each with a different import name type.

As a potential improvement for the future, I do think it might make sense to allow this attribute on a per-function basis as an override. That is not a blocker in any way, though.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 25, 2022
…leywiser

Implementation of import_name_type

Fixes rust-lang#96534 by implementing rust-lang/compiler-team#525

Symbols that are exported or imported from a binary on 32bit x86 Windows can be named in four separate ways, corresponding to the [import name types](https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#import-name-type) from the PE-COFF spec. The exporting and importing binaries must use the same name encoding, otherwise mismatches can lead to link failures due to "missing symbols" or to 0xc0000139 (`STATUS_ENTRYPOINT_NOT_FOUND`) errors when the executable/library is loaded. For details, see the comments on the raw-dylib feature's rust-lang#58713. To generate the correct import libraries for these DLLs, therefore, rustc must know the import name type for each `extern` function, and there is currently no way for users to provide this information.

This change adds a new `MetaNameValueStr` key to the `#[link]` attribute called `import_name_type`, and which accepts one of three values: `decorated`, `noprefix`, and `undecorated`.

A single DLL is likely to export all its functions using the same import type name, hence `import_name_type` is a parameter of `#[link]` rather than being its own attribute that is applied per-function. It is possible to have a single DLL that exports different functions using different import name types, but users could express such cases by providing multiple export blocks for the same DLL, each with a different import name type.

Note: there is a fourth import name type defined in the PE-COFF spec, `IMPORT_ORDINAL`. This case is already handled by the `#[link_ordinal]` attribute. While it could be merged into `import_type_name`, that would not make sense as `#[link_ordinal]` provides per-function information (namely the ordinal itself).

Design decisions (these match the MCP linked above):
* For GNU, `decorated` matches the PE Spec and MSVC rather than the default behavior of `dlltool` (i.e., there will be a leading `_` for `stdcall`).
* If `import_name_type` is not present, we will keep our current behavior of matching the environment (MSVC vs GNU) default for decorating.
* Using `import_name_type` on architectures other than 32bit x86 will result in an error.
* Using `import_name_type` with link kinds other than `"raw-dylib"` will result in an error.
@Dylan-DPC
Copy link
Member

failed in rollup

@bors r-

@bors rollup=never

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2022
@dpaoliello
Copy link
Contributor Author

@Dylan-DPC - I don't believe that failure is related to this change: the code that I added should only be enabled if someone actually uses the import_name_type property on the #[link] attribute, and that can only be used on x86 Windows.

@JohnTitor
Copy link
Member

Yeah, @dpaoliello is right, @bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Aug 25, 2022

📌 Commit f847388 has been approved by wesleywiser

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 25, 2022
@bors
Copy link
Contributor

bors commented Aug 26, 2022

⌛ Testing commit f847388 with merge b771848c3bda7198ca2aa39d18b62bf36e0de759...

@bors
Copy link
Contributor

bors commented Aug 26, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 26, 2022
@rust-log-analyzer

This comment has been minimized.

@dpaoliello
Copy link
Contributor Author

@wesleywiser build has been fixed, can we please retry?

@wesleywiser
Copy link
Member

Sure!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 26, 2022

📌 Commit cc49c3e has been approved by wesleywiser

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 Aug 26, 2022
@bors
Copy link
Contributor

bors commented Aug 27, 2022

⌛ Testing commit cc49c3e with merge 9845f4c...

@bors
Copy link
Contributor

bors commented Aug 27, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 9845f4c to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9845f4c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.5% [2.1%, 2.9%] 3
Improvements ✅
(primary)
-2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)
-4.4% [-5.8%, -3.8%] 5
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.2% [-3.9%, -2.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.2% [-3.9%, -2.5%] 2

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

raw-dylib feature does not support all cases for stdcall functions on i686-pc-windows-*