Skip to content

[TG-3694] Parse and store inner class information #2493

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 8 commits into from
Jul 5, 2018
Merged
Show file tree
Hide file tree
Changes from 6 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
1 change: 1 addition & 0 deletions jbmc/src/java_bytecode/java_bytecode_convert_class.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ void java_bytecode_convert_classt::convert(
class_type.set(ID_interface, c.is_interface);
class_type.set(ID_synthetic, c.is_synthetic);
class_type.set_final(c.is_final);
class_type.set_is_inner_class(c.is_inner_class);
if(c.is_enum)
{
if(max_array_length != 0 && c.enum_elements > max_array_length)
Expand Down
1 change: 1 addition & 0 deletions jbmc/src/java_bytecode/java_bytecode_parse_tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ class java_bytecode_parse_treet
bool is_interface = false;
bool is_synthetic = false;
bool is_annotation = false;
bool is_inner_class = false;
bool attribute_bootstrapmethods_read = false;
size_t enum_elements=0;

Expand Down
82 changes: 82 additions & 0 deletions jbmc/src/java_bytecode/java_bytecode_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ class java_bytecode_parsert:public parsert
void rfields(classt &parsed_class);
void rmethods(classt &parsed_class);
void rmethod(classt &parsed_class);
void
rinner_classes_attribute(classt &parsed_class, const u4 &attribute_length);
void rclass_attribute(classt &parsed_class);
void rRuntimeAnnotation_attribute(annotationst &);
void rRuntimeAnnotation(annotationt &);
Expand Down Expand Up @@ -1576,6 +1578,81 @@ void java_bytecode_parsert::relement_value_pair(
}
}

/// Corresponds to the element_value structure
/// Described in Java 8 specification 4.7.6
/// https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.6
/// Parses the InnerClasses attribute for the current parsed class,
/// which contains an array of information about its inner classes. We are
/// interested in getting information only for inner classes, which is
/// determined by checking if the parsed class matches any of the inner classes
/// in its inner class array. If the parsed class is not an inner class, then it
/// is ignored. When a parsed class is an inner class, the accessibility
/// information for the parsed class is overwritten, and the parsed class is
/// marked as an inner class.
void java_bytecode_parsert::rinner_classes_attribute(
classt &parsed_class,
const u4 &attribute_length)
{
std::string name = parsed_class.name.c_str();
u2 number_of_classes = read_u2();
u4 number_of_bytes_to_be_read = number_of_classes * 8 + 2;
INVARIANT(
number_of_bytes_to_be_read == attribute_length,
"The number of bytes to be read for the InnerClasses attribute does not "
"match the attribute length.");

const auto pool_entry_lambda = [this](u2 index) -> pool_entryt & {
return pool_entry(index);
};
const auto remove_separator_char = [](std::string str, char ch) {
str.erase(std::remove(str.begin(), str.end(), ch), str.end());
return str;
};

for(int i = 0; i < number_of_classes; i++)
{
u2 inner_class_info_index = read_u2();
UNUSED u2 outer_class_info_index = read_u2();
UNUSED u2 inner_name_index = read_u2();
u2 inner_class_access_flags = read_u2();

if(inner_class_info_index == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment for why would be good here? My reading of the spec is it can't be zero?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could still do with a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

continue;

std::string inner_class_info_name =
class_infot(pool_entry(inner_class_info_index))
.get_name(pool_entry_lambda);
bool is_private = inner_class_access_flags & ACC_PRIVATE;
bool is_public = inner_class_access_flags & ACC_PUBLIC;
bool is_protected = inner_class_access_flags & ACC_PROTECTED;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: These three lines give a clang-tidy warning in clion, due to the implicit cast from integer to bool. This could be fixed as follows - bool is_protected = (inner_class_access_flags & ACC_PROTECTED) != 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Or a read_bitflag method but I wouldn't block this PR on it since this is a common approach used in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through the rest of this file, all the other instances of this have already been fixed to avoid the implicit cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link
Contributor

Choose a reason for hiding this comment

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

I considered it to be an optional "NIT" comment. I guess it would have been helpful if @thk123 had commented with something along the lines of "This is not a NIT, fix it."


// If the original parsed class name matches the inner class name,
// the parsed class is an inner class, so overwrite the parsed class'
// access information and mark it as an inner class
parsed_class.is_inner_class =
remove_separator_char(id2string(parsed_class.name), '.') ==
remove_separator_char(inner_class_info_name, '/');
if(!parsed_class.is_inner_class)
continue;
// Note that if outer_class_info_index == 0, the inner class is an anonymous
// or local class, and is treated as private.
if(outer_class_info_index == 0)
{
// This is a marker for an anonymous or local class
// which are treated as private
parsed_class.is_private = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if spotted this before, but having three booleans to represent a single value is error prone, I'd prefer an enum in the classt

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a common approach in this file and it effectively matches the java bytecode.

parsed_class.is_protected = false;
parsed_class.is_public = false;
}
else
{
parsed_class.is_private = is_private;
parsed_class.is_protected = is_protected;
parsed_class.is_public = is_public;
}
}
}

void java_bytecode_parsert::rclass_attribute(classt &parsed_class)
{
u2 attribute_name_index=read_u2();
Expand Down Expand Up @@ -1640,6 +1717,11 @@ void java_bytecode_parsert::rclass_attribute(classt &parsed_class)
parsed_class.attribute_bootstrapmethods_read = true;
read_bootstrapmethods_entry(parsed_class);
}
else if(attribute_name == "InnerClasses")
{
Copy link
Contributor

@thomasspriggs thomasspriggs Jun 29, 2018

Choose a reason for hiding this comment

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

This section should be moved into a new function/method, to avoid rclass_attribute getting too long.

java_bytecode_parsert::rinner_classes_attribute(
parsed_class, attribute_length);
}
else
skip_bytes(attribute_length);
}
Expand Down
10 changes: 10 additions & 0 deletions jbmc/src/java_bytecode/java_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,16 @@ class java_class_typet:public class_typet
return set(ID_access, access);
}

