[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(S): 8209598: Use log_error for error message in CDS code
From: Ioi Lam <ioi.lam () oracle ! com>
Date: 2018-10-26 1:39:03
Message-ID: f7ef2485-fbec-7245-d359-356e27d77a48 () oracle ! com
[Download RAW message or body]
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
> > > > > > > >
> > > > > >
> > > >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic