Repository navigation
Doesn't build on x86_64 musl #4
Description
Hi! I'm trying to build firefox 87.0 on a musl system, and this crate won't build there, which stops the whole build process.
I opened rust-lang/libc#2121 to solve the libc constants, but I'm not sure how to fix the getregs dependency.
I have added this patch, which copies the actual restriction used by the nix crate (would be nice if Rust allowed querying for function availability instead of having to copy assignments):
--- third_party/rust/minidump_writer_linux/src/linux_ptrace_dumper.rs
+++ third_party/rust/minidump_writer_linux/src/linux_ptrace_dumper.rs
@@ -108,7 +108,7 @@ impl LinuxPtraceDumper {
Err(_) => continue,
}
}
- #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+ #[cfg(all(target_os = "linux", any(target_arch = "x86_64", target_arch = "x86"), target_env = "gnu"))]
{
// On x86, the stack pointer is NULL or -1, when executing trusted code in
// the seccomp sandbox. Not only does this cause difficulties down the line
but I still get the following build failure:
5:44.40 Compiling minidump_writer_linux v0.1.0 (https://github.com/msirringhaus/minidump_writer_linux.git?rev=9191af36343846b2c7ada65b9602b481b717c4d8#9191af36)
5:44.81 error[E0425]: cannot find function `getregs` in module `ptrace`
5:44.81 --> /builddir/firefox-87.0/third_party/rust/minidump_writer_linux/src/thread_info/thread_info_x86.rs:85:61
5:44.81 |
5:44.81 85 | let regs = Self::getregset(tid).or_else(|_| ptrace::getregs(unistd::Pid::from_raw(tid)))?;
5:44.81 | ^^^^^^^ not found in `ptrace`
5:45.53 error: aborting due to previous error
I can't figure out how to clean up that file to work on musl, it seems just throwing in a #[cfg ...] won't be enough.
Activity
firefox: update to 87.0.
firefox: update to 87.0.
msirringhaus commentedon Mar 24, 2021
Thanks for reporting this.
Yes, the missing
getregsis also a problem for other platforms, unfortunately.The quickest workaround to build Firefox would probably be to switch back to the old breakpad code.
In
toolkit/moz.configuregrep forMOZ_OXIDIZED_BREAKPADor ratherdef oxidized_breakpad(target)and return False there in case musl is used.At least until we figure a way out to do this properly in this crate.
ericonr commentedon Mar 24, 2021
Indeed, I'm looking into this now. It seems
oxidized_breakpadcan return true even when crashreporter is disabled, which is kind of a bug as well (firefox one, though). I'm patchingmoz.configurefor now, then. Thanks :)ericonr commentedon Mar 24, 2021
hm, maybe it will work in a new nix version?
https://github.com/nix-rust/nix/blob/7ed28204ec7de979d64802ee1621d5d09be61858/src/sys/ptrace/linux.rs#L188-L197
msirringhaus commentedon Mar 24, 2021
Yes, this plus your
libc-PR looks like it might already do the trick.To test, you could just pull this crate, update the
nix-version inCargo.tomland add the missingAT_*-definitions by hand in the files that use them?I don't have a musl-system at hand at the moment.
If it does work, there would need to be a bugreport at Mozilla, asking for a new version of
nix.Unfortunately, the vendored crates in Firefox are usually quite old as updating them has the potential of breaking other things.
So it seems to be done rarely.
ericonr commentedon Mar 24, 2021
It will probably work, given nix-rust/nix#1198, I can test in a bit!
Hm, I will try to create a bug report, haven't used the firefox bug tracker ever. Might be nice to fix nix so it works on x86-musl as well, not only x86_64.
msirringhaus commentedon Mar 24, 2021
A test would be good. Might be some other symbols missing.
If it works, just post here. I can file that bug for you on Mozilla-side.
One Mozilla employee already said, he would update it for us. And look into applying your
libc- patch to their vendored crate (or updatelibcaltogether)ericonr commentedon Mar 25, 2021
Good thing you asked for a test :)
I applied this diff
and got back:
I'm not sure how to proceed forward, but there seem to be two issues: a type mismatch and an API mismatch.
firefox: update to 87.0.
msirringhaus commentedon Mar 25, 2021
Ah yes, I remember the first one. There was some breaking API changes in
nix::ptrace.You can just do
ptrace::detach(pid, None).The second one is new to me. Maybe because the version of
libcimported by us and the one re-exported bynixdoesn't match.I'm not entirely sure, though. You could try replacing all
libc::user_regs_structwithnix::libc::user_regs_structinsidesrc/thread_info/thread_info_x86.rs(but potentially outside as well)firefox: update to 87.0.
ericonr commentedon Mar 25, 2021
Was indeed enough, but idk if this counts as punting the problem forward? Maybe most (all?) of the
libcusage should be in the form ofnix::libc?Whissi commentedon Mar 27, 2021
FYI, the proposed patch doesn't work:
ericonr commentedon Mar 27, 2021
@Whissi you're missing the previous patch that updates the nix version.
Whissi commentedon Mar 28, 2021
OK, I think I managed to create a patch for Firefox 87.0 with the change and all crate updates, https://dev.gentoo.org/~whissi/stuff/firefox-87.0-musl-minidump_writer_linux.patch.gz -- at least it will allow me to build on glibc x86_64. But on a msul system, it will fail:
ericonr commentedon Mar 28, 2021
Hm, have to wait for the libc release with those constants, I think (my patch pulled from libc git)... Maybe keeping a libc crate only for constants in here would help not being bound by the version nix is using.
msirringhaus commentedon Mar 29, 2021
By the way, I've submitted a patch to fix the bug that this crate was being built, even if crashreporter is disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1701623
gabrielesvelto commentedon Oct 22, 2021
Firefox is now vendoring libc 0.2.97 which should include the fix above and nix 0.15.0. Would that be enough to fix this?
msirringhaus commentedon Oct 22, 2021
No, from what I can tell the
getregs()in nix is only available since version 0.18.0.I've just spun up an Alpine linux VM to check this. If I bump nix to 0.18.0 and do some minor adjustments to the changed API, it builds.
But
cargo teststill fails, because of missing functions inlibc.msirringhaus commentedon Oct 22, 2021
Addendum: The missing function in
libcforcargo testisgetcontext(), which musl itself does not implement, apparently.So, we either have to require
libucontextto be installed and link against that somehow. Or look at our usages of them and try to patch them out for musl.tl;dr: nix version 0.18.0 should be enough for building this on musl systems, but not for running
cargo teston them.gabrielesvelto commentedon Oct 22, 2021
I see, thanks for checking!
Fix FreeBSD review findings: real crash context, OsString paths, stac…
Fix FreeBSD review findings: real crash context, OsString paths, stac…