Check the InnerClass attribute of the enclosing class

[ChengJin01]

The change is to check to see whether an valid entry for
the specified class exists in the InnerClass attribute
of its enclosing class given the InnerClass attribute
of both the inner class and its enclosing class must
remain consistent with each other.

Fixes: #12992

Signed-off-by: Cheng Jin jincheng@ca.ibm.com

[ChengJin01]

The changes were verified in internal builds & open sourced builds

Reviewer: @pshipton
FYI: @gacholio, @tajila

[pshipton]

Adding new fields to ROMClass is the only way to make this work? Can you pls describe what other solutions were tried that didn't add new fields, and why they didn't work.

If we do go with this, it's going to break shared cache compatibility so the shared cache generation number needs to be updated as part of this change.

[ChengJin01]

Adding new fields to ROMClass is the only way to make this work? Can you pls describe what other solutions were tried that didn't add new fields, and why they didn't work.

1. The first option I used to try without modifying ROMClass (e.g. at ChengJin01@26b6cd0) was to simply add the remaining inner class entries of the class file to the existing innerClasses of J9ROMClass, in which case
   [1] all native code with declared class at /runtime/jcl/common/java_lang_Class.cpp must be updated to exclude these newly added items in innerClasses (which is infeasible in that a class with a NULL outer class name doesn't mean it is an inner class of a specified enclosing class). In addition, any code specific to declared classes must be updated accordingly.
   [2] the outer class info in the entries are removed during the class transformation in ClassFileWriter::writeAttributes() at /runtime/bcutil/ClassFileWriter.cpp, which inevitably causes problem (no way to differentiate them from other entries) in building ROM class from the transformed class.

2. Another option I tried at ChengJin01@8817c04 was to count the entries separately (still need to add a new field skippedInnerClassCount) but still ended up with pretty much the same issues (and even more failures) as the first option above.

The fundamental reason for these options is that it is difficult to correctly identify these remaining inner class entries in building ROMClass in any case (e.g. after the class transformation)

So the only safe way to avoid changing the existing code to handle these tricky situations is adding the remaining entries separately to a new field in ROMClass, in which case it won't affect any existing implementation even though there is any update related to the inner class entries in the future.

the shared cache generation number needs to be updated as part of this change.

I just updated the number at runtime/shared_common/OSCache.hpp in our case.

[hangshao0]

If the cache generation is updated, it needs to be documented in release notes.

[DanHeidinga]

@ChengJin01 with this change, are we still removing parts of the InnerClass attribute when writing the romClass? My reading of the code is we've just spread them into a different format - innerClasses & enclosedInnerClasses. Is that correct?

An easy check on this would be if the innerClassCount + enclosedInnerClassCount == InnerClasses_attribute.number_of_classes

[ChengJin01]

Hi @DanHeidinga,

are we still removing parts of the InnerClass attribute when writing the romClass?

Yes. There is no big difference in dealing with enclosedInnerClasses as compared to the existing code in handling innerClasses given the only thing we care is the inner class itself.

My reading of the code is we've just spread them into a different format - innerClasses & enclosedInnerClasses. Is that correct?

Correct. We avoid changing the existing implementation on declared classes by splitting InnerClass into two places, which is easy for us to check whether the requested class is the inner class of the enclosing class as long as it exists in enclosedInnerClasses.

An easy check on this would be if the innerClassCount + enclosedInnerClassCount == InnerClasses_attribute.number_of_classes

Strictly speaking, all the remaining entries (except the entries that the current class itself is the inner class which are already excluded in the existing code) of the InnerClass attribute are added to enclosedInnerClasses, which guarantees that the check on the relationship between the inner classes and its enclosing class covers any case for inner class. Thus, enclosedInnerClassCount is undoubtedly the result of (InnerClasses_attribute.number_of_classes - innerClassCount) for the enclosing class.

[gacholio]

I am going on vacation for a week, so I will not get to this (especially as the approach is still under active discussion).

[ChengJin01]

Hi @DanHeidinga and @pshipton, any other concern on this PR?

[pshipton]

jenkins test sanity win jdk8

[pshipton]

@DanHeidinga @gacholio @tajila any concerns before this is merged?

[gacholio]

Some minor formatting complaints.

[gacholio]

jenkins test sanity win jdk8

[hangshao0]

I see the cache generation is incremented by this PR. Just a reminder that this should be documented in the release notes. (Doc change last time on this was eclipse-openj9/openj9-docs#347)