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

[Jiangli Zhou]

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.

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

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