Skip to content
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.

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

8276126: Dump time class transformation causes heap objects of non-boot classes to be archived #6484

Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -71,6 +71,7 @@
bool HeapShared::_closed_regions_mapped = false;
bool HeapShared::_open_regions_mapped = false;
bool HeapShared::_is_loaded = false;
bool HeapShared::_disable_writing = false;
address HeapShared::_narrow_oop_base;
int HeapShared::_narrow_oop_shift;
DumpedInternedStrings *HeapShared::_dumped_interned_strings = NULL;
@@ -155,10 +155,18 @@ class HeapShared: AllStatic {

// Can this VM write heap regions into the CDS archive? Currently only G1+compressed{oops,cp}
static bool can_write() {
CDS_JAVA_HEAP_ONLY(return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);)
CDS_JAVA_HEAP_ONLY(
if (_disable_writing) {
return false;
}
return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);
)
NOT_CDS_JAVA_HEAP(return false;)
}

static void disable_writing() {
CDS_JAVA_HEAP_ONLY(_disable_writing = true;)
}
// Can this VM map archived heap regions? Currently only G1+compressed{oops,cp}
static bool can_map() {
CDS_JAVA_HEAP_ONLY(return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);)
@@ -188,6 +196,7 @@ class HeapShared: AllStatic {

private:
#if INCLUDE_CDS_JAVA_HEAP
static bool _disable_writing;
static bool _closed_regions_mapped;
static bool _open_regions_mapped;
static bool _is_loaded;
@@ -1253,7 +1253,8 @@ char* ClassLoader::skip_uri_protocol(char* source) {

// Record the shared classpath index and loader type for classes loaded
// by the builtin loaders at dump time.
void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik, const ClassFileStream* stream) {
void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik,
const ClassFileStream* stream, bool redefined) {
Arguments::assert_is_dumping_archive();
assert(stream != NULL, "sanity");

@@ -1337,10 +1338,10 @@ void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik, const Cl
}
}

// No path entry found for this class. Must be a shared class loaded by the
// No path entry found for this class: most likely a shared class loaded by the
// user defined classloader.
if (classpath_index < 0) {
assert(ik->shared_classpath_index() < 0, "Sanity");
if (classpath_index < 0 && !SystemDictionaryShared::is_builtin_loader(ik->class_loader_data())) {
assert(ik->shared_classpath_index() < 0, "not assigned yet");
ik->set_shared_classpath_index(UNREGISTERED_INDEX);
SystemDictionaryShared::set_shared_class_misc_info(ik, (ClassFileStream*)stream);
return;
@@ -1359,7 +1360,7 @@ void ClassLoader::record_result(JavaThread* current, InstanceKlass* ik, const Cl
ik->name()->utf8_length());
assert(file_name != NULL, "invariant");

ClassLoaderExt::record_result(classpath_index, ik);
ClassLoaderExt::record_result(classpath_index, ik, stream, redefined);
}
#endif // INCLUDE_CDS

@@ -365,7 +365,8 @@ class ClassLoader: AllStatic {
static int num_module_path_entries();
static void exit_with_path_failure(const char* error, const char* message);
static char* skip_uri_protocol(char* source);
static void record_result(JavaThread* current, InstanceKlass* ik, const ClassFileStream* stream);
static void record_result(JavaThread* current, InstanceKlass* ik,
const ClassFileStream* stream, bool redefined);
#endif

static char* lookup_vm_options();
@@ -24,7 +24,9 @@

#include "precompiled.hpp"
#include "cds/filemap.hpp"
#include "cds/heapShared.hpp"
#include "classfile/classFileParser.hpp"
#include "classfile/classFileStream.hpp"
#include "classfile/classLoader.inline.hpp"
#include "classfile/classLoaderExt.hpp"
#include "classfile/classLoaderData.inline.hpp"
@@ -227,7 +229,8 @@ void ClassLoaderExt::setup_search_paths(JavaThread* current) {
ClassLoaderExt::setup_app_search_path(current);
}

void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* result) {
void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* result,
const ClassFileStream* stream, bool redefined) {
Copy link
Member

@iklam iklam Nov 30, 2021

Choose a reason for hiding this comment

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

The stream argument is not used.

Copy link
Member Author

@calvinccheung calvinccheung Nov 30, 2021

Choose a reason for hiding this comment

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

Thanks for spotting this. I've fixed it.

Arguments::assert_is_dumping_archive();

// We need to remember where the class comes from during dumping.
@@ -245,4 +248,21 @@ void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* resu
}
result->set_shared_classpath_index(classpath_index);
result->set_shared_class_loader_type(classloader_type);
#if INCLUDE_CDS_JAVA_HEAP
if (DumpSharedSpaces && AllowArchivingWithJavaAgent && classloader_type == ClassLoader::BOOT_LOADER &&
Copy link
Contributor

@yminqi yminqi Dec 1, 2021

Choose a reason for hiding this comment

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

the function is call during dump time (the assert at first line of this function check that) so DumpSharedSpace can be removed.
I am a little confused by this part: If 'redefined' is 'true', The 'redefined' only should be enough for set disable_writing() whether other conditions stands or not?

Copy link
Member Author

@calvinccheung calvinccheung Dec 1, 2021

Choose a reason for hiding this comment

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

The Arguments::assert_is_dumping_archive() includes both static and dynamic dumping. The archiving of heap objects is performed only during static dumping. So the check for DumpShareSpaces is needed.

The redefined flag alone is not enough to disable writing heap objects to archive. If a class loaded by an AppClassLoader and is redefined, it should not disable archiving heap objects. We currently have some test cases under runtime/cds/appcds/javaldr to test that.

Copy link
Contributor

@yminqi yminqi Dec 1, 2021

Choose a reason for hiding this comment

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

Thanks for the explanation.

classpath_index < 0 && HeapShared::can_write() && redefined) {
// During static dump, classes for the built-in loaders are always loaded from
// known locations (jimage, classpath or modulepath), so classpath_index should
// always be >= 0.
// The only exception is when a java agent is used during dump time (for testing
// purposes only). If a class is transformed by the agent, the CodeSource of
// this class may point to an unknown location. This may break heap object archiving,
// which requires all the boot classes to be from known locations. This is an
// uncommon scenario (even in test cases). Let's simply disable heap object archiving.
ResourceMark rm;
log_warning(cds)("CDS heap objects cannot be written because class %s maybe modified by ClassFileLoadHook.",
result->external_name());
HeapShared::disable_writing();
}
#endif // INCLUDE_CDS_JAVA_HEAP
}
@@ -29,6 +29,7 @@
#include "classfile/moduleEntry.hpp"
#include "utilities/macros.hpp"

class ClassFileStream;
class ClassListParser;

class ClassLoaderExt: public ClassLoader { // AllStatic
@@ -111,7 +112,8 @@ class ClassLoaderExt: public ClassLoader { // AllStatic
return _has_non_jar_in_classpath;
}

static void record_result(const s2 classpath_index, InstanceKlass* result);
static void record_result(const s2 classpath_index, InstanceKlass* result,
const ClassFileStream* stream, bool redefined);
static void set_has_app_classes() {
_has_app_classes = true;
}
@@ -211,7 +211,7 @@ InstanceKlass* KlassFactory::create_from_stream(ClassFileStream* stream,

#if INCLUDE_CDS
if (Arguments::is_dumping_archive()) {
ClassLoader::record_result(THREAD, result, stream);
ClassLoader::record_result(THREAD, result, stream, old_stream != stream);
}
#endif // INCLUDE_CDS

@@ -354,6 +354,7 @@ hotspot_appcds_dynamic = \
-runtime/cds/appcds/LambdaEagerInit.java \
-runtime/cds/appcds/LambdaProxyClasslist.java \
-runtime/cds/appcds/LambdaVerificationFailedDuringDump.java \
-runtime/cds/appcds/LambdaWithJavaAgent.java \
-runtime/cds/appcds/LambdaWithOldClass.java \
-runtime/cds/appcds/LongClassListPath.java \
-runtime/cds/appcds/LotsOfClasses.java \
@@ -0,0 +1,93 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/

/*
* @test
* @bug 8276126
* @summary Test static dumping with java agent transforming a class loaded
* by the boot class loader.
* @requires vm.cds.write.archived.java.heap
* @requires vm.jvmti
* @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds/test-classes
* @compile test-classes/Hello.java
* @compile test-classes/TransformBootClass.java
* @run driver LambdaWithJavaAgent
*/

import jdk.test.lib.cds.CDSOptions;
import jdk.test.lib.cds.CDSTestUtils;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;
import jdk.test.lib.helpers.ClassFileInstaller;

public class LambdaWithJavaAgent {

public static String agentClasses[] = {
TransformBootClass.class.getName(),
};

public static void main(String[] args) throws Exception {
String mainClass = Hello.class.getName();
String namePrefix = "lambda-with-java-agent";
JarBuilder.build(namePrefix, mainClass);

String appJar = TestCommon.getTestJar(namePrefix + ".jar");
String classList = namePrefix + ".list";
String archiveName = namePrefix + ".jsa";

String agentJar =
ClassFileInstaller.writeJar("TransformBootClass.jar",
ClassFileInstaller.Manifest.fromSourceFile("test-classes/TransformBootClass.mf"),
agentClasses);
String useJavaAgent = "-javaagent:" + agentJar + "=jdk/internal/math/FDBigInteger";

// dump class list
CDSTestUtils.dumpClassList(classList, "-cp", appJar, mainClass);

// create archive with the class list
CDSOptions opts = (new CDSOptions())
.addPrefix("-XX:ExtraSharedClassListFile=" + classList,
"-cp", appJar,
"-XX:+AllowArchivingWithJavaAgent",
useJavaAgent,
"-Xlog:class+load,cds+class=debug,cds")
.setArchiveName(archiveName);
OutputAnalyzer output = CDSTestUtils.createArchiveAndCheck(opts);
output.shouldContain("CDS heap objects cannot be written because class jdk.internal.math.FDBigInteger maybe modified by ClassFileLoadHook")
.shouldContain("Skipping jdk/internal/math/FDBigInteger: Unsupported location")
.shouldMatch(".class.load.*jdk.internal.math.FDBigInteger.*source.*modules");

// run with archive
CDSOptions runOpts = (new CDSOptions())
.addPrefix("-cp", appJar, "-Xlog:class+load,cds=debug",
"-XX:+AllowArchivingWithJavaAgent",
useJavaAgent)
.setArchiveName(archiveName)
.setUseVersion(false)
.addSuffix(mainClass);
output = CDSTestUtils.runWithArchive(runOpts);
TestCommon.checkExecReturn(output, 0, true,
"Hello source: shared objects file");
}
}
@@ -0,0 +1,62 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/

import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.Instrumentation;
import java.lang.instrument.IllegalClassFormatException;
import java.security.ProtectionDomain;

public class TransformBootClass implements ClassFileTransformer {

static String targetClass = null;

public byte[] transform(ClassLoader loader, String name, Class<?> classBeingRedefined,
ProtectionDomain pd, byte[] buffer) throws IllegalClassFormatException {

if (name.equals(targetClass)) {
System.out.println("Transforming class " + name);
return buffer;
}
return null;
}

private static Instrumentation savedInstrumentation;

public static void premain(String agentArguments, Instrumentation instrumentation) {
System.out.println("TransformBootClass.premain() is called");
instrumentation.addTransformer(new TransformBootClass(), /*canRetransform=*/true);
savedInstrumentation = instrumentation;
if (agentArguments != null) {
targetClass = agentArguments;
}
}

public static Instrumentation getInstrumentation() {
return savedInstrumentation;
}

public static void agentmain(String args, Instrumentation inst) throws Exception {
premain(args, inst);
}
}
@@ -0,0 +1,5 @@
Manifest-Version: 1.0
Premain-Class: TransformBootClass
Agent-Class: TransformBootClass
Can-Retransform-Classes: true
Can-Redefine-Classes: true