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

[Ioi Lam]

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