# HG changeset patch # User hseigel # Date 1484946596 18000 # Node ID bfb383279a16b1b3500db957bef5d51a23cce8dc # Parent 39d4bc6c9989aaebc9267b0041458a4aa9f7b17d 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 diff -r 39d4bc6c9989 -r bfb383279a16 hotspot/src/share/vm/classfile/modules.cpp --- 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(); } } diff -r 39d4bc6c9989 -r bfb383279a16 hotspot/src/share/vm/classfile/packageEntry.cpp --- 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() { diff -r 39d4bc6c9989 -r bfb383279a16 hotspot/src/share/vm/classfile/packageEntry.hpp --- 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 { 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* _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* _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); diff -r 39d4bc6c9989 -r bfb383279a16 hotspot/src/share/vm/runtime/reflection.cpp --- 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; }