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

1.30.0 fails to build for target powerpc-unknown-netbsd #55465

Closed
he32 opened this issue Oct 29, 2018 · 30 comments
Closed

1.30.0 fails to build for target powerpc-unknown-netbsd #55465

he32 opened this issue Oct 29, 2018 · 30 comments
Assignees
Labels
O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state O-netbsd Operating system: NetBSD O-PowerPC Target: PowerPC processors P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@he32
Copy link
Contributor

he32 commented Oct 29, 2018

I've cross-built and built natively rust 1.29.2 for (among others) powerpc-unknown-netbsd, and this appears to work fine. I'm now trying to upgrade to rust 1.30.0, and have managed to build it natively (on amd64 / x86_64) and cross-built for sparc64. However, the cross-build for the powerpc target fails with rust type mismatch compile errors, such as this:

   Compiling openssl v0.10.11
...
error[E0308]: mismatched types
  --> vendor/openssl/src/nid.rs:87:62
   |
87 |                 .map(|nameptr| str::from_utf8(CStr::from_ptr(nameptr).to_bytes()).unwrap())
   |                                                              ^^^^^^^ expected u8, found i8
   |
   = note: expected type `*const u8`
              found type `*mut i8`

error[E0308]: mismatched types
  --> vendor/openssl/src/nid.rs:98:62
   |
98 |                 .map(|nameptr| str::from_utf8(CStr::from_ptr(nameptr).to_bytes()).unwrap())
   |                                                              ^^^^^^^ expected u8, found i8
   |
   = note: expected type `*const u8`
              found type `*mut i8`

Another example is:

error[E0308]: mismatched types
  --> vendor/openssl/src/version.rs:63:24
   |
63 |         CStr::from_ptr(OpenSSL_version(OPENSSL_VERSION))
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u8, found i8

Now, what separates the NetBSD/powerpc target from amd64 and sparc64 is that on NetBSD/powerpc, the char type is unsigned char, whereas on the other two, it is signed char.

Has rust between 1.29.2 and 1.30.0 introduced code which makes assumptions about the signedness of the C type "char"? I've tried diff'ing the various openssl dirs between the two versions, but have unfortunately not found the problem.

@he32
Copy link
Contributor Author

he32 commented Oct 29, 2018

Oh, btw, the cross-build of rust 1.30.0 from NetBSD/amd64 to NetBSD/earmv7hf fails the same way, and NetBSD/earmv7hf also has char == unsigned char.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Oct 29, 2018
Upstream changes:

