HOL-Light: improve tooling and CI#1634
Conversation
CBMC Results (ML-KEM-768)
Full Results (191 proofs)
|
CBMC Results (ML-KEM-512)Full Results (191 proofs)
|
CBMC Results (ML-KEM-1024)Full Results (191 proofs)
|
724e0b1 to
9a3e91a
Compare
|
Modified the two remaining occurences of _MEMSAFE to _SAFE in order to fit the expected format for aarch proofs. |
25cad96 to
3e258d8
Compare
mkannwischer
left a comment
There was a problem hiding this comment.
Thanks @L-series - I left a couple of comments.
This PR actually takes another approach that I had in mind: I thought that after running each HOL-Light proof we check for the expected theorems to be present.
The approach implemented here is more like a linter. But I support the addition!
The -a flag is highly appreciated - thank you!
Thank you for the review! Indeed, the idea I had in mind was that one should avoid using compute on proofs which do not contain the required theorems. However, I see now that there is still value running the HOL CI even if the routine is only partially proven. |
mkannwischer
left a comment
There was a problem hiding this comment.
Thanks @L-series for the changes! I'm happy with the overall approach now.
Two comments still need to be addressed. Thank you!
|
@L-series, gentle ping. Could you please adjust this PR so we can get this merged? |
|
@L-series, are you still working on this? |
|
@L-series, do you expect to finish this PR soon? |
Apologies for the long delay, I was away traveling this last month. I just pushed some changes and I've reworked However, I just wanted to make sure that this is okay with you as it would require renaming around 15 proofs. |
Thanks for picking this upa gain @L-series! Welcome back! |
318c17a to
ac97d80
Compare
|
@L-series, is this ready for review? Thanks for the work! |
|
@mkannwischer Yes it is! Please let me know if the renaming scheme is appropriate. Given that the HOL proofing CI is passing I assume I didn't break any of the proofs 😅 |
8a3f6d0 to
a583d82
Compare
| if ! grep -qE "^[[:space:]]*let ${theorem}[[:space:]]+=[[:space:]]+(time[[:space:]]+)?prove" "$file"; then | ||
| gh_error "${routine}" "" "Missing theorem" "${file}: ${theorem} not found" | ||
| success=false |
There was a problem hiding this comment.
Suggestion: Can we complement list_proofs.sh by list_thms.sh which runs this grep and emits all (so-declared) theorems? I'd like to have a little logic as possible in lint itself.
There was a problem hiding this comment.
I don't think this is done yet, is it?
| local expected=( | ||
| "${prefix}_CORRECT" | ||
| "${prefix}_SUBROUTINE_CORRECT" | ||
| "${prefix}_SUBROUTINE_${safe}" | ||
| ) | ||
| if [[ $arch == "x86_64" ]]; then | ||
| expected+=( | ||
| "${prefix}_${safe}" | ||
| "${prefix}_NOIBT_SUBROUTINE_CORRECT" | ||
| "${prefix}_NOIBT_SUBROUTINE_${safe}" | ||
| ) | ||
| fi |
There was a problem hiding this comment.
It would be nice to make this logic and exception more prominent... I wonder if one could define this in some TOML/YAML in proofs/{arch}... not sure if it's worth it. Ideas?
There was a problem hiding this comment.
Hmm.. I think the advantages of such a move are twofold:
- It would make the maintaining the naming conventions and exceptions such as
rej_uniformsimpler and more visible as they are in the same directory as the proofs. - It could be useful if there are any ideas to reuse or refer to the naming conventions elsewhere in the project.
However, if the sole use is within the linting script for CI, I think that it would be preferable to keep it as is as we would not have to add any additional files or TOML/YAML parsing logic.
L-series
left a comment
There was a problem hiding this comment.
I just realized that the comments I had written many weeks ago as well as last week which were labelled "pending", were in a "pending to be submitted" not a "pending to be reviewed" state. I wasn't aware that one had to submit the comments through the files changed page 😓
| if ! grep -qE "^[[:space:]]*let ${theorem}[[:space:]]+=[[:space:]]+(time[[:space:]]+)?prove" "$file"; then | ||
| gh_error "${routine}" "" "Missing theorem" "${file}: ${theorem} not found" | ||
| success=false |
| local expected=( | ||
| "${prefix}_CORRECT" | ||
| "${prefix}_SUBROUTINE_CORRECT" | ||
| "${prefix}_SUBROUTINE_${safe}" | ||
| ) | ||
| if [[ $arch == "x86_64" ]]; then | ||
| expected+=( | ||
| "${prefix}_${safe}" | ||
| "${prefix}_NOIBT_SUBROUTINE_CORRECT" | ||
| "${prefix}_NOIBT_SUBROUTINE_${safe}" | ||
| ) | ||
| fi |
There was a problem hiding this comment.
Hmm.. I think the advantages of such a move are twofold:
- It would make the maintaining the naming conventions and exceptions such as
rej_uniformsimpler and more visible as they are in the same directory as the proofs. - It could be useful if there are any ideas to reuse or refer to the naming conventions elsewhere in the project.
However, if the sole use is within the linting script for CI, I think that it would be preferable to keep it as is as we would not have to add any additional files or TOML/YAML parsing logic.
This commit introduces a new flag --arch to the hol_light command of the tests script that allows user specification of which architecture to run/list the proofs for. If not passed, the behavior is unchanged. Signed-off-by: Andreas Hatziiliou <andreas.hatziiliou@savoirfairelinux.com>
We add a check to the linting script called check-theorems that ensures that all HOL-Light proofs provide the expected set of theorems depending on the architecture. Signed-off-by: Andreas Hatziiliou <andreas.hatziiliou@savoirfairelinux.com>
This commit aims to resolve the two following issues:
testsscript to allow cross-platform proof checking #1585Note that the
check-theoremsscript will currently fail the HOL-Light CI because for AArch, the naming convention for the memory safety proof (usually _SAFE) is not respected inmlkem_rej_uniform(it uses _MEMSAFE). @hanno-becker please advise if I should modify this.