RFR: 8276184: Exclude lambda proxy class from the CDS archive if its caller class is excluded [v3]

[Ioi Lam]

On Thu, 4 Nov 2021 22:24:33 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> The proposed patch has been redone completely to address the problem when a caller class is being excluded
>> from the archive, the associated lambda proxy class(es) was not excluded.
>> 
>> The caller class could be excluded if:
>> - it is signed;
>> - it has been redefined after its lambda proxy class was created.
>
> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove trailing whitespace

Looks good. I have some comments about a potential bug and suggestion for more testing.

Also, SystemDictionaryShared::cleanup_lambda_proxy_class_dictionary() is called separately for static and dynamic dumps. I think we should move this function to the end of 
SystemDictionaryShared::check_excluded_classes().

src/hotspot/share/classfile/systemDictionaryShared.cpp line 1617:

> 1615:     InstanceKlass* caller_ik = key.caller_ik();
> 1616:     if (SystemDictionaryShared::check_for_exclusion_impl(caller_ik, true)) {
> 1617:       // If the caller class is excluded, unregister all the associated lambda proxy classes so that

As we discussed offline, I think we should add a comment here that SystemDictionaryShared::is_excluded_class() cannot be called because with dynamic dumps, caller_ik could be a class in the base archive.

Also, I think we should test different combinations of how a proxy class can be excluded by the two conditions in this function:

(1) caller_ik is excluded
(2) proxy itself is excluded (because it implements an interface with an old class version
(3) both (1) and (2) are true

src/hotspot/share/classfile/systemDictionaryShared.cpp line 1626:

> 1624:     for (int i = 0; i < info._proxy_klasses->length(); i++) {
> 1625:       InstanceKlass* ik = info._proxy_klasses->at(i);
> 1626:       if (SystemDictionaryShared::check_for_exclusion_impl(ik, true)) {

With the original code, if the list has two elements, only the first one is removed -- after the first iteration, `i` becomes 1, but `_proxy_klasses->length()` is also 1, and thus the loop will terminate.

I think the loop should be reversed:

for (int i = info._proxy_klasses->length() - 1; i >= 0; i--) {
  if (...) {
    info._proxy_klasses->remove_at(i);
  }
}


For test coverage, we should also have two test case:

- caller class Foo1 that uses a single instance of an lambda based on old interface
- caller class Foo2 that uses two instances of an lambda based on old interface

src/hotspot/share/classfile/systemDictionaryShared.hpp line 190:

> 188: 
> 189: public:
> 190:   static bool check_for_exclusion_impl(InstanceKlass* k, bool silent);

Since check_for_exclusion_impl is used only by CleanupDumpTimeLambdaProxyClassTable, I think it's better to keep it private, but declare CleanupDumpTimeLambdaProxyClassTable as a friend class of SystemDictionaryShared. We already do that for ExcludeDumpTimeSharedClasses.

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/RedefineCallerClassTest.java line 29:

> 27:  * @summary If the caller class is redefined during dump time, the caller class
> 28:  *          and its lambda proxy class should not be archived.
> 29:  * @requires vm.cds

Add `@bug .....`

test/hotspot/jtreg/runtime/cds/appcds/test-classes/Hello.java line 28:

> 26:   public static void main(String args[]) {
> 27:     System.out.println("Hello World");
> 28:     System.out.println(getRunnable());

Since this class is used by many test cases, I think it's better to control its behavior by


if (args.length > 1 && args[0].equals("testlambda")) {
  System.out.println(getRunnable());
}


And also add `@bug JDK-8276184` and a note in SignJar.java, something like "testlambda is for testing JDK-8276184".

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

PR: https://git.openjdk.java.net/jdk/pull/6205