Building Firefox with rustc 1.34.0-nightly fails with LLVM ERROR: broken function found. #58881
Comments
Centril
added
the
A-LLVM
jonas-schievink
added
T-compiler
|
Unfortuntely this probably isn't too actionable unless we can reduce it to the point where we can give it to upstream LLVM |
|
Ok, I tried to figure out why it wasn't reproducing outside of the build system, and manage to get some STR: |
|
v0.5.1 works, v0.5.0 does not. https://github.com/djg/cubeb-rs/compare/98f5a8b4..d18f114f cc @djg |
|
Well, the |
|
Ok, this test-case crashes for me: use std::os::raw::c_char;
pub type cubeb_log_callback = Option<unsafe extern "C" fn(*const c_char, ...)>;
extern "C" {
pub static g_cubeb_log_callback: cubeb_log_callback;
}
pub fn foo(index: u32) {
unsafe {
let op = "Adding";
let dev = "sink";
if let Some(log_callback) = g_cubeb_log_callback {
let cstr = ::std::ffi::CString::new("%s:%d: {} {} index {}\n").unwrap();
log_callback(cstr.as_ptr(), file!(), line!(), op, dev, index);
}
}
}
fn main() {
let args = ::std::env::args().count();
foo(args as u32);
}The code is completely unsound of course... Passing a static string into a C variadic function makes no sense. Maybe rustc should warn about that? |
|
This is the most reduced thing I could come up with: extern "C" {
pub static g_cubeb_log_callback: unsafe extern "C" fn(*const u8, ...);
}
fn main() {
unsafe {
let f = "file";
let op = "Adding";
let dev = "sink";
let cstr = b"foo\0";
g_cubeb_log_callback(cstr.as_ptr(), f, op, dev);
}
} |
|
Thanks for the reduction @emilio! I can indeed also reproduce with: # Today's nightly, with LLVM assertions
$ rustup-toolchain-install-master a9da8fc9c267c08cfdb8cf5b39da14f154d12939 --alt
$ rustc +a9da8fc9c267c08cfdb8cf5b39da14f154d12939-alt foo.rs
warning: `extern` block uses type `()` which is not FFI-safe: tuples have unspecified layout
--> foo.rs:2:38
|
2 | pub static g_cubeb_log_callback: unsafe extern "C" fn(*const u8, ...);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: #[warn(improper_ctypes)] on by default
= help: consider using a struct instead
Wrong types for attribute: byval inalloca nest noalias nocapture nonnull readnone readonly signext sret zeroext dereferenceable(1) dereferenceable_or_null(1)
call void (i8*, ...) %4(i8* %5, { i64, i64 } %10, { i64, i64 } %14, { i64, i64 } byval noalias nocapture dereferenceable(16) %18)
in function _ZN3foo4main17h0e6b3bfd37376208E
LLVM ERROR: Broken function found, compilation aborted!I'm gonna cc @rust-lang/compiler on this because this actually looks more like a codegen bug on our end than an LLVM bug, and LLVM may have just added more validation or something like that. |
|
Yeah, that sounds right. Thanks for taking a look Alex! FWIW, I worked around the bug in Firefox in https://bugzilla.mozilla.org/show_bug.cgi?id=1532645 (by updating the dependency to one that doesn't have the unsound code), so this is no longer blocking anyone, hopefully. |
|
looks like bad cidegen to me. dereferenceable attribute is put on an
argumant that has a non-pointer type. As other arguments have no attributes
at all i suspect that we end up setting attributes intended for other
arguments onto the last one.
…On Tue, Mar 5, 2019, 17:44 Alex Crichton ***@***.***> wrote:
Thanks for the reduction @emilio <https://github.com/emilio>! I can
indeed also reproduce with:
# Today's nightly, with LLVM assertions
$ rustup-toolchain-install-master a9da8fc --alt
$ rustc +a9da8fc9c267c08cfdb8cf5b39da14f154d12939-alt foo.rs
warning: `extern` block uses type `()` which is not FFI-safe: tuples have unspecified layout
--> foo.rs:2:38
|2 | pub static g_cubeb_log_callback: unsafe extern "C" fn(*const u8, ...);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: #[warn(improper_ctypes)] on by default
= help: consider using a struct instead
Wrong types for attribute: byval inalloca nest noalias nocapture nonnull readnone readonly signext sret zeroext dereferenceable(1) dereferenceable_or_null(1)
call void (i8*, ...) %4(i8* %5, { i64, i64 } %10, { i64, i64 } %14, { i64, i64 } byval noalias nocapture dereferenceable(16) %18)in function _ZN3foo4main17h0e6b3bfd37376208E
LLVM ERROR: Broken function found, compilation aborted!
I'm gonna cc @rust-lang/compiler
<https://github.com/orgs/rust-lang/teams/compiler> on this because this
actually looks more like a codegen bug on our end than an LLVM bug, and
LLVM may have just added more validation or something like that.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#58881 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AApc0oxA3OaHiKnAX0wqVFFcPvGQAk5lks5vTpDrgaJpZM4baocY>
.
|
rust-lang/rust#58881 doesn't repro with cubeb-backend v5.0.1, so use that so that other Rust nightly users don't get their builds busted. Differential Revision: https://phabricator.services.mozilla.com/D22092 --HG-- extra : moz-landing-system : lando
rust-lang/rust#58881 doesn't repro with cubeb-backend v5.0.1, so use that so that other Rust nightly users don't get their builds busted. Differential Revision: https://phabricator.services.mozilla.com/D22092
|
@emilio do you happen to know wether esr is affected as well? It has cubeb-0.4.1 pulled in, but I'm note sure if the logic here is that <=cubeb-0.5.0 is affected, or only =cubeb-0.5.0? |
|
@stefson I don't know off-hand, would need to test. |
|
triage: assuming P-high since it originated as a Firefox build problem. |
|
assigning to self for initial investigation. |
|
(leaving nominated for now to keep myself honest in case I somehow get to next week's meeting without looking at this. if I do investigate and it bears fruit I'll remove nomination at that time.) |
|
This version is a little bit smaller than @emilio's great reduction above: extern "C" {
fn variadic(arg: u8, ...);
}
fn main() {
unsafe {
let f = "file";
variadic(0, f, f, f);
}
}(The main feature here is that it demonstrates the problem without using a callback to a static variable, which I had mistakenly thought was an important part of replicating the problem.) |
|
Might this be related to the recent variadic function changes? cc @dlrobertson |
|
(FYI, bisection indicates this was injected between nightly-2019-02-28 and nightly-2019-03-01.) |
|
@varkor that theory sounds very plausible, given the following bors merges from the point I bisected: 350674b Auto merge of #58250 - Zoxc:rustc-interface-1, r=oli-obk |
|
It is very likely I caused this. I'll look into it |
|
tagging as regression from stable-to-nightly. |
pnkfelix
added
regression-from-stable-to-nightly
|
Appears to be an issue if a structure is passed to a C-variadic. E.g. extern "C" {
fn variadic(arg: u8, ...);
}
fn main() {
unsafe {
let f = b"file\0";
variadic(0, f.as_ptr(), f.as_ptr(), f.as_ptr()); // does not ICE
variadic(0, f, f, f); // will ICE
}
} |
Fix LLVM IR generated for C-variadic arguments It is possible to create malformed LLVM IR given variadic arguments that are aggregate types. This occurs due to improper tracking of the current argument in the functions list of arguments. Fixes: rust-lang#58881
Fix LLVM IR generated for C-variadic arguments It is possible to create malformed LLVM IR given variadic arguments that are aggregate types. This occurs due to improper tracking of the current argument in the functions list of arguments. Fixes: #58881
rust-lang/rust#58881 doesn't repro with cubeb-backend v5.0.1, so use that so that other Rust nightly users don't get their builds busted. Differential Revision: https://phabricator.services.mozilla.com/D22092 UltraBlame original commit: 9286348b9c33218e956417ce8675851ddbccaeaa
rust-lang/rust#58881 doesn't repro with cubeb-backend v5.0.1, so use that so that other Rust nightly users don't get their builds busted. Differential Revision: https://phabricator.services.mozilla.com/D22092 UltraBlame original commit: 9286348b9c33218e956417ce8675851ddbccaeaa
rust-lang/rust#58881 doesn't repro with cubeb-backend v5.0.1, so use that so that other Rust nightly users don't get their builds busted. Differential Revision: https://phabricator.services.mozilla.com/D22092 UltraBlame original commit: 9286348b9c33218e956417ce8675851ddbccaeaa
Unfortunately I haven't been able to reproduce outside of the Firefox build system. I assume this is a regression from #58408, cc @alexcrichton.
The text was updated successfully, but these errors were encountered: