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

[David Holmes]

This all seems fine. If we can get a second reviewer I will sponsor the 
push.

Thanks,
David

On 20/11/2015 11:24 AM, David Holmes wrote:
> On 19/11/2015 8:04 PM, Thomas Stüfe wrote:
>> On Thu, Nov 19, 2015 at 10:04 AM, David Holmes <david.holmes at oracle.com
>> <mailto: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>
>>         <mailto: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>>
>>                  <mailto: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.
>
> Ah yes! And that is trueInDebug, so in fact this is always being tested!
> Okay.
>
> No need for any additional tests in that case. I'll take the patch and
> test it out internally.
>
> Thanks,
> David
>
>
>> 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
>>
>>
>>
>>