Skip to content

8366498: Simplify ClassFileParser::parse_super_class #27026

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.

Sign up for GitHub

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 5 commits into from
Closed

8366498: Simplify ClassFileParser::parse_super_class #27026

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d353bfe
8366498: Wrong InstanceKlass::cast() in ClassFileParser::parse_super_…
iklam Sep 1, 2025
dec4af7
Simplify parse_super_klass() -> check_super_class()
iklam Sep 1, 2025
256716f
removed test-only assert
iklam Sep 1, 2025
8390278
@coleenp review - updated asserts
iklam Sep 4, 2025
4422804
Merge branch 'master' into 8366498-wrong-instance-klass-cast-in-parse…
iklam Sep 4, 2025
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
46 changes: 22 additions & 24 deletions src/hotspot/share/classfile/classFileParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3803,39 +3803,33 @@ AnnotationArray* ClassFileParser::allocate_annotations(const u1* const anno,
return annotations;
}

const InstanceKlass* ClassFileParser::parse_super_class(ConstantPool* const cp,
const int super_class_index,
const bool need_verify,
TRAPS) {
void ClassFileParser::check_super_class(ConstantPool* const cp,
const int super_class_index,
const bool need_verify,
TRAPS) {
assert(cp != nullptr, "invariant");
const InstanceKlass* super_klass = nullptr;

if (super_class_index == 0) {
guarantee_property(_class_name == vmSymbols::java_lang_Object(),
"Invalid superclass index %u in class file %s",
super_class_index,
CHECK_NULL);
CHECK);
} else {
guarantee_property(valid_klass_reference_at(super_class_index),
"Invalid superclass index %u in class file %s",
super_class_index,
CHECK_NULL);
CHECK);

// Ioi note: remove this in final commit.
assert(!cp->tag_at(super_class_index).is_klass(), "should not have been resolved");

// The class name should be legal because it is checked when parsing constant pool.
// However, make sure it is not an array type.
bool is_array = false;
if (cp->tag_at(super_class_index).is_klass()) {
super_klass = InstanceKlass::cast(cp->resolved_klass_at(super_class_index));
if (need_verify)
is_array = super_klass->is_array_klass();
} else if (need_verify) {
is_array = (cp->klass_name_at(super_class_index)->char_at(0) == JVM_SIGNATURE_ARRAY);
}
if (need_verify) {
guarantee_property(!is_array,
"Bad superclass name in class file %s", CHECK_NULL);
guarantee_property(cp->klass_name_at(super_class_index)->char_at(0) != JVM_SIGNATURE_ARRAY,
"Bad superclass name in class file %s", CHECK);
}
}
return super_klass;
}

OopMapBlocksBuilder::OopMapBlocksBuilder(unsigned int max_blocks) {
Expand Down Expand Up @@ -5667,10 +5661,10 @@ void ClassFileParser::parse_stream(const ClassFileStream* const stream,

// SUPERKLASS
_super_class_index = stream->get_u2_fast();
_super_klass = parse_super_class(cp,
_super_class_index,
_need_verify,
CHECK);
check_super_class(cp,
_super_class_index,
_need_verify,
CHECK);

// Interfaces
_itfs_len = stream->get_u2_fast();
Expand Down Expand Up @@ -5777,8 +5771,8 @@ void ClassFileParser::post_process_parsed_stream(const ClassFileStream* const st
"java.lang.Object cannot implement an interface in class file %s",
CHECK);
}
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 somewhat strange logic. Seems like there should be an assert that _super_klass == nullptr then an 'else' at 5779 and an assert that _super_class_index != 0. The trailing else is confusing and it checks something here at 5775.

// We check super class after class file is parsed and format is checked
if (_super_class_index > 0 && nullptr == _super_klass) {
// Set _super_klass after class file is parsed and format is checked
if (_super_class_index > 0) {
Symbol* const super_class_name = cp->klass_name_at(_super_class_index);
if (_access_flags.is_interface()) {
// Before attempting to resolve the superclass, check for class format
Expand All @@ -5789,6 +5783,7 @@ void ClassFileParser::post_process_parsed_stream(const ClassFileStream* const st
}
Handle loader(THREAD, _loader_data->class_loader());
if (loader.is_null() && super_class_name == vmSymbols::java_lang_Object()) {
// fast path to avoid lookup
_super_klass = vmClasses::Object_klass();
} else {
_super_klass = (const InstanceKlass*)
Expand All @@ -5798,6 +5793,9 @@ void ClassFileParser::post_process_parsed_stream(const ClassFileStream* const st
true,
CHECK);
}
} else {
assert(_class_name == vmSymbols::java_lang_Object(), "already checked");
_super_klass = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessary. We have already check the name and _super_klass is initialized to null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it help people who wonder why _super_klass is not updated in this case. Maybe I can change the assignment to

asssert(_super_klass == nullptr, "already initialized");

Copy link
Member

Choose a reason for hiding this comment

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

I would just have:

else {
  // The class is Object - so no superclass
}

Copy link
Member Author

Choose a reason for hiding this comment

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

My asserts say the same thing, except they comes with proofs that they are not lying.

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 like the 'else' either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the trailing else and shuffled the asserts around as @coleenp suggested.

}

if (_super_klass != nullptr) {
Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/classfile/classFileParser.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,10 @@ class ClassFileParser {
bool* has_nonstatic_concrete_methods,
TRAPS);

const InstanceKlass* parse_super_class(ConstantPool* const cp,
const int super_class_index,
const bool need_verify,
TRAPS);
void check_super_class(ConstantPool* const cp,
const int super_class_index,
const bool need_verify,
TRAPS);

// Field parsing
void parse_field_attributes(const ClassFileStream* const cfs,
Expand Down