Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows
David Holmes
David.Holmes at oracle.com
Wed Jul 20 14:29:38 PDT 2011
Volker Simonis said the following on 07/21/11 01:29:
> I must say that I in general don't like the idea to have a static
> method which is dependent on another static method being called
> before. This is just an implicit dependency which is not documented
> nor enforced anywhere.
>
> Your patch may fix the current problem, but what if somebody will use
> VMError::print_stack_trace() or frame::print_on_error() in the future.
> How can he know that he will have to initialize Decoder first?
I have to agree. The initialization code should be called from the
methods that need it - as is actually done in one Windows case.
Further, if we aren't going to do any cleanup then what is the point of
having uninitialize ? Seems to be dead code.
David
> Regards,
> Volker
>
> On Wed, Jul 20, 2011 at 4:46 PM, Zhengyu Gu <zhengyu.gu at oracle.com> wrote:
>> Hi,
>>
>> Sorry for delay. I am having trouble to upload webrev to
>> cr.openjdk.java.net, probably due to re-ip here.
>>
>> This revision, Decoder class is reverted back to AllStatic.
>> Decoder::initialize() is added at Step 60, and Decoder won't be
>> uninitialized.
>>
>> Here is the link to internal webrev:
>> http://j2se.east.sun.com/~zg131198/7067266/webrev.01/
>>
>> Here is the diff:
>>
>> bash-3.00$ hg diff
>> diff --git a/src/share/vm/utilities/decoder.hpp
>> b/src/share/vm/utilities/decoder.hpp
>> --- a/src/share/vm/utilities/decoder.hpp
>> +++ b/src/share/vm/utilities/decoder.hpp
>> @@ -45,7 +45,7 @@ class ElfFile;
>> #endif // _WINDOWS
>>
>>
>> -class Decoder: public StackObj {
>> +class Decoder: AllStatic {
>>
>> public:
>> // status code for decoding native C frame
>> @@ -61,9 +61,6 @@ class Decoder: public StackObj {
>> };
>>
>> public:
>> - Decoder() { initialize(); };
>> - ~Decoder() { uninitialize(); };
>> -
>> static bool can_decode_C_frame_in_vm();
>>
>> static void initialize();
>> diff --git a/src/share/vm/utilities/vmError.cpp
>> b/src/share/vm/utilities/vmError.cpp
>> --- a/src/share/vm/utilities/vmError.cpp
>> +++ b/src/share/vm/utilities/vmError.cpp
>> @@ -453,6 +453,8 @@ void VMError::report(outputStream* st) {
>> );
>>
>> STEP(60, "(printing problematic frame)")
>> + // initialize decoder to decode C frames
>> + Decoder::initialize();
>>
>> // Print current frame if we have a context (i.e. it's a crash)
>> if (_context) {
>> @@ -565,9 +567,6 @@ void VMError::report(outputStream* st) {
>> // see if it's a valid frame
>> if (fr.pc()) {
>> st->print_cr("Native frames: (J=compiled Java code, j=interpreted,
>> Vv=VM code, C=native code)");
>> -
>> - // initialize decoder to decode C frames
>> - Decoder decoder;
>>
>> int count = 0;
>> while (count++ < StackPrintLimit) {
>>
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>> On 7/18/2011 10:24 AM, Volker Simonis wrote:
>>> On Mon, Jul 18, 2011 at 3:47 PM, Zhengyu Gu<zhengyu.gu at oracle.com> wrote:
>>>> I have to disagree, I would much prefer to revert back it AllStatic
>>>> implementation. Followings are the reasons:
>>>>
>>>> 1. Uninitialize() was intended to shutdown decoder in low memory
>>>> scenarios,
>>>> as the decoder is a secondary function. It may not be very clear in this
>>>> usecase, but it is a scenario in the project I am currently working on.
>>>>
>>> I don't know what project you are working on, but in the Hotspot as we
>>> can see it, the Decoder::decode() functionality is only accessed from
>>> os::dll_address_to_function_name(). This in turn is only called from
>>> two places:
>>> - from the VM error handler (in vmError.cpp) trough
>>> frame::print_on_error() which calls it trough print_C_frame()
>>> - from the FlatProfiler trough trough FlatProfiler::record_vm_tick()
>>>
>>>> 2. Performance: Initial/uninitial decoder for decoding every symbol,
>>>> definitely not optimal. In Unix implementation, it was programmed to
>>>> maintain a symbol cache, which will not completely defeated, as it has to
>>>> reopen/reparse elf file to decoder every frame. Again, decoder
>>>> performance
>>>> will be one of key factors for my other project.
>>> I didn't propose to create a new Decoder object for the decoding of
>>> each symbol. Instead a decoder object could be passed into
>>> os::dll_address_to_function_name() and frame::print_on_error(). The
>>> user of the functionality could then decide how long he wants to keep
>>> the Decoder object and its resources alive. In the case of the
>>> FlatProfiler this would probably be the whole VM lifetime, for the
>>> vmError use case it would be probably for the time of a complete stack
>>> trace.
>>>
>>>> I will post new webrev to reflect AllStatic implementation.
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>> On 7/18/2011 5:25 AM, Volker Simonis wrote:
>>>>> 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
>>
More information about the hotspot-runtime-dev
mailing list