RFR(S): 8209598: Use log_error for error message in CDS code

[Calvin Cheung]

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
>>>>>>>
>>>>>
>>>
>