RFR(S): 8209598: Use log_error for error message in CDS code
Ioi Lam
ioi.lam at oracle.com
Fri Oct 26 01:39:03 UTC 2018
Looks good. Thanks
- Ioi
On 10/25/18 3:51 PM, Calvin Cheung wrote:
> Hi Ioi,
>
> I've updated classLoader.cpp based on your suggestions:
>
> http://cr.openjdk.java.net/~ccheung/8209598/webrev.03/src/hotspot/share/classfile/classLoader.cpp.sdiff.html
>
> thanks,
> Calvin
>
> On 10/25/18, 2:03 PM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> 1474 bool path_stat = get_canonical_path(path,
>> canonical_class_src_path, JVM_MAXPATHLEN);
>> 1475 assert(path_stat, "Bad src path");
>>
>> 1478 path_stat = get_canonical_path(ent->name(),
>> canonical_path_table_entry, JVM_MAXPATHLEN);
>> 1479 assert(path_stat, "Bad shared path");
>>
>> I think the variable "path_stat" is not clear -- it seems to suggest
>> some sort of statistics. How about just use 'bool success'?
>>
>> Also, I think we need a comment to say why this assert is here.
>> Something like
>>
>> // At this point all the paths have already been checked in
>> ....where?..... and
>> // must be valid.
>> assert(success, "must be valid path");
>>
>> The rest of the changes look good.
>>
>> Thanks
>>
>> - Ioi
>>
>>
>>
>>
>> On 10/25/18 10:09 AM, Calvin Cheung wrote:
>>> Hi Jiangli,
>>>
>>> Thanks for your review.
>>>
>>> On 10/24/18, 4:39 PM, Jiangli Zhou wrote:
>>>> Hi Calvin,
>>>>
>>>> - src/hotspot/share/classfile/classLoader.cpp
>>>>
>>>> 1474 if (!get_canonical_path(path, canonical_class_src_path,
>>>> JVM_MAXPATHLEN)) {
>>>> 1475 log_error(cds)("Bad pathname %s. CDS dump aborted.", path);
>>>> 1476 vm_exit(1);
>>>> 1477 }
>>>>
>>>> 1480 if (!get_canonical_path(ent->name(),
>>>> canonical_path_table_entry, JVM_MAXPATHLEN)) {
>>>> 1481 log_error(cds)("Bad pathname %s. CDS dump aborted.",
>>>> ent->name());
>>>> 1482 vm_exit(1);
>>>> 1483 }
>>>>
>>>> By the time ClassLoader::record_result() is called, it should be
>>>> guaranteed that the paths from the class stream source and the
>>>> recorded path table are valid. Both of the above log_error &
>>>> vm_exit should be replaced by asserting the return value of
>>>> get_canonical_path() is true.
>>> I've made the above change.
>>>>
>>>> - src/hotspot/share/memory/metaspaceShared.cpp
>>>>
>>>> 1648 if (IgnoreUnverifiableClassesDuringDump) {
>>>> 1649 // This is useful when running JCK or SQE tests. You
>>>> should not
>>>> 1650 // enable this when running real apps.
>>>> 1651 SystemDictionary::remove_classes_in_error_state();
>>>> 1652 } else {
>>>> 1653 log_error(cds)("Please remove the unverifiable classes
>>>> from your class list and try again");
>>>> 1654 exit(1);
>>>> 1655 }
>>>>
>>>> IgnoreUnverifiableClassesDuringDump is enabled by default and we
>>>> don't include unverifiable classes in the archive. The above
>>>> comment is outdated. We probably can remove the diagnostic option,
>>>> IgnoreUnverifiableClassesDuringDump altogether now. We used to quit
>>>> dumping process when encountering unverifiable classes (we didn't
>>>> have the class removal part), then
>>>> IgnoreUnverifiableClassesDuringDump was added but set to false
>>>> initially (to keep the old behavior).
>>>> IgnoreUnverifiableClassesDuringDump was turned on by default later.
>>>> The current behavior is the proper behavior and we can clean up the
>>>> code.
>>>
>>> I've updated the comment. I think we should handle the removal of
>>> the flag in a separate bug.
>>>
>>> I've also added a vm_exit_during_cds_dumping() function as suggested
>>> by David so that we don't need to do explicit exit(1) at various
>>> places.
>>>
>>> Here's an updated webrev:
>>> http://cr.openjdk.java.net/~ccheung/8209598/webrev.02/
>>>
>>> thanks,
>>> Calvin
>>>>
>>>> Thanks,
>>>>
>>>> Jiangli
>>>>
>>>>
>>>> On 10/23/18 9:36 PM, Calvin Cheung wrote:
>>>>> Ioi, David,
>>>>>
>>>>> Here's an updated webrev:
>>>>> http://cr.openjdk.java.net/~ccheung/8209598/webrev.01/
>>>>>
>>>>> I've run those 2 tests locally on linux-x64.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> On 10/23/18, 8:50 PM, Ioi Lam wrote:
>>>>>>
>>>>>>
>>>>>> On 10/23/18 7:27 PM, Calvin Cheung wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/23/18, 5:18 PM, Ioi Lam wrote:
>>>>>>>> Hi Calvin,
>>>>>>>>
>>>>>>>> if (PrintSharedArchiveAndExit) {
>>>>>>>> if (PrintSharedDictionary) {
>>>>>>>> - tty->print_cr("\nShared classes:\n");
>>>>>>>> + log_info(cds)("\nShared classes:\n");
>>>>>>>> SystemDictionary::print_shared(tty);
>>>>>>>> }
>>>>>>>> if (_archive_loading_failed) {
>>>>>>>> - tty->print_cr("archive is invalid");
>>>>>>>> + log_error(cds)("archive is invalid");
>>>>>>>> vm_exit(1);
>>>>>>>> } else {
>>>>>>>> - tty->print_cr("archive is valid");
>>>>>>>> + log_info(cds)("archive is valid");
>>>>>>>> vm_exit(0);
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> I think this part should use print_cr, because the option says
>>>>>>>> "Print ...". It shouldn't be necessary to explicitly set
>>>>>>>> -Xlog:cds in order to get the printed message.
>>>>>>> Should I just revert log_info changes and leave the log_error
>>>>>>> there?
>>>>>>
>>>>>> I think the log_error should be reverted as well, because it
>>>>>> would look out of place with the rest of the output.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>>>
>>>>>>>> If you revert this, I think the two test cases also can be
>>>>>>>> reverted.
>>>>>>> Yes, the changes to the two tests were due to the log_info changes.
>>>>>>>>
>>>>>>>> The rest of the changes look OK.
>>>>>>> Thanks for your review.
>>>>>>>
>>>>>>> Calvin
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> - Ioi
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/23/18 2:45 PM, Calvin Cheung wrote:
>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209598
>>>>>>>>>
>>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8209598/webrev.00/
>>>>>>>>>
>>>>>>>>> Use log_error(cds) instead of tty->print_cr for CDS error
>>>>>>>>> messages. Also converted 2 CDS info messages to log_info(cds).
>>>>>>>>>
>>>>>>>>> Testing: mach5 hs-tier{1,2,3}
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Calvin
>>>>>>>>
>>>>>>
>>>>
>>
More information about the hotspot-runtime-dev
mailing list