const bool get_is_inner_class() const
{
return get_bool(ID_is_inner_class);
}

void set_is_inner_class(const bool &is_inner_class)
{
return set(ID_is_inner_class, is_inner_class);
}

bool get_final()
{
return get_bool(ID_final);
Expand Down
1 change: 1 addition & 0 deletions jbmc/unit/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ SRC += java_bytecode/goto-programs/class_hierarchy_output.cpp \
java_bytecode/java_bytecode_convert_class/convert_java_annotations.cpp \
java_bytecode/java_bytecode_convert_method/convert_invoke_dynamic.cpp \
java_bytecode/java_bytecode_parse_generics/parse_generic_class.cpp \
java_bytecode/java_bytecode_parser/parse_java_attributes.cpp \
java_bytecode/java_object_factory/gen_nondet_string_init.cpp \
java_bytecode/java_bytecode_parse_lambdas/java_bytecode_parse_lambda_method_table.cpp \
java_bytecode/java_bytecode_parse_lambdas/java_bytecode_convert_class_lambda_method_handles.cpp \
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
115 changes: 115 additions & 0 deletions jbmc/unit/java_bytecode/java_bytecode_parser/InnerClasses.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
public class InnerClasses {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a non-public class to this file (in case that means there are two inner class attributes?) Unless that isn't true, in which case might not be worth it

Copy link
Contributor Author

@jeannielynnmoulton jeannielynnmoulton Jul 2, 2018

Choose a reason for hiding this comment

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

Added a non-public outer class 👍

public class PublicInnerClass {
public int i;
public PublicInnerClass(int i) {
this.i = i;
}
}
class DefaultInnerClass {
int i;
DefaultInnerClass(int i) {
this.i = i;
}
}
protected class ProtectedInnerClass {
protected int i;
protected ProtectedInnerClass(int i) {
this.i = i;
}
}
private class PrivateInnerClass {
private int i;
private PrivateInnerClass(int i) {
this.i = i;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you extend this test with some Inner Inner class examples, to show that things still work when the classes are nested an additional layer deep?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also extend this with an anonymous class and a local class?

}

class InnerClassesDefault {
public class PublicInnerClass {
public int i;
public PublicInnerClass(int i) {
this.i = i;
}
}
class DefaultInnerClass {
int i;
DefaultInnerClass(int i) {
this.i = i;
}
}
protected class ProtectedInnerClass {
protected int i;
protected ProtectedInnerClass(int i) {
this.i = i;
}
}
private class PrivateInnerClass {
private int i;
private PrivateInnerClass(int i) {
this.i = i;
}
}
}

class InnerClassesDeeplyNested {
class SinglyNestedClass {
int i;
SinglyNestedClass(int i) {
this.i = i;
}
public class PublicDoublyNestedInnerClass {
public int i;
public PublicDoublyNestedInnerClass(int i) {
this.i = i;
}
}
class DefaultDoublyNestedInnerClass {
int i;
DefaultDoublyNestedInnerClass(int i) {
this.i = i;
}
}
protected class ProtectedDoublyNestedInnerClass {
protected int i;
protected ProtectedDoublyNestedInnerClass(int i) {
this.i = i;
}
}
private class PrivateDoublyNestedInnerClass {
private int i;
private PrivateDoublyNestedInnerClass(int i) {
this.i = i;
}
}
}
}

class ContainsAnonymousClass {
interface InnerInterface {
int i = 0;
}
public InnerInterface anonymousPublic = new InnerInterface() {
int i = 1;
};
InnerInterface anonymousDefault = new InnerInterface() {
int i = 2;
};
protected InnerInterface anonymousProtected = new InnerInterface() {
int i = 3;
};
private InnerInterface anonymousPrivate = new InnerInterface() {
int i = 4;
};
}

class ContainsLocalClass {
public void test() {
class LocalClass {
int i = 55;
LocalClass(int i) {
this.i = i;
}
}
}
}
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading