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

[Ron Durbin]

Ok, thanks Dan
I will get started making the changes.
Reviewers if you want to wait for the new version I should have
it out by Monday.



> -----Original Message-----
> From: Daniel D. Daugherty
> Sent: Friday, May 02, 2014 1:48 PM
> To: Ron Durbin
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: code review round 0 for JDK-8028749 java -version crashes with 'fatal error:
> heap walk aborted with error 1'
> 
> 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
>