Port the standard crates to PNaCl/NaCl. #29289
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -28,7 +28,7 @@ macro_rules! rtassert { | |||
|
|
|||
| pub mod args; | |||
| pub mod at_exit_imp; | |||
| pub mod backtrace; | |||
| #[cfg_attr(target_os = "nacl", allow(dead_code))] pub mod backtrace; | |||
There was a problem hiding this comment.
Could this be added as an inner attribute instead perhaps?
DiamondLovesYou
force-pushed
the
pnacl-std-crates
branch
3 times, most recently
from
fb25f74 to
32e400d
Compare
| pub fn black_box<T>(dummy: T) -> T { | ||
| // we need to "use" the argument in some way LLVM can't | ||
| // introspect. | ||
| unsafe {asm!("" : : "r"(&dummy))} | ||
| dummy | ||
| } | ||
| #[cfg(all(target_os = "nacl", target_arch = "le32"))] | ||
| #[inline(never)] | ||
| pub fn black_box<T>(dummy: T) -> T { dummy } |
There was a problem hiding this comment.
This is kind of bad. Is there any way at all to do this on NaCl?
Maybe we should switch to a black_box intrinsic, if we can do better inside rustc.
There was a problem hiding this comment.
This isn't a problem for NaCl which is native anyway, and thus supports asm!(); I think you're referring to PNaCl/JS.
I think you've made a fine point w.r.t. putting this in rustc so that its impl can be based off of allow_asm, I just wonder why #[inline(never)] isn't enough on it's own (and thus wouldn't need to use asm in the first place). For example:
pub fn main() {
#[inline(never)]
fn black_box<T>(dummy: T) -> T {
dummy
}
let two = black_box(2);
let five = black_box(5);
let ten = black_box(10);
assert_eq!(two * five, ten);
}This compiles to (optimized):
...
; Function Attrs: uwtable
define internal void @_ZN4main20ha51c51acd3994054eaaE() unnamed_addr #0 {
entry-block:
%ten = alloca i32, align 4
%addr_of = alloca i32, align 4
%0 = alloca %"2.core::fmt::Arguments", align 8
%1 = alloca [2 x %"2.core::fmt::ArgumentV1"], align 8
%2 = tail call fastcc i32 @_ZN4main9black_box21h10503287336754422282E(i32 2)
%3 = tail call fastcc i32 @_ZN4main9black_box21h10503287336754422282E(i32 5)
%4 = bitcast i32* %ten to i8*
call void @llvm.lifetime.start(i64 4, i8* %4)
%5 = tail call fastcc i32 @_ZN4main9black_box21h10503287336754422282E(i32 10)
store i32 %5, i32* %ten, align 4
%6 = mul i32 %3, %2
%7 = bitcast i32* %addr_of to i8*
call void @llvm.lifetime.start(i64 4, i8* %7)
store i32 %6, i32* %addr_of, align 4
%8 = icmp eq i32 %6, %5
br i1 %8, label %next-block, label %then-block-61-
...Is there some corner case I'm not aware of?
There was a problem hiding this comment.
black_box should be #[inline(always)] fwiw (LLVM will inline the current definition virtually always anyway), the call cost is supposed to be missing there.
I didn't notice the #[inline(never)] in your case, I thought you just left it being a noop, but disabling inlining should work, except, well: there are corner cases: try leaving only one black_box call for any type you use it with.
There was a problem hiding this comment.
inline(never) also hasn't worked historically when compiling with LTO. (I haven't looked into it for a long time, though.)
There was a problem hiding this comment.
I think this is fine for now in general, however, if benchmarking isn't the best experience on JS it's probably not the end of the world.
There was a problem hiding this comment.
I think this should be an LLVM intrinsic; by taking a reference to dummy, rustc inserts an alloca+store+load for it and passes the alloca to the asm call. While the alloca+store+load would normally be removed by mem2reg, because of the inline asm (which requires access to a pointer to the value), it can't because the asm reference forces the pointer value to be live. Also, just reading dummy (ie not &dummy) doesn't work either because constprop (in my earlier example) will do its thang (with sass), much to our chagrin.
I'll be sending a email to llvm-dev proposing llvm.black_box Soon(tm).
DiamondLovesYou
force-pushed
the
pnacl-std-crates
branch
from
32e400d to
7f33107
Compare
| pub fn set_cloexec(&self) { | ||
| unsafe { | ||
| let previous = c::fnctl(self.fd, c::F_GETFD); | ||
| let ret = c::fnctl(self.fd, c::F_SETFD, previous | c::FD_CLOEXEC); |
There was a problem hiding this comment.
Ah and just to confirm, newlib doesn't have ioctl? Looking at the recent source versions it looks like they do?
There was a problem hiding this comment.
Newlib can have ioctl (for instance, linux has it), but not every platform provides one. In NaCl's case, nacl_io handles all ioctl and fnctl operations in place of libc (specifically, nacl_io provides a wrapper around the PPAPI IRT, allowing use of the usual set of file descriptor operations from within Chrome's sandbox), but it expects FD_CLOEXEC to be set via fnctl instead of ioctl. Also, FIOCLEX isn't defined for any platform.
|
Don't merge! I just realized I never updated the |
DiamondLovesYou
force-pushed
the
pnacl-std-crates
branch
from
7f33107 to
a7d93c9
Compare
|
Crisis averted! |
No description provided.