Skip to content

Revert the change to the use of unbounded arrays #6232

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 1 commit into from
Jul 21, 2021

Conversation

martin-cs
Copy link
Collaborator

This was introduced in #6194 but reported as breaking in #6230.

  • 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.

This was introduced in diffblue#6194 but reported as breaking in diffblue#6230.
@martin-cs
Copy link
Collaborator Author

@SaswatPadhi it would be great if you could let me know if this fixes your issue.

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #6232 (fbbc5fd) into develop (0002950) will increase coverage by 8.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6232      +/-   ##
===========================================
+ Coverage    67.40%   75.41%   +8.01%     
===========================================
  Files         1157     1459     +302     
  Lines        95236   161447   +66211     
===========================================
+ Hits         64197   121761   +57564     
- Misses       31039    39686    +8647     
Flag Coverage Δ
cproversmt2 ?
regression ?
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/util/string_container.cpp 52.94% <0.00%> (-47.06%) ⬇️
src/solvers/prop/prop.cpp 42.85% <0.00%> (-41.76%) ⬇️
src/solvers/flattening/boolbv_member.cpp 53.65% <0.00%> (-38.65%) ⬇️
src/cpp/cpp_storage_spec.cpp 65.00% <0.00%> (-35.00%) ⬇️
src/util/cmdline.h 66.66% <0.00%> (-33.34%) ⬇️
src/solvers/strings/array_pool.h 66.66% <0.00%> (-33.34%) ⬇️
src/solvers/strings/string_refinement.h 66.66% <0.00%> (-33.34%) ⬇️
...rs/strings/string_concatenation_builtin_function.h 0.00% <0.00%> (-33.34%) ⬇️
src/cbmc/c_test_input_generator.cpp 60.00% <0.00%> (-30.33%) ⬇️
src/analyses/local_cfg.h 75.00% <0.00%> (-25.00%) ⬇️
... and 1438 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 06c563a...fbbc5fd. Read the comment docs.

Copy link
Contributor

@TGWDB TGWDB left a comment

Choose a reason for hiding this comment

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

Happy to approve (specially for faster turn around). Any chance of a regression test?

@martin-cs
Copy link
Collaborator Author

@TGWDB I would love a regression test and was hoping that @SaswatPadhi might be able to provide one.

@SaswatPadhi
Copy link
Contributor

Thanks for the fix, @martin-cs.

We noticed this on a large s2n proof harness, but let me try to reduce it to a small regression test. I would look into large / dynamically allocated arrays, as you suggested.

@martin-cs
Copy link
Collaborator Author

Thanks @SaswatPadhi

@SaswatPadhi
Copy link
Contributor

This is the most I could shrink it to:

#include <assert.h>
#include <stdint.h>
#include <stdlib.h>

static const uint16_t extensions[] = { 0, 1, 2, 3, 4, 5 };

#define COUNT (sizeof(extensions) / sizeof(extensions[0]))

typedef struct {
  uint8_t *data;
  uint32_t size;
  uint32_t allocated;
  unsigned growable :1;
} ext;

struct context {
  ext exts[COUNT];
};

struct conn {
  void *test;
  struct context ctx;
};

int main() {
  struct conn *s = malloc(sizeof(*s));
  if(s != NULL) s->test = NULL;
}

If I shrink the ext struct further, the invariant violation disappears, so may be there is some issue with alignment? Not sure ...

@martin-cs
Copy link
Collaborator Author

I have tried this example with the latest develop ( 06c563a ) and it doesn't crash. Can you double check and say something about the environment?

@TGWDB
Copy link
Contributor

TGWDB commented Jul 20, 2021

I also tested the example on cbmc release 5.33.0, 5.34.0, and a branch with the fix and had no crashes for any of them (on Ubuntu 20.04 LTS). That said, when I was working on original PR that had this error, there were some behaviours that appeared to be system specific w.r.t. array sizes.

@SaswatPadhi
Copy link
Contributor

Sorry, I should have mentioned the full commandline:

cbmc --malloc-may-fail --malloc-fail-null --pointer-check test.c

@martin-cs
Copy link
Collaborator Author

Ah, yes, that does it! Thanks @SaswatPadhi

@feliperodri
Copy link
Collaborator

Any updates on that PR? @martin-cs @kroening

@feliperodri feliperodri added aws Bugs or features of importance to AWS CBMC users aws-high bugfix labels Jul 21, 2021
@martin-cs
Copy link
Collaborator Author

@feliperodri I am working on the test case that @SaswatPadhi sent and will add it ASAP.

@kroening kroening merged commit 432fb2e into diffblue:develop Jul 21, 2021
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Dec 8, 2022
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.

This reverts diffblue#6232, but now includes additional changes to the array
theory to handle nested struct members no different from members of a
top-level struct.

Fixes: diffblue#2018
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Dec 8, 2022
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.

This reverts diffblue#6232, but now includes additional changes to the array
theory to handle nested struct members no different from members of a
top-level struct.

Fixes: diffblue#2018
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
Labels
aws Bugs or features of importance to AWS CBMC users aws-high bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants