On Fri, 8 Apr 2022 08:13:33 GMT, Thomas Schatzl <tscha...@openjdk.org> wrote:
> Hi all, > > can I have reviews for this change that adds dedicated filler objects to > the VM? > > Currently, when formatting areas of dead objects all gcs use instances of > j.l.Object and int-arrays. > > This has the drawback of not being easily able to discern whether a given > object is actually dead (and should never be referenced) or just a regular > j.l.Object/int array. > > This also makes enhanced error detection (any reference to such an object is > an error - i.e. detecting references to such objects) and to skip potentially > already unloaded classes when scanning areas of the heap below TAMS, G1 uses > its prev bitmap. > Other collectors do not have this extra information at the moment, so they > can't (and don't) do this kind of verification. > > With [JDK-8210708](https://bugs.openjdk.java.net/browse/JDK-8210708) the prev > bitmap will effectively be removed in G1; G1 will format the dead areas with > these filler objects to avoid coming across unloaded classes. This is fine > wrt to normal operation, however, this looses the existing enhanced > verification mentioned above. > > This change proposes to add dedicated VM-internal filler objects, i.e. > equivalents of j.l.Object and int-arrays. > > This has the following benefits: > > - keep this error detection (actually making it much simpler) and allowing > similar verification for other collectors. (This change does not add this) > > - this also makes it "easy" to detect references to filler objects in > debugging tools - you only need to know the two klasses (or just get their > friendly name) to see whether that reference may actually be valid (or refers > to the inside such an object). References to these classes in the crash file > may also allow the issue to be more clear. > > This causes some minor changes to external behavior: > > - logs/heap dumps now contain instances of these objects - which seems fine > as previously they have just been reported as part of j.l.Object/int-arrays > statistics. The VM spec also does not guarantee whether a particular kind of > object should/should not show there anyway afaik. > > - if the application ever gets to instantiate a reference to such an object > somehow, any enabled verification will crash the VM. That's bad luck for > messing with internal classes, but that's the purpose of these objects. > > The change takes care that getting a reference will not be possible by normal > means (i.e. via Class.forName() etc) - which should be sufficient to avoid > the issue. Actually, existing mechanisms seem to be sufficient. > > > Testing: tier1-8 > > There is one question I would like the reviewers to specially think about, > the name of the filler array klass. I just used > `Ljava/internal/vm/FillerArray;` for that, looking at other internal > symbols/klasses, but I'm not sure this adheres to naming guidelines. > > Thanks go to @iklam for helping out with the change. > > Thanks, > Thomas Changes requested by iklam (Reviewer). src/hotspot/share/classfile/systemDictionaryShared.cpp line 1727: > 1725: ArchivedMirrorPatcher::update_array_klasses(k); > 1726: } > 1727: > ArchivedMirrorPatcher::update_array_klasses(Universe::fillerArrayKlassObj()); I think this is not necessary. `Universe::fillerArrayKlassObj()` shares the same mirror as `Universe::intArrayKlassObj()`, which has already been updated in the loop above. `ArchivedMirrorPatcher::update_array_klasses(k)` will essentially do `k->mirror->pointer_back_to_klass += delta`, so it will incorrectly set the pointer when delta is not zero. I would suggest running with -XX:ArchiveRelocationMode=1 -Xlog:cds -Xlog:class+load=debug and step into the following code: void java_lang_Class::update_archived_mirror_native_pointers(oop archived_mirror) { assert(MetaspaceShared::relocation_delta() != 0, "must be"); Klass* k = ((Klass*)archived_mirror->metadata_field(_klass_offset)); archived_mirror->metadata_field_put(_klass_offset, (Klass*)(address(k) + MetaspaceShared::relocation_delta())); <<<< HERE src/hotspot/share/memory/universe.cpp line 205: > 203: } > 204: // Hide _fillerArrayKlassObj from JVMTI > 205: // closure->do_klass(_fillerArrayKlassObj); Maybe the comment should be more explicit? // We don't do the following because it will confuse JVMTI. // _fillerArrayKlassObj is used only by GC, which doesn't need to see // this klass from basic_type_classes_do(). ------------- PR: https://git.openjdk.java.net/jdk/pull/8156
