RFR(xs): [windows] small fixes to os::check_heap()

[Thomas Stüfe]

Hi David,

On Thu, Nov 19, 2015 at 8:49 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 19/11/2015 4:52 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> On Thu, Nov 19, 2015 at 4:56 AM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi Thomas,
>>
>>     On 19/11/2015 2:40 AM, Thomas Stüfe wrote:
>>
>>         Hi all,
>>
>>         could you please sponsor and review these tiny changes to
>>         os::check_heap()
>>         on windows:
>>
>>         webrev:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8143233/webrev.00/webrev/
>>         bug:
>> https://bugs.openjdk.java.net/browse/JDK-8143233?filter=22798
>>
>>         Fixes two issues:
>>         - GetProcessHeap() is not necessarily the CRT heap, so we need
>>         to use
>>         _get_heap_handle(); instead
>>
>>
>>     I'm unclear why we want the CRT heap as opposed to the process heap?
>>
>>
>> os::check_heap is used in a context that lets me assume that the CRT
>> heap is meant (e.g. in os::malloc, os::realloc, triggered by
>> MallocVerifyInterval
>> <
>> http://ld8443:8080/source/s?defs=MallocVerifyInterval&project=integ-hotspot
>> >).
>> Also, we do not explicitly use the Process Default Heap, so there is no
>> use in checking it.
>>
>>     But in any case according to this as of VS2012 the CRT heap and
>>     process default heap are the same:
>>
>>
>> http://stackoverflow.com/questions/18955871/why-does-get-heap-handle-equal-to-getprocessheap
>>
>>
>> True, but
>> 1) we still support VS 2010
>> (https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms)
>>
>
> Wasn't aware of that :)
>
> 2) arguably, use of _get_heap_handle() better documents the intent here
>> 3) Microsoft may decide at some point to switch back to a separate heap
>> for the CRT instead of using the process default heap.
>>
>
> Okay. Only issue I have is actually testing the change. I don't see that
> we currently have any jtreg tests that exercise this heap checking :( Did I
> miss something?
>
>
I did not find anything. This is also difficult to test, because heap
overwriters are difficult to trigger in a way that they destroy the CRT
heap structures just enough to trigger an error in WalkHeap.

But we added this fix a while ago to our code base and I remember that I
did test it then. I am quite convinced that it actually worked because
after I swapped GetProcessHeap with _get_heap_handle(), I got real errors,
hit the fatal(), and only then detected that I better add the HeapUnlock()
to prevent deadlocks when VMError::report_and_die() does malloc().

But if you insist, I could invest a bit of time to at least manually test
the change.

Kind regards, Thomas



> Thanks,
> David
>
>
>
> Regards, Thomas
>>
>>         - We need to always unlock the heap before exiting with fatal()
>>         in case we
>>         found an error.
>>
>>
>>     Ok.
>>
>>     Thanks,
>>     David
>>
>>         Kind Regards, Thomas
>>
>>
>>