RFR: 8144732: VM_HeapDumper hits assert with bad dump_len

[Andreas Eriksson]

Hi,

New webrev with support for non-segmented dumps removed:
http://cr.openjdk.java.net/~aeriksso/8144732/webrev.01/

I changed calculate_array_max_length a bit (in addition to removing the 
is_segmented check), to avoid a type underflow that could happen.

On 2016-02-08 15:40, Dmitry Samersoff wrote:
> Andreas,
>
> On 2016-02-08 17:18, Andreas Eriksson wrote:
>> Hi,
>>
>> On 2016-02-08 13:28, Dmitry Samersoff wrote:
>>> Andreas,
>>>
>>> Sorry for delay.
>>>
>>> Code changes looks good for me.
>>>
>>> But behavior of non-segmented dump is not clean for me (1074, if dump is
>>> not segmented than the size of the dump is always less than max_bytes
>>> and code below (1086) will not be executed.
>>>
>>> I think today we may always write a segmented heap dump and
>>> significantly simplify logic.
>> Do you mean remove support for dumping non-segmented entirely?
> Correct!
>
> I think it brings more problems than solves (but it's my personal opinion).
>
>> Could I do that as part of this fix? And would it require a CCC request?
> I guess not, it doesn't change anything visible for outside world but
> let me re-check it.
>
> Nothing have changed for heaps larger that 2Gb, dump of small heap will
> contain one more header.
>
> All existing tools have to support both segmented and not segmented
> dumps so it shouldn't break anything.
>
> PS: Please also update:
>
> ./share/vm/runtime/globals.hpp
> 1058:   develop(size_t, SegmentedHeapDumpThreshold, 2*G,
>
> ./serviceability/sa/jmap-hprof/JMapHProfLargeHeapTest.java
>
> -Dmitry
>
>>> Also, I think that current_record_length() don't need as much asserts.
>>> one assert(dump_end == (size_t)current_offset(), "checking"); is enough.
>> I'd like to keep the assert(dump_len <= max_juint, "bad dump length");
>> in case there is ever a similar problem on some other code path if
>> that's ok with you. I have no problem with removing the dump_start
>> assert though.

I removed this assert after all, since at a later point there is a 
warning("record is too large"); that checks the same thing.

The hprof parser also has more problems with long arrays (see for 
example JDK-8149790 <https://bugs.openjdk.java.net/browse/JDK-8149790>) 
but I think that will require larger changes so I won't do it as part of 
this fix.

Regards,
Andreas

> OK.
>
> -Dmitry
>
>
>> Regards,
>> Andreas
>>
>>> -Dmitry
>>>
>>> On 2016-02-01 19:20, Andreas Eriksson wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for dumping of long arrays.
>>>>
>>>> Bug:
>>>> 8144732: VM_HeapDumper hits assert with bad dump_len
>>>> https://bugs.openjdk.java.net/browse/JDK-8144732
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~aeriksso/8144732/webrev.00/
>>>>
>>>> Problem:
>>>> The hprof format uses an u4 as a record length field, but arrays can be
>>>> longer than that (counted in bytes).
>>>>
>>>> Fix:
>>>> Truncate the dump length of the array using a new function,
>>>> calculate_array_max_length. For a given array it 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 change is moving functions needed by
>>>> calculate_array_max_length to the DumpWriter or DumperSupport class so
>>>> that they can be accessed.
>>>>
>>>> Regards,
>>>> Andreas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160216/9f19cabf/attachment-0001.html>