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

David Holmes david.holmes at oracle.com
Thu Oct 25 23:41:37 UTC 2018

Hi Calvin,

The code changes look good to me - thanks.

The bug synopsis and description needs updating to reflect the actual change though.

Thanks, David

On 26/10/2018 8:51 AM, 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 pathstat = getcanonicalpath(path, canonicalclasssrcpath, JVMMAXPATHLEN); 1475     assert(pathstat, "Bad src path"); 1478       pathstat = getcanonicalpath(ent->name(), canonicalpathtableentry, JVMMAXPATHLEN); 1479       assert(pathstat, "Bad shared path"); I think the variable "pathstat" 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 (!getcanonicalpath(path, canonicalclasssrcpath, JVMMAXPATHLEN)) { 1475       logerror(cds)("Bad pathname %s. CDS dump aborted.", path); 1476       vmexit(1); 1477     } 1480       if (!getcanonicalpath(ent->name(), canonicalpathtableentry, JVMMAXPATHLEN)) { 1481         logerror(cds)("Bad pathname %s. CDS dump aborted.", ent->name()); 1482         vmexit(1); 1483       } By the time ClassLoader::recordresult() 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 logerror & vmexit should be replaced by asserting the return value of getcanonicalpath() 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::removeclassesinerrorstate(); 1652     } else { 1653       logerror(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 vmexitduringcdsdumping() 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->printcr("\nShared classes:\n"); +      loginfo(cds)("\nShared classes:\n"); SystemDictionary::printshared(tty); } if (archiveloadingfailed) { -      tty->printcr("archive is invalid"); +      logerror(cds)("archive is invalid"); vmexit(1); } else { -      tty->printcr("archive is valid"); +      loginfo(cds)("archive is valid"); vmexit(0); } } I think this part should use printcr, 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 loginfo changes and leave the logerror there? I think the logerror 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 loginfo 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 logerror(cds) instead of tty->printcr for CDS error messages. Also converted 2 CDS info messages to loginfo(cds). Testing: mach5 hs-tier{1,2,3} thanks, Calvin

More information about the hotspot-runtime-dev mailing list