Skip to content

[TG-8293] Constant propagation for empty strings and string concatenation [blocks: #4941] #4885

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

Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
public class Main {
public void test() {
String s1 = "abc";
String s2 = "xyz";
String s3 = s1 + s2;
assert s3.equals("abcxyz");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test --property "java::Main.test:()V.assertion.1" --cp `../../../../scripts/format_classpath.sh . ../../../lib/java-models-library/target/core-models.jar`
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
public class Main {
public void test1() {
String s1 = "abc";
String s2 = "xyz";
String s3 = s1 + s2;
assert s3.length() == 7;
}

public void test2() {
String s1 = "abc";
String s2 = "xyz";
String s3 = s1 + s2;
assert s3.startsWith("abcdefg");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
CORE
Main.class
--function Main.test1
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
--
--
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
CORE
Main.class
--function Main.test2
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
public class Main {
public void test() {
String s1 = "ディフ";
String s2 = "ブルー";
String s3 = s1 + s2;
assert s3.length() == 6;
assert s3.startsWith("ディフブルー");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test --property "java::Main.test:()V.assertion.1" --property "java::Main.test:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
public class Main {
public void test() {
String s1 = "\t";
String s2 = "\\";
String s3 = s1 + s2;
assert s3.length() == 2;
assert s3.startsWith("\t\\");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test --property "java::Main.test:()V.assertion.1" --property "java::Main.test:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for being a bore, but regression tests that check for success only verify that cbmc doesn't do anything. They should always be paired with a test checking for failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussion with @danpoe, it seems to be unnecessary to "assertion failure" tests for each case, as not many bad things can happen.
We do want to check that we're not completely discarding the trace, so a few tests for assertion failure makes sense. The tests currently included now look sufficient to me.

--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
public class Main {
public void test() {
StringBuilder sb = new StringBuilder("abc");
sb.append("xyz");
String s = sb.toString();
assert s.length() == 6;
assert s.startsWith("abcxyz");
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely the tests should check equals; whether that decomposes into startsWith and length is the model's business

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test doesn't use the models. I've not included them for tests where they are not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 I think it's not sufficient to have tests for the successful case. We should have tests that check some assertion is violated.
Otherwise we're only checking for under-approximation, and assertions are true when unreachable.
See https://github.com/danpoe/cbmc/tree/feature/simplify-expression-strings/jbmc/regression/jbmc-strings/CompareToConstantEvaluation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added by @yumibagge

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test --property "java::Main.test:()V.assertion.1" --property "java::Main.test:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be exactly 1 VCC if you use --property?

Copy link
Contributor Author

@danpoe danpoe Aug 7, 2019

Choose a reason for hiding this comment

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

--property is passed in twice here, so yes I think the value should be 2. I'll change it to use the precise value.

Copy link
Contributor Author

@danpoe danpoe Aug 8, 2019

Choose a reason for hiding this comment

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

So it turns out that jbmc translates those assertions to something like:

if(length_return_value != 6)
{
  assert(false);
}

So if length_return_value is 6, we don't symex the assertion and thus get 0 VCCs before simplification. This is probably not stable behaviour though so I'll leave it as is. The important thing is that after simplification we have 0 remaining VCCs.

^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
public class Main {
public void test1() {
StringBuilder sb1 = new StringBuilder("abc");
String s2 = "";
sb1.append(s2);
assert sb1.length() == 3;
assert sb1.toString().startsWith("abc");
}

public void test2() {
StringBuilder sb1 = new StringBuilder();
String s2 = "abc";
sb1.append(s2);
assert sb1.length() == 3;
assert sb1.toString().startsWith("abc");
}

public void test3() {
StringBuilder sb1 = new StringBuilder();
String s2 = "";
sb1.append(s2);
assert sb1.length() == 0;
assert sb1.toString().startsWith("");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test1 --property "java::Main.test1:()V.assertion.1" --property "java::Main.test1:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test2 --property "java::Main.test2:()V.assertion.1" --property "java::Main.test2:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test3 --property "java::Main.test3:()V.assertion.1" --property "java::Main.test3:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
public class Main {
public void test() {
StringBuilder sb = new StringBuilder();
String s = sb.toString();
assert s.isEmpty();
assert s.length() == 0;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
public class Main {
public void test() {
String s1 = "abc";
String s2 = "xyz";
String s3 = s1 + s2;
assert s3.length() == 6;
assert s3.startsWith("abcxyz");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test --property "java::Main.test:()V.assertion.1" --property "java::Main.test:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
public class Main {
public void test1() {
String s1 = "abc";
String s2 = "";
String s3 = s1 + s2;
assert s3.length() == 3;
assert s3.startsWith("abc");
}

public void test2() {
String s1 = "";
String s2 = "abc";
String s3 = s1 + s2;
assert s3.length() == 3;
assert s3.startsWith("abc");
}

public void test3() {
String s1 = "";
String s2 = "";
String s3 = s1 + s2;
assert s3.length() == 0;
assert s3.startsWith("");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test1 --property "java::Main.test1:()V.assertion.1" --property "java::Main.test1:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test2 --property "java::Main.test2:()V.assertion.1" --property "java::Main.test2:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test3 --property "java::Main.test3:()V.assertion.1" --property "java::Main.test3:()V.assertion.2"
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
public class Main {
public void test1(String s1) {
String s2 = "abc";
assert (s1 + s2).startsWith("abc");
}

public void test2(String s) { assert ("" + s).startsWith("abc"); }

public void test3(String s) { assert (s + s).startsWith("abc"); }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CORE symex-driven-lazy-loading-expected-failure
Main.class
--function Main.test1 --property "java::Main.test1:(Ljava/lang/String;)V.assertion.1"
^Generated [0-9]+ VCC\(s\), 1 remaining after simplification$
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
--
--
This test checks that constant propagation does not happen, since a constant
result cannot be determined from the arguments to `+`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CORE symex-driven-lazy-loading-expected-failure
Main.class
--function Main.test2 --property "java::Main.test2:(Ljava/lang/String;)V.assertion.1"
^Generated [0-9]+ VCC\(s\), 1 remaining after simplification$
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
--
--
This test checks that constant propagation does not happen, since a constant
result cannot be determined from the arguments to `+`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CORE symex-driven-lazy-loading-expected-failure
Main.class
--function Main.test3 --property "java::Main.test3:(Ljava/lang/String;)V.assertion.1"
^Generated [0-9]+ VCC\(s\), 1 remaining after simplification$
^EXIT=10$
^SIGNAL=0$
^VERIFICATION FAILED$
--
--
This test checks that constant propagation does not happen, since a constant
result cannot be determined from the arguments to `+`.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
public class Main {
public void test() {
String s = new String();
assert s.isEmpty();
assert s.startsWith("");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
Main.class
--function Main.test
^Generated [0-9]+ VCC\(s\), 0 remaining after simplification$
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
--
24 changes: 1 addition & 23 deletions jbmc/src/java_bytecode/java_string_literals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,12 @@ Author: Chris Smowton, [email protected]
#include <util/arith_tools.h>
#include <util/expr_initializer.h>
#include <util/namespace.h>
#include <util/string_utils.h>
#include <util/unicode.h>

#include <iomanip>
#include <sstream>

/// Replace non-alphanumeric characters with `_xx` escapes, where xx are hex
/// digits. Underscores are replaced by `__`.
/// \param to_escape: string to escape
/// \return string with non-alphanumeric characters escaped
static std::string escape_non_alnum(const std::string &to_escape)
{
std::ostringstream escaped;
for(auto &ch : to_escape)
{
if(ch=='_')
escaped << "__";
else if(isalnum(ch))
escaped << ch;
else
escaped << '_'
<< std::hex
<< std::setfill('0')
<< std::setw(2)
<< (unsigned int)ch;
}
return escaped.str();
}

/// Convert UCS-2 or UTF-16 to an array expression.
/// \par parameters: `in`: wide string to convert
/// \return Returns a Java char array containing the same wchars.
Expand Down
Loading