On Sat, 11 Dec 2021 01:55:50 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> **Background:**
>>
>> In the Java Language, Enums can be tested for equality, so the constants in
>> an Enum type must be unique. Javac compiles an enum declaration like this:
>>
>>
>> public enum Day { SUNDAY, MONDAY ... }
>>
>>
>> to
>>
>>
>> public class Day extends java.lang.Enum {
>> public static final SUNDAY = new Day("SUNDAY");
>> public static final MONDAY = new Day("MONDAY"); ...
>> }
>>
>>
>> With CDS archived heap objects, `Day::<clinit>` is executed twice: once
>> during `java -Xshare:dump`, and once during normal JVM execution. If the
>> archived heap objects references one of the Enum constants created at dump
>> time, we will violate the uniqueness requirements of the Enum constants at
>> runtime. See the test case in the description of
>> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731)
>>
>> **Fix:**
>>
>> During -Xshare:dump, if we discovered that an Enum constant of type X is
>> archived, we archive all constants of type X. At Runtime, type X will skip
>> the normal execution of `X::<clinit>`. Instead, we run
>> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X
>> that were saved at dump time.
>>
>> This is safe as we know that `X::<clinit>` has no observable side effect --
>> it only creates the constants of type X, as well as the synthetic value
>> `X::$VALUES`, which cannot be observed until X is fully initialized.
>>
>> **Verification:**
>>
>> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for
>> similar problems where the archived heap objects reference a static field
>> that may be recreated at runtime. There are some manual steps involved, but
>> I analyzed the potential problems found by the tool are they are all safe
>> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details.
>> An example trace of this tool can be found at
>> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt
>>
>> **Testing:**
>>
>> Passed Oracle CI tiers 1-4. WIll run tier 5 as well.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or
> a rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains four additional commits since the
> last revision:
>
> - Merge branch 'master' into 8275731-heapshared-enum
> - added exclusions needed by "java -Xshare:dump -ea -esa"
> - Comments from @calvinccheung off-line
> - 8275731: CDS archived enums objects are recreated at runtime
I don't really know this code well enough to do a good code review. I had some
comments though.
src/hotspot/share/cds/cdsHeapVerifier.cpp line 165:
> 163:
> 164: ResourceMark rm;
> 165: for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {
Can this call instead
void InstanceKlass::do_local_static_fields(void f(fieldDescriptor*, Handle,
TRAPS), Handle mirror, TRAPS) {
and have this next few lines in the function?
src/hotspot/share/cds/cdsHeapVerifier.cpp line 254:
> 252: InstanceKlass* ik = InstanceKlass::cast(k);
> 253: for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {
> 254: if (!fs.access_flags().is_static()) {
same here. It only saves a couple of lines but then you can have the function
outside this large function.
src/hotspot/share/cds/cdsHeapVerifier.hpp line 52:
> 50: mtClassShared,
> 51: HeapShared::oop_hash> _table;
> 52:
Is this only used inside cdsHeapVerifier? if so it should be in the .cpp file.
There's also an ArchiveableStaticFieldInfo. Not sure how they are related.
src/hotspot/share/cds/heapShared.cpp line 433:
> 431: oop mirror = k->java_mirror();
> 432: int i = 0;
> 433: for (JavaFieldStream fs(k); !fs.done(); fs.next()) {
This seems like it should also use InstanceKlass::do_local_static_fields.
src/hotspot/share/cds/heapShared.cpp line 482:
> 480: copy_open_objects(open_regions);
> 481:
> 482: CDSHeapVerifier::verify();
Should all this be DEBUG_ONLY ?
src/hotspot/share/cds/heapShared.hpp line 236:
> 234: oop _referrer;
> 235: oop _obj;
> 236: CachedOopInfo() :_subgraph_info(), _referrer(), _obj() {}
Should these be initialized to nullptr? does this do this?
-------------
PR: https://git.openjdk.java.net/jdk/pull/6653
