Reuse CrateNum for proc-macro crates even when cross-compiling #86876

jyn514 · 2021-07-05T01:32:41Z

Proc-macros are always compiled for the host, so this should be the same
in every way as recompiling the crate.

I am not sure why the previous code special-cased the target, since the
compiler properly gives an error when trying to load a crate for a
different host:

error[E0461]: couldn't find crate `dependency` with expected target triple x86_64-unknown-linux-gnu
  --> /home/joshua/rustc4/src/test/ui/cfg-dependent.rs:8:2
   |
LL |     dependency::is_64();
   |     ^^^^^^^^^^
   |
   = note: the following crate versions were found:
           crate `dependency`, target triple i686-unknown-linux-gnu: /home/joshua/rustc4/build/x86_64-unknown-linux-gnu/test/ui/cfg-dependent/auxiliary/libdependency.so

I think another possible fix is to remove the check altogether. But I'm
not sure, and this fix works, so I'm not making the larger change here.

Fixes #56935.

r? @petrochenkov cc @alexcrichton

jyn514 · 2021-07-05T01:35:21Z

For the record, the check was originally added in 5f295f2.

bjorn3 · 2021-07-05T09:50:41Z

compiler/rustc_metadata/src/creader.rs

@@ -598,7 +598,10 @@ impl<'a> CrateLoader<'a> {
        // don't want to match a host crate against an equivalent target one
        // already loaded.
        let root = library.metadata.get_root();
-        Ok(Some(if locator.triple == self.sess.opts.target_triple {
+        // Proc-macro crates are always compiled for the host target, so they should be reused even if we're cross-compiling.


How does this interact with -Zdual-proc-macro? It is used by bootstrap to ensure that cross-compilation of rustc works fine. It causes both the host and target version of a proc macro to be registered in the crate metadata allowing either to be used at runtime depending on if rustc runs on the original host or the target.

I'm not sure. How can I test?

The for sure test would be to try cross-compiling rustc and then try to compile a program linking against rustc_driver using the cross-compiled rustc. Alternatively I think the following would also work:

compile my_proc_macro for host in dir a

compile my_proc_macro for target with the same filename in dir b

compile my_crate for target with the host the host my_proc_macro as --extern and both a and b as -L and -Zdual-proc-macros as argument. (make sure to use my_proc_macro; otherwise rustc won't actually load it)

try to link against my_crate using a compiler for the target, but the same version of rustc.

I tried unsuccessfully to test this :( https://zulip-archive.rust-lang.org/131828tcompiler/56888HowdoIcrosscompilerustcforadifferenthost.html

petrochenkov · 2021-07-14T12:58:35Z

I'd be happy with any fix that passes through existing rustc bootstrap CI and new tests from #56935.
This is a temporary hack anyway, neither the target nor proc macro checks don't make any sense to me.

(I don't know right now what is the proper solution, but it will require a refactoring. I started such refactoring (#56935 (comment)) and continuing it is still in my work queue, but I never get to it due to review queue being overloaded (rust-lang/compiler-team#444).)

petrochenkov · 2021-07-14T13:00:57Z

compiler/rustc_metadata/src/creader.rs

@@ -598,7 +598,10 @@ impl<'a> CrateLoader<'a> {
        // don't want to match a host crate against an equivalent target one
        // already loaded.
        let root = library.metadata.get_root();
-        Ok(Some(if locator.triple == self.sess.opts.target_triple {
+        // Proc-macro crates are always compiled for the host target, so they should be reused even if we're cross-compiling.


It's better to remove this comment or replace it with something like "we don't know what this condition means".
Right now it may look plausible to an unsuspecting reader, but it doesn't really make sense.

Proc-macros are always compiled for the host, so this should be the same in every way as recompiling the crate. I am not sure why the previous code special-cased the target, since the compiler properly gives an error when trying to load a crate for a different host: ``` error[E0461]: couldn't find crate `dependency` with expected target triple x86_64-unknown-linux-gnu --> /home/joshua/rustc4/src/test/ui/cfg-dependent.rs:8:2 | LL | dependency::is_64(); | ^^^^^^^^^^ | = note: the following crate versions were found: crate `dependency`, target triple i686-unknown-linux-gnu: /home/joshua/rustc4/build/x86_64-unknown-linux-gnu/test/ui/cfg-dependent/auxiliary/libdependency.so ``` I think another possible fix is to remove the check altogether. But I'm not sure, and this fix works, so I'm not making the larger change here.

jyn514 · 2021-07-15T00:40:08Z

I'd be happy with any fix that passes through existing rustc bootstrap CI and new tests from #56935.

Added in src/test/ui/crate-loading/cross-compiled-proc-macro.rs.

@bors r=petrochenkov rollup=never (subtle code that no one really understands)

bors · 2021-07-15T00:40:10Z

📌 Commit 68b9598 has been approved by petrochenkov

bors · 2021-07-15T02:39:43Z

⌛ Testing commit 68b9598 with merge b919797...

bors · 2021-07-15T05:22:51Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing b919797 to master...

rust-lang/rust#56935 was closed via rust-lang/rust#86876. Suggested-by: bjorn3 <bjorn3_gh@protonmail.com> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

jyn514 added A-resolve Area: Path resolution A-cross Area: Cross compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-proc-macros Area: Procedural macros labels Jul 5, 2021

rust-highfive assigned petrochenkov Jul 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2021

This comment has been minimized.

Sign in to view

jyn514 force-pushed the 56935-target-crate-num branch from 6882454 to 9045650 Compare July 5, 2021 02:00

bjorn3 reviewed Jul 5, 2021

View reviewed changes

petrochenkov reviewed Jul 14, 2021

View reviewed changes

petrochenkov 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 Jul 14, 2021

jyn514 force-pushed the 56935-target-crate-num branch from 9045650 to 68b9598 Compare July 15, 2021 00:37

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 Jul 15, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2021

bors merged commit b919797 into rust-lang:master Jul 15, 2021

rustbot added this to the 1.55.0 milestone Jul 15, 2021

jyn514 deleted the 56935-target-crate-num branch July 15, 2021 12:07

jyn514 mentioned this pull request Jul 20, 2021

unexpected panic in rustc #87286

Open

ojeda mentioned this pull request Jan 22, 2022

rustc wanted features & bugfixes Rust-for-Linux/linux#355

Open

47 tasks

jyn514 mentioned this pull request Apr 18, 2022

rustdoc: Optimize and refactor doc link resolution #96135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse CrateNum for proc-macro crates even when cross-compiling #86876

Reuse CrateNum for proc-macro crates even when cross-compiling #86876

jyn514 commented Jul 5, 2021

jyn514 commented Jul 5, 2021

This comment has been minimized.

bjorn3 Jul 5, 2021

jyn514 Jul 5, 2021

bjorn3 Jul 5, 2021

jyn514 Jul 5, 2021

petrochenkov commented Jul 14, 2021

petrochenkov Jul 14, 2021

jyn514 commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

Reuse CrateNum for proc-macro crates even when cross-compiling #86876

Reuse CrateNum for proc-macro crates even when cross-compiling #86876

Conversation

jyn514 commented Jul 5, 2021

jyn514 commented Jul 5, 2021

This comment has been minimized.

bjorn3 Jul 5, 2021

Choose a reason for hiding this comment

jyn514 Jul 5, 2021

Choose a reason for hiding this comment

bjorn3 Jul 5, 2021

Choose a reason for hiding this comment

jyn514 Jul 5, 2021

Choose a reason for hiding this comment

petrochenkov commented Jul 14, 2021

petrochenkov Jul 14, 2021

Choose a reason for hiding this comment

jyn514 commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021