Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows

[David Holmes]

John Coomes said the following on 07/16/11 07:01:
> Zhengyu Gu (zhengyu.gu at oracle.com) wrote:
>> Forward to openjdk for review:
>>
>> Webrev: http://cr.openjdk.java.net/~zgu/7067266/webrev.00/
> 
> The change looks correct.
> 
> It's not new in this change, but I find it strange that static
> initialization of Decoder is triggered by creating an unused instance.
> It'd be clearer to call Decoder::initialize() instead, and then
> there'd be no need for the comment:
> 
> 	// initialize decoder to decode C frames
> 
> Decoder should also be changed to inherit from AllStatic instead of
> StackObj, but that's getting beyond this bug fix.

This is extremely confusing code. We have static data being manipulated 
by stack-scoped local instances, where the constructor does an 
initialize function and the destructor does an uninitalize that cleans 
up various resources. In addition we have a static function on Windows:

bool Decoder::can_decode_C_frame_in_vm() {
   initialize();
   return  _can_decode_in_vm;
}

which initializes but never cleans up! And the use of these two 
mechanism can be interspersed.

To top it off we create the Decoder instances in places where there is 
absolutely no indication that somewhere down in the call chain the 
Decoder functionality will be used! This is truly awful code.

Volker's patch to move the Decoder into print_C_frame would seem much 
cleaner, but even it doesn't go far enough as we have redundant 
initialize() in can_decode_C_frame_in_vm, and I don't see why we should 
be working with static functions instead of the Decoder object that was 
created.

Zhengyu is concerned that the repeated init/un-init might be too much if 
moved into print_C_frame, but his fix goes to the other extreme by 
giving the Decoder "global" scope in the error reporting process so we 
won't do any cleanup until right at the end (where we will presumably 
terminate the process) - so this seems to defeat the purpose of having 
the cleanup code in the first place.

Given that we are in the process of dying I don't see that we should be 
overly concerned about overhead so I'd be inclined to go with Volker's 
fix and at least do the init and cleanup in the location where the 
Decoder is actually used.

David
-----


> -John
> 
>> -------- Original Message --------
>> Subject: 	Re: Code review request: 7067266 Decoder class not properly 
>> initialized on Windows
>> Date: 	Fri, 15 Jul 2011 11:52:44 -0600
>> From: 	Daniel D. Daugherty <daniel.daugherty at oracle.com>
>> Reply-To: 	daniel.daugherty at oracle.com
>> To: 	Zhengyu Gu <zhengyu.gu at oracle.com>
>> CC: 	hotspot-runtime-dev-confidential 
>> <hotspot-runtime-dev-confidential at sun.com>, volker-simonis at gmail.com
>>
>>
>>
>> Thumbs up on the code.
>>
>> Zhengyu, this came in via hotspot-runtime-dev at openjdk.java.net
>> so this review should go out on that alias also (and the webrev
>> copied to cr.openjdk.java.net)
>>
>> Dan
>>
>>
>> On 7/15/11 11:26 AM, Zhengyu Gu wrote:
>>> This is a simple fix that moves Decoder initialization to step 60 in 
>>> vmError.cpp, so the decoder can decode "Problematic frame:" in hs file 
>>> header.
>>>
>>> The bug was reported by volker-simonis at gmail.com 
>>> <mailto:volker-simonis at gmail.com> from SAP with proposed patch. The 
>>> solution was to move decoder initialization code to
>>> print_C_frame() function, but it is less optimal, since decoder will 
>>> have to be initialized and uninitialized every time in/out of the 
>>> print_C_frame() function.
>>>
>>> CR: http://monaco.sfbay.sun.com/detail.jsf?cr=7067266
>>> Webrev: http://j2se.east.sun.com/~zg131198/7067266/webrev/
>>>
>>> Testcase:
>>>   A simple Java program with JNI call that crashes VM:
>>>
>>> JNIEXPORT void JNICALL Java_jni_1excpt_Main_crashVM (JNIEnv *, jclass) {
>>>   int* p = NULL;
>>>   *p = 1;
>>> }
>>>
>>> Without fix, hs file header:
>>>
>>> #
>>> # A fatal error has been detected by the Java Runtime Environment:
>>> #
>>> #  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x5b431308, pid=7860, 
>>> tid=4140
>>> #
>>> # JRE version: 6.0_23-b05
>>> # Java VM: Java HotSpot(TM) Client VM (19.0-b09 mixed mode, sharing 
>>> windows-x86 )
>>> # Problematic frame:
>>> # C  [mydll.dll+0x11308]
>>> #
>>> # If you would like to submit a bug report, please visit:
>>> # http://java.sun.com/webapps/bugreport/crash.jsp
>>> # The crash happened outside the Java Virtual Machine in native code.
>>> # See problematic frame for where to report the bug.
>>> #
>>>
>>>
>>> With fix:
>>>
>>> #
>>> # A fatal error has been detected by the Java Runtime Environment:
>>> #
>>> #  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x5c981308, pid=1652, 
>>> tid=5964
>>> #
>>> # JRE version: 6.0_23-b05
>>> # Java VM: Java HotSpot(TM) Client VM (21.0-b16-fastdebug mixed mode 
>>> windows-x86 )
>>> # Problematic frame:
>>> # C  [mydll.dll+0x11308]  Java_jni_1excpt_Main_crashVM+0x28
>>> #
>>> # Failed to write core dump. Minidumps are not enabled by default on 
>>> client versions of Windows
>>> #
>>> # If you would like to submit a bug report, please visit:
>>> # http://bugreport.sun.com/bugreport/crash.jsp
>>> # The crash happened outside the Java Virtual Machine in native code.
>>> # See problematic frame for where to report the bug.
>>> #
>>>
>>>
>>> Thanks,
>>>
>>> -Zhengyu