RFR: 8336919: Cleanup and rename tags in placeholders code

[David Holmes]

On Mon, 22 Jul 2024 16:17:37 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change does some renaming of tags, fields and function name (resolve_super_or_fail => resolve_with_circularity_detection) in the Placeholders and SystemDictionary (class loading) code.  resolve_super_or_fail implied that it's always the super class, which it is not, especially with the upcoming changes to valhalla.  Renaming the function to resolve/load and check for circularity error is more accurate. These are changes made by @fparain brought over from the valhalla branch.
> Tested with tier1-4.

Hi Coleen,

Still mulling over the actual rename as it seems a little vague - but with appropriate comment updates it may become clearer. Lots of whitespace/indent/alignment issues need to be fixed.

Thanks

src/hotspot/share/classfile/classFileParser.cpp line 839:

> 837: 
> 838:         // Call resolve_super so class circularity is checked
> 839:         interf = SystemDictionary::resolve_with_circularity_detection(

Comment needs updating

src/hotspot/share/classfile/classFileParser.cpp line 841:

> 839:         interf = SystemDictionary::resolve_with_circularity_detection(
> 840:                                                   _class_name,
> 841:                                                   unresolved_klass,

Parameter alignment is now incorrect

src/hotspot/share/classfile/classFileParser.cpp line 5943:

> 5941:                                                                super_class_name,
> 5942:                                                                loader,
> 5943:                                                                _protection_domain,

Parameter alignment is now incorrect

src/hotspot/share/classfile/placeholders.cpp line 202:

> 200: // on store ordering here.
> 201: static PlaceholderEntry* add_entry(Symbol* class_name, ClassLoaderData* loader_data,
> 202:                             Symbol* next_klass_name){

Indent is wrong

src/hotspot/share/classfile/placeholders.hpp line 90:

> 88:   SeenThread*       _loadInstanceThreadQ; // loadInstance thread
> 89:                                     // This can't be multiple threads since class loading waits for
> 90:                                     // this token to be removed.

Comments should line up

src/hotspot/share/classfile/placeholders.hpp line 102:

> 100:   bool remove_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action);
> 101: 
> 102:   SeenThread*        circularityThreadQ()        const { return _circularityThreadQ; }

There's no alignment here so the (existing) extra space before `const` is not needed.

src/hotspot/share/classfile/placeholders.hpp line 115:

> 113:      _circularityThreadQ(nullptr), _loadInstanceThreadQ(nullptr), _defineThreadQ(nullptr) { }
> 114: 
> 115:   Symbol*            next_klass_name()     const { return _next_klass_name; }

There's no alignment here so the (existing) extra space before `const` is not needed.

src/hotspot/share/classfile/systemDictionary.cpp line 412:

> 410:                                                        Handle protection_domain,
> 411:                                                        bool is_superclass,
> 412:                                                        TRAPS) {

Parameter alignment needs fixing

src/hotspot/share/classfile/systemDictionary.cpp line 508:

> 506:   assert (!CDSConfig::is_dumping_static_archive(), "should not be parallel loading static archive");
> 507:   Klass* superk = SystemDictionary::resolve_with_circularity_detection(name,
> 508:                                                           superclassname,

Parameter alignment needs fixing

src/hotspot/share/classfile/systemDictionary.cpp line 511:

> 509:                                                           class_loader,
> 510:                                                           protection_domain,
> 511:                                                           false,

Please explain this change!

src/hotspot/share/classfile/systemDictionary.cpp line 580:

> 578:   // name must be in the form of "java/lang/Object" -- cannot be "Ljava/lang/Object;"
> 579:   assert(name != nullptr && !Signature::is_array(name) &&
> 580:          !Signature::has_envelope(name), "invalid class name: %s", name == nullptr ? "nullptr" : name->as_C_string());

`name->as_C_String` requires a `ResourceMark` to be active - we probably should not assume/expect the caller has one.

src/hotspot/share/classfile/systemDictionary.hpp line 108:

> 106:   // parse_interfaces, resolve_instance_class_or_null, load_shared_class
> 107:   // "class_name" is the class whose super class or interface is being resolved.
> 108:   static InstanceKlass* resolve_with_circularity_detection(Symbol* class_name,

Preceding comment needs updating if this is not necessarily for super class/interface.

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20279#pullrequestreview-2197840395
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690484258
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690485020
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690485212
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690493431
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690496606
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690497383
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690497836
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690498829
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690499870
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690500292
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690502222
PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1690503499