Andreas, Thumbs up! (Reviewed)
-Dmitry On 2016-01-18 16:07, Andreas Eriksson wrote: > > > On 2016-01-18 13:58, Dmitry Samersoff wrote: >> Andreas, >> >> On 2016-01-18 15:26, Andreas Eriksson wrote: >>> I believe I then have to cast it to size_t at 496, 497 and 498? >> I hope not. >> >>> I'm not sure how write works in regards to returning ssize_t vs a size_t >>> length. >> It's a tricky part. >> >> The maximum number of bytes that could be actually written by write() in >> one shot is implementation dependent and limited by fs (or VFS) driver. >> Typically it is less than MAX_INT. >> >> If len overflow it, write just return the number of bytes actually >> written without setting an error. >> >>> Maybe I should change it back to checking for return value < 0 (instead >>> of only checking for OS_ERR), and then keep 'n' as ssize_t? >> I would prefer this way. > > Done. > Uploaded new webrev if you want to take a look: > http://cr.openjdk.java.net/~aeriksso/8129419/webrev.03/hotspot/ > > - Andreas > >> >> -Dmitry >> >>> - Andreas >>> >>> On 2016-01-18 13:11, Dmitry Samersoff wrote: >>>> Андреас, >>>> >>>> 481: It's better to declare n as ssize_t and remove cast at 488 >>>> >>>> Looks good for me! >>>> >>>> -Dmitry >>>> >>>> >>>> On 2016-01-18 14:42, Andreas Eriksson wrote: >>>>> Hi, >>>>> >>>>> New webrev: >>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.02/hotspot/ >>>>> >>>>> Write_internal handles writes less than len, and EINTR. >>>>> >>>>> EINTR is taken care of by os::write, but to use it I had to remove an >>>>> assert from the solaris os::write, which checked that a JavaThread was >>>>> in_native. Since heap dumping is done from the vm_thread (not a >>>>> JavaThread) the assert crashed the VM. >>>>> >>>>> Regards, >>>>> Andreas >>>>> >>>>> On 2016-01-13 13:39, Andreas Eriksson wrote: >>>>>> Hi, >>>>>> >>>>>> On 2015-12-29 21:27, Dmitry Samersoff wrote: >>>>>>> Andreas, >>>>>>> >>>>>>> Great work. All but write_internal looks good for me (see below). >>>>>>> >>>>>>> >>>>>>> HprofReader.java: >>>>>>> >>>>>>> Nit - length -= skipped; should be after if skipped == 0 >>>>>>> >>>>>>> heapDumper.cpp:480 >>>>>>> >>>>>>> 1. For windows you can do : >>>>>>> assert(len < (size_t)UINT_MAX, ... ); >>>>>>> ::write( ..., (uint) (len & UINT_MAX)); >>>>>>> >>>>>>> 2. You should check for n < len and if it is true, >>>>>>> deal with errno. n = 0 is legitimate case, >>>>>>> so assert is not necessary. >>>>>> I only think n = 0 is valid if write is called with length 0, which >>>>>> write_internal will never do. >>>>>> Otherwise, according to posix, n will be -1 if an error occurred, or >>>>>> greater than zero if some bytes were written. [1] >>>>>> If some bytes but not all were written then the while(len > 0) loop >>>>>> will make sure we try to write the rest of the bytes. >>>>>> It looks like windows write should never return 0 when len != 0 >>>>>> either. [2] >>>>>> >>>>>> I should however handle the -1 result with errno EINTR, working on a >>>>>> new webrev. >>>>>> >>>>>> Thanks, >>>>>> Andreas >>>>>> >>>>>> [1] >>>>>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html >>>>>> If write() is interrupted by a signal before it writes any data, it >>>>>> shall return -1 with errno set to [EINTR]. >>>>>> If write() is interrupted by a signal after it successfully writes >>>>>> some data, it shall return the number of bytes written. >>>>>> >>>>>> [2] https://msdn.microsoft.com/en-us/library/1570wh78(v=vs.80).aspx >>>>>> >>>>>>> -Dmitry >>>>>>> >>>>>>> On 2015-12-28 17:02, Andreas Eriksson wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> Here is the webrev for type changes. >>>>>>>> Top-repo: >>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.01/jdk9-hs-rt/ >>>>>>>> Hotspot: >>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.01/hotspot/ >>>>>>>> >>>>>>>> The windows_x64 native write function uses a length of type >>>>>>>> uint, not >>>>>>>> size_t. I added an ifdef for that case and handled it, but better >>>>>>>> suggestions are welcome. >>>>>>>> >>>>>>>> Also found and fixed another problem in the hprof parser when >>>>>>>> skipping >>>>>>>> over bytes. >>>>>>>> >>>>>>>> I've not included the tests, they have OOM issues on several JPRT >>>>>>>> hosts, >>>>>>>> and until I've figured out a way to fix that I wont be pushing >>>>>>>> them. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Andreas >>>>>>>> >>>>>>>> On 2015-12-14 19:34, Andreas Eriksson wrote: >>>>>>>>> Hi Dmitry, thanks for looking at this. >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2015-12-14 18:30, Dmitry Samersoff wrote: >>>>>>>>>> Andreas, >>>>>>>>>> >>>>>>>>>> Nice cleanup. >>>>>>>>>> >>>>>>>>>> Some generic comments below. >>>>>>>>>> >>>>>>>>>> 1. It would be excellent if you can split webrev into two parts, >>>>>>>>>> one >>>>>>>>>> part with types cleanup and other part with array truncation >>>>>>>>>> related >>>>>>>>>> changes or ever push these changes separately as it address >>>>>>>>>> different >>>>>>>>>> problems. >>>>>>>>>> >>>>>>>>>> Type cleanup could be reviewed quickly, but review of array >>>>>>>>>> truncation >>>>>>>>>> requires some time. >>>>>>>>>> >>>>>>>>>> (We already have two different CRs: JDK-8129419 for type cleanup >>>>>>>>>> and >>>>>>>>>> JDK-8144732 for array truncation) >>>>>>>>> Yes, that's a good point. >>>>>>>>> >>>>>>>>>> 2. >>>>>>>>>> it might be better to use size_t and jlong (or julong) across all >>>>>>>>>> file >>>>>>>>>> and get rid of all other types with a few exclusion. >>>>>>>>> I'll see what I can do, and we can look at it closer once I've >>>>>>>>> split >>>>>>>>> the type changes into a separate webrev. >>>>>>>>> >>>>>>>>>> 3. >>>>>>>>>> Each assert costs some time in nightly testing, so we probably >>>>>>>>>> don't >>>>>>>>>> need as much asserts. >>>>>>>>> Alright. >>>>>>>>> >>>>>>>>>> 4. write_internal: >>>>>>>>>> >>>>>>>>>> There are couple of cases where ::write writes less >>>>>>>>>> bytes than >>>>>>>>>> requested and doesn't return -1. >>>>>>>>>> So we should check if ::write returns a value less that len >>>>>>>>>> and if it >>>>>>>>>> happens deal with errno - either repeat entire write >>>>>>>>>> attempt(EINTR/EAGAIN) or abort writing. >>>>>>>>> Actually, I meant to ask about that, but forgot: >>>>>>>>> I tried using os::write, which handles EINTR/EAGAIN if necessary >>>>>>>>> (depending on platform), but Solaris has an assert that fails: >>>>>>>>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/5a42c1dde332/src/os/solaris/vm/os_solaris.cpp#l5692 >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> It checks that os::write is called by a JavaThread that is >>>>>>>>> in_native, >>>>>>>>> which we are not, since we are in the VM_thread doing a >>>>>>>>> safepoint_op. >>>>>>>>> Does anyone know if that assert is really needed? It's only >>>>>>>>> done for >>>>>>>>> Solaris. >>>>>>>>> >>>>>>>>>> 5. we should handle zero-length array in >>>>>>>>>> calculate_array_max_length - >>>>>>>>>> it's a legitimate case >>>>>>>>> Not sure what you mean here, I believe it does handle zero-length >>>>>>>>> arrays. >>>>>>>>> It should just fall through without taking any of the if-clauses. >>>>>>>>> (Or do you mean that I should add a return immediately at the >>>>>>>>> top if >>>>>>>>> length is zero?) >>>>>>>>> >>>>>>>>>> 6. max_juint is less than SegmentedHeapDumpThreshold so >>>>>>>>>> non-segmented >>>>>>>>>> heapdump can't contain huge array and it's better to check for >>>>>>>>>> segmented >>>>>>>>>> dump before any other checks. >>>>>>>>> Correct me if I'm wrong here, but SegmentedHeapDumpThreshold >>>>>>>>> could in >>>>>>>>> theory be set so that we have a non-segmented heap dump while >>>>>>>>> still >>>>>>>>> having huge arrays. >>>>>>>>> Not sure if this is worth taking into account, since it most >>>>>>>>> likely >>>>>>>>> would lead to other problems as well, and the flag is develop >>>>>>>>> only, so >>>>>>>>> it can't happen in product builds. >>>>>>>>> What do you think would be best to do here? >>>>>>>>> >>>>>>>>> - Andreas >>>>>>>>> >>>>>>>>>> -Dmitry >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 2015-12-14 18:38, Andreas Eriksson wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> Please review this fix for dumping of long arrays, and general >>>>>>>>>>> cleanup >>>>>>>>>>> of types in heapDumper.cpp. >>>>>>>>>>> >>>>>>>>>>> Problem: >>>>>>>>>>> At several places in heapDumper.cpp overflows could happen when >>>>>>>>>>> dumping >>>>>>>>>>> long arrays. >>>>>>>>>>> Also the hprof format uses an u4 as a record length field, but >>>>>>>>>>> arrays >>>>>>>>>>> can be longer than that (counted in bytes). >>>>>>>>>>> >>>>>>>>>>> Fix: >>>>>>>>>>> Many types that were previously signed are changed to equivalent >>>>>>>>>>> unsigned types and/or to a larger type to prevent overflow. >>>>>>>>>>> The bulk of the change though is the addition of >>>>>>>>>>> calculate_array_max_length, which for a given array returns the >>>>>>>>>>> number >>>>>>>>>>> of elements we can dump. That length is then used to truncate >>>>>>>>>>> arrays >>>>>>>>>>> that are too long. >>>>>>>>>>> Whenever an array is truncated a warning is printed: >>>>>>>>>>> Java HotSpot(TM) 64-Bit Server VM warning: cannot dump array of >>>>>>>>>>> type >>>>>>>>>>> object[] with length 1,073,741,823; truncating to length >>>>>>>>>>> 536,870,908 >>>>>>>>>>> >>>>>>>>>>> Much of the rest of the change is moving functions needed by >>>>>>>>>>> calculate_array_max_length to the DumpWriter or DumperSupport >>>>>>>>>>> class so >>>>>>>>>>> that they can be accessed. >>>>>>>>>>> >>>>>>>>>>> Added a test that relies on the hprof parser, which also had a >>>>>>>>>>> couple of >>>>>>>>>>> overflow problems (top repo changes). >>>>>>>>>>> I've also run this change against a couple of other tests, but >>>>>>>>>>> they are >>>>>>>>>>> problematic in JPRT because they are using large heaps and >>>>>>>>>>> lots of >>>>>>>>>>> disk. >>>>>>>>>>> >>>>>>>>>>> Bug: >>>>>>>>>>> 8129419: heapDumper.cpp: assert(length_in_bytes > 0) failed: >>>>>>>>>>> nothing to >>>>>>>>>>> copy >>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8129419 >>>>>>>>>>> >>>>>>>>>>> Also fixed in this change is the problems seen in these two >>>>>>>>>>> bugs: >>>>>>>>>>> 8133317: Integer overflow in heapDumper.cpp leads to corrupt >>>>>>>>>>> HPROF >>>>>>>>>>> dumps >>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8133317 >>>>>>>>>>> >>>>>>>>>>> 8144732: VM_HeapDumper hits assert with bad dump_len >>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8144732 >>>>>>>>>>> >>>>>>>>>>> Webrev: >>>>>>>>>>> Top repo: >>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.00/jdk9-hs-rt/ >>>>>>>>>>> >>>>>>>>>>> Hotspot: >>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.00/hotspot/ >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Andreas >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.