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

[Thomas Stüfe]

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

> On 19/11/2015 6:25 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> On Thu, Nov 19, 2015 at 8:49 AM, David Holmes <david.holmes at oracle.com
>> <mailto: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>
>>         <mailto: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.
>>
>
> I don't need a test that actually corrupts the heap and verifies that this
> detects it. I just want something that will actually run the code being
> changed. I'll see if we have anything internally.
>
>
-XX:+VerifyBeforeExit causes the VmThread to execute os::check_heap on
shutdown. I tried to test it and to step thru with VS2010, but have some
problems debugging the libjvm. I will take a closer look later.

Maybe a Whitebox Test would be the right thing to do, or would that be
overdone?

Kind Regards, Thomas


> Thanks,
> David
>
>
> 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
>>
>>
>>
>>