Use gcc and minimal libc headers for all CBMC proofs - #1879
Conversation
CBMC Results (ML-KEM-1024)
Full Results (191 proofs)
|
CBMC Results (ML-KEM-768)
Full Results (191 proofs)
|
CBMC Results (ML-KEM-512)
Full Results (191 proofs)
|
010bb0e to
9cc67d2
Compare
|
|
||
| # Disable any setting of EXTERNAL_SAT_SOLVER, and choose SMT backend instead | ||
| EXTERNAL_SAT_SOLVER= | ||
| CBMCFLAGS=--bitwuzla |
There was a problem hiding this comment.
Those solver changes seem unrelated or at least unexplained.
There was a problem hiding this comment.
Just maintaining reasonable performance for the current CI runs seemed a reasonable thing to do under the auspices of this PR. As as I said below, adding new CI runs to cover ILP32 requires some thinking and budget decisions that are best handled separately.
| # or "LP64" - 64-bit systems where "long and pointer are 64-bit" | ||
| # | ||
| # if CBMC_DM is not set, default to LP64 | ||
| CBMC_DM ?= LP64 |
There was a problem hiding this comment.
Can you plumb that into tests cbmc ... as well please?
| CBMC_DM_FLAG = --LP64 | ||
| PROOF_INCLUDE = $(CBMC_ROOT)/cbmc/include/LP64 | ||
| else ifeq ($(strip $(CBMC_DM)),ILP32) | ||
| GOTO_CC_DM_FLAG = --32 |
There was a problem hiding this comment.
Does the 32-bit mode work?
There was a problem hiding this comment.
Yes... but running it locally on my Mac I observed some proofs became really slow. Some tweaking of proof Makefiles might be required (i.e. the setting of CBMC_OBJECT_BITS to suit the smaller value of SIZE_MAX), but I hope to address that under a new PR.
There was a problem hiding this comment.
Thanks @rod-chapman, it's great to see progress in this direction.
Please can you confirm the status of the 32-bit model? I don't think we should include it without also testing it in CI. If we are not ready for that, yet, I suggest trimming down the PR to the addition of the infrastructure and the 64-bit header.
Please also integrate the choice of data model into tests cbmc.
Please also clean up the CI; the second commit message is insufficient, and there are solver changes in the first commit which are unrelated and/or unexplained.
|
Thanks for your thoughts. Yes... we need the CI runners to change to add at least 1 32-bit proof run, but I wanted to discuss that separately, under a new PR. How many CI runs in 32-bit mode should we do? What will that cost in terms of runners? I thought that a policy discussion was warranted on those topics before going ahead. With the proof changes - I noticed that several proofs became slower with bitwuzla. I tried switching them back to z3, and they got faster. I don't have an "explanation" for this per se... they're just faster with z3 now. |
|
I see this PR as adding the plumbing to support multiple data models. I had hoped to add the "porcelain" layer (i.e. updating "tests cbmc" and CI actions) following requirements and policy decisions that I don't feel qualified to make alone. |
9cc67d2 to
979c068
Compare
|
I am looking at the proof performance for ntt_butterfly_block() on my Grv4 machine now. I will update commit messages as soon as I have good data. |
979c068 to
173b172
Compare
|
@rod-chapman Yes, it's fine to add the plumbing, but let's add 32-bit only when we can actually exercise at least a few functions with it. I think the management of the proof dimensions (function, solver, data model) could be piggy backed atop the draft pq-code-package/mldsa-native#1122 for supporting multiple solvers (needs to be ported to mlkem-native; I can do that). Please use Claude, precedent, and your judgement, to make the necessary changes to |
|
OK... I will update "tests" as part of the "plumbing" in this PR. I think "multi-solver" support is still a long way off, and independent of this work on stabilization and multi-data-model support. As I said, my local benchmarks have cvc5 a long way behind z3 on CBMC-generated problems, but it would be a good way to drive improvement of cvc5 in the long term. |
0eab717 to
60a8c71
Compare
|
As I feared, proofs for some functions with ILP32 are very slow. For example - over 1 hour for indcpa_enc(). We may need assistance of CBMC team to diagnose this, so suggest we defer actual CI runs for ILP32 until a later PR. |
I don't think it's so far off -- one should not approach it as a monolithic change, but as a gradual one. And it relates to this PR insofar as it, too, adds a verification dimension to CBMC, and we need a way to manage them. |
1. CBMC Makefile.common now always uses gcc for pre-processing.
2. Specify -nostdinc for pre-processing, so host-dependent include
files are NOT used, since this leads to proof instability across
platforms.
3. Instead, specify and add our own minimal set the the standard
LibC header files, but only what we need for mlkem-native and nothing
else, all based solely on what is required by ISO C99 (7.17 - 7.21)
4. First two sets of header files are for
4.1. The "LP64" data model, where "Long and Pointers are 64 bits",
which is that used by common 64-bit platforms such as AArch64/Linux,
x86_64/Linux and AArch64/macOS.
4.2. The "ILP32" data model, for 32-bit systems such as 32-bit ARM,
RV32, and most embedded 32-bit microcontrollers.
A third set of headers for the LLP64 (64-bit Windows) will follow.
5. Environment variable CBMC_DM controls selection of the data-model
in Makefile.common. It defaults to "LP64"
6. Update four proofs to use Z3 instead of bitwuzla, which turn out
to be more efficient with CBMC 6.11 and these header files.
Signed-off-by: Rod Chapman <rodchap@amazon.com>
Proof timing experiments on this functions on Grv4/Ubuntu: Constant parameters: CBMC 6.11, z3 4.15.3, LP64 data model CBMC_OBJECT_BITS=B below 1. z3 default, K=2, B=12: unsat in 3m59s 2. z3 smt_only, K=2, B=12: unsat in 6m26s 3. z3 default, K=2, B=9: unsat in 20m7s 4. z3 smt_only, K=2, B=9: unsat in 4m18s So far, case 1 is winning, so let's try modulating B: 5. z3 default, K=2, B=13: unsat in 3m10s 6. z3 default, K=2, B=14: unsat in 3m15s So case 5 now winning. Let's try other values of K 7. z3 default, K=3, B=13: unsat in 3m11s 8. z3 default, K=4, B=13: unsat in 3m11s I also find that macOS is fine with those settings, in similar times. I also find that using z3 5.1.0 with case 5 terminates in 2m47s, so we can expect a small improvement with we upgrade to 5.1.0 Signed-off-by: Rod Chapman <rodchap@amazon.com>
1. CBMC's Makefile.common runs proofs with the chosen data model based on the setting of the CBMC_DM environment variable. If not set, this defaults to LP64. The "result", "smt" and "report" makefile targets are updated to report the setting of MLKEM_K and CBMC_DM to the user to confirm what is being run. 2. The run-cbmc-proofs.py script is similarly updated to use CBMC_DM (or LP64 by default), and to confirm the setting of MLKEM_K and CBMC_DM to the user before spawning litani. 3. Finally, the top-level "tests cbmc" script is updated to add a new command-line option "--dm" with legal values "LP64" and "ILP32", also defaulting to "LP64" if not specified. Updates to GitHub actions to exercise these new options are TBD. Signed-off-by: Rod Chapman <rodchap@amazon.com>
60a8c71 to
d4b81e6
Compare
CBMC Makefile.common now always uses gcc for pre-processing.
Specify -nostdinc for pre-processing, so host-dependent include files are NOT used, since this leads to proof instability across platforms.
Instead, specify and add our own minimal set the the standard LibC header files, but only what we need for mlkem-native and nothing else, all based solely on what is required by ISO C99 (7.17 - 7.21)
First two sets of header files are for
4.1. The "LP64" data model, where "Long and Pointers are 64 bits", which is that used by common 64-bit platforms such as AArch64/Linux, x86_64/Linux and AArch64/macOS.
4.2. The "ILP32" data model, for 32-bit systems such as 32-bit ARM, RV32, and most embedded 32-bit microcontrollers.
A third set of headers for the LLP64 (64-bit Windows) will follow.
Environment variable CBMC_DM controls selection of the data-model in Makefile.common. It defaults to "LP64"
Update four proofs to use Z3 instead of bitwuzla, which turn out to be more efficient with CBMC 6.11 and these header files.