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

refill_budget_check_ccorres #821

Open
wants to merge 2 commits into
base: rt
Choose a base branch
from
Open

Conversation

michaelmcinerney
Copy link
Contributor

Test with seL4/seL4#1323

Signed-off-by: Michael McInerney <michael.mcinerney@proofcraft.systems>
Signed-off-by: Michael McInerney <michael.mcinerney@proofcraft.systems>
@michaelmcinerney michaelmcinerney self-assigned this Oct 2, 2024
@michaelmcinerney michaelmcinerney added the MCS related to `rt` branch and mixed-criticality systems label Oct 2, 2024
@michaelmcinerney michaelmcinerney marked this pull request as ready for review October 2, 2024 14:46
apply simp
apply simp
apply wpsimp
apply wpsimp
Copy link
Member

Choose a reason for hiding this comment

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

can shorten this by using simp+ and wpsimp+, or even wpsimp+ to cover all 4 steps

apply (clarsimp simp: get_refill_tail_def getRefillTail_def read_refill_tail_def
readRefillTail_def getSchedContext_def[symmetric]
read_sched_context_get_sched_context
readSchedContext_def getObject_def[symmetric])
Copy link
Member

Choose a reason for hiding this comment

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

dunno if this is from older proofs, but checking that you know you can do clarsimp simp: ... simp flip: ... to not need the [symmetric]

apply (rule refill_head_ccorres)
apply clarsimp
apply clarsimp
apply clarsimp
Copy link
Member

Choose a reason for hiding this comment

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

clarsimp+

apply (rule ccorres_split_nothrow)
apply (clarsimp simp: setRefillTl_def updateRefillTl_def)
apply (rule updateSchedContext_ccorres_lemma3
[where P="\<lambda>sc. scRefillTail sc < length (scRefills sc)" and P'="?abs"])
Copy link
Member

Choose a reason for hiding this comment

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

[ should be two spaces deeper than u, am seeing one on the github diff (saw this above as well in same file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any particular reason that we would want two spaces here, rather than one? I thought that as long as it's strictly to the right of the name of the rule, it would be fine.

Copy link
Member

Choose a reason for hiding this comment

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

as discussed offline: indent is nearly always by 2 spaces, except for subgoal-indentation where it's two spaces plus number of subgoals - 1
Also, there are other issues (the modification belongs to the lemma not to rule);
it might be better to have ..._lemma3[ to indicate the lemma is being modified, then wrap how it's modified onto the next line, with the where 2 in from updateSched.... Might be a tight fit still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in these cases, it's nicest to just have it all on the one line, even though it does go over the 100 character blue line a little bit (by 5 characters or so). If that definitely doesn't sound good, then I can go with your suggestion.

apply (clarsimp simp: typ_heap_simps' sc_ptr_to_crefill_ptr_def
csched_context_relation_def)
apply (clarsimp simp: typ_heap_simps csched_context_relation_def)
apply normalise_obj_at'
Copy link
Member

Choose a reason for hiding this comment

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

unexpected to see normalise_obj_at' so distant from the subgoal-solving statement, anything interesting going on herer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe some of these normalise_obj_at' can be removed or moved elsewhere. Maybe I don't understand what you mean by it being distant from the subgoal-solving statement. Were you expecting it to be further up or further down?

Copy link
Member

Choose a reason for hiding this comment

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

Well, normally I see it used, and then the subgoal is discharged immediately, or on the next step. This one goes on for a bit until it's actually discharged, making me wonder if something is going the long way around.

sc_valid_refills_def vs_all_heap_simps obj_at_def refill_budget_check_defs
update_sched_context_set_refills_rewrite schedule_used_defs)
method handle_overrun_simple
= (clarsimp simp: handle_overrun_def)?,
Copy link
Member

Choose a reason for hiding this comment

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

this looks nearly identical to the previous method, am I missing why we need two methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are slightly different. It's possible that they could be combined into one method that could be used instead of the two, but it could be rather annoying to try this.

in hoare_strengthen_post[rotated])
apply (fastforce simp: valid_refills_def vs_all_heap_simps)
apply (intro hoare_vcg_conj_lift_pre_fix; (solves head_insufficient_loop_simple)?)
apply (wpsimp wp: head_insufficient_loop_refills_sum)
Copy link
Member

Choose a reason for hiding this comment

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

this looks like you can combine the rest of the proof into a (wpsimp wp: ... | clarsimp simp: ...)+, I don't think there's much of interest here for the proof length

Copy link
Member

@Xaphiosis Xaphiosis left a comment

Choose a reason for hiding this comment

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

Nice work. That was a much bigger change than I expected. I have minor/cleanup comments mostly.
On the "prove" commit, you're changing the spec, would be nice to have some more info, because the commit diff is huge, but the message makes it sound like something rather simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MCS related to `rt` branch and mixed-criticality systems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants