TG-592 Implemented the correct instantiation procedure for not contains constraints #1407

jasigal · 2017-09-20T12:54:56Z

The method of insantiating not contains constraints is correct, see TG-591. This involved a localized change to string_constraint_instantiation.cpp:instantiate_not_contains, but also involved the activation of counter-examples in string_refinementt. The reason for this is that the new (correct) instances are relatively weak, and the solver will end up with an empty index set. The current behavior of returning SAT in this case is incorrect, and will be fixed shortly by TG-672. Additionally, the choice of indices at which to instantiate not contains constraints has been changed to be more sensible. Finally, the relevant unit tests have been updated.

jasigal · 2017-09-20T13:01:20Z

@peterschrammel @romainbrenguier Please review if able.

jasigal · 2017-09-22T09:57:23Z

ping @romainbrenguier @peterschrammel
The test-gen/cbmc issue is resolved now, so test-gen CI will pass (the build stage for sure at least).

romainbrenguier

Looks good, I just have some minor comments for improvement.
Also should we add unit tests for that?

romainbrenguier · 2017-09-20T13:07:23Z

src/solvers/refinement/string_refinement.cpp

@@ -1235,22 +1235,25 @@ bool string_refinementt::check_axioms()

    if(use_counter_example)


Could we put this block into a new function, and call it from dec_solve? Because this is not really checking axioms but adding new things to the solver.

In my refactor ticket, I really rearrange dec_solve, including this. Is that okay?

romainbrenguier · 2017-09-20T13:08:08Z

src/solvers/refinement/string_refinement.cpp

@@ -66,7 +66,7 @@ generator_info(const string_refinementt::infot &in)

 string_refinementt::string_refinementt(const infot &info, bool):
  supert(bv_refinement_info(info)),
-  use_counter_example(false),
+  use_counter_example(true),


Since it is never modified it makes sense to remove this use_counter_example

romainbrenguier · 2017-09-22T12:49:05Z

src/solvers/refinement/string_constraint_instantiation.cpp

+  {
+    const exprt i0=pair.first;
+    const exprt i1=pair.second;
+    const minus_exprt val(i0, i1);


can you comment on why we look at this value of val?

romainbrenguier · 2017-09-22T12:49:44Z

src/solvers/refinement/string_constraint_instantiation.cpp

+    const equal_exprt relevancy(f, i1);
+    const and_exprt premise(relevancy, axiom.premise(), universal_bound);
+
+    const not_exprt differ(equal_exprt(s0[i0], s1[i1]));


you can use notequal_exprt

romainbrenguier · 2017-09-22T12:52:02Z

src/solvers/refinement/string_constraint_instantiation.cpp

+    const and_exprt existential_bound(
+      binary_relation_exprt(axiom.exists_lower_bound(), ID_le, i1),
+      binary_relation_exprt(axiom.exists_upper_bound(), ID_gt, i1));
+    const and_exprt body(differ, existential_bound);


formula about indices (like existential_bound) should go to the premise (see my notes here: https://docs.google.com/a/diffblue.com/document/d/1hx5H_whNSdVQy15CwwEClxaVdmevCHJis18-hDhvV9Y/edit?usp=sharing)

... but it does not matter here because we are not constructing a string_constraintt

romainbrenguier · 2017-09-22T12:54:30Z

src/solvers/refinement/string_refinement.cpp

  const string_constraint_generatort &generator)
 {
-  const string_exprt s0=to_string_expr(axiom.s0());
-  const string_exprt s1=to_string_expr(axiom.s1());
+  const string_exprt &s0=to_string_expr(axiom.s0());


The type of axiom.s0 should be string_exprt by definition

romainbrenguier · 2017-09-22T12:55:00Z

src/solvers/refinement/string_refinement.cpp

+  const auto &current_index_set1=current_index_set.find(s1.content());
+
+  if(index_set0!=index_set.end() && index_set1!=index_set.end() &&
+     current_index_set0!=index_set.end() && current_index_set1!=index_set.end())


put each condition on a separate line

romainbrenguier · 2017-09-22T12:55:42Z

src/solvers/refinement/string_refinement.h

@@ -39,7 +39,7 @@ class string_refinementt final: public bv_refinementt
    bool string_non_empty=false;
    /// Concretize strings after solver is finished
    bool trace=false;
-    bool use_counter_example=false;
+    bool use_counter_example=true;


we could remove this variable has it is never modified

I think it should still be kept, as it was kept before, because it can have a big effect on the solver. If you're not using not contains constraints, I think it's pretty unnecessary based on our tests. However, it will probably add a good number of new lemmas. I think there may a command line argument in test-gen that can set this.

How would you decide when to turn off/on that flag?

Would leave it on by default so that more tests are produced by test-gen, but would be turned off it test-gen mem'd or timed out (and could be tracked to strings). Then you could turn it off, which would keep correctness but possibly produce less tests when not contains constraints are involved.

romainbrenguier · 2017-09-22T12:57:37Z

unit/solvers/refinement/string_constraint_instantiation/instantiate_not_contains.cpp

@@ -81,6 +78,17 @@ std::set<exprt> full_index_set(const string_exprt &s)
  return ret;
 }

+/// Create the cartesian product of two sets.
+template<class T, class U>
+std::set<std::pair<T, U>> product(const std::set<T> ts, const std::set<U> us)


depending on the usage, you could replace this set by a vector since all insertions are going to be distinct.

The sets are used because sets are used in string_refinementt, where a set is needed because insertions aren't unique. Hence I had to use sets here, and why this utility function uses sets.

jasigal · 2017-09-22T13:44:51Z

@romainbrenguier comments addressed. We have unit tests for now as well.

peterschrammel · 2017-09-22T16:17:04Z

src/solvers/refinement/string_constraint_instantiation.cpp

+  {
+    // We have s0[x+f(x)] and s1[f(x)], so to have i0 indexing s0 and i1
+    // indexing s1, we need x = i0 - i1 and f(i0 - i1) = f(x) = i1.
+    const exprt i0=pair.first;


copy needed? (some more instances in this patch)

Nope, fixed it to a reference now.

peterschrammel · 2017-09-22T16:18:40Z

src/solvers/refinement/string_refinement.h

@@ -39,7 +39,7 @@ class string_refinementt final: public bv_refinementt
    bool string_non_empty=false;
    /// Concretize strings after solver is finished
    bool trace=false;
-    bool use_counter_example=false;
+    bool use_counter_example=true;


How would you decide when to turn off/on that flag?

…ns constraints The method of insantiating not contains constraints is correct, see TG-591. This involved a localized change to `string_constraint_instantiation.cpp:instantiate_not_contains`, but also involved the activation of counter-examples in `string_refinementt`. The reason for this is that the new (correct) instances are relatively weak, and the solver will end up with an empty index set. The current behavior of returning SAT in this case is incorrect, and will be fixed shortly by TG-672. Additionally, the choice of indices at which to instantiate not contains constraints has been changed to be more sensible. Finally, the relevant unit tests have been updated.

d0d3620 Merge remote-tracking branch 'upstream/develop' into security-scanner-support 875d554 Merge branch 'develop' of github.com:diffblue/cbmc into develop 3a06eda don't rely on index expressions to be vector or arrays 6d6e1df Merge pull request diffblue#1407 from jasigal/fix/instantiate-not-contains#TG-592 8da3eef Merge pull request diffblue#1414 from antlechner/antonia/Character 0b56d97 Fix signatures for methods in java.lang.Character e4e2b10 TG-592 Implemented the correct instantiation procedure for not contains constraints ae83e4e Added install command for required projects. 0e70863 Merge pull request diffblue#1279 from NathanJPhillips/feature/expr_dynamic_cast 1e9837e Merge pull request diffblue#1405 from smowton/cleanup/remove_instanceof 7eb3c76 Merge pull request diffblue#1411 from diffblue/revert-1322-feature/java_support_generics 3b3fee3 Revert "TG-374 Feature/java support generics" 0e14431 Merge pull request diffblue#1408 from reuk/reuk/regression-filenames 2ddb0bc Simplify remove_instanceof logic b4f57ee Merge pull request diffblue#1410 from LAJW/fix-test-gen bcfb914 Remove invariant causing failure in unit test generation involving CharSequences 93149be Merge pull request diffblue#1322 from mgudemann/feature/java_support_generics c731b98 Merge pull request diffblue#1404 from LAJW/string-refinement-functional e5f5e8b Use `optionalt` for signature f68e817 Parse LocalVariabletypetable / Signature attributes use in types 585fb66 Merge pull request diffblue#1400 from reuk/reuk/enable-compile-commands da4df03 Switch from pointers to optionalt 413fc1b Automatically deduce test names from dir names 05d5a9b Address commenters' suggestions c48170e Merge pull request diffblue#192 from diffblue/smowton/feature/split_frontend_final_stage 0823f05 allow goto-cc -E 00cbad7 test for va_args 96f2d9e Added a PRECONDITION assert/invariant b8ab624 Code review fixup 775d9dd Renamed can_cast_expr c372eb3 Used typeinfo functions instead of rolling own 95f6505 Comment on types for which expr_dynamic_cast is not implemented d35710f Made validate_expr non-template de8a8d0 Implementation of expr_dynamic_cast f763691 Tidy up remove_instanceof f4df5c6 Add tests for mixed GOTO and C input 215d5bf Split the entry-point-generation phase into two parts ab347d5 Merge pull request diffblue#195 from diffblue/bugfix/missing-const_cast 73fba6e Fixed missing const_cast 751e8f5 Adhere to lintage b544a4b Fix unit test build 9b8a025 Fix get lambdas bee20f1 Remove unused add_lemma parameter d03866d static add axioms for string assings 1531f0e static debug_model f5c1b29 static get_array 7f11ccf static set_char_array_equality 229568a static Instantiate not contains 78303df Static concretize strings, lengths and results 918297c static concretize length a4c8cf5 Static index set functions e5e1ff4 static index_set functions 65ad3db Public methods made public 2eed573 Static add_axioms for strings 666c146 make static check_axioms bcd6111 Static is_axiom_sat 01b8301 static is_valid_string_constraint cfb47db Remove unused functions from string_refinement header 8e69d6d Make functions static 0cec13d Merge pull request diffblue#1360 from KPouliasis/konst_splice_call 9f9f30d fixup return value eaf97f6 Simplify remove_instanceof logic bc30987 Merge pull request diffblue#1235 from romainbrenguier/feature/string-max-input-length#948 0323ed0 Merge pull request diffblue#1349 from peterschrammel/cleanup/use-preformatted-output 621da87 Tests for new option string-max-input-length 440d19f Adding string max input length option e957025 Print results to result stream instead of status ad6484e Print XML and JSON objects on message stream 4969295 Tidy up remove_instanceof 6cb2f90 Merge pull request diffblue#1398 from diffblue/json_direct_return f365afe Merge pull request diffblue#1392 from reuk/reuk/fixup-appveyor-regressions 4a1565b Enable compile command output 523f028 Merge pull request diffblue#1368 from diffblue/use_size_t 358b435 prefer a ranged for over indexed iteration 69a8eaf use std::size_t for container indices cef7659 counters need to be size_t 2a7a0e8 the step number needs to be a size_t 9d48225 json_irept now returns ireps and jsont directly f22a864 Merge pull request diffblue#1386 from diffblue/return_directly 2b050a0 Merge pull request diffblue#1372 from diffblue/preconditions 6a509a8 goto-instrument no longer needs partial inlining by default e3f75d3 fixup coverage tests 1ff6bf2 missing include ac022e2 inlined functions are no longer ignored when doing coverage 4cb72b3 check error message in test 3a4ebeb use preconditions in the library f443b18 partial inlining is no longer needed 5624b15 instrumentation for preconditions 54e80da added __CPROVER_precondtion(c, d) 1d81035 Merge pull request diffblue#1393 from diffblue/byte_extract_is_binary 4e1fe93 Merge pull request diffblue#1396 from diffblue/String6-fixup 9497b02 Merge pull request diffblue#1395 from diffblue/msvc_unistd 17c6ca0 MSVC doesn't have strcasecmp and strncasecmp; use header for free() 237b31a Visual Studio doesn't have unistd.h; signal.h isn't needed for the MSVC build 5ef9c17 Revert 20e4def (preformatted_output flag) 3d4c794 Pass ostream instead of using cout in natural_loops 2716410 Update linter to cope with CBMC subtree 63fc53b Stop using sed to modify scripts! 58b75cf Merge pull request diffblue#1307 from zemanlx/coverity da91319 Adapt to upstream change in write_goto_binary interface and languaget 177a13d Add Coverity scan 22fb7c1 added splice-call feature in goto instrument It prepends a nullary function call say foo in the body of another call say moo when called as --splice-call moo,foo added tests a400c23 Merge pull request diffblue#1387 from thk123/feature/disable-mac-builds 68f2862 byte_extract expressions are binary expressions ec6ad09 Merge pull request diffblue#1379 from diffblue/remove-symex 1c1d3c2 remove path-symex and the symex tool f96ff48 Merge pull request diffblue#1384 from thk123/bugfix/regresssion-makefile 785bf43 Disable OSX builds 428b985 return STL containers directly for some variants of compute_called_functions 4bdd9c4 Corrected regression makefile 2b2a841 Convert display_index_set to a free function 9fff116 Remove constructor boilerplate 150bab1 Group string_refinement arguments 317c1c6 Group bv_refinement config variables bf47f81 Use expr_cast in string_exprt casts c5fa708 Refactor integer conversions f99c8ff Replace exceptions with optional git-subtree-dir: cbmc git-subtree-split: d0d3620

romainbrenguier requested review from peterschrammel and romainbrenguier September 20, 2017 13:02

jasigal force-pushed the fix/instantiate-not-contains#TG-592 branch 3 times, most recently from 99eb60b to c53a80a Compare September 22, 2017 09:56

romainbrenguier approved these changes Sep 22, 2017

View reviewed changes

jasigal force-pushed the fix/instantiate-not-contains#TG-592 branch 2 times, most recently from 4dd312c to 709000b Compare September 22, 2017 13:42

peterschrammel approved these changes Sep 22, 2017

View reviewed changes

jasigal force-pushed the fix/instantiate-not-contains#TG-592 branch from 709000b to e4e2b10 Compare September 22, 2017 16:46

kroening merged commit 6d6e1df into diffblue:develop Sep 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TG-592 Implemented the correct instantiation procedure for not contains constraints #1407

TG-592 Implemented the correct instantiation procedure for not contains constraints #1407

jasigal commented Sep 20, 2017

jasigal commented Sep 20, 2017

jasigal commented Sep 22, 2017

romainbrenguier left a comment

romainbrenguier Sep 20, 2017

jasigal Sep 22, 2017

romainbrenguier Sep 20, 2017

romainbrenguier Sep 22, 2017

romainbrenguier Sep 22, 2017

romainbrenguier Sep 22, 2017

romainbrenguier Sep 22, 2017

romainbrenguier Sep 22, 2017

romainbrenguier Sep 22, 2017

romainbrenguier Sep 22, 2017

jasigal Sep 22, 2017

peterschrammel Sep 22, 2017

jasigal Sep 22, 2017

romainbrenguier Sep 22, 2017

jasigal Sep 22, 2017

jasigal commented Sep 22, 2017

peterschrammel Sep 22, 2017

jasigal Sep 22, 2017

peterschrammel Sep 22, 2017

		@@ -1235,22 +1235,25 @@ bool string_refinementt::check_axioms()

		if(use_counter_example)

TG-592 Implemented the correct instantiation procedure for not contains constraints #1407

TG-592 Implemented the correct instantiation procedure for not contains constraints #1407

Conversation

jasigal commented Sep 20, 2017

jasigal commented Sep 20, 2017

jasigal commented Sep 22, 2017

romainbrenguier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasigal commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment