Looks good! Thanks, /Staffan
> On 4 mar 2015, at 15:14, Kevin Walls <kevin.wa...@oracle.com> wrote: > > > Sure, I updated the webrev: > > http://cr.openjdk.java.net/~kevinw/8073688/webrev.01/ > > Thanks > Kevin > > > On 04/03/2015 14:05, Staffan Larsen wrote: >>> On 4 mar 2015, at 14:07, Kevin Walls <kevin.wa...@oracle.com> wrote: >>> >>> >>> Hi Staffan -- >>> >>> staticness: I could have argued that either way, it doesn't need to be >>> static. A static count of how many HotSpotTypeDataBase duplicate type >>> errors we have seen, or one count per HotSpotTypeDataBase? Any doubt I >>> will make it per HotSpotTypeDatabase. As I sometimes load multiple cores >>> in kjdb, or load them multiple times, I should be encouraging less >>> staticness in the SA overall: so yes let me remove that static! (updated >>> webrev in same location) >> Thanks. >> >>> >>> Well spotted on the structs loop: 8-) >>> readVMTypes, readVMntConstants, readVMLongConstants may increase, or call >>> something could increase, the counter. >>> readVMStructs does not. >>> >>> These are four methods that are so similar you'd imagine they should get >>> merged into one method one day. It seemed wrong to me to make the four >>> methods yet more different, by taking out the check on duplicateDefCount >>> for one of them. >>> >>> Also, maybe readVMStructs should be checking for duplicates anyway, and >>> maybe one day it will. I'm guessing it doesn't because it was harder in >>> that method compared to the others. 8-) >>> >>> So I put it there for regularity, but if you think it's misleading I'll >>> take it out. >> Yes, please take it out. >> >> Thanks, >> /Staffan >> >>> In practice, as long as we have the duplicate check in readVMTypes(), we >>> have the protection we need as this is called first. It is highly unlikely >>> that would succeed and another of the methods would fail, the other checks >>> are just making the code consistent. >>> >>> Thanks >>> Kevin >>> >>> >>> >>> On 04/03/2015 12:29, Staffan Larsen wrote: >>>>> On 4 mar 2015, at 12:29, Kevin Walls <kevin.wa...@oracle.com> wrote: >>>>> >>>>> Thanks Dmitry - >>>>> >>>>> I will explain the counter's presence where we introduce it: >>>>> >>>>> // Counter to ensure read loops terminate: >>>>> private static final int MAX_DUPLICATE_DEFINITIONS = 100; >>>>> private static int duplicateDefCount = 0; >>>> Should duplicateDefCount really be static? >>>> >>>> For the use of duplicateDefCount on line 477 (readVMStructs()) I do not >>>> see where it is increased inside the loop? Can you clarify? >>>> >>>> /Staffan >>>> >>>>> Thanks >>>>> Kevin >>>>> >>>>> >>>>> On 03/03/2015 20:10, Dmitry Samersoff wrote: >>>>>> Kevin, >>>>>> >>>>>> The fix looks good for me, but please add a comment explaining the >>>>>> problem we are solving here. >>>>>> >>>>>> -Dmitry >>>>>> >>>>>> On 2015-03-03 16:15, Kevin Walls wrote: >>>>>>> Hi, >>>>>>> >>>>>>> This is a review request for a way to make the SA tools protect >>>>>>> themselves from infinite loops during initialisation. >>>>>>> >>>>>>> Attaching jmap (for example) to a JVM can fail, infinitely writing an >>>>>>> error - and filling a disk if being logged to a file. This reproduces >>>>>>> on a Solaris package based install, but not with other distribution >>>>>>> bundles. In those packages, there's a link JDK/jre/lib/sparc/libjvm -> >>>>>>> client/libjvm.so. If a server/libjvm.so is loaded and running, we see >>>>>>> libverify.so pull in client/libjvm.so, as it finds the symlink in its >>>>>>> $ORIGIN, in preference to finding the already loaded libjvm.so. >>>>>>> >>>>>>> Symbol lookup in the SA is fooled by having multiple libjvm.so loaded. >>>>>>> There are various things wrong with that. Protection against zero >>>>>>> strides through the type arrays and a maximum count for duplicated types >>>>>>> will protect us from a few possible problems. >>>>>>> >>>>>>> I'll also work a bug for the "install" repo where we create that >>>>>>> symlink, but the tools need to protect themselves from this kind of >>>>>>> problem. >>>>>>> >>>>>>> Testing was manual. >>>>>>> >>>>>>> bug >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8073688 >>>>>>> >>>>>>> webrev >>>>>>> http://cr.openjdk.java.net/~kevinw/8073688/webrev.00/ >>>>>>> >>>>>>> Thanks >>>>>>> Kevin >
