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

Thomas Stüfe thomas.stuefe at gmail.com
Fri Nov 20 09:31:32 UTC 2015

Thanks David!

On Fri, Nov 20, 2015 at 7:38 AM, David Holmes <david.holmes at oracle.com> 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::checkheap() 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 getheaphandle(); instead

I'm unclear why we want the CRT heap as opposed to the process heap? os::checkheap 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 getheaphandle() 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 getheaphandle(), I got real errors, hit the fatal(), and only then detected that I better add the HeapUnlock() to prevent deadlocks when VMError::reportanddie() 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::checkheap 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

More information about the hotspot-runtime-dev mailing list