Skip to content

Commit 94b473e

Browse files
author
Thomas Schatzl
committed
8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark
Reviewed-by: stefank, coleenp
1 parent 900d967 commit 94b473e

File tree

6 files changed

+135
-17
lines changed

6 files changed

+135
-17
lines changed

src/hotspot/share/classfile/classLoaderData.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,8 @@ class ClassLoaderData : public CHeapObj<mtClass> {
9898
};
9999

100100
friend class ClassLoaderDataGraph;
101-
friend class ClassLoaderDataGraphIterator;
101+
template <bool keep_alive>
102+
friend class ClassLoaderDataGraphIteratorBase;
102103
friend class ClassLoaderDataGraphKlassIteratorAtomic;
103104
friend class ClassLoaderDataGraphKlassIteratorStatic;
104105
friend class ClassLoaderDataGraphMetaspaceIterator;
@@ -253,6 +254,7 @@ class ClassLoaderData : public CHeapObj<mtClass> {
253254
ClassLoaderMetaspace* metaspace_non_null();
254255

255256
inline oop class_loader() const;
257+
inline oop class_loader_no_keepalive() const;
256258

257259
// Returns true if this class loader data is for a loader going away.
258260
// Note that this is only safe after the GC has computed if the CLD is

src/hotspot/share/classfile/classLoaderData.inline.hpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ inline oop ClassLoaderData::class_loader() const {
3939
return _class_loader.resolve();
4040
}
4141

42+
inline oop ClassLoaderData::class_loader_no_keepalive() const {
43+
assert(!_unloading, "This oop is not available to unloading class loader data");
44+
assert(_holder.is_null() || holder_no_keepalive() != NULL , "This class loader data holder must be alive");
45+
return _class_loader.peek();
46+
}
47+
4248
inline bool ClassLoaderData::is_boot_class_loader_data() const {
4349
return this == _the_null_class_loader_data || class_loader() == NULL;
4450
}

src/hotspot/share/classfile/classLoaderDataGraph.cpp

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -314,18 +314,21 @@ LockedClassesDo::~LockedClassesDo() {
314314

315315
// Iterating over the CLDG needs to be locked because
316316
// unloading can remove entries concurrently soon.
317-
class ClassLoaderDataGraphIterator : public StackObj {
317+
template <bool keep_alive = true>
318+
class ClassLoaderDataGraphIteratorBase : public StackObj {
318319
ClassLoaderData* _next;
319320
Thread* _thread;
320321
HandleMark _hm; // clean up handles when this is done.
321-
Handle _holder;
322322
NoSafepointVerifier _nsv; // No safepoints allowed in this scope
323323
// unless verifying at a safepoint.
324324

325325
public:
326-
ClassLoaderDataGraphIterator() : _next(ClassLoaderDataGraph::_head), _thread(Thread::current()), _hm(_thread) {
327-
_thread = Thread::current();
328-
assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
326+
ClassLoaderDataGraphIteratorBase() : _next(ClassLoaderDataGraph::_head), _thread(Thread::current()), _hm(_thread) {
327+
if (keep_alive) {
328+
assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
329+
} else {
330+
assert_at_safepoint();
331+
}
329332
}
330333

331334
ClassLoaderData* get_next() {
@@ -335,8 +338,10 @@ class ClassLoaderDataGraphIterator : public StackObj {
335338
cld = cld->next();
336339
}
337340
if (cld != NULL) {
338-
// Keep cld that is being returned alive.
339-
_holder = Handle(_thread, cld->holder());
341+
if (keep_alive) {
342+
// Keep cld that is being returned alive.
343+
Handle(_thread, cld->holder());
344+
}
340345
_next = cld->next();
341346
} else {
342347
_next = NULL;
@@ -345,6 +350,9 @@ class ClassLoaderDataGraphIterator : public StackObj {
345350
}
346351
};
347352

353+
using ClassLoaderDataGraphIterator = ClassLoaderDataGraphIteratorBase<true /* keep_alive */>;
354+
using ClassLoaderDataGraphIteratorNoKeepAlive = ClassLoaderDataGraphIteratorBase<false /* keep_alive */>;
355+
348356
void ClassLoaderDataGraph::loaded_cld_do(CLDClosure* cl) {
349357
ClassLoaderDataGraphIterator iter;
350358
while (ClassLoaderData* cld = iter.get_next()) {
@@ -422,16 +430,19 @@ void ClassLoaderDataGraph::classes_unloading_do(void f(Klass* const)) {
422430
}
423431
}
424432

425-
#define FOR_ALL_DICTIONARY(X) ClassLoaderDataGraphIterator iter; \
426-
while (ClassLoaderData* X = iter.get_next()) \
427-
if (X->dictionary() != NULL)
428-
429433
void ClassLoaderDataGraph::verify_dictionary() {
430-
FOR_ALL_DICTIONARY(cld) {
431-
cld->dictionary()->verify();
434+
ClassLoaderDataGraphIteratorNoKeepAlive iter;
435+
while (ClassLoaderData* cld = iter.get_next()) {
436+
if (cld->dictionary() != nullptr) {
437+
cld->dictionary()->verify();
438+
}
432439
}
433440
}
434441

442+
#define FOR_ALL_DICTIONARY(X) ClassLoaderDataGraphIterator iter; \
443+
while (ClassLoaderData* X = iter.get_next()) \
444+
if (X->dictionary() != NULL)
445+
435446
void ClassLoaderDataGraph::print_dictionary(outputStream* st) {
436447
FOR_ALL_DICTIONARY(cld) {
437448
st->print("Dictionary for ");
@@ -648,7 +659,7 @@ Klass* ClassLoaderDataGraphKlassIteratorAtomic::next_klass() {
648659
}
649660

650661
void ClassLoaderDataGraph::verify() {
651-
ClassLoaderDataGraphIterator iter;
662+
ClassLoaderDataGraphIteratorNoKeepAlive iter;
652663
while (ClassLoaderData* cld = iter.get_next()) {
653664
cld->verify();
654665
}

src/hotspot/share/classfile/classLoaderDataGraph.hpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ class ClassLoaderDataGraph : public AllStatic {
3737
friend class ClassLoaderDataGraphMetaspaceIterator;
3838
friend class ClassLoaderDataGraphKlassIteratorAtomic;
3939
friend class ClassLoaderDataGraphKlassIteratorStatic;
40-
friend class ClassLoaderDataGraphIterator;
40+
template <bool keep_alive>
41+
friend class ClassLoaderDataGraphIteratorBase;
4142
friend class VMStructs;
4243
private:
4344
// All CLDs (except the null CLD) can be reached by walking _head->_next->...

src/hotspot/share/classfile/dictionary.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -633,7 +633,7 @@ void Dictionary::verify() {
633633
// class loader must be present; a null class loader is the
634634
// bootstrap loader
635635
guarantee(cld != NULL &&
636-
(cld->the_null_class_loader_data() || cld->class_loader()->is_instance()),
636+
(cld->is_the_null_class_loader_data() || cld->class_loader_no_keepalive()->is_instance()),
637637
"checking type of class_loader");
638638

639639
ResourceMark rm;
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/*
2+
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test Class unloading test with concurrent mark
26+
* @summary Make sure that verification during gc does not prevent class unloading
27+
* @bug 8280454
28+
* @requires vm.gc.G1
29+
* @requires vm.opt.final.ClassUnloading
30+
* @requires vm.opt.final.ClassUnloadingWithConcurrentMark
31+
* @modules java.base/jdk.internal.misc
32+
* @library /test/lib
33+
* @library classes
34+
* @build sun.hotspot.WhiteBox test.Empty
35+
* @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
36+
* @run main/othervm -Xbootclasspath/a:. -Xmn8m -XX:+UseG1GC -XX:+VerifyDuringGC -XX:+AlwaysTenure -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xlog:gc,class+unload=debug UnloadTestWithVerifyDuringGC
37+
*/
38+
import sun.hotspot.WhiteBox;
39+
import jdk.test.lib.classloader.ClassUnloadCommon;
40+
41+
/**
42+
* Test that verifies that classes are unloaded using concurrent mark with G1 when they are no
43+
* longer reachable even when -XX:+VerifyDuringGC is enabled
44+
*
45+
* The test creates a class loader, uses the loader to load a class and creates an instance
46+
* of that class. The it nulls out all the references to the instance, class and class loader
47+
* and tries to trigger class unloading using a concurrent mark. Then it verifies that the class
48+
* is no longer loaded by the VM.
49+
*/
50+
public class UnloadTestWithVerifyDuringGC {
51+
private static final WhiteBox wb = WhiteBox.getWhiteBox();
52+
53+
private static void waitUntilConcMarkFinished() throws Exception {
54+
while (wb.g1InConcurrentMark()) {
55+
try {
56+
Thread.sleep(1);
57+
} catch (InterruptedException e) {
58+
System.out.println("Got InterruptedException while waiting for concurrent mark to finish");
59+
throw e;
60+
}
61+
}
62+
}
63+
64+
private static void triggerUnloadingWithConcurrentMark() throws Exception {
65+
// Try to unload classes using concurrent mark. First wait for any currently running concurrent
66+
// cycle.
67+
waitUntilConcMarkFinished();
68+
wb.g1StartConcMarkCycle();
69+
waitUntilConcMarkFinished();
70+
}
71+
72+
private static String className = "test.Empty";
73+
74+
public static void main(String... args) throws Exception {
75+
ClassUnloadCommon.failIf(wb.isClassAlive(className), "is not expected to be alive yet");
76+
77+
ClassLoader cl = ClassUnloadCommon.newClassLoader();
78+
Class<?> c = cl.loadClass(className);
79+
Object o = c.newInstance();
80+
81+
ClassUnloadCommon.failIf(!wb.isClassAlive(className), "should be live here");
82+
83+
String loaderName = cl.getName();
84+
int loadedRefcount = wb.getSymbolRefcount(loaderName);
85+
System.out.println("Refcount of symbol " + loaderName + " is " + loadedRefcount);
86+
87+
// Move everything into the old gen so that concurrent mark can unload.
88+
wb.youngGC();
89+
cl = null; c = null; o = null;
90+
triggerUnloadingWithConcurrentMark();
91+
92+
ClassUnloadCommon.failIf(wb.isClassAlive(className), "should have been unloaded");
93+
94+
int unloadedRefcount = wb.getSymbolRefcount(loaderName);
95+
System.out.println("Refcount of symbol " + loaderName + " is " + unloadedRefcount);
96+
ClassUnloadCommon.failIf(unloadedRefcount != (loadedRefcount - 1), "Refcount must be decremented");
97+
}
98+
}

0 commit comments

Comments
 (0)