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