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

[CN] Fix #380 (Have ptr_eq typechecks its arguments) #522

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsainati1
Copy link
Contributor

Fixes #380

backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
@dsainati1 dsainati1 marked this pull request as ready for review August 22, 2024 13:12
@dsainati1 dsainati1 requested a review from dc-mak August 26, 2024 17:53
Copy link
Contributor

@dc-mak dc-mak left a comment

Choose a reason for hiding this comment

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

On the right track overall, thank you. Should be ready to merge in a couple of iterations I think.

backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/compile.ml Outdated Show resolved Hide resolved
backend/cn/lib/compile.ml Outdated Show resolved Hide resolved
backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
@dsainati1 dsainati1 requested a review from dc-mak August 27, 2024 13:23
Copy link
Contributor

@dc-mak dc-mak left a comment

Choose a reason for hiding this comment

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

Last round of changes - just a few tweaks, thank you so much. If you can rebase and squash that would also be helpful.

backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/builtins.ml Outdated Show resolved Hide resolved
backend/cn/lib/indexTerms.ml Outdated Show resolved Hide resolved
backend/cn/lib/core_to_mucore.ml Show resolved Hide resolved
@dc-mak
Copy link
Contributor

dc-mak commented Aug 28, 2024

Also once this PR is done and merged, can you do a quick PR adding .mli files for Core_to_mucore and Compile? https://github.com/rems-project/cerberus/blob/master/backend/cn/ONBOARDING.md#adding-mli-files

@dc-mak
Copy link
Contributor

dc-mak commented Aug 28, 2024

There look to be CI failures. Can you please investigate? One of them is code-formatting.
For runtime testing related things, you can ask @rbanerjee20 if you get stuck.

Copy link
Contributor

@dc-mak dc-mak left a comment

Choose a reason for hiding this comment

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

Lgtm but CI is failing.

@dsainati1
Copy link
Contributor Author

Hmm, I don't know exactly what the failing runtime tests are or how to invoke them locally, and the CI job needs to be approved by a maintainer.

@dc-mak
Copy link
Contributor

dc-mak commented Aug 28, 2024

Are you able to view which CI jobs pass or fail? And the logs of those or not?

Regardless, if you look in the workflow file, you'll find the command: https://github.com/rems-project/cerberus/actions/runs/10599783302/workflow?pr=522#L85 : cd tests && ./run-cn-exec.sh

Before running the file, you can open it to see timeout 20 "${CHECK_SCRIPT}" "${SCRIPT_OPT}" "$file" &> /dev/null called in a function which is called in a loop. Alternatively, you can uncomment set -xv for debugging and run the file itself.

It will produce a bunch of output, including something like /home/dhruv/.opam/4.14.1/lib/cn/runtime/libexec/cn-runtime-single-file.sh -oq tests/cn/missing_resource_indirect.error.c which can be simplified down to
${OPAM_SWITCH_PREFIX}/lib/cn/runtime/libexec/cn-runtime-single-file.sh -oq tests/cn/missing_resource_indirect.error.c
or
./runtime/libcn/libexec/cn-runtime-single-file.sh -oq tests/cn/missing_resource_indirect.error.c

We should add this to the ONBOARDING doc.

@dc-mak
Copy link
Contributor

dc-mak commented Aug 28, 2024

If you follow a similar process with the CI build workflow, you'll see that slf_incr2.c in the tutorial repo is now failing (because ptr_eq is now stricter).

replace function_sig in compile.ml with LogicalFunctions.definition

Merge github.com:rems-project/cerberus

reset to origin master

append builtin cn functions to ail_functions in core_to_mucore

add builtin flag to translate_cn_expr

WIP: replace ptr_eq with cn_function version

also don't warn on inequal for builtins

replace ptr_eq with a cn function

rewrite is_null

translate addr_eq and prov_eq

refactor

Merge branch 'master' of github.com:rems-project/cerberus

revert to using LogicalFunctions.definition for compile.ml

refactor builtins to use logicalfunctions.definition

remove unused function

add min and max bits to builtin defs

add comment

respond to review

Merge branch 'master' of github.com:rems-project/cerberus

use old function_sig type in compile.ml

fix addr_eq

Merge branch 'rems-project:master' into master

respond to review
@dsainati1
Copy link
Contributor Author

I submitted rems-project/cn-tutorial#83 to the tutorial repo to fix the issue with slf_incr2.c, and fixed the runtime issue I believe.

@dsainati1 dsainati1 requested a review from dc-mak August 28, 2024 16:37
@dc-mak
Copy link
Contributor

dc-mak commented Aug 28, 2024

The tutorial is still failing for some reason even though your change was merged in rems-project/cn-tutorial#83

The runtime is now showing a further bug - it needs to be told where to find and substitute definitions of the builtins.

Copy link
Contributor

@dc-mak dc-mak left a comment

Choose a reason for hiding this comment

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

See above.

@rbanerjee20
Copy link
Contributor

The runtime is now showing a further bug - it needs to be told where to find and substitute definitions of the builtins.

I'll take a look

@rbanerjee20
Copy link
Contributor

This is what I'm getting when running the single-file runtime testing script on slf_incr2.c:

slf_incr2.c:19:21: error: Type error
            ptr_eq (vs_.pv,
                    ~~~^~~ 
Expression 'vs_.pv' has type 'u32'.
I expected it to have type 'pointer' because of other location (<builtin>) 

Failed to generate C files from CN-annotatated source.

Is this the same error you were talking about @dc-mak ?

@dsainati1
Copy link
Contributor Author

That should be fixed, the tutorial's version of slf_incr2.c should be updated to use == instead of ptr_eq there.

@rbanerjee20
Copy link
Contributor

Ah - the CN runtime testing CI relies on the cn-runtime-testing branch of the CN tutorial, not main. It will use main soon but this is still a WIP. I can update that branch and see if it helps with the Cerberus CI

@rbanerjee20
Copy link
Contributor

Great, so runtime testing CI seems to be working again with that same change from the main branch of CN tutorial now also applied to cn-runtime-testing. I did try the script locally on slf_incr2.c and got the following 2 warnings though:

cn.c:43:34: warning: integer literal is too large to be represented in a signed integer type, interpreting as unsigned [-Wimplicitly-unsigned-literal]
  return convert_to_cn_bits_i64(-9223372036854775808);
                                 ^
cn.c:78:33: warning: integer literal is too large to be represented in a signed integer type, interpreting as unsigned [-Wimplicitly-unsigned-literal]
  return convert_to_cn_bits_u64(18446744073709551615);
                                ^
2 warnings generated.

Looks a bit suspicious that these kinds of integer literals are being generated - probably worth investigating even though the runtime testing CI is happy now.

@rbanerjee20
Copy link
Contributor

And the remaining CI failure seems to be CN test-gen-related - @ZippeyKeys12 would be the right person to ask about this.

@dc-mak
Copy link
Contributor

dc-mak commented Aug 29, 2024

Great, so runtime testing CI seems to be working again with that same change from the main branch of CN tutorial now also applied to cn-runtime-testing. I did try the script locally on slf_incr2.c and got the following 2 warnings though:

cn.c:43:34: warning: integer literal is too large to be represented in a signed integer type, interpreting as unsigned [-Wimplicitly-unsigned-literal]
  return convert_to_cn_bits_i64(-9223372036854775808);
                                 ^
cn.c:78:33: warning: integer literal is too large to be represented in a signed integer type, interpreting as unsigned [-Wimplicitly-unsigned-literal]
  return convert_to_cn_bits_u64(18446744073709551615);
                                ^
2 warnings generated.

Looks a bit suspicious that these kinds of integer literals are being generated - probably worth investigating even though the runtime testing CI is happy now.

Known issue: #536

@dc-mak dc-mak changed the title Have ptr_eq typecheck its arguments [CN] Fix: ptr_eq typechecks its arguments Sep 3, 2024
@dc-mak dc-mak changed the title [CN] Fix: ptr_eq typechecks its arguments [CN] Fix #380 (Have ptr_eq typechecks its arguments) Sep 3, 2024
@dc-mak dc-mak marked this pull request as draft September 3, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CN] ptr_eq typing (confusingly) not strict enough
3 participants