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

[Volker Simonis]

What about making the Decoder::decode() and all the other methods in
the Decoder class instance methods?
Then it would be clear that users of the decode() method would have to
create (and initialize) a Decode object and it would be their
responsibility when they create it and and when they uninitialize and
clean it up. Depending on the platform, the Decoder class may still
decide to keep its resources globally (as static data members) or just
for the lifetime of a Decoder object.

With the current solution the problem is (as pointed out by David)
that it is not clear for callers if they have to do initialization or
not, and this even differs between Unix and Windows.

@Zhengyu: will you prepare a new webrev or should I submit one?

Regards,
Vovlker

On Sat, Jul 16, 2011 at 12:59 AM, David Holmes <David.Holmes at oracle.com> wrote:
> 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
>