RFR (trivial): 8247388: Minimal build broken after JDK-8240245 (undefined reference to `MetaspaceShared::_use_optimized_module_handling')

[David Holmes]

On 12/06/2020 4:36 pm, Ao Qi wrote:
> On Fri, Jun 12, 2020 at 1:22 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>>
>>
>> On 6/11/20 7:45 AM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 6/11/20 9:30 AM, David Holmes wrote:
>>>> On 11/06/2020 9:55 pm, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> On further viewing, I think adding NOT_CDS_RETURN to
>>>>>
>>>>> MetaspaceShared::disable_optimized_module_handling();
>>>>>
>>>>> would be a lot better actually.
>>>>
>>>> That's fine by me to get this fixed for the build. But what Ao Qi has
>>>> highlights that this code is IMO in the wrong place to begin with.
>>>> This logic should be in Arguments::create_module_property not the
>>>> general purpose Arguments::add_property. I will file a follow up bug
>>>> to address that.
>>>
>>> I think this is fine also, but the cleanup I'd like to see is if
>>> metaspaceShared.cpp is not included in the INCLUDE_CDS build, then the
>>> declarations should not be included either.
>>>
>>
>> I think the posted webrev is fine. Because jdk15 has been forked now,
>> the fix should be pushed to jdk15 (and then it will be automatically
>> synced down to jdk/jdk??).
>>
>> Adding NOT_CDS_RETURN
>> MetaspaceShared::disable_optimized_module_handling() would be nice to
>> have, but this code still needs to be #ifdef'ed
>>
>> + #if INCLUDE_CDS
>>       if (is_internal_module_property(key) ||
>>           strcmp(key, "jdk.module.main") == 0) {
>>         MetaspaceShared::disable_optimized_module_handling();
>>         log_info(cds)("Using optimized module handling disabled due to incompatible property: %s=%s", key, value);
>>       }
>> + #endif
> 
> also:
> 
> +#if INCLUDE_CDS
>         MetaspaceShared::disable_optimized_module_handling();
>         log_info(cds)("Using optimized module handling disabled due to
> bootclasspath was appended");
> +#endif
> 
> then I think webrev.01 has to include all the changes in webrev.00
> 
> Shell we push the change and let the build work?

I think push webrev.00. As Ioi stated we don't want to execute the other 
code unnecessarily in a build without CDS.

Thanks,
David
-----

> 
> Thanks,
> Ao Qi
> 
>>
>>
>> Or else you might see an unexpected log message in minimum build.
>>
>> I think we can fix the stylistic issues in the bug JDK-8247449 "Revisit
>> the argument processing logic for
>> MetaspaceShared::disable_optimized_module_handling()", which David has
>> just filed.
>>
>> I've added comments in the above bug the explain why the code was
>> written like that in JDK-8240245.
>>
>> Thanks
>> - Ioi
>>
>>> thanks,
>>> Coleen
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Coleen
>>>>>
>>>>> On 6/11/20 7:53 AM, coleen.phillimore at oracle.com wrote:
>>>>>> This looks good and trivial.
>>>>>> thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 6/11/20 6:15 AM, Ao Qi wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Could you please review this patch?
>>>>>>>
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247388
>>>>>>> webrev: http://cr.openjdk.java.net/~aoqi/8247388/webrev.00/
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ao Qi
>>>>>>>
>>>>>>
>>>>>
>>>
>