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

[David Holmes]

On 19/11/2015 8:39 PM, Mikael Gerdin wrote:
> On 2015-11-19 11:04, Thomas Stüfe wrote:
>> 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?
>
> There is support for creating jtreg tests which use jni, perhaps you
> could just create a jni function which allocates something in the CRT
> heap and corrupts the heap somehow?

If this code is actually completely untested, which so far seems to be 
the case, that might be something that needs to be done. But that is out 
of scope for this bug in my opinion.

David

> /Mikael
>
>>
>> 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
>>>>
>>>>
>>>>
>>>>
>