Repository navigation
std::fs::create_dir not failing on existing directory on aarch64, debug #50516
Description
I have this piece of code:
src/main.rs:
use std::fs::create_dir;
fn main() {
create_dir("hello").expect("1");
create_dir("hello").expect("2");
}
When I tried to run it on aarch64, debug mode, I get empty output. However, I would expect the second except to fail (provided hello didn't exist before).
In fact, when I run the same code with cargo run --release, this is the behavior I get:
$ cargo run --release
Finished release [optimized] target(s) in 0.04s
Running `target/release/dir-exists-aarch64`
thread 'main' panicked at '2: Os { code: 17, kind: AlreadyExists, message: "File exists" }', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
I've tried it on both stable and nightly:
rustc 1.25.0 (84203cac6 2018-03-25)
binary: rustc
commit-hash: 84203cac67e65ca8640b8392348411098c856985
commit-date: 2018-03-25
host: aarch64-unknown-linux-gnu
release: 1.25.0
LLVM version: 6.0
rustc 1.27.0-nightly (428ea5f6b 2018-05-06)
binary: rustc
commit-hash: 428ea5f6b9e7c6e5ee3294fe9f105e77e89ab407
commit-date: 2018-05-06
host: aarch64-unknown-linux-gnu
release: 1.27.0-nightly
LLVM version: 6.0
Any thoughts?
Activity
cuviper commentedon May 7, 2018
Can you run it under
straceto see its actual syscalls? Debug and release builds should be identical in this respect, apart from different memory addresses between runs.yrashk commentedon May 7, 2018
Of course!
Debug:
Release:
cuviper commentedon May 7, 2018
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.
yrashk commentedon May 7, 2018
I don't even know where to start on this, to be honest (besides comparing LLVM 5 and 6)... any suggestions?
yrashk commentedon May 7, 2018
And it's interesting that the issue manifests only in debug, not in release
cuviper commentedon May 7, 2018
Something is getting clobbered, or otherwise initialized improperly. Stepping through this in GDB, I found:
That
mode: 511is octal 0777 as expected, butrecursiveis abool-- it should only betrue(1) orfalse(0), and in this case we expectfalse! But the generated code behaves as if that 254 istrue, then proceeds in the manner ofcreate_dir_all, which doesn't error for existing paths.yrashk commentedon May 7, 2018
Interesting! But stepping through the same code in release,
recursiveis 0?cuviper commentedon May 7, 2018
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.
yrashk commentedon May 7, 2018
Gotcha, as expected. So the main question now is why does
recursivebecome 254. I am pretty sure I don't have any knowledge of internals to suggest anything valuable at this moment. But if you can at least point at the direction you think is worth exploring, I'll try to do my best to help with the research.yrashk commentedon May 7, 2018
I believe #48673 is somewhat related. I can see 254 there as well!
yrashk commentedon May 7, 2018
Trying out the suggestion from the above issue (wrt to GlobalISel):
works as expected!
cuviper commentedon May 8, 2018
OK, that gives something to compare more directly. Both create the
DirBuilderthe same:When the fast-isel
create_dirgets that, it just copiesw0andw1onto the stack:Under the default (GlobalISel), it sets the precise bits in
x8. Thebfxil ... #0, #32setsmodeat lsb 0 width 32, andbfi ... #32, #1setsrecursiveat lsb 32, width 1. The remaining bits ofx8are untouched! (i.e. garbage) Thenx8is written on the stack in the same place as before.Then
DirBuilder::_createlooks the same for both: a condition on the entire byte at offset#4branching into the different tail calls. But with the GlobalISel, this is where we see our 254,0b11111110where only the lsb0was set for ourfalse.cuviper commentedon May 8, 2018
Even this shows the wrong
recursive:11 remaining items
cuviper commentedon May 9, 2018
Here's the IR of
DirBuilder::_createwithout optimization -- note the%4 = trunc i8 %3 to i1.With optimization, we instead get
%4 = icmp eq i8 %3, 0.(and note in this bug we have pre-compiled, optimized
stdcode, mixed with debug user code.)cuviper commentedon May 9, 2018
That
!range !184in the optimized case is!184 = !{i8 0, i8 2}.cuviper commentedon May 9, 2018
The
!range !0in the unoptimized case has the same definition,!0 = !{i8 0, i8 2}.cuviper commentedon May 9, 2018
Summary so far:
boolas ani8with range [0,2), thentrunc i8 to i1truncredundant, and doesn't bother.boolas ani8, explicitly zero-extended fromi1.booldirectly, without zero-extending or otherwise filling bits.DirBuilderthis is astore { i32, i1 }. LLVM sees this as a 33-bit store, and the remaining bits are unspecified, perstd::fs::create_dirnot failing on existing directory on aarch64, debug #50516 (comment).i1values, but we see here that AArch64 GlobalISel does not. They'll probably fix that (llvm 37397), but we're still in bug-prone territory.Possible changes on Rust's side:
booldirectly withload i1, rather thanload i8range [0,2) then truncated toi1.boolasi8in aggregates, soDirBuilderwould be{ i32, i8 }.struct { int i; bool b; }becomestype { i32, i8 }. It still usesi1for immediate bools in arguments and return values though.@eddyb, any thoughts on this?
eddyb commentedon May 11, 2018
(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
loadof ani1followed by azexttoi8has to mask out all bits but the lowest one, instead of being a noop.Does AArch64 GlobalISel do that masking? If not, that seems like a bug on LLVM's side.
Also, how does this interact with
_Bool? It does seem rather problematic IMO.Note that rustc creates LLVM aggregates containing
i1only from scalar pairs, where the two values are usually kept outside of the aggregate, and only combined for returning the pair.When the scalar pair's fields are accessed through a pointer to the scalar pair, that's where I think things start breaking down. I would've used the memory type (
i8) but that created more issues.Sadly I can't remember if any of the issues were since obsoleted.
OTOH, #50277 tried to switch all bools to
i1but it hit SROA not understandingi1as well asi8.tstellar commentedon May 11, 2018
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.
eddyb commentedon May 11, 2018
@tstellar We do that, and the only exception is an optimization for types that can be passed around as two scalars (integer/float/pointer).
LLVM is really bad at handling booleans, IMO, not being able to optimally handle memory accesses with the type it forces you to use for immediates.
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).
cuviper commentedon May 11, 2018
Aha, I was confused trying to find all the generated
i1s -- I found it for scalar pairs, but thought I was missing something when I couldn't find it elsewhere.Problem: Repository::new_named_item duplicate check on aarch64
cuviper commentedon Jun 15, 2018
With #51566, that
bfi x8, x9, #32, #1becomesbfi x8, x9, #32, #8, copying a whole byte instead of just one bit. Otherwise the asm is identical, and the original reproducer and myprintlnexample now both work correctly.[-]std::fs:create_dir not failing on existing directory on aarch64, debug[/-][+]`std::fs::create_dir` not failing on existing directory on aarch64, debug[/+]workingjubilee commentedon Feb 10, 2023
Taking a closer look instead of merely retagging, it seems this was resolved in #51583 so I am closing this.