Skip to content

Add --load-containing-class-only option #2633

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file not shown.
8 changes: 8 additions & 0 deletions jbmc/regression/jbmc/load-containing-class-only/External.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class External {

public static int get() {
assert(false);
return 40;
}

}
Binary file not shown.
7 changes: 7 additions & 0 deletions jbmc/regression/jbmc/load-containing-class-only/Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class Test {

public static int test() {
return External.get();
}

}
10 changes: 10 additions & 0 deletions jbmc/regression/jbmc/load-containing-class-only/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
CORE
Test.class
--function Test.test --load-containing-class-only
^EXIT=0$
^SIGNAL=0$
^VERIFICATION SUCCESSFUL$
--
^warning: ignoring
--
Check that External.class is not loaded when using --load-containing-class-only
Copy link
Contributor

Choose a reason for hiding this comment

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

May be good to have a test that shows the opposite is true without the option.

38 changes: 32 additions & 6 deletions jbmc/src/java_bytecode/java_bytecode_language.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,16 @@ Author: Daniel Kroening, [email protected]

#include <string>

#include <util/symbol_table.h>
#include <util/suffix.h>
#include <util/config.h>
#include <json/json_parser.h>
#include <util/cmdline.h>
#include <util/config.h>
#include <util/expr_iterator.h>
#include <util/invariant.h>
#include <util/journalling_symbol_table.h>
#include <util/string2int.h>
#include <util/invariant.h>
#include <json/json_parser.h>
#include <util/string_utils.h>
#include <util/suffix.h>
#include <util/symbol_table.h>

#include <goto-programs/class_hierarchy.h>

Expand Down Expand Up @@ -124,7 +125,32 @@ void java_bytecode_languaget::get_language_options(const cmdlinet &cmd)
std::back_inserter(extra_methods),
build_load_method_by_regex);

