Fix linking for symbols starting with ? on i686-pc-windows-msvc #130808

checkraisefold · 2024-09-25T02:39:00Z

When using the export_name attribute to specifically export a symbol beginning with a question mark on the i686-pc-windows-msvc target, that symbol will fail to link and throw a linker error 100% of the time.
Issue writeup.

Closes #44282

I'm not sure if this is a proper solution, but LLVM does the same check which causes this issue, and is applied to all 32- and 64-bit Windows COFF objects (maybe the same patch should be applied for 64 bit windows as well then?). I am more unsure of whether this is the proper place for such a solution (and if the exact conditions of is_like_windows are proper for this usecase), or if the underscore should be stripped elsewhere, but it seems like the most correct place.

I'm also unsure if there are any backwards compatibility ramifications here. There shouldn't be, because binaries with exported symbols starting with ? for this target failed to link because of this issue anyway, but still.

try-job: i686-mingw

rustbot · 2024-09-25T02:39:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TaKO8Ki (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

checkraisefold · 2024-09-25T02:54:20Z

Just tested this patch with rust-lld; the original issue exists with lld being used as well, not just MSVC's linker, and is fixed by this patch.

jieyouxu · 2024-09-25T06:18:21Z

cc @bjorn3, @petrochenkov and @ChrisDenton, in case anyone has any clues about this 😅

compiler/rustc_codegen_ssa/src/back/symbol_export.rs

jieyouxu · 2024-11-16T08:50:37Z

Rerolling a reviewer. r? compiler

alex-semenyuk · 2024-12-28T09:42:34Z

@checkraisefold
From wg-triage. Could you please take a look question above

rustbot · 2024-12-31T05:30:55Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

checkraisefold · 2024-12-31T05:36:18Z

Apologies for the time it took to get to this - aside from being busy, for a relative Rust newbie (and contributing to the compiler, lol) especially the testing harness is very intimidating and it takes a while to iterate on my beautiful Ryzen 5 3600.

I was hoping to be able to fit this into a codegen or assembly test with some FileCheck annotations, but the technical difficulties of that are very annoying compared to a general run-make test - the compilation fails without this fix, which I tested, and it's a whole lot easier to implement.

checkraisefold · 2024-12-31T08:33:47Z

Seeing as the test no longer sucks-
@rustbot review

Additionally, run-make label can be removed

cjgillot · 2025-01-08T20:32:05Z

r? compiler

compiler/rustc_codegen_ssa/src/back/symbol_export.rs

Co-authored-by: David Wood <[email protected]>

davidtwco · 2025-01-24T15:15:29Z

@bors r+ rollup

bors · 2025-01-24T15:15:32Z

📌 Commit 9c3b53d has been approved by davidtwco

It is now in the queue for this repository.

…nking, r=davidtwco Fix linking for symbols starting with ? on i686-pc-windows-msvc When using the `export_name` attribute to specifically export a symbol beginning with a question mark on the `i686-pc-windows-msvc` target, that symbol will fail to link and throw a linker error 100% of the time. [Issue writeup.](rust-lang#44282 (comment)) Closes rust-lang#44282 I'm not sure if this is a proper solution, but [LLVM does the same check](https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Mangler.cpp#L48-L49) which causes this issue, and is applied to [all 32- and 64-bit Windows COFF objects](https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/DataLayout.h#L255-L257) (maybe the same patch should be applied for 64 bit windows as well then?). I am *more* unsure of whether this is the proper place for such a solution (and if the exact conditions of is_like_windows are proper for this usecase), or if the underscore should be stripped elsewhere, but it seems like the most correct place. I'm also unsure if there are any backwards compatibility ramifications here. There shouldn't be, because binaries with exported symbols starting with `?` for this target failed to link because of this issue anyway, but still.

Rollup of 7 pull requests Successful merges: - rust-lang#130808 (Fix linking for symbols starting with ? on i686-pc-windows-msvc) - rust-lang#133138 (Target modifiers (special marked options) are recorded in metainfo) - rust-lang#133154 (Reword resolve errors caused by likely missing crate in dep tree) - rust-lang#135707 (Shorten linker output even more when `--verbose` is not present) - rust-lang#135764 (Fix tests on LLVM 20) - rust-lang#135785 (use `PassMode::Direct` for vector types on `s390x`) - rust-lang#135818 (tests: Port `translation` to rmake.rs) Failed merges: - rust-lang#133951 (Make the wasm_c_abi future compat warning a hard error) r? `@ghost` `@rustbot` modify labels: rollup try-job: aarch64-apple try-job: i686-mingw try-job: x86_64-gnu-llvm-19-3

…nking, r=davidtwco Fix linking for symbols starting with ? on i686-pc-windows-msvc When using the `export_name` attribute to specifically export a symbol beginning with a question mark on the `i686-pc-windows-msvc` target, that symbol will fail to link and throw a linker error 100% of the time. [Issue writeup.](rust-lang#44282 (comment)) Closes rust-lang#44282 I'm not sure if this is a proper solution, but [LLVM does the same check](https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Mangler.cpp#L48-L49) which causes this issue, and is applied to [all 32- and 64-bit Windows COFF objects](https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/DataLayout.h#L255-L257) (maybe the same patch should be applied for 64 bit windows as well then?). I am *more* unsure of whether this is the proper place for such a solution (and if the exact conditions of is_like_windows are proper for this usecase), or if the underscore should be stripped elsewhere, but it seems like the most correct place. I'm also unsure if there are any backwards compatibility ramifications here. There shouldn't be, because binaries with exported symbols starting with `?` for this target failed to link because of this issue anyway, but still.

Rollup of 6 pull requests Successful merges: - rust-lang#130808 (Fix linking for symbols starting with ? on i686-pc-windows-msvc) - rust-lang#133154 (Reword resolve errors caused by likely missing crate in dep tree) - rust-lang#135707 (Shorten linker output even more when `--verbose` is not present) - rust-lang#135764 (Fix tests on LLVM 20) - rust-lang#135785 (use `PassMode::Direct` for vector types on `s390x`) - rust-lang#135818 (tests: Port `translation` to rmake.rs) r? `@ghost` `@rustbot` modify labels: rollup try-job: aarch64-apple try-job: i686-mingw try-job: x86_64-gnu-llvm-19-3

matthiaskrgr · 2025-01-25T12:01:22Z

@bors r- rollup=iffy
#136049 (comment)

matthiaskrgr · 2025-01-25T12:02:08Z

@bors try

bors · 2025-01-25T12:03:19Z

⌛ Trying commit 9c3b53d with merge e53ded5...

…ing, r=<try> Fix linking for symbols starting with ? on i686-pc-windows-msvc When using the `export_name` attribute to specifically export a symbol beginning with a question mark on the `i686-pc-windows-msvc` target, that symbol will fail to link and throw a linker error 100% of the time. [Issue writeup.](rust-lang#44282 (comment)) Closes rust-lang#44282 I'm not sure if this is a proper solution, but [LLVM does the same check](https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Mangler.cpp#L48-L49) which causes this issue, and is applied to [all 32- and 64-bit Windows COFF objects](https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/DataLayout.h#L255-L257) (maybe the same patch should be applied for 64 bit windows as well then?). I am *more* unsure of whether this is the proper place for such a solution (and if the exact conditions of is_like_windows are proper for this usecase), or if the underscore should be stripped elsewhere, but it seems like the most correct place. I'm also unsure if there are any backwards compatibility ramifications here. There shouldn't be, because binaries with exported symbols starting with `?` for this target failed to link because of this issue anyway, but still. try-job: i686-mingw

bors · 2025-01-25T13:55:05Z

☀️ Try build successful - checks-actions
Build commit: e53ded5 (e53ded5d975c4b7b232e5675ef5ed71e50e8d2f1)

matthiaskrgr · 2025-01-25T14:10:50Z

Hm, I guess that this will clash with #135707 then 🤔
@bors=davidtwco

jyn514 · 2025-01-25T20:59:15Z

Hm, I guess that this will clash with #135707 then 🤔

it shouldn't, #135707 only compares the linker invocation on x64 linux because it's too fragile to do anything else.

rustbot assigned TaKO8Ki Sep 25, 2024

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 Sep 25, 2024

jieyouxu added O-windows-msvc Toolchain: MSVC, Operating system: Windows A-linkage Area: linking into static, shared libraries and binaries labels Sep 25, 2024

jieyouxu reviewed Sep 25, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/back/symbol_export.rs Outdated Show resolved Hide resolved

rustbot assigned nnethercote and unassigned TaKO8Ki Nov 16, 2024

nnethercote 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2024

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Dec 30, 2024

This comment has been minimized.

Sign in to view

checkraisefold force-pushed the fix-questionmark-linking branch from a79b6c2 to 2ec1dce Compare December 30, 2024 05:44

rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Dec 30, 2024

checkraisefold force-pushed the fix-questionmark-linking branch from 2ec1dce to 6994ee3 Compare December 30, 2024 05:47

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Dec 31, 2024

This comment has been minimized.

Sign in to view

checkraisefold added 3 commits December 31, 2024 00:31

Fix linking for symbols starting with ?

8099510

Add a test

784d47b

Significant test improvements

a4c628c

checkraisefold force-pushed the fix-questionmark-linking branch from c84f1b7 to a4c628c Compare December 31, 2024 08:33

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 31, 2024

rustbot unassigned nnethercote Dec 31, 2024

rustbot assigned davidtwco and unassigned cjgillot Jan 8, 2025

davidtwco reviewed Jan 13, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/back/symbol_export.rs Outdated Show resolved Hide resolved

Match LLVM behavior more closely

f81ebfb

Co-authored-by: David Wood <[email protected]>

This comment has been minimized.

Sign in to view

Accept tidy changes

bf36338

This comment has been minimized.

Sign in to view

Apply tidy suggestion (2)

9c3b53d

davidtwco approved these changes Jan 24, 2025

View reviewed changes

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 Jan 24, 2025

matthiaskrgr mentioned this pull request Jan 25, 2025

Rollup of 7 pull requests #136045

Closed

matthiaskrgr mentioned this pull request Jan 25, 2025

Rollup of 6 pull requests #136049

Closed

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 Jan 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix linking for symbols starting with ? on i686-pc-windows-msvc #130808

Fix linking for symbols starting with ? on i686-pc-windows-msvc #130808

checkraisefold commented Sep 25, 2024 •

edited by matthiaskrgr

Loading

rustbot commented Sep 25, 2024

checkraisefold commented Sep 25, 2024

jieyouxu commented Sep 25, 2024

jieyouxu commented Nov 16, 2024

alex-semenyuk commented Dec 28, 2024

This comment has been minimized.

rustbot commented Dec 31, 2024

checkraisefold commented Dec 31, 2024 •

edited

Loading

This comment has been minimized.

checkraisefold commented Dec 31, 2024 •

edited

Loading

cjgillot commented Jan 8, 2025

This comment has been minimized.

This comment has been minimized.

davidtwco commented Jan 24, 2025

bors commented Jan 24, 2025

matthiaskrgr commented Jan 25, 2025

matthiaskrgr commented Jan 25, 2025

bors commented Jan 25, 2025

bors commented Jan 25, 2025

matthiaskrgr commented Jan 25, 2025

jyn514 commented Jan 25, 2025 •

edited

Loading

Fix linking for symbols starting with ? on i686-pc-windows-msvc #130808

Are you sure you want to change the base?

Fix linking for symbols starting with ? on i686-pc-windows-msvc #130808

Conversation

checkraisefold commented Sep 25, 2024 • edited by matthiaskrgr Loading

rustbot commented Sep 25, 2024

checkraisefold commented Sep 25, 2024

jieyouxu commented Sep 25, 2024

jieyouxu commented Nov 16, 2024

alex-semenyuk commented Dec 28, 2024

This comment has been minimized.

rustbot commented Dec 31, 2024

checkraisefold commented Dec 31, 2024 • edited Loading

This comment has been minimized.

checkraisefold commented Dec 31, 2024 • edited Loading

cjgillot commented Jan 8, 2025

This comment has been minimized.

This comment has been minimized.

davidtwco commented Jan 24, 2025

bors commented Jan 24, 2025

matthiaskrgr commented Jan 25, 2025

matthiaskrgr commented Jan 25, 2025

bors commented Jan 25, 2025

bors commented Jan 25, 2025

matthiaskrgr commented Jan 25, 2025

jyn514 commented Jan 25, 2025 • edited Loading

checkraisefold commented Sep 25, 2024 •

edited by matthiaskrgr

Loading

checkraisefold commented Dec 31, 2024 •

edited

Loading

checkraisefold commented Dec 31, 2024 •

edited

Loading

jyn514 commented Jan 25, 2025 •

edited

Loading