8171971: Fix timing bug in JVM management of package export lists
Summary: Reduce the number of fields that maintain export state and use Module_lock to access these fields
Reviewed-by: acorn, sspitsyn, lfoltan
--- a/hotspot/src/share/vm/classfile/modules.cpp Thu Jan 19 21:52:51 2017 -0800
+++ b/hotspot/src/share/vm/classfile/modules.cpp Fri Jan 20 16:09:56 2017 -0500
@@ -1,5 +1,5 @@
/*
-* Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+* Copyright (c) 2016, 2017, 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
@@ -823,10 +823,7 @@
package_entry->name()->as_C_string(),
module_entry->name()->as_C_string());
- // Mark package as exported to all unnamed modules, unless already
- // unqualifiedly exported.
- if (!package_entry->is_unqual_exported()) {
- package_entry->set_is_exported_allUnnamed();
- }
+ // Mark package as exported to all unnamed modules.
+ package_entry->set_is_exported_allUnnamed();
}
}
--- a/hotspot/src/share/vm/classfile/packageEntry.cpp Thu Jan 19 21:52:51 2017 -0800
+++ b/hotspot/src/share/vm/classfile/packageEntry.cpp Fri Jan 20 16:09:56 2017 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2017, 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
@@ -37,8 +37,8 @@
// Returns true if this package specifies m as a qualified export, including through an unnamed export
bool PackageEntry::is_qexported_to(ModuleEntry* m) const {
+ assert(Module_lock->owned_by_self(), "should have the Module_lock");
assert(m != NULL, "No module to lookup in this package's qualified exports list");
- MutexLocker m1(Module_lock);
if (is_exported_allUnnamed() && !m->is_named()) {
return true;
} else if (!has_qual_exports_list()) {
@@ -98,15 +98,8 @@
}
if (m == NULL) {
- // NULL indicates the package is being unqualifiedly exported
- if (has_qual_exports_list()) {
- // Legit to transition a package from being qualifiedly exported
- // to unqualified. Clean up the qualified lists at the next
- // safepoint.
- _exported_pending_delete = _qualified_exports;
- }
-
- // Mark package as unqualifiedly exported
+ // NULL indicates the package is being unqualifiedly exported. Clean up
+ // the qualified list at the next safepoint.
set_unqual_exported();
} else {
@@ -115,14 +108,19 @@
}
}
+// Set the package as exported to all unnamed modules unless the package is
+// already unqualifiedly exported.
void PackageEntry::set_is_exported_allUnnamed() {
MutexLocker m1(Module_lock);
if (!is_unqual_exported()) {
- _is_exported_allUnnamed = true;
+ _export_flags = PKG_EXP_ALLUNNAMED;
}
}
-// Remove dead module entries within the package's exported list.
+// Remove dead module entries within the package's exported list. Note that
+// if all of the modules on the _qualified_exports get purged the list does not
+// get deleted. This prevents the package from illegally transitioning from
+// exported to non-exported.
void PackageEntry::purge_qualified_exports() {
assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
if (_must_walk_exports &&
@@ -160,18 +158,9 @@
void PackageEntry::delete_qualified_exports() {
assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
- if (_exported_pending_delete != NULL) {
- // If a transition occurred from qualified to unqualified, the _qualified_exports
- // field should have been NULL'ed out.
- assert(_qualified_exports == NULL, "Package's exported pending delete, exported list should not be active");
- delete _exported_pending_delete;
- }
-
if (_qualified_exports != NULL) {
delete _qualified_exports;
}
-
- _exported_pending_delete = NULL;
_qualified_exports = NULL;
}
@@ -314,6 +303,11 @@
}
}
+bool PackageEntry::exported_pending_delete() const {
+ assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
+ return (is_unqual_exported() && _qualified_exports != NULL);
+}
+
// Remove dead entries from all packages' exported list
void PackageEntryTable::purge_all_package_exports() {
assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
@@ -344,13 +338,17 @@
}
}
+// This function may be called from debuggers so access private fields directly
+// to prevent triggering locking-related asserts that could result from calling
+// getter methods.
void PackageEntry::print(outputStream* st) {
ResourceMark rm;
st->print_cr("package entry " PTR_FORMAT " name %s module %s classpath_index "
INT32_FORMAT " is_exported_unqualified %d is_exported_allUnnamed %d " "next " PTR_FORMAT,
p2i(this), name()->as_C_string(),
(module()->is_named() ? module()->name()->as_C_string() : UNNAMED_MODULE),
- _classpath_index, _is_exported_unqualified, _is_exported_allUnnamed, p2i(next()));
+ _classpath_index, _export_flags == PKG_EXP_UNQUALIFIED,
+ _export_flags == PKG_EXP_ALLUNNAMED, p2i(next()));
}
void PackageEntryTable::verify() {
--- a/hotspot/src/share/vm/classfile/packageEntry.hpp Thu Jan 19 21:52:51 2017 -0800
+++ b/hotspot/src/share/vm/classfile/packageEntry.hpp Fri Jan 20 16:09:56 2017 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2017, 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
@@ -34,8 +34,8 @@
// A PackageEntry basically represents a Java package. It contains:
// - Symbol* containing the package's name.
// - ModuleEntry* for this package's containing module.
-// - a flag indicating if package is exported unqualifiedly
-// - a flag indicating if this package is exported to all unnamed modules.
+// - a field indicating if the package is exported unqualifiedly or to all
+// unnamed modules.
// - a growable array containing other module entries that this
// package is exported to.
//
@@ -44,9 +44,9 @@
// - qualified exports: the package has been explicitly qualified to at least
// one particular module or has been qualifiedly exported
// to all unnamed modules.
-// Note: _is_exported_allUnnamed is a form of a qualified
-// export. It is equivalent to the package being
-// explicitly exported to all current and future unnamed modules.
+// Note: being exported to all unnamed is a form of a qualified
+// export. It is equivalent to the package being explicitly
+// exported to all current and future unnamed modules.
// - unqualified exports: the package is exported to all modules.
//
// A package can transition from:
@@ -56,21 +56,53 @@
// A package cannot transition from:
// - being unqualifiedly exported, to exported qualifiedly to a specific module.
// This transition attempt is silently ignored in set_exported.
+// - being qualifiedly exported to not exported.
+// Because transitions are only allowed from less exposure to greater exposure,
+// the transition from qualifiedly exported to not exported would be considered
+// a backward direction. Therefore the implementation considers a package as
+// qualifiedly exported even if its export-list exists but is empty.
//
// The Mutex Module_lock is shared between ModuleEntry and PackageEntry, to lock either
// data structure.
+
+// PKG_EXP_UNQUALIFIED and PKG_EXP_ALLUNNAMED indicate whether the package is
+// exported unqualifiedly or exported to all unnamed modules. They are used to
+// set the value of _export_flags. Field _export_flags and the _qualified_exports
+// list are used to determine a package's export state.
+// Valid states are:
+//
+// 1. Package is not exported
+// _export_flags is zero and _qualified_exports is null
+// 2. Package is unqualifiedly exported
+// _export_flags is set to PKG_EXP_UNQUALIFIED
+// _qualified_exports may or may not be null depending on whether the package
+// transitioned from qualifiedly exported to unqualifiedly exported.
+// 3. Package is qualifiedly exported
+// _export_flags may be set to PKG_EXP_ALLUNNAMED if the package is also
+// exported to all unnamed modules
+// _qualified_exports will be non-null
+// 4. Package is exported to all unnamed modules
+// _export_flags is set to PKG_EXP_ALLUNNAMED
+// _qualified_exports may or may not be null depending on whether the package
+// is also qualifiedly exported to one or more named modules.
+#define PKG_EXP_UNQUALIFIED 0x0001
+#define PKG_EXP_ALLUNNAMED 0x0002
+#define PKG_EXP_UNQUALIFIED_OR_ALL_UNAMED (PKG_EXP_UNQUALIFIED | PKG_EXP_ALLUNNAMED)
+
class PackageEntry : public HashtableEntry<Symbol*, mtModule> {
private:
ModuleEntry* _module;
+ // Indicates if package is exported unqualifiedly or to all unnamed. Access to
+ // this field is protected by the Module_lock.
+ int _export_flags;
// Used to indicate for packages with classes loaded by the boot loader that
// a class in that package has been loaded. And, for packages with classes
// loaded by the boot loader from -Xbootclasspath/a in an unnamed module, it
// indicates from which class path entry.
s2 _classpath_index;
- bool _is_exported_unqualified;
- bool _is_exported_allUnnamed;
bool _must_walk_exports;
- GrowableArray<ModuleEntry*>* _exported_pending_delete; // transitioned from qualified to unqualified, delete at safepoint
+ // Contains list of modules this package is qualifiedly exported to. Access
+ // to this list is protected by the Module_lock.
GrowableArray<ModuleEntry*>* _qualified_exports;
TRACE_DEFINE_TRACE_ID_FIELD;
@@ -80,17 +112,14 @@
public:
void init() {
_module = NULL;
+ _export_flags = 0;
_classpath_index = -1;
- _is_exported_unqualified = false;
- _is_exported_allUnnamed = false;
_must_walk_exports = false;
- _exported_pending_delete = NULL;
_qualified_exports = NULL;
}
// package name
Symbol* name() const { return literal(); }
- void set_name(Symbol* n) { set_literal(n); }
// the module containing the package definition
ModuleEntry* module() const { return _module; }
@@ -98,37 +127,39 @@
// package's export state
bool is_exported() const { // qualifiedly or unqualifiedly exported
- return (is_unqual_exported() || has_qual_exports_list() || is_exported_allUnnamed());
+ assert_locked_or_safepoint(Module_lock);
+ return ((_export_flags & PKG_EXP_UNQUALIFIED_OR_ALL_UNAMED) != 0) || has_qual_exports_list();
}
// Returns true if the package has any explicit qualified exports or is exported to all unnamed
bool is_qual_exported() const {
+ assert_locked_or_safepoint(Module_lock);
return (has_qual_exports_list() || is_exported_allUnnamed());
}
- // Returns true if there are any explicit qualified exports
+ // Returns true if there are any explicit qualified exports. Note that even
+ // if the _qualified_exports list is now empty (because the modules that were
+ // on the list got gc-ed and deleted from the list) this method may still
+ // return true.
bool has_qual_exports_list() const {
- assert(!(_qualified_exports != NULL && _is_exported_unqualified),
- "_qualified_exports set at same time as _is_exported_unqualified");
- return (_qualified_exports != NULL);
+ assert_locked_or_safepoint(Module_lock);
+ return (!is_unqual_exported() && _qualified_exports != NULL);
}
bool is_exported_allUnnamed() const {
- assert(!(_is_exported_allUnnamed && _is_exported_unqualified),
- "_is_exported_allUnnamed set at same time as _is_exported_unqualified");
- return _is_exported_allUnnamed;
+ assert_locked_or_safepoint(Module_lock);
+ return (_export_flags == PKG_EXP_ALLUNNAMED);
}
bool is_unqual_exported() const {
- assert(!(_qualified_exports != NULL && _is_exported_unqualified),
- "_qualified_exports set at same time as _is_exported_unqualified");
- assert(!(_is_exported_allUnnamed && _is_exported_unqualified),
- "_is_exported_allUnnamed set at same time as _is_exported_unqualified");
- return _is_exported_unqualified;
+ assert_locked_or_safepoint(Module_lock);
+ return (_export_flags == PKG_EXP_UNQUALIFIED);
}
+
+ // Explicitly set _export_flags to PKG_EXP_UNQUALIFIED and clear
+ // PKG_EXP_ALLUNNAMED, if it was set.
void set_unqual_exported() {
assert(Module_lock->owned_by_self(), "should have the Module_lock");
- _is_exported_unqualified = true;
- _is_exported_allUnnamed = false;
- _qualified_exports = NULL;
+ _export_flags = PKG_EXP_UNQUALIFIED;
}
- bool exported_pending_delete() const { return (_exported_pending_delete != NULL); }
+
+ bool exported_pending_delete() const;
void set_exported(ModuleEntry* m);
--- a/hotspot/src/share/vm/runtime/reflection.cpp Thu Jan 19 21:52:51 2017 -0800
+++ b/hotspot/src/share/vm/runtime/reflection.cpp Fri Jan 20 16:09:56 2017 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -534,22 +534,26 @@
PackageEntry* package_to = new_class->package();
assert(package_to != NULL, "can not obtain new_class' package");
- // Once readability is established, if module_to exports T unqualifiedly,
- // (to all modules), than whether module_from is in the unnamed module
- // or not does not matter, access is allowed.
- if (package_to->is_unqual_exported()) {
- return ACCESS_OK;
- }
+ {
+ MutexLocker m1(Module_lock);
+
+ // Once readability is established, if module_to exports T unqualifiedly,
+ // (to all modules), than whether module_from is in the unnamed module
+ // or not does not matter, access is allowed.
+ if (package_to->is_unqual_exported()) {
+ return ACCESS_OK;
+ }
- // Access is allowed if both 1 & 2 hold:
- // 1. Readability, module_from can read module_to (established above).
- // 2. Either module_to exports T to module_from qualifiedly.
- // or
- // module_to exports T to all unnamed modules and module_from is unnamed.
- // or
- // module_to exports T unqualifiedly to all modules (checked above).
- if (!package_to->is_qexported_to(module_from)) {
- return TYPE_NOT_EXPORTED;
+ // Access is allowed if both 1 & 2 hold:
+ // 1. Readability, module_from can read module_to (established above).
+ // 2. Either module_to exports T to module_from qualifiedly.
+ // or
+ // module_to exports T to all unnamed modules and module_from is unnamed.
+ // or
+ // module_to exports T unqualifiedly to all modules (checked above).
+ if (!package_to->is_qexported_to(module_from)) {
+ return TYPE_NOT_EXPORTED;
+ }
}
return ACCESS_OK;
}