Skip to content

Restrict linker version script of proc-macro crates to just its two symbols #114470

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Aug 4, 2023

Restrict linker version script of proc-macro crates to just the two symbols of each proc-macro crate.

The main known effect of doing this is to stop including #[no_mangle] symbols in the linker version script.

Background:

The combination of a proc-macro crate with an import of another crate that itself exports a no_mangle function was broken for a period of time, because:

Fix #111888
Fix #99978.

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2023

r? @davidtwco

(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 Aug 4, 2023
@pnkfelix
Copy link
Member Author

pnkfelix commented Aug 4, 2023

r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned davidtwco Aug 4, 2023
@wesleywiser wesleywiser added A-linkage Area: linking into static, shared libraries and binaries beta-nominated Nominated for backporting to the compiler in the beta channel. labels Aug 4, 2023
@rust-log-analyzer

This comment has been minimized.

fix to test as proposed by wesleywiser

Co-authored-by: Wesley Wiser <wwiser@gmail.com>
@petrochenkov petrochenkov self-assigned this Aug 4, 2023
@petrochenkov petrochenkov removed their assignment Aug 5, 2023
@bjorn3
Copy link
Member

bjorn3 commented Aug 7, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 7, 2023

📌 Commit a2058dd has been approved by bjorn3

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 7, 2023
@wesleywiser
Copy link
Member

@bors p=1

Bumping priority as this fixes a P-high regression and is beta-nominated.

@bors
Copy link
Collaborator

bors commented Aug 7, 2023

⌛ Testing commit a2058dd with merge 0c68940e5658fa62941986c021cc6b447f48e281...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Aug 7, 2023

💔 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 7, 2023
@pnkfelix
Copy link
Member Author

pnkfelix commented Aug 8, 2023

To only update this specific test, also pass `--test-args proc-macro/no-mangle-in-proc-macro-issue-111888.rs`

error: 1 errors occurred comparing output.
status: exit status: 0
command: RUSTC_ICE="0" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/proc-macro/no-mangle-in-proc-macro-issue-111888.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=wasm32-unknown-unknown" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/proc-macro/no-mangle-in-proc-macro-issue-111888" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/wasm32-unknown-unknown/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/proc-macro/no-mangle-in-proc-macro-issue-111888/auxiliary"
--- stderr -------------------------------
warning: dropping unsupported crate type `proc-macro` for target `wasm32-unknown-unknown`

warning: 1 warning emitted

Whoops, I guess I need to add wasm32-unknown-unknown to the targets I include in my local testing.

I'm skimming and I'm not seeing a clear pattern in how the other proc-macro crates in this directory avoid hitting this error. I'm guessing most of them are handling it via // force-host (and others via // ignore-wasm32), but it is strange that not all of the crate_type = "proc-macro" include at least one of those two header entries...

(oh, maybe the ones that don't need either of those two header entries are all compile-fail tests anyway... but derive-test.rs is still an exception to all of those conditions... I'm guessing --test is what sidesteps it in that case, I have no idea what --test will do on a proc-macro crate...)

@pnkfelix
Copy link
Member Author

pnkfelix commented Aug 8, 2023

@bors r=bjorn3

@bors
Copy link
Collaborator

bors commented Aug 8, 2023

📌 Commit a2a7f27 has been approved by bjorn3

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 8, 2023
@bjorn3
Copy link
Member

bjorn3 commented Aug 8, 2023

I'm guessing --test is what sidesteps it in that case, I have no idea what --test will do on a proc-macro crate...

--test will force a crate type of bin. See collect_crate_types in rustc_interface::util.

@bors
Copy link
Collaborator

bors commented Aug 8, 2023

⌛ Testing commit a2a7f27 with merge b442d5cfa218c1af27110b73fe6e86e4dcb4ffe5...

@bors
Copy link
Collaborator

bors commented Aug 8, 2023

💔 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 8, 2023
@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bjorn3
Copy link
Member

bjorn3 commented Aug 8, 2023

thread 'main' panicked at 'client.read_exact(&mut header) failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:310:9

@bors retry spurious connection error for remote test client

@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 8, 2023
@bors
Copy link
Collaborator

bors commented Aug 9, 2023

⌛ Testing commit a2a7f27 with merge a946c1e...

@bors
Copy link
Collaborator

bors commented Aug 9, 2023

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing a946c1e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 9, 2023
@bors bors merged commit a946c1e into rust-lang:master Aug 9, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 9, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a946c1e): 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.0% [0.3%, 1.6%] 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [2.5%, 2.6%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.1% [-3.1%, -3.1%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.766s -> 633.338s (0.25%)

@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 10, 2023
@cuviper cuviper mentioned this pull request Aug 12, 2023
@cuviper cuviper modified the milestones: 1.73.0, 1.72.0 Aug 12, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 12, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2023
[beta] backport

* Restrict linker version script of proc-macro crates to just its two symbols rust-lang#114470
* bootstrap: config: fix version comparison bug rust-lang#114440
* lint/ctypes: only try normalize rust-lang#113921
* Avoid tls access while iterating through mpsc thread entries rust-lang#113861
* Substitute types before checking inlining compatibility. rust-lang#113802
* Revert "fix: bug etc/bash_complettion -> src/etc/... to avoid copy error" rust-lang#113579
* lint/ctypes: fix () return type checks rust-lang#113457
* Rename and allow cast_ref_to_mut lint rust-lang#113422
* Ignore flaky clippy tests. rust-lang#113621

r? cuviper
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries beta-accepted Accepted for backporting to the compiler in the beta channel. 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