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

[Calvin Cheung]

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