RFR: 8336919: Cleanup and rename tags in placeholders code [v4]

[Coleen Phillimore]

On Wed, 31 Jul 2024 04:28:13 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix some comments
>
> src/hotspot/share/classfile/systemDictionary.cpp line 509:
> 
>> 507:   // The only thing that takes different action for is_superclass is dumping the static archive, which doesn't
>> 508:   // reach this path.
>> 509:   assert (!CDSConfig::is_dumping_static_archive(), "should not be dumping static archive");
> 
> I don't think this assert is correct.
> 
> It may be true that *some* calls made by CDS will not come to here (i.e., the code with the `// Special processing for handling UNREGISTERED shared classes` comment).
> 
> However, it's not guaranteed that EVERY call made when dumping the static archive will not come to here.

This is why I wanted you to look at it.  If you follow this code, it's calling 

      // Special processing for handling UNREGISTERED shared classes.
      InstanceKlass* k = SystemDictionaryShared::lookup_super_for_unregistered_class(class_name,
                             next_name, is_superclass);

With is_superclass is true, which does this.  which is wrong:

    if (class_name->equals(parser->current_class_name())) {
      // When this function is called, all the numbered super and interface types
      // must have already been loaded. Hence this function is never recursively called.
      if (is_superclass) {
        return parser->lookup_super_for_current_class(super_name);
      } else {
        return parser->lookup_interface_for_current_class(super_name);
      }

Maybe it's not the parsing_thread, whatever that means and doesn't get here.  I can remove the assert, which is the pre-existing code, but file an bug to look at this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20279#discussion_r1698428176