Skip to content

Load static initializers for enum types returned by opaque methods #3124

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 2 commits into from
Oct 9, 2018
Merged
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.
5 changes: 5 additions & 0 deletions jbmc/regression/jbmc/NondetEnumOpaqueReturn/Color.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
public enum Color {
RED,
GREEN,
BLUE
}
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
public class NondetEnumOpaqueReturn {

public static void canChooseSomeConstant() {
Opaque o = new Opaque();
Color c = o.getC();
if (c == null)
return;
assert c != null;
boolean isRed = c.name().startsWith("RED") && c.name().length() == 3
Copy link
Contributor

Choose a reason for hiding this comment

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

startsWith -> equals?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's equivalent, yes, (and you wouldn't need c.name().length() == 3) but pointer analysis doesn't cope very well with equals because it's defined in Object and overridden almost everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

🚫 This looks like a workaround.equals also has to work...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smowton @peterschrammel I originally had equals in my original enum tests but @romainbrenguier suggested that I use startsWith and length instead because it's quicker, and we don't want these tests to take too long. equals would work as well, it would just take a bit longer I think.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that as long as we are sure that equals works as well (but takes a bit longer).

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 checked locally just to be sure, and using equals works too, all the tests would pass with that. I'll leave the tests as they are, since they are testing properties of the returned enums, not String methods, and it's better to save a bit of time here.

&& c.ordinal() == 0;
boolean isGreen = c.name().startsWith("GREEN") && c.name().length() == 5
&& c.ordinal() == 1;
boolean isBlue = c.name().startsWith("BLUE") && c.name().length() == 4
&& c.ordinal() == 2;
assert (isRed || isGreen || isBlue);
}

public static void canChooseRed() {
Opaque o = new Opaque();
Color c = o.getC();
if (c == null)
return;
boolean isGreen = c.name().startsWith("GREEN") && c.name().length() == 5
&& c.ordinal() == 1;
boolean isBlue = c.name().startsWith("BLUE") && c.name().length() == 4
&& c.ordinal() == 2;
assert (isGreen || isBlue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these negative assertions but canChooseSomeConstant is a positive assertion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Positive assertions check for correctness whereas negative assertions check for reachability (as unreachable assertions cannot be violated by cbmc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the positive assertion checks that jbmc will always choose something in the set of the three constants, and the negative assertions check that for any two given constants, there is something else that it can choose (namely, the third constant).

}

public static void canChooseGreen() {
Opaque o = new Opaque();
Color c = o.getC();
if (c == null)
return;
boolean isRed = c.name().startsWith("RED") && c.name().length() == 3
&& c.ordinal() == 0;
boolean isBlue = c.name().startsWith("BLUE") && c.name().length() == 4
&& c.ordinal() == 2;
assert (isRed || isBlue);
}

public static void canChooseBlue() {
Opaque o = new Opaque();
Color c = o.getC();
if (c == null)
return;
boolean isRed = c.name().startsWith("RED") && c.name().length() == 3
&& c.ordinal() == 0;
boolean isGreen = c.name().startsWith("GREEN") && c.name().length() == 5
&& c.ordinal() == 1;
assert (isRed || isGreen);
}

}
9 changes: 9 additions & 0 deletions jbmc/regression/jbmc/NondetEnumOpaqueReturn/Opaque.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
public class Opaque {

Color c;

public Color getC() {
return c;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CORE
NondetEnumOpaqueReturn.class
--function NondetEnumOpaqueReturn.canChooseSomeConstant --cp `../../../../scripts/format_classpath.sh . ../../../src/java_bytecode/library/core-models.jar`
^VERIFICATION SUCCESSFUL$
^EXIT=0$
^SIGNAL=0$
--
--
The test checks that the name and ordinal fields of nondet-initialized enums
which have been returned by an opaque method correspond to those of an enum
constant of the same type.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CORE
NondetEnumOpaqueReturn.class
--function NondetEnumOpaqueReturn.canChooseRed --cp `../../../../scripts/format_classpath.sh . ../../../src/java_bytecode/library/core-models.jar`
^VERIFICATION FAILED$
--
--
Test 1 of 3 to check that any of the enum constants can be chosen.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CORE
NondetEnumOpaqueReturn.class
--function NondetEnumOpaqueReturn.canChooseGreen --cp `../../../../scripts/format_classpath.sh . ../../../src/java_bytecode/library/core-models.jar`
^VERIFICATION FAILED$
--
--
Test 2 of 3 to check that any of the enum constants can be chosen.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CORE
NondetEnumOpaqueReturn.class
--function NondetEnumOpaqueReturn.canChooseBlue --cp `../../../../scripts/format_classpath.sh . ../../../src/java_bytecode/library/core-models.jar`
^VERIFICATION FAILED$
--
--
Test 3 of 3 to check that any of the enum constants can be chosen.
31 changes: 0 additions & 31 deletions jbmc/src/java_bytecode/ci_lazy_methods.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -392,37 +392,6 @@ void ci_lazy_methodst::initialize_instantiated_classes(
// As in class_loader, ensure these classes stay available
for(const auto &id : extra_instantiated_classes)
needed_lazy_methods.add_needed_class("java::" + id2string(id));

// Special case for enums. We may want to generalise this, see TG-4689
// and the comment in java_object_factoryt::gen_nondet_pointer_init.
for(const auto &found_class : needed_lazy_methods.get_instantiated_classes())
{
const auto &class_type = to_java_class_type(ns.lookup(found_class).type);
if(class_type.get_base("java::java.lang.Enum"))
add_clinit_call(found_class, ns.get_symbol_table(), needed_lazy_methods);
}
}

/// Helper function for `initialize_instantiated_classes`.
/// For a given class id that is being noted as needed in `needed_lazy_methods`,
/// notes that its static initializer is also needed.
/// This applies the same logic to the given class that
/// `java_bytecode_convert_methodt::get_clinit_call` applies e.g. to classes
/// whose constructor we call in a method body. This duplication is unavoidable
/// due to the fact that ci_lazy_methods essentially has to go through the same
/// logic as __CPROVER_start in its initial setup.
/// \param class_id: The given class id
/// \param symbol_table: Used to look up occurrences of static initializers
/// \param [out] needed_lazy_methods: Gets notified of any static initializers
/// that need to be loaded
void ci_lazy_methodst::add_clinit_call(
const irep_idt &class_id,
const symbol_tablet &symbol_table,
ci_lazy_methods_neededt &needed_lazy_methods)
{
const irep_idt &clinit_wrapper = clinit_wrapper_name(class_id);
if(symbol_table.symbols.count(clinit_wrapper))
needed_lazy_methods.add_needed_method(clinit_wrapper);
}

/// Get places where virtual functions are called.
Expand Down
5 changes: 0 additions & 5 deletions jbmc/src/java_bytecode/ci_lazy_methods.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,6 @@ class ci_lazy_methodst:public messaget
const namespacet &ns,
ci_lazy_methods_neededt &needed_lazy_methods);

void add_clinit_call(
const irep_idt &class_id,
const symbol_tablet &symbol_table,
ci_lazy_methods_neededt &needed_lazy_methods);

void gather_virtual_callsites(
const exprt &e,
std::unordered_set<exprt, irep_hash> &result);
Expand Down
27 changes: 27 additions & 0 deletions jbmc/src/java_bytecode/ci_lazy_methods_needed.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Author: Chris Smowton, [email protected]
/// Context-insensitive lazy methods container

#include "ci_lazy_methods.h"
#include "java_static_initializers.h"

#include <java_bytecode/select_pointer_type.h>
#include <string>
Expand All @@ -25,6 +26,24 @@ void ci_lazy_methods_neededt::add_needed_method(
callable_methods.insert(method_symbol_name);
}

/// For a given class id, note that its static initializer is needed.
/// This applies the same logic to the given class that
/// `java_bytecode_convert_methodt::get_clinit_call` applies e.g. to classes
/// whose constructor we call in a method body. This duplication is unavoidable
/// because ci_lazy_methods essentially has to go through the same logic as
/// __CPROVER_start in its initial setup, and because return values of opaque
/// methods need to be considered in ci_lazy_methods too.
/// \param class_id: The given class id
/// \param symbol_table: Used to look up occurrences of static initializers
void ci_lazy_methods_neededt::add_clinit_call(
const irep_idt &class_id,
const symbol_tablet &symbol_table)
{
const irep_idt &clinit_wrapper = clinit_wrapper_name(class_id);
if(symbol_table.symbols.count(clinit_wrapper))
add_needed_method(clinit_wrapper);
}

/// Notes class `class_symbol_name` will be instantiated, or a static field
/// belonging to it will be accessed. Also notes that its static initializer is
/// therefore reachable.
Expand All @@ -42,6 +61,14 @@ bool ci_lazy_methods_neededt::add_needed_class(
if(symbol_table.symbols.count(cprover_validate))
add_needed_method(cprover_validate);

// Special case for enums. We may want to generalise this, the comment in
// \ref java_object_factoryt::gen_nondet_pointer_init (TG-4689).
namespacet ns(symbol_table);
const auto &class_type =
to_java_class_type(ns.lookup(class_symbol_name).type);
if(class_type.get_base("java::java.lang.Enum"))
add_clinit_call(class_symbol_name, symbol_table);

return true;
}

Expand Down
8 changes: 3 additions & 5 deletions jbmc/src/java_bytecode/ci_lazy_methods_needed.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,6 @@ class ci_lazy_methods_neededt
pointer_type_selector(pointer_type_selector)
{}

std::unordered_set<irep_idt> get_instantiated_classes()
{
return instantiated_classes;
}

void add_needed_method(const irep_idt &);
// Returns true if new
bool add_needed_class(const irep_idt &);
Expand All @@ -61,6 +56,9 @@ class ci_lazy_methods_neededt

const select_pointer_typet &pointer_type_selector;

void
add_clinit_call(const irep_idt &class_id, const symbol_tablet &symbol_table);

void initialize_instantiated_classes_from_pointer(
const pointer_typet &pointer_type,
const namespacet &ns);
Expand Down