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

[David Holmes]

Ping! Second reviewer please!

Thanks
David

On 20/11/2015 4:38 PM, David Holmes wrote:
> 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
>>>
>>>
>>>
>>>