code review round 0 for JDK-8028749 java -version crashes with 'fatal error: heap walk aborted with error 1'

[Daniel D. Daugherty]

On 5/2/14 12:08 PM, Ron Durbin wrote:
> Greetings,
>
> This fix resolves:
>
> JDK-8028749 java -version crashes with
>             'fatal error: heap walk aborted with error 1'
>
> Summary:
> When the gflags are enabled it appears that Windows does not
> support application heap checking. This fix disables part of the
> JVM heap check based on the Windows HeapWalk() API when certain
> gflags checks are enabled.
>
> Testing:
> These code changes have tested by completing a successful HotSpot build
> with windows heap checks turned on and off.
> The heap check manipulation was done via gflages.exe
> with a reboot following each change before each test run.
>
> The success criteria was a successfully completed build without
> the java command crashing and leaving a mini dump.
> This fix disables part of the JVM heap check based on the Windows
> HeapWalk() API when certain gflags checks are enabled.
>
> You will find the webrev by following the URL below.
>
> http://cr.openjdk.java.net/~rdurbin/JDK-8028749-webrev/0-jdk9-hs-runtime/

src/os/windows/vm/os_windows.cpp
     I think I see a flaw in the logic and you were right all along...
     Sorry I didn't see this when you did the whiteboard talk on Tues.

     Consider if this:

     line 5020: DWORD err = GetLastError();

     returns a value ERROR_FOO. In the old logic, we would always
     call fatal() on line 5020:

     line 5019: if (err != ERROR_NO_MORE_ITEMS && err != 
ERROR_CALL_NOT_IMPLEMENTED) {
     line 5020:   fatal(err_msg("heap walk aborted with error %d", err));

     In the new logic, we no longer call fatal() with a value of ERROR_FOO:

     line 5027:  if (err != ERROR_NO_MORE_ITEMS &&
     line 5028:      err != ERROR_CALL_NOT_IMPLEMENTED &&
     line 5029:      (did_first_heapwalk_call && err == 
ERROR_INVALID_FUNCTION)) {
     line 5030:    fatal(err_msg("heap walk aborted with error %d", err));

     The logic phrases on lines 5027 and 5028 are both still true with
     a value of ERROR_FOO, but the logic phrase on line 5030 will never
     be true with a value of ERROR_FOO.

     On Tuesday, you said we should put the new check inside the existing
     if-statment and you were right. Jerry and I were both wrong. Sigh...

     So this should do it:

     line 5027: if (err != ERROR_NO_MORE_ITEMS && err != 
ERROR_CALL_NOT_IMPLEMENTED) {
line 5028:   if (!did_first_heapwalk_call && err == 
ERROR_INVALID_FUNCTION) {
line 5029:     // only ignore this error on first HeapWalk() call
line 5030:   } else {
line 5031:     fatal(err_msg("heap walk aborted with error %d", err));
line 5032:   }
line 5033: }

     An ERROR_FOO value will always fail the logic phrase on line 5028
     so it will always call fatal on line 5031. ERROR_INVALID_FUNCTION
     only gets a pass on the first call to HeapWalk().

     It's ugly, but it's the only way I see to avoid duplicating the
     fatal() line.

Dan