Language
 * Procedural macros are now available. These kinds of macros allow
   for more powerful code generation. There is a new chapter available
   in the Rust Programming Language book that goes further in depth.
 * You can now use keywords as identifiers using the raw identifiers
   syntax (r#), e.g. let r#for = true;
 * Using anonymous parameters in traits is now deprecated with a
   warning and will be a hard error in the 2018 edition.
 * You can now use crate in paths. This allows you to refer to the
   crate root in the path, e.g. use crate::foo; refers to foo in
   src/lib.rs.
 * Using a external crate no longer requires being prefixed with
   ::. Previously, using a external crate in a module without a
   use statement required let json = ::serde_json::from_str(foo);
   but can now be written as let json = serde_json::from_str(foo);.
 * You can now apply the #[used] attribute to static items to
   prevent the compiler from optimising them away, even if they
   appear to be unused, e.g. #[used] static FOO: u32 = 1;
 * You can now import and reexport macros from other crates with
   the use syntax. Macros exported with #[macro_export] are now
   placed into the root module of the crate. If your macro relies
   on calling other local macros, it is recommended to export with
   the #[macro_export(local_inner_macros)] attribute so users won't
   have to import those macros.
 * You can now catch visibility keywords (e.g. pub, pub(crate)) in
   macros using the vis specifier.
 * Non-macro attributes now allow all forms of literals, not just
   strings. Previously, you would write #[attr("true")], and you
   can now write #[attr(true)].
 * You can now specify a function to handle a panic in the Rust
   runtime with the #[panic_handler] attribute.

Compiler
 * Added the riscv32imc-unknown-none-elf target.
 * Added the aarch64-unknown-netbsd target

Libraries
 * ManuallyDrop now allows the inner type to be unsized.

Stabilized APIs
 * Ipv4Addr::BROADCAST
 * Ipv4Addr::LOCALHOST
 * Ipv4Addr::UNSPECIFIED
 * Ipv6Addr::LOCALHOST
 * Ipv6Addr::UNSPECIFIED
 * Iterator::find_map
 * The following methods are replacement methods for trim_left,
   trim_right, trim_left_matches, and trim_right_matches, which
   will be deprecated in 1.33.0:
 * str::trim_end_matches
 * str::trim_end
 * str::trim_start_matches
 * str::trim_start

Cargo
 * cargo run doesn't require specifying a package in workspaces.
 * cargo doc now supports --message-format=json. This is equivalent
   to calling rustdoc --error-format=json.
 * Cargo will now provide a progress bar for builds.

Misc
 * rustdoc allows you to specify what edition to treat your code
   as with the --edition option.
 * rustdoc now has the --color (specify whether to output color)
   and --error-format (specify error format, e.g. json) options.
 * We now distribute a rust-gdbgui script that invokes gdbgui with
   Rust debug symbols.
 * Attributes from Rust tools such as rustfmt or clippy are now
   available, e.g. #[rustfmt::skip] will skip formatting the next
   item.


Pkgsrc changest:
 * Explicitly list bootstrap kit version number for each kit we carry,
   so that one entry's version doesn't "bleed into" following kits.
 * Tweak for handling "earmv7hf" CPU type for NetBSD in the bootstrap.py
   script
 * Add two patches from Debian for sparc64; rust would generate code
   generating unaligned accesses, causing SIGBUS on sparc64
 * Update most of the bootstrap kits to version 1.29.2; need minimum
   1.29.0 to build 1.30.0.
 * Rust regrettably doesn't build for powerpc or earmv7hf in this version,
   most probably due to "char" being "unsigned char" on these platforms.
   Ref. rust-lang/rust#55465
@he32
Copy link
Contributor Author

he32 commented Oct 30, 2018

https://pastebin.com/UXUeEHdJ contains a more complete build error log, for 1 month forward.

@memoryruins memoryruins added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. O-netbsd Operating system: NetBSD O-PowerPC Target: PowerPC processors labels Oct 30, 2018
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 8, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

T-compiler triage. Tagged this stable-to-stable regression with T-compiler.

It needs a P-priority tag. I don't think powerpc-*-netbsd is a Tier 1 platform ... I don't know whether that means this should be considered P-medium ... it would probably be a good idea to at least double-check what happened here.

(It seems like it should in theory be relatively easy to make a standalone test for this and then bisect to what nightly injected it...)

@he32
Copy link
Contributor Author

he32 commented Nov 9, 2018

Note that this problem also appears when (cross-)building for NetBSD/earmv7hf.
That platform also has char == u_char. And I believe we would also see this when
building for NetBSD/aarch64; it has the same feature that char is unsigned.

@memoryruins memoryruins added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Nov 9, 2018
@he32
Copy link
Contributor Author

he32 commented Nov 28, 2018

I've been looking around, and have found the file src/vendor/libc/src/unix/bsd/netbsdlike/netbsd/mod.rs, which unconditionally defines c_char to i8. This file comes directly from the tar.gz file for rustc 1.30.1. However, I've not been able to find an entry on github for this exact file. I have however found src/liblibc/src/unix/bsd/netbsdlike/netbsd/mod.rs which is on github, and which is quite a bit newer than the src/vendor/ file; among the "missing" fixes is the commit which fixed the signedness of c_char on NetBSD for powerpc and arm, but apparently also other missing commits.

The question is therefore now: where does src/vendor/libc/src/unix/bsd/netbsdlike/netbsd/mod.rs come from, and why has it fallen so far behind the (presumed) original at src/liblibc/src/unix/bsd/netbsdlike/netbsd/mod.rs?

@memoryruins
Copy link
Contributor

memoryruins commented Nov 28, 2018

The commit for fixing the signedness is in rust-lang/libc#1057

vendor is also in the nightly tar.gz, except the folder is now at top-level rather than inside src.
It is still out of date in that vendor/libc.

edit: I assume it’s to provide a local copy of any crates.io deps when it’s shipped. Something may be still using an outdated libc. Forwarding to others who would know what stage the vendor folder is included.

edit 2: When the source tarball is produced, a builder on CI populates the vendor directory using cargo vendor

@he32
Copy link
Contributor Author

he32 commented Nov 28, 2018

The commit for fixing the signedness is in rust-lang/libc#1057

Yep, I know. It's a bit frustrating having to re-discover old and already-solved bugs.

I'm trying to copy over the contents from the "new" directory to the vendor/libc/ tree, and doctoring the src/vendor/libc/.cargo-checksum.json file by modifying the mod.rs checksum and adding the checksum for all the "new" files, but the build (cargo?) still insists on doing downloads in the build phase (and failing...), which it didn't before I messed with this. Is this because the "package" checksum is now wrong, and how is it (re)computed?

@memoryruins
Copy link
Contributor

memoryruins commented Nov 28, 2018

Is this because the "package" checksum is now wrong, and how is it (re)computed?

cc @alexcrichton

After grepping the source tarball for libc deps,

src/bootstrap/Cargo.toml
43:libc = "0.2"

src/librustc_codegen_ssa/Cargo.toml
19:libc = "0.2.43"

vendor/wait-timeout/Cargo.toml
21:libc = "0.2"

vendor/termion/Cargo.toml
13:libc = "0.2.8"

vendor/commoncrypto-sys/Cargo.toml
15:libc = "0.2"

src/tools/compiletest/Cargo.toml
20:libc = "0.2"

src/tools/cargo/Cargo.toml
41:libc = "0.2"

src/stdsimd/crates/stdsimd/Cargo.toml
23:libc = "0.2"

librustc_codegen_ssa is using an outdated libc.

src/librustc_codegen_ssa/Cargo.toml
19:libc = "0.2.43"

libc 0.2.43 was released hours before the patch for arm and powerpc was merged.

0.2.44 is now on crates.io

It would be better if librustc_codegen_ssa and the other deps in Cargo.lock were updated rather than you needing to doctor the files, especially if it's all that's needed.

@eddyb @bjorn3 could this version bump be included in the refactoring occurring in #56198? Could it be set to 0.2 like the rest of the crates in rustc?

edit: if the refactor might take awhile, then a version bump could be its own PR (and backported?)

@eddyb
Copy link
Member

eddyb commented Nov 28, 2018

edit: if the refactor might take awhile, then a version bump could be its own PR (and backported?)

Yeah that's better.

@memoryruins
Copy link
Contributor

memoryruins commented Nov 28, 2018

Sounds good. Guess it would need changing librustc_codegen_ssa's libc in Cargo.toml to 0.2 then running cargo update -p libc:0.2.44 for updating everything else in the lockfile.

@eddyb
Copy link
Member

eddyb commented Nov 28, 2018

@memoryleak47 Just change Cargo.toml and run ./x.py check (you can stop it after it changes Cargo.lock).

@memoryruins
Copy link
Contributor

memoryruins commented Nov 28, 2018

Ah, thanks! It needed to be set exactly to 0.2.44 then it updated places in Cargo.lock.

Also, I did a lackluster grep earlier as there are more places that are not updated (e.g. rls and rustfmt have 0.2.43 in their locks). Perhaps most importantly, src/liblibc submodule is 0.2.43.

Is there a preferred way for updating git submodules?

@he32
Copy link
Contributor Author

he32 commented Nov 28, 2018

librustc_codegen_ssa is using an outdated libc.

src/librustc_codegen_ssa/Cargo.toml
19:libc = "0.2.43"

libc 0.2.43 was released hours before the patch for arm and powerpc was merged.

0.2.44 is now on crates.io

I'm new to this area, but ... if I understand correctly, version 0.2.44 of libc should satisfy the version requirement libc = "0.2.43", as well as libc = "0.2" (and libc = "0.2.8" for that matter), again IIUC, the = operator in libc = "0.2.43" isn't strictly a "must be exactly equal to" operator, but merely a "must be API backward compatible with" operator, according to the semver rules, so it actually means libc >= "0.2.43" && libc < 1.0.0, where the ">=" and "<" operators have traditional semantics on lexicographical numbering. So the question then becomes what version of libc existed when rustc version 1.30.0 and 1.30.1 were packaged up and released? Looking at src/liblibc/ it seems that libc 0.2.44 existed at the time. If 0.2.44 existed, why wasn't it used in preference to 0.2.43? I'm probably just not understanding how a rustc release is put together.

It would be better if librustc_codegen_ssa was updated rather than you needing to doctor the files, especially if it's all that's needed.

I agree, that would be best. However, I'm just a poor sod of a packager, so work off the released .tar.gz files for rust, and don't know what's involved in updating librustc_codegen_ssa -- that's probably not something I can do myself(?) Besides, my personal curiosity is left with an unanswered question: must the "package" checksum be updated when adding new files (I'm guessing "yes"), and how is it computed? (I already have the generation of the individual file checksums sorted out.)

So for me the alternative is probably to wait for the next release, 1.31.0 (?), and hoping that it will have a fix for this problem?

@eddyb
Copy link
Member

eddyb commented Nov 28, 2018

@memoryruins I don't know about liblibc, but what I usually do is cd into that directory, git fetch a branch, then git checkout FETCH_HEAD (to switch to the just-fetched branch) and then go back to the Rust directory, and commit the changed submodule (don't run x.py in between, it will reset!).

If 0.2.44 existed, why wasn't it used in preference to 0.2.43? I'm probably just not understanding how a rustc release is put together.

It's because Cargo.lock exists, which ensures deterministic builds. The versions in use depend on it, and it's never fully recomputed AFAIK. A version existing and being compatible means nothing unless something triggers a recompute.

I think the problem is 0.2.44 didn't exist back when the crate was first added, and it's partly my fault for reusing Cargo.lock changes when rebasing instead of redoing them (which would pick up the latest version).

(Wait, how is rustc_codegen_ssa already in a release?! I thought it happened after 1.30)

@he32
Copy link
Contributor Author

he32 commented Nov 28, 2018

Perhaps most importantly, src/liblibc submodule is 0.2.43.

Really?!? Well, the contents of src/unix/bsd/netbsdlike/netbsd/ is at least different between src/vendor/libc/ and src/liblibc/ in 1.30.1, so they can't then both be 0.2.43. There's differences in the mod.rs file, and powerpc.rs only exists in one of them (the newer one). But, yes, I see that both directories' Cargo.toml say version = "0.2.43" which can't be ... accurate.

@eddyb
Copy link
Member

eddyb commented Nov 28, 2018

@he32 The submodule doesn't have to be a version released on crates.io.
Ideally we'd stop using submodules, which will solve this problem.

cc @alexcrichton

@memoryruins
Copy link
Contributor

memoryruins commented Nov 28, 2018

@eddyb thanks again for the tips! and yea,librustc_codegen_ssa was a mistake on my part earlier and does not appear to be the main part that was around before 1.30 that needs to be updated for this issue.

@he32
Copy link
Contributor Author

he32 commented Nov 28, 2018

@he32 The submodule doesn't have to be a version released on crates.io.

Ah. Makes it a bit more difficult to identify what you actually have, especially from the tar.gz file which isn't linked up to git...

Ideally we'd stop using submodules, which will solve this problem.

I'm guessing that won't happen short-term...

Anyway, thanks both of you for being patient and for working to narrow down this problem; I believe you now have enough info that this will be sorted out eventually.

@eddyb
Copy link
Member

eddyb commented Nov 28, 2018

@pnkfelix
Copy link
Member

pnkfelix commented Nov 29, 2018

after 21 days I still don't know what P-label to assign here. Nominating for discussion at T-compiler meeting.

(based on the very recent discussion, I am personally inclined to tag as P-high. If we don't get to it in today's meeting, that is probably what priority I will select.)

@pnkfelix
Copy link
Member

discussed at T-compiler meeting. tagging as P-high and assigning to self to take point on working out what's gone on here.

@pnkfelix pnkfelix self-assigned this Nov 29, 2018
@pnkfelix pnkfelix added the P-high High priority label Nov 29, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Dec 6, 2018

Okay, so I reviewed the discussion in this ticket.

There are a couple different issues:

Background: The libc version specified by librustc_codegen_ssa/Cargo.toml says 0.2.43, and the Cargo.lock at the root of the project has pinned us to 0.2.43. A quick ack reveals these other references to libc versions in Cargo.tomls:

src/tools/cargo/Cargo.toml:42:libc = "0.2"
src/tools/compiletest/Cargo.toml:20:libc = "0.2"
src/stdsimd/crates/stdsimd/Cargo.toml:27:libc = "0.2"
src/libpanic_abort/Cargo.toml:14:libc = { path = "../rustc/libc_shim" }
src/libunwind/Cargo.toml:16:libc = { path = "../rustc/libc_shim" }
src/libpanic_unwind/Cargo.toml:15:libc = { path = "../rustc/libc_shim" }
src/librustc_codegen_ssa/Cargo.toml:19:libc = "0.2.43"
src/dlmalloc/Cargo.toml:22:libc = { version = "0.2", default-features = false }
src/libstd/Cargo.toml:20:libc = { path = "../rustc/libc_shim" }
src/bootstrap/Cargo.toml:43:libc = "0.2"

So, some crates in the distribution are definitely using libc 0.2.43, which precedes the fix from rust-lang/libc#1057. @he32 has been working around this by manually doctoring the tarball.

It would almost certainly simplify @he32's life if we forced a version upgrade of our libc.

One way to do this would simply be to replace 0.2.43 with 0.2.44 in at least one of the Cargo.toml files, e.g. in src/librustc_codegen_ssa/Cargo.toml.

@eddyb has suggested that if we didn't use the crates.io libc, and instead linked directly to the github repository for rust-lang/libc, then we'd inherit such fixes automatically.

  • Update: No, I must have misinterpreted eddyb's suggestion, given later dialogue with eddyb.

I suspect that while @eddyb's suggestion is the right long term solution, it also sounds a little bit more contentious (and certainly a bit more effort).

So, here is a plan for addressing this:

  1. Make a trivial PR that changes the version number from 0.2.43 to 0.2.44, to fix this bug. Rebuilding atop such a PR should force a Cargo.lock update, which will also be part of the PR. We can debate on that PR whether or not to backport such a change.

  2. File an issue saying "we should point to the rust-lang/libc repo, not be tied to a specific crates.io libc version. Update: The struckout text is the opposite of what @eddyb wanted. I think he wants us to stop pointing at rust-lang/libc, and instead use the crates.io libc everywhere in the rust-lang/rust repo.

    • Maybe that issue will have to go through T-compiler RFC, I'm not sure.
  3. Assuming T-compiler agrees that the issue resulting from step 2 is a step we actually want to take, then we resolve that issue via a follow-up PR later on that rewrites things so that we don't point to the crates.io libc.

The main reason I want to break things down this way is that steps 2 and 3 are internal maintenance/process issues. They are not bugs in the same way that this issue (#55465) is.

@he32
Copy link
Contributor Author

he32 commented Dec 6, 2018 via email

@pnkfelix
Copy link
Member

pnkfelix commented Dec 6, 2018

Q for @eddyb: when you said "Ideally we'd stop using submodules" ... did you mean to say "ideally we'd stop using a crates.io hosted crate?"

Until I carefully re-read your comments, I had thought you were pushing for us to stop using a crates.io hosted crate, and instead point at a git submodule at the rust-lang/libc repo.

But my usual interpretation of the word "submodule" is that it means specifically a git submodule...
which means I should interpret your comment as saying we'd stop using git submodules to refer to libc versions. But then you pointed to src/liblibc, and I thought in that comment that you were giving that as an example of what we should do. (And src/liblibc is a git submodule; its one that points to https://github.com/rust-lang/libc.git ...)

@eddyb
Copy link
Member

eddyb commented Dec 6, 2018

@pnkfelix I definitely only meant git submodule by "submodule".
In the comment where I refer to src/liblibc, I'm only explaining how you can figure out what the submodule (which we'd ideally get rid of) was at, for a given released version.

@pnkfelix
Copy link
Member

@eddyb not sure you saw it but PR #56092 might be of interest to you

kennytm added a commit to kennytm/rust that referenced this issue Dec 13, 2018
…rsion, r=alexcrichton

Update libc version required by rustc

This is meant to be an easy-to-backport fix for rust-lang#55465
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
…rsion, r=alexcrichton

Update libc version required by rustc

This is meant to be an easy-to-backport fix for rust-lang#55465
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
…rsion, r=alexcrichton

Update libc version required by rustc

This is meant to be an easy-to-backport fix for rust-lang#55465
@pietroalbini
Copy link
Member

This will be fixed in Rust 1.31.1, scheduled to be released on Thursday 20th.

@he32
Copy link
Contributor Author

he32 commented Dec 17, 2018 via email

@pnkfelix
Copy link
Member

T-compiler triage: I'll leave this open until the issue filer (he32) can confirm its fixed.

@he32
Copy link
Contributor Author

he32 commented Dec 21, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state O-netbsd Operating system: NetBSD O-PowerPC Target: PowerPC processors P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants