Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows
Zhengyu Gu
zhengyu.gu at oracle.com
Wed Jul 20 07:46:36 PDT 2011
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