-
Notifications
You must be signed in to change notification settings - Fork 273
[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
Changes from 6 commits
457bac9
c336455
b28562b
1930aef
c0c1029
6ce7b13
9ba55e2
5350133
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 &); | ||
|
@@ -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) | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -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") | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section should be moved into a new function/method, to avoid |
||
java_bytecode_parsert::rinner_classes_attribute( | ||
parsed_class, attribute_length); | ||
} | ||
else | ||
skip_bytes(attribute_length); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
public class InnerClasses { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed