Skip to content

Enable and use the automatic limit for array-as-uninterpreted-function (duplicated, fixes 6130) #6194

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

Merged
merged 6 commits into from
Jul 9, 2021

Conversation

TGWDB
Copy link
Contributor

@TGWDB TGWDB commented Jun 21, 2021

This is a recreation of PR #2108 that should fix Issue #6130 . This has been created as a PR to run CI after rebasing on develop (as of this morning).

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

TGWDB added 3 commits June 11, 2021 19:14
This is a duplicate of PR diffblue#2108. This is here for alternative author
management.

Fixes diffblue#6130
This is a duplicate of PR diffblue#2108. This is here for alternative author
management.

Fixes diffblue#6130
@TGWDB TGWDB changed the title Issue6130 auto array Enable and use the automatic limit for array-as-uninterpreted-function (duplicated, fixes 6130) Jun 21, 2021
@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #6194 (a607546) into develop (a9ad2b8) will increase coverage by 0.22%.
The diff coverage is 92.00%.

❗ Current head a607546 differs from pull request most recent head 8659dac. Consider uploading reports for the commit 8659dac to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6194      +/-   ##
===========================================
+ Coverage    75.39%   75.61%   +0.22%     
===========================================
  Files         1456     1456              
  Lines       161080   161129      +49     
===========================================
+ Hits        121440   121832     +392     
+ Misses       39640    39297     -343     
Impacted Files Coverage Δ
src/solvers/flattening/boolbv.h 76.19% <ø> (ø)
src/util/mathematical_expr.cpp 50.00% <ø> (+32.97%) ⬆️
src/solvers/flattening/boolbv_width.cpp 75.23% <25.00%> (-1.47%) ⬇️
src/util/std_expr.cpp 68.42% <73.23%> (+7.95%) ⬆️
...s/variable-sensitivity/interval_abstract_value.cpp 84.00% <100.00%> (ø)
...variable-sensitivity/value_set_abstract_object.cpp 97.41% <100.00%> (ø)
...rc/analyses/variable-sensitivity/widened_range.cpp 100.00% <100.00%> (ø)
src/analyses/variable-sensitivity/widened_range.h 100.00% <100.00%> (ø)
src/solvers/flattening/boolbv.cpp 81.15% <100.00%> (+0.52%) ⬆️
src/solvers/flattening/boolbv_quantifier.cpp 94.89% <100.00%> (-0.21%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9be0eb...8659dac. Read the comment docs.

@TGWDB
Copy link
Contributor Author

TGWDB commented Jun 21, 2021

@martin-cs Any chance of approval, this is the same as #2108 that you approved (just rebased and passing newer CI).

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

As I said in #2108 , I am OK with this but there is some kind of bug in equality of unbounded arrays which I have yet to reliably trigger via C or locate. I figure that this might expose it so it would be good to have a few tests of what happens when an array is over MAX_FLATTENED_ARRAY_SIZE.

if(total>(1<<30)) // realistic limit
throw analysis_exceptiont("array too large for flattening");

if(total < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case does not seem to be handled any more. Maybe it could become an invariant?

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 did some checking and this error handling should be left in since it can trigger an exception if not handled. I've pushed a fix here.

This reinstates and improves the error handling for very large
arrays and vectors with the limit of 1 << 30 (from old code).
This prevents exceptions in the cast which can occur for extremely
large values.
@TGWDB TGWDB force-pushed the issue6130-auto-array branch 2 times, most recently from 058714b to bc02565 Compare June 22, 2021 13:06
TGWDB added 2 commits June 22, 2021 14:07
This adds a regression test to check that the size limit for
arrays is caught and cleanly handled (with appropriate error).
@TGWDB TGWDB merged commit cd1ec89 into diffblue:develop Jul 9, 2021
martin-cs pushed a commit to martin-cs/cbmc that referenced this pull request Jul 16, 2021
This was introduced in diffblue#6194 but reported as breaking in diffblue#6230.
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Sep 24, 2024
Previously, the command line permitted setting uninterpreted functions
to "never" or "always", where "never" actually was the default. The
"automatic" mode could not be enabled in any way.

We previously attempted to do this in in diffblue#6194 (inspired by diffblue#2108, but
not picking up all its changes), but then reverted the gist of the
change in diffblue#6232 as `array-bug-6230/main.c` demonstrated lingering
issues. This PR now addresses the flaw in the array theory back-end.

We may still run into performance regressions as the threshold of 1000
bits of total size of the array object is possibly lower than where the
cost of bit-blasting exceeds the cost of constraints produced by our
current array theory implementation. Two of our existing regression
tests already demonstrate this problem, hence those now use
`--arrays-uf-never`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants