-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
std::fs::create_dir
not failing on existing directory on aarch64, debug
#50516
Comments
Can you run it under |
Of course! Debug:
Release:
|
Well, at least the kernel is behaving as expected. 🙂 I can reproduce this on Fedora 27, using the official Rust 1.25 (w/ LLVM 6). Fedora 27's own rust-1.25.0 package (linked to LLVM 5) does not have the problem! But I also tried Fedora rawhide's package, where it does have LLVM 6, and the problem resurfaces. |
I don't even know where to start on this, to be honest (besides comparing LLVM 5 and 6)... any suggestions? |
And it's interesting that the issue manifests only in debug, not in release |
Something is getting clobbered, or otherwise initialized improperly. Stepping through this in GDB, I found:
That |
Interesting! But stepping through the same code in release, |
Under optimization, even with debuginfo the locals are too far gone to inspect. But I can at least see in this bit of code that it's taking the latter path.
|
Gotcha, as expected. So the main question now is why does |
I believe #48673 is somewhat related. I can see 254 there as well! |
Trying out the suggestion from the above issue (wrt to GlobalISel):
works as expected! |
OK, that gives something to compare more directly. Both create the
When the fast-isel
Under the default (GlobalISel), it sets the precise bits in
Then
|
Even this shows the wrong fn main() {
println!("{:?}", std::fs::DirBuilder::new());
}
|
@cuviper this is excellent research! I am wondering, given that this works fine in release, does this mean GlobalISel works differently in release, or is GlobalISel not used in release? |
Could someone attach an LLVM IR dump from the example posted by@cuviper ? |
@yrashk It's the latter, LLVM only uses GlobalISel for @tstellar Here you go: print-dirbuilder.tar.gz The first block in start:
%_12 = alloca { i32, i1 }, align 4
%_10 = alloca i32*, align 8
%_9 = alloca [1 x { i8*, i8* }], align 8
%_2 = alloca %"core::fmt::Arguments", align 8
; call std::fs::DirBuilder::new
%0 = call { i32, i1 } @_ZN3std2fs10DirBuilder3new17h4be627efa14f8660E()
store { i32, i1 } %0, { i32, i1 }* %_12, align 4
br label %bb1 With // %bb.0: // %start
sub sp, sp, #144 // =144
str x30, [sp, #128] // 8-byte Folded Spill
.cfi_def_cfa_offset 144
.cfi_offset w30, -16
adrp x1, _ZN56_$LT$std..fs..DirBuilder$u20$as$u20$core..fmt..Debug$GT$3fmt17h81b1b3fa607a0cf5E
add x1, x1, :lo12:_ZN56_$LT$std..fs..DirBuilder$u20$as$u20$core..fmt..Debug$GT$3fmt17h81b1b3fa607a0cf5E
adrp x8, .Lbyte_str.3
add x8, x8, :lo12:.Lbyte_str.3
adrp x9, .Lbyte_str.4
add x9, x9, :lo12:.Lbyte_str.4
str x1, [sp, #40] // 8-byte Folded Spill
str x8, [sp, #32] // 8-byte Folded Spill
str x9, [sp, #24] // 8-byte Folded Spill
bl _ZN3std2fs10DirBuilder3new17h4be627efa14f8660E
// implicit-def: %x8
mov w9, w0
bfxil x8, x9, #0, #32
mov w9, w1
bfi x8, x9, #32, #1
str x8, [sp, #120] So again we see With // %bb.0: // %start
sub sp, sp, #128 // =128
str x30, [sp, #112] // 8-byte Folded Spill
.cfi_def_cfa_offset 128
.cfi_offset w30, -16
bl _ZN3std2fs10DirBuilder3new17h4be627efa14f8660E
str w0, [sp, #104]
and w0, w1, #0x1
strb w0, [sp, #108] |
Why does rust emit i1 for its bool type instead of i8? |
I don't know the history of |
Here's an interesting comment about the rust/src/librustc_trans/type_of.rs Lines 362 to 368 in ac287ed
|
It seems like there is a mismatch then, because DirBuilder::_create() seems to assume that bool is 8-bits, but rust only emits a 1-bit store here. |
Seems like that last statement in the comment may the thing we need to challenge, that " |
An i1 value in memory will be 8-bits, so I think that statement is more or less correct. The problem as I see it is that rust is assuming that the i1 value will be zero extended before it is stored, but according the the LLVM language ref the value of the other 7-bits is unspecified: http://llvm.org/docs/LangRef.html#id196 |
Here's the IR of ; std::fs::DirBuilder::_create
; Function Attrs: uwtable
define void @_ZN3std2fs10DirBuilder7_create17he9982b501e5a1dccE(%"core::result::Result<(), io::error::Error>"* noalias nocapture sret dereferenceable(16), { i32, i1 }* noalias readonly dereferenceable(8) %self, %"path::Path"* noalias nonnull readonly %path.0, i64 %path.1) unnamed_addr #1 {
start:
%1 = getelementptr inbounds { i32, i1 }, { i32, i1 }* %self, i32 0, i32 1
%2 = bitcast i1* %1 to i8*
%3 = load i8, i8* %2, align 1, !range !0
%4 = trunc i8 %3 to i1
br i1 %4, label %bb1, label %bb2
bb1: ; preds = %start
; call std::fs::DirBuilder::create_dir_all
call void @_ZN3std2fs10DirBuilder14create_dir_all17hcd76f7c5d629ca4bE(%"core::result::Result<(), io::error::Error>"* noalias nocapture sret dereferenceable(16) %0, { i32, i1 }* noalias readonly dereferenceable(8) %self, %"path::Path"* noalias nonnull readonly %path.0, i64 %path.1)
br label %bb3
bb2: ; preds = %start
%5 = bitcast { i32, i1 }* %self to i32*
; call std::sys::unix::fs::DirBuilder::mkdir
call void @_ZN3std3sys4unix2fs10DirBuilder5mkdir17hc7463290099457b9E(%"core::result::Result<(), io::error::Error>"* noalias nocapture sret dereferenceable(16) %0, i32* noalias readonly dereferenceable(4) %5, %"path::Path"* noalias nonnull readonly %path.0, i64 %path.1)
br label %bb4
bb3: ; preds = %bb1
br label %bb5
bb4: ; preds = %bb2
br label %bb5
bb5: ; preds = %bb4, %bb3
ret void
} With optimization, we instead get ; std::fs::DirBuilder::_create
; Function Attrs: uwtable
define void @_ZN3std2fs10DirBuilder7_create17he9982b501e5a1dccE(%"core::result::Result<(), io::error::Error>"* noalias nocapture sret dereferenceable(16), { i32, i1 }* noalias nocapture readonly dereferenceable(8) %self, %"path::Path"* noalias nonnull readonly %path.0, i64 %path.1) unnamed_addr #1 {
start:
%1 = getelementptr inbounds { i32, i1 }, { i32, i1 }* %self, i64 0, i32 1
%2 = bitcast i1* %1 to i8*
%3 = load i8, i8* %2, align 1, !range !184
%4 = icmp eq i8 %3, 0
br i1 %4, label %bb2, label %bb1
bb1: ; preds = %start
; call std::fs::DirBuilder::create_dir_all
tail call fastcc void @_ZN3std2fs10DirBuilder14create_dir_all17hcd76f7c5d629ca4bE(%"core::result::Result<(), io::error::Error>"* noalias nocapture nonnull sret dereferenceable(16) %0, { i32, i1 }* noalias nonnull readonly dereferenceable(8) %self, %"path::Path"* noalias nonnull readonly %path.0, i64 %path.1)
br label %bb5
bb2: ; preds = %start
%5 = getelementptr inbounds { i32, i1 }, { i32, i1 }* %self, i64 0, i32 0
; call std::sys::unix::fs::DirBuilder::mkdir
tail call fastcc void @_ZN3std3sys4unix2fs10DirBuilder5mkdir17hc7463290099457b9E(%"core::result::Result<(), io::error::Error>"* noalias nocapture nonnull sret dereferenceable(16) %0, i32* noalias nonnull readonly dereferenceable(4) %5, %"path::Path"* noalias nonnull readonly %path.0, i64 %path.1)
br label %bb5
bb5: ; preds = %bb2, %bb1
ret void
} (and note in this bug we have pre-compiled, optimized |
That |
The |
Summary so far:
Possible changes on Rust's side:
@eddyb, any thoughts on this? |
(I had written a comment but I deleted it because I missed a bit of information)
Alright, I didn't expect this. It seems counter-intuitive because now a Note that rustc creates LLVM aggregates containing OTOH, #50277 tried to switch all bools to |
Forgetting about the LLVM bug for a moment, in general I would recommend that rust uses i8 as the memory type for bool. Rust is effectively already doing this by bitcasting i1* to 18* before loads and stores, but I think it is better to be explicit about this in the IR. clang also uses i8 as the memory type for bools and matching clang means you are more likely to get favorable results from the optimizer. |
@tstellar We do that, and the only exception is an optimization for types that can be passed around as two scalars (integer/float/pointer). Anyway, I guess we try to switch the aggregate type for scalar pairs to contain the memory types of the two components and somehow do all the conversions needed, and maybe that will both fix this bug and not regress any optimizations (or even unregress others). |
Aha, I was confused trying to find all the generated |
Tests pertaining to duplicate name fail in debug mode on aarch64, the OS error is never returned. Solution: use FastISel instead of GlobalISel Tracking issue has been filed with Rust: rust-lang/rust#50516
With #51566, that |
std::fs::create_dir
not failing on existing directory on aarch64, debug
Taking a closer look instead of merely retagging, it seems this was resolved in #51583 so I am closing this. |
I have this piece of code:
src/main.rs:
When I tried to run it on aarch64, debug mode, I get empty output. However, I would expect the second
except
to fail (providedhello
didn't exist before).In fact, when I run the same code with
cargo run --release
, this is the behavior I get:I've tried it on both stable and nightly:
Any thoughts?
The text was updated successfully, but these errors were encountered: