Skip to content

Fix lazy loading of enum static initializers #2754

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
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 added jbmc/regression/jbmc/NondetEnumArgField/Color.class
Binary file not shown.
5 changes: 5 additions & 0 deletions jbmc/regression/jbmc/NondetEnumArgField/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.
11 changes: 11 additions & 0 deletions jbmc/regression/jbmc/NondetEnumArgField/MyClass.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
public class MyClass {

private Color c;

public int myMethod() {
if (c == null)
return 10;
return 20;
}

}
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
public class NondetEnumArgField {

public static void test(MyClass mc) {
int result = mc.myMethod();
assert result == 10;
}

}
9 changes: 9 additions & 0 deletions jbmc/regression/jbmc/NondetEnumArgField/test.desc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CORE
NondetEnumArgField.class
--function NondetEnumArgField.test --cp `../../../../scripts/format_classpath.sh . ../../../src/java_bytecode/library/core-models.jar`
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't the field be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, I just wanted it to assert that it can in fact come up with a non-null enum.

^VERIFICATION FAILED$
--
--
The test checks that even when none of the enum constants are referenced in user
code, a nondet enum (in this case, a field of an argument) is still correctly
loaded and initialized.
35 changes: 16 additions & 19 deletions jbmc/src/java_bytecode/ci_lazy_methods.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,16 +382,6 @@ void ci_lazy_methodst::initialize_instantiated_classes(
if(param.type().id()==ID_pointer)
{
const pointer_typet &original_pointer = to_pointer_type(param.type());
const auto &original_type = ns.follow(original_pointer.subtype());
// Special case for enums. We may want to generalise this, see TG-4689
// and the comment in java_object_factoryt::gen_nondet_pointer_init.
if(
can_cast_type<java_class_typet>(original_type) &&
to_java_class_type(original_type).get_base("java::java.lang.Enum"))
{
add_clinit_call_for_pointer_type(
original_pointer, ns.get_symbol_table(), needed_lazy_methods);
}
needed_lazy_methods.add_all_needed_classes(original_pointer);
}
}
Expand All @@ -407,28 +397,35 @@ 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"))
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 we can test on ID_enumeration for class types instead of this comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, I've been doing this all over the place and no one told me about ID_enumeration. 😕 I'd like to get this merged quickly to fix my previous mistake, happy if I change all the get_bases in a follow-up PR?

add_clinit_call(found_class, ns.get_symbol_table(), needed_lazy_methods);
}
}

/// Helper function for `initialize_instantiated_classes`.
/// For a given pointer_typet 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 class of `pointer_type` that
/// 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 pointer_type: The given pointer_typet
/// \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_for_pointer_type(
const pointer_typet &pointer_type,
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 &pointer_id =
to_symbol_type(pointer_type.subtype()).get_identifier();
const irep_idt &clinit_wrapper = clinit_wrapper_name(pointer_id);
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);
}
Expand Down
4 changes: 2 additions & 2 deletions jbmc/src/java_bytecode/ci_lazy_methods.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ class ci_lazy_methodst:public messaget
const namespacet &ns,
ci_lazy_methods_neededt &needed_lazy_methods);

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

Expand Down
5 changes: 5 additions & 0 deletions jbmc/src/java_bytecode/ci_lazy_methods_needed.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ 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 Down