Skip to content

Conversation

michael-schwarz
Copy link
Member

@michael-schwarz michael-schwarz commented Jul 25, 2025

Instead of using the domain_shims library to simulate all aspects of domains, this provides just a shim for DLS as we don't use multiple domains for OCaml 4 anyway.

This fixes goblint/analyzer#1781 without any principled rework.

@michael-schwarz michael-schwarz marked this pull request as ready for review July 25, 2025 12:24
@michael-schwarz michael-schwarz requested a review from sim642 July 25, 2025 12:24
@michael-schwarz michael-schwarz changed the title Replace dependy on domain_shims with lightweight DLS shim for single-threaded OCaml Replace dependency on domain_shims with lightweight DLS shim for single-threaded OCaml Jul 25, 2025
Copy link
Member

@sim642 sim642 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the clear race condition, I think doing it like this in CIL won't really fix the problem. For the same reason like our Zarith patch: it needs to be fork and pin of the original package, not in a new package, because other things we depend on would probably still use the original domain_shims.

I think we should just remove the global state from Pretty and be done with it. There's no reason it shouldn't be possible and there's every reason to have less global state anyway.

Comment on lines +14 to +15
let v = k.initialiser () in
k.value <- Some v;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this really works. There's a race condition here (yes, on OCaml 4 without threads!):
If the signal handler fires between these two lines to set something, then this get (!) will overwrite the "new" value with the initial one.

There's a good reason why domain_shims uses atomicity (although via a Dirk-style mutex). I think atomicity is inevitable in a correct solution: it's just that the atomicity probably should be proper, not using a mutex, possibly with a lock-free data structure.

@michael-schwarz
Copy link
Member Author

For the same reason like our Zarith patch: it needs to be fork and pin of the original package, not in a new package, because other things we depend on would probably still use the original domain_shims.

It is not a general fix of the problem in Domain_shims (it doesn't work for multiple domains simulated via threads).

This is a fix to get back pre-DLS behavior from OCaml 4. Yes, the usage in Goblint is problematic, but it has been problematic all along, and this solution is no less problematic then the old solution using ref.
Having random fluctuations with results all the time in our nightly jobs is bad, as people will not notice when actual changes happen.

I think we should just remove the global state from Pretty and be done with it.

If you want to commit resources to this within the next few days, sure. Otherwise, I think this is a good stop-gap until someone invests the time in such a re-write of Pretty (it has more state than just these few things btw).

@sim642
Copy link
Member

sim642 commented Jul 25, 2025

If you want to commit resources to this within the next few days, sure.

It was 30 min of straightforward work: PR #187.

@michael-schwarz
Copy link
Member Author

Closing in favor of #187. It is the more principled approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SV-COMP Errors: exception Sys_error("Mutex.lock: Resource deadlock avoided")

2 participants