On Wed, 13 Nov 2024 11:42:11 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
> Remove Hotspot code that passes protection_domain around class loading so
> that checkPackageAccess can be called and the result stored. With the
> removal of the Security Manager in JEP 486, this code no longer does anything.
>
> Tested with tier1-4.
This is a great cleanup!
I may have missed something, but it seems to me that
`java_security_AccessControlContext` is all dead code now too.
Thanks
src/hotspot/share/ci/ciEnv.cpp line 1613:
> 1611:
> 1612: // The very first entry is the InstanceKlass of the root method of
> the current compilation in order to get the right
> 1613: // (class loader???) protection domain to load subsequent classes
> during replay compilation.
Suggestion: simply have:
// The very first entry is the InstanceKlass of the root method of the current
compilation .
The rest of the comment doesn't really make sense even before your change as
this method basically just prints the class name
src/hotspot/share/classfile/dictionary.cpp line 80:
> 78:
> 79: void Dictionary::Config::free_node(void* context, void* memory, Value
> const& value) {
> 80: delete value; // Call DictionaryEntry destructor
`using Value = XXX` seems like an unwanted/unnecessary abstraction in this
code, because depending on what `XX` is you either will or won't need to call
`delete`. That is a more general cleanup though.
src/hotspot/share/classfile/javaClasses.hpp line 1545:
> 1543: static int _static_security_offset;
> 1544: static int _static_allow_security_offset;
> 1545: static int _static_never_offset;
Guess these were missed by the main PR as they are unused. :)
src/hotspot/share/classfile/systemDictionary.hpp line 239:
> 237: // compute java_mirror (java.lang.Class instance) for a type ("I",
> "[[B", "LFoo;", etc.)
> 238: // Either the accessing_klass or the CL can be non-null, but not both.
> 239: // callee will fill in CL from AK, if they are needed
Suggestion:
// Callee will fill in CL from accessing_klass, if they are needed.
src/hotspot/share/logging/logTag.hpp line 163:
> 161: LOG_TAG(preview) /* Trace loading of preview feature types */ \
> 162: LOG_TAG(promotion) \
> 163: LOG_TAG(protectiondomain) /* "Trace protection domain verification" */
> \
Not 100% sure about this. We don't really have a policy for "deprecating" or
removing log tags. I think it unlikely anyone enables this logging "just
because", so it seems okay for this case.
-------------
PR Review: https://git.openjdk.org/jdk/pull/22064#pullrequestreview-2435096096
PR Review Comment: https://git.openjdk.org/jdk/pull/22064#discussion_r1841595529
PR Review Comment: https://git.openjdk.org/jdk/pull/22064#discussion_r1841597831
PR Review Comment: https://git.openjdk.org/jdk/pull/22064#discussion_r1841688595
PR Review Comment: https://git.openjdk.org/jdk/pull/22064#discussion_r1841691487
PR Review Comment: https://git.openjdk.org/jdk/pull/22064#discussion_r1841698260
