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

[David Holmes]

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.

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