Issue #31109 uncovered two semi-related problems:
* A panic in `str::parse::<f64>`
* A panic in `rustc::middle::const_eval::lit_to_const` where the result of float parsing was unwrapped.
This series of commits fixes both issues and also drive-by-fixes some things I noticed while tracking down the parsing panic.
Abort on stack overflow instead of re-raising SIGSEGV
We use guard pages that cause the process to abort to protect against
undefined behavior in the event of stack overflow. We have a handler
that catches segfaults, prints out an error message if the segfault was
due to a stack overflow, then unregisters itself and returns to allow
the signal to be re-raised and kill the process.
This caused some confusion, as it was unexpected that safe code would be
able to cause a segfault, while it's easy to overflow the stack in safe
code. To avoid this confusion, when we detect a segfault in the guard
page, abort instead of the previous behavior of re-raising SIGSEGV.
To test this, we need to adapt the tests for segfault to actually check
the exit status. Doing so revealed that the existing test for segfault
behavior was actually invalid; LLVM optimizes the explicit null pointer
reference down to an illegal instruction, so the program aborts with
SIGILL instead of SIGSEGV and the test didn't actually trigger the
signal handler at all. Use a C helper function to get a null pointer
that LLVM can't optimize away, so we get our segfault instead.
This is a [breaking-change] if anyone is relying on the exact signal
raised to kill a process on stack overflow.
Closes#31273
We use guard pages that cause the process to abort to protect against
undefined behavior in the event of stack overflow. We have a handler
that catches segfaults, prints out an error message if the segfault was
due to a stack overflow, then unregisters itself and returns to allow
the signal to be re-raised and kill the process.
This caused some confusion, as it was unexpected that safe code would be
able to cause a segfault, while it's easy to overflow the stack in safe
code. To avoid this confusion, when we detect a segfault in the guard
page, abort instead of the previous behavior of re-raising the SIGSEGV.
To test this, we need to adapt the tests for segfault to actually check
the exit status. Doing so revealed that the existing test for segfault
behavior was actually invalid; LLVM optimizes the explicit null pointer
reference down to an illegal instruction, so the program aborts with
SIGILL instead of SIGSEGV and the test didn't actually trigger the
signal handler at all. Use a C helper function to get a null pointer
that LLVM can't optimize away, so we get our segfault instead.
This is a [breaking-change] if anyone is relying on the exact signal
raised to kill a process on stack overflow.
Closes#31273
The scope of these refactorings is a little bit bigger than the title implies. See each commit for details.
I’m submitting this for nitpicking now (the first 4 commits), because I feel the basic idea/implementation is sound and should work. I will eventually expand this PR to cover the translator changes necessary for all this to work (+ tests), ~~and perhaps implement a dynamic dropping scheme while I’m at it as well.~~
r? @nikomatsakis
This change also modifies the dep graph infrastructure to keep track of the number of active tasks, so that even if we are not building the full dep-graph, we still get assertions when there is no active task and one does something that would add a read/write edge. This is particularly helpful since, if the assertions are *not* active, you wind up with the error happening in the message processing thread, which is too late to know the correct backtrace.
~~Before landing, I need to do some performance measurements. Those are underway.~~
See measurements below. No real effect on time.
r? @michaelwoerister
Have the `ObligationForest` keep some per-tree state (or type `T`) and have it give a mutable reference for use when processing obligations. In this case, it will be a hashmap. This obviously affects the work that @soltanmm has been doing on snapshotting. I partly want to toss this out there for discussion.
Fixes#31157. (The test in question goes to approx. 30s instead of 5 minutes for me.)
cc #30977.
cc @aturon @arielb1 @soltanmm
r? @aturon who reviewed original `ObligationForest`
If a new cleanup is added to a cleanup scope, the cached exits for that
scope are cleared, so all previous cleanups have to be translated
again. In the worst case this means that we get N distinct landing pads
where the last one has N cleanups, then N-1 and so on.
As new cleanups are to be executed before older ones, we can instead
cache the number of already translated cleanups in addition to the
block that contains them, and then only translate new ones, if any and
then jump to the cached ones, getting away with linear growth instead.
For the crate in #31381 this reduces the compile time for an optimized
build from >20 minutes (I cancelled the build at that point) to about 11
seconds. Testing a few crates that come with rustc show compile time
improvements somewhere between 1 and 8%. The "big" winner being
rustc_platform_intrinsics which features code similar to that in #31381.
Fixes#31381
Without this patch, `compiler-rt` fails to build when the `CFLAGS` environment variable contains a `-Werror=*` flag (for example `-Werror=format-security`).
The build system was removing only the `-Werror` part from the flag, thus passing an unrecognized `=*` (for example `=format-security`) argument to gcc.
The first commit improves detection of unused imports -- it should have been part of #30325. Right now, the unused import in the changed test would not be reported.
The rest of the commits are miscellaneous, independent clean-ups in resolve that I didn't think warranted individual PRs.
r? @nrc
These accessors are used to get at the last modification, last access, and
creation time of the underlying file. Currently not all platforms provide the
creation time, so that currently returns `Option`.
These accessors are used to get at the last modification, last access, and
creation time of the underlying file. Currently not all platforms provide the
creation time, so that currently returns `Option`.
- use `symlink_file` and `symlink_dir` instead of the old `soft_link`
- create a junction instead of a directory symlink for testing recursive_rmdir (as it causes the
same troubles, but can be created by users without `SeCreateSymbolicLinkPrivilege`)
- `remove_dir_all` was unable to remove directory symlinks and junctions
- only run tests that create symlinks if we have the right permissions.
- rename `Path2` to `Path`
- remove the global `#[allow(deprecated)]` and outdated comments
- After factoring out `create_junction()` from the test `directory_junctions_are_directories` and
removing needlessly complex code, what I was left with was:
```
#[test]
#[cfg(windows)]
fn directory_junctions_are_directories() {
use sys::fs::create_junction;
let tmpdir = tmpdir();
let foo = tmpdir.join("foo");
let bar = tmpdir.join("bar");
fs::create_dir(&foo).unwrap();
check!(create_junction(&foo, &bar));
assert!(bar.metadata().unwrap().is_dir());
}
```
It test whether a junction is a directory instead of a reparse point. But it actually test the
target of the junction (which is a directory if it exists) instead of the junction itself, which
should always be a symlink. So this test is invalid, and I expect it only exists because the
author was suprised by it. So I removed it.
Some things that do not yet work right:
- relative symlinks do not accept forward slashes
- the conversion of paths for `create_junction` is hacky
- `remove_dir_all` now messes with the internal data of `FileAttr` to be able to remove symlinks.
We should add some method like `is_symlink_dir()` to it, so code outside the standard library
can see the difference between file and directory symlinks too.
The structure of the old translator as well as MIR assumed that drop glue cannot possibly panic and
translated the drops accordingly. However, in presence of `Drop::drop` this assumption can be
trivially shown to be untrue. As such, the Rust code like the following would never print number 2:
```rust
struct Droppable(u32);
impl Drop for Droppable {
fn drop(&mut self) {
if self.0 == 1 { panic!("Droppable(1)") } else { println!("{}", self.0) }
}
}
fn main() {
let x = Droppable(2);
let y = Droppable(1);
}
```
While the behaviour is allowed according to the language rules (we allow drops to not run), that’s
a very counter-intuitive behaviour. We fix this in MIR by allowing `Drop` to have a target to take
on divergence and connect the drops in such a way so the leftover drops are executed when some drop
unwinds.
Note, that this commit still does not implement the translator part of changes necessary for the
grand scheme of things to fully work, so the actual observed behaviour does not change yet. Coming
soon™.
See #14875.
We used to have CallKind only because there was a requirement to have all successors in a
contiguous memory block. Now that the requirement is gone, remove the CallKind and instead just
have the necessary information inline.
Awesome!