if(cmd.isset("java-cp-include-files"))
if(cmd.isset("load-containing-class-only"))
{
const std::string function_to_analyse = cmd.get_value("function");
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes --function is set in the command line

const std::size_t first_dollar_index =
function_to_analyse.find_first_of('$');
const std::size_t last_dot_index = function_to_analyse.find_last_of('.');
std::string class_name = function_to_analyse;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to copy function_to_analyse here, it can be used in the substr call instead.

if(first_dollar_index != std::string::npos)
class_name = class_name.substr(0, first_dollar_index);
else
class_name = class_name.substr(0, last_dot_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem to handle the case where there is no dot.

Copy link
Contributor

Choose a reason for hiding this comment

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

There always should be one in a Java function ID (but make an INVARIANT)

replace_all(class_name, ".", "\\/");
java_cp_include_files = class_name +
"(\\$.*)?\\.class|"
"java\\/.*|"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think / needs escaping?

Copy link
Contributor

Choose a reason for hiding this comment

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

Loading all inner classes is a bit weird, should they only be inners of the desired class?

"javax\\/.*|"
"sun\\/.*|"
"org\\/cprover\\/.*|"
"com\\/diffblue\\/annotation\\/.*";
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 is obviously not great. Please suggest ways of making this more general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not actually making it more general, but you might do (javax|sun|org\\/cprover|com\\/diffblue\\/annotation)\\/.*

Copy link
Contributor

Choose a reason for hiding this comment

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

These hard-coded values make the option a bit misleading (it is not only loading the containing class). Maybe these could be added through java-cp-include-files as additional files to include (that have to be given explicitly) on the command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romainbrenguier That might indeed be better. Will try that.


if(cmd.isset("java-cp-include-files"))
warning() << "--java-cp-include-files ignored due to "
"--load-containing-class-only"
<< eom;
}
else if(cmd.isset("java-cp-include-files"))
{
java_cp_include_files=cmd.get_value("java-cp-include-files");
// load file list from JSON file
Expand Down
3 changes: 3 additions & 0 deletions jbmc/src/java_bytecode/java_bytecode_language.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ Author: Daniel Kroening, [email protected]
"(java-max-input-tree-depth):" /* will go away */ \
"(max-nondet-tree-depth):" \
"(java-max-vla-length):" \
"(load-containing-class-only)" \
"(java-cp-include-files):" \
"(lazy-methods)" /* will go away */ \
"(no-lazy-methods)" \
Expand All @@ -59,6 +60,8 @@ Author: Daniel Kroening, [email protected]
" never initialize reference-typed parameter to the\n" /* NOLINT(*) */ \
" entry point with null\n" /* NOLINT(*) */ \
" --java-max-vla-length N limit the length of user-code-created arrays\n" /* NOLINT(*) */ \
" --load-containing-class-only only load the class containing the " \
" method under test and the library classes" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this differ from lazy loading? Obviously I'm not a Java expert, but as a mere user I'm pretty confused by this vast array of options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lazy-loading will load all the classes that are needed to analyse the program. This option is more aggressive as it will cut out anything that leaves the class that contains the method under test. It's a wild over-approximation but it's an easy way of analysing blocks that would otherwise be intractable by jbmc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, there is all the work that @hannes-steffenhagen-diffblue has done (7952f2c and related commits). Is doing it on a by-class level a much better approach for Java? Also, without your explanation above I would not have guess the semantics of this new option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tautschnig The documentation is a bit sparse, I can easily agree with that. It's mostly the fault of the --help string format which we don't want to make to heavy. Where would a good place be to write an extended description?
At this point it's not clear how well a per-class approach would work. It's an experimental feature (I will document is as such).

Copy link
Member

@peterschrammel peterschrammel Jul 31, 2018

Choose a reason for hiding this comment

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

Is doing it on a by-class level a much better approach for Java?

Unfortunately, it's more complicated than that due to Java's compiler-generated methods, inner classes, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Near-duplicate of test-gen's single-function-only option? Perhaps both should be generalised into a function whitelist?

" --java-cp-include-files r regexp or JSON list of files to load\n" \
" (with '@' prefix)\n" \
" --no-lazy-methods load and translate all methods given on\n" \
Expand Down
9 changes: 8 additions & 1 deletion jbmc/src/java_bytecode/java_class_loader_limit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ void java_class_loader_limitt::setup_class_load_limit(
// '@' signals file reading with list of class files to load
use_regex_match = java_cp_include_files[0] != '@';
if(use_regex_match)
{
regex_matcher=std::regex(java_cp_include_files);
debug() << "Limit loading to classes matching : " << java_cp_include_files
Copy link
Contributor

Choose a reason for hiding this comment

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

Class-loading is limited to...

<< eom;
}
else
{
assert(java_cp_include_files.length()>1);
Expand Down Expand Up @@ -54,7 +58,10 @@ bool java_class_loader_limitt::load_class_file(const std::string &file_name)
if(use_regex_match)
{
std::smatch string_matches;
return std::regex_match(file_name, string_matches, regex_matcher);
if(std::regex_match(file_name, string_matches, regex_matcher))
return true;
debug() << file_name + " discarded since not matching loader regexp" << eom;
return false;
}
else
// load .class file only if it is in the match set
Expand Down
13 changes: 13 additions & 0 deletions src/util/string_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,16 @@ std::string escape(const std::string &s)

return result;
}

void replace_all(
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation would be nice.

std::string &str,
const std::string &from,
const std::string &to)
{
size_t start_pos = 0;
while((start_pos = str.find(from, start_pos)) != std::string::npos)
Copy link
Contributor

Choose a reason for hiding this comment

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

A for-loop maybe clearer.

{
str.replace(start_pos, from.length(), to);
start_pos += to.length();
}
}
2 changes: 2 additions & 0 deletions src/util/string_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,6 @@ Stream &join_strings(
/// programming language.
std::string escape(const std::string &);

void replace_all(std::string &, const std::string &, const std::string &);
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping the name of the parameters could be nice.


#endif