-
Notifications
You must be signed in to change notification settings - Fork 416
genmc: Bug fixes #4733
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
genmc: Bug fixes #4733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The last commit mentions issues with memory allocation; what exactly are you referring to?
|
Also, please run |
|
Reminder, once the PR becomes ready for a review, use |
Ah, the PR description mentions "Fixes overflowing on large allocations". Is this about a single large allocation, or many allocations accumulating to exhaust the 4GB per-thread limit? How large is "large"? Is it possible to test for that? What happens now when the per-thread 4GB memory region is exhausted? |
Each thread can allocate up to 4GB. If the limit is exceeded, the allocator would silently wrap around 0 and handle duplicate addresses. The new commit makes GenMC crash if the limit is exceeded, and doesn't overflow if a size/alignment >2^32 is passed to |
GenMC has a single API that was handled in a collection of different files. This commit collects all API wrappers to Exploration.cpp. (The Setup.cpp file remains intact as it contains setting translation and setup functions.)
This commit makes the code a bit more compact by using ERROR_ON() whenever possible. It also renames a particularly verbose variable used in ERROR's condition.
Use scoped constexprs instead.
These comments explain the default values used internally in (some) calls of the GenMC library, which is unnecessary.
c964074 to
8f8c08e
Compare
That may make it tricky to add a test then, depending on the exit status this will trigger. |
The alternative would be to return an error code to Miri and let Miri crash. |
|
I tried what happened when one thread allocates more than 4GB memory in total (but not at the same time, i.e. we allocate chunks of 512MB and free them again): With this PR, that leads to a panic: EDIT: I get the same with an 8GB allocation, using this test: fn main() {
let _v = Vec::<u8>::with_capacity(8 * 1024 * 1024 * 1024);
}So, I can't reproduce the GenMC crash. |
The alternative would be to make Miri emit a nice error saying we ran out of memory for this thread. :) We don't have to resolve that in this PR, but ideally we'd have some tests before we mark the item in #4572 as resolved. |
8f8c08e to
bf65640
Compare
|
Here's a little patch to give the user some more explanation for why they may run out of address space in genmc mode: diff --git a/src/diagnostics.rs b/src/diagnostics.rs
index bb8ba1969..8e252d306 100644
--- a/src/diagnostics.rs
+++ b/src/diagnostics.rs
@@ -362,6 +362,10 @@ pub fn report_result<'tcx>(
vec![
note!("this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support"),
],
+ ResourceExhaustion(ResourceExhaustionInfo::AddressSpaceFull) if ecx.machine.data_race.as_genmc_ref().is_some() =>
+ vec![
+ note!("in GenMC mode, the address space is limited to 4GB per thread, and addresses cannot be reused")
+ ],
UndefinedBehavior(AlignmentCheckFailed { .. })
if ecx.machine.check_alignment == AlignmentCheck::Symbolic
=> |
bf65640 to
d5b2fca
Compare
This comment has been minimized.
This comment has been minimized.
d5b2fca to
4a7d48c
Compare
Indeed, sorry about that. I thought I made GenMC panic in all cases, but that's not the case. For allocations taking place via I also incorporated your other comments. |
GenMC's handleFree() does return an optional<Error>, so we may as well use that value.
GenMC can currently allocate up to 4GB per thread. If it cannot allocated any more memory, it will return nullptr. This commit adds a test in Miri that ensures we gracefully throw if this ever happens.
GenMC's allocator can currently only allocate up to 4GB per thread. Authored-by: Ralf Jung
This version fixes some issues with atomic_{umix,umax} operation
and memory allocation.
4a7d48c to
ec626b9
Compare
|
@rustbot ready |
|
Looks great, thanks :) |
This PR bumps the GenMC version to 0.16.1:
atomic_fetch_{umax,umin}(GenMC correctness issues #4572)Exploration.cpp)r? @RalfJung
(Each commit includes a description and can be reviewed separately.)