Skip to content
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

Add static module for top-level Control Spec context #801

Merged
merged 4 commits into from
Aug 4, 2022

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Jul 28, 2022

This adds the static Analyses.ControlSpecC module, which corresponds to the context of the entire top-level Spec. This allows individual analyses to actually use ctx.control_context, get a result of meaningful type and use them (opaquely) inside its own domains and global constraint variables.

Previously this was problematic in multiple ways:

  1. ARINC analysis (and its extraction) unsafely get (or really reconstruct) the corresponding base analysis context and use its hash to identify the context:
    let base_context = BaseMain.context_cpa f @@ Obj.obj @@ ctx.presub "base" in
    let context_hash = Hashtbl.hash (base_context, ctx.local.pid) in
    { ctx.local with ctx = Ctx.of_int (Int64.of_int context_hash) }

    This in itself is prone to two problems:
    1. Hashes may collide.
    2. Other analyses other than base may also have context components.
  2. To avoid the aforementioned problems, a safe way is to implement a Spec functor instead, which wraps the entire combined analysis and can thus directly access its C module. The GraphML witness generation takes this route.
    Implementing such an analysis as a functor is inconvenient and composes poorly with other analyses.

@sim642 sim642 added the cleanup Refactoring, clean-up label Jul 28, 2022
@sim642 sim642 self-assigned this Jul 28, 2022
@jerhard jerhard self-requested a review July 28, 2022 12:10
Copy link
Member

@jerhard jerhard left a comment

Choose a reason for hiding this comment

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

Currently, neither ControlSpecC nor the already existing control_context seem to be used, or am I missing something?

@michael-schwarz
Copy link
Member

This should be used by #220 (or the ARINC analyses, but I'll open a PR to drop them tomorrow).

@sim642
Copy link
Member Author

sim642 commented Aug 2, 2022

Currently, neither ControlSpecC nor the already existing control_context seem to be used, or am I missing something?

That's correct. I initially tried to use this for #796 in order to track constraint variables where a witness hint was already used, but that didn't work as well as I had hoped (not the fault of this PR).

I also thought whether I could reimplement WitnessConstraints lifter as an analysis instead using this PR, but that also doesn't work out, because that also records dependencies between paths, which this cannot really handle.

So there's no immediate need for this unless we plan to refactor #220 to use this.

@sim642
Copy link
Member Author

sim642 commented Aug 3, 2022

Since unassuming once turned out to be a futile idea according to GobCon discussions, this will not be used by #796.

However, since #812 removes ctx.presub but its replacement #220 currently would require it, this will be necessary for #811. Thus this will be needed. At minimum #220 could then be refactored to just use ControlSpecC.hash instead of base context hash. A further improvement could then replace hashes with the real things via this module.

@sim642 sim642 marked this pull request as ready for review August 3, 2022 15:31
@michael-schwarz michael-schwarz merged commit b6e4544 into master Aug 4, 2022
@michael-schwarz michael-schwarz deleted the control-c branch August 4, 2022 06:45
@sim642 sim642 added this to the v2.0.0 milestone Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Refactoring, clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants