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

[Mikael Gerdin]

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?

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