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