Ok with me. Thanks, /Staffan
> On 4 mar 2015, at 18:41, Kevin Walls <kevin.wa...@oracle.com> wrote: > > > Hi, > > While this is in memory: I'd like to request review for an 8u backport. > > The changeset imports cleanly into jdk8u/hs-dev/hotspot > > Thanks > Kevin > > > On 04/03/2015 15:17, Staffan Larsen wrote: >> 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 >
