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

Volker Simonis volker.simonis at gmail.com
Wed Jul 20 08:29:01 PDT 2011 

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?

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