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

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

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
Contributor

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
Contributor

bors commented Aug 7, 2023

⌛ Testing commit a2058dd with merge 0c68940e5658fa62941986c021cc6b447f48e281...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

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
Contributor

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
Contributor

bors commented Aug 8, 2023

⌛ Testing commit a2a7f27 with merge b442d5cfa218c1af27110b73fe6e86e4dcb4ffe5...

@bors
Copy link
Contributor

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
Contributor

bors commented Aug 9, 2023

⌛ Testing commit a2a7f27 with merge a946c1e...

@bors
Copy link
Contributor

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
11 checks passed
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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