On 1 okt 2013, at 15:15, David Holmes <david.hol...@oracle.com> wrote:
> On 23/09/2013 2:41 AM, Staffan Larsen wrote:
>> Dmitry: Thanks for the review.
>>
>> Yasumasa: Thanks for your contribution! I have pushed the changes into HS25
>> and will push them to 7u60 as well.
>
> I've been on vacation so could not respond in time. I am concerned about this
> fix. Other than printing a message on the console it pretends that the init
> has succeeded! That seems wrong to me. What are the consequences of doing
> this?
The change is an improvement to the previous error message, but looking at the
code again there is similar code in the block below. That code calls
vm_exit_during_initialization() if the init failed. The only problem with that
is that AttachListener::init() can be called not just during initialization, so
it seems a bit drastic to kill the JVM if the init fails.
In the case where AttachListener::init() is called during initialization it
makes sense to kill the VM if AttachListener::init() fails, in other cases an
error message would be better. We now have an ugly combination...
If AttachListener::init() fails during runtime, I think it has no other
consequences than attach operations not working.
/Staffan
>
> David
> -----
>
>> /Staffan
>>
>> On 22 sep 2013, at 01:51, Dmitry Samersoff <dmitry.samers...@oracle.com>
>> wrote:
>>
>>> Yasumasa,
>>>
>>> Looks good for me.
>>>
>>> -Dmitry
>>>
>>> On 2013-09-21 14:43, Yasumasa Suenaga wrote:
>>>> Hi Staffan,
>>>>
>>>>> Can you update your patch, please?
>>>>
>>>> Of course!
>>>> I've attached new patch in this email.
>>>>
>>>> Could you check this?
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>> On 2013/09/21 15:36, Staffan Larsen wrote:
>>>>>
>>>>>
>>>>>> On 20 sep 2013, at 16:49, Yasumasa Suenaga<y...@ysfactory.dip.jp>
>>>>>> wrote:
>>>>>>
>>>>>>> On 2013/09/20 23:34, Staffan Larsen wrote:
>>>>>>>
>>>>>>>> On 20 sep 2013, at 16:24, Yasumasa Suenaga<y...@ysfactory.dip.jp>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> I thought your code too. However...
>>>>>>>>
>>>>>>>> - These code is different from other code (rule?).
>>>>>>>
>>>>>>> Well, you are introducing a new macro that is also different from
>>>>>>> other code, so I'm not sure how valid that argument is.
>>>>>>
>>>>>> My macro is modified from "CATCH" in exceptions.hpp:
>>>>>>
>>>>>> #define CATCH \
>>>>>> THREAD); if (HAS_PENDING_EXCEPTION) { \
>>>>>> oop ex = PENDING_EXCEPTION; \
>>>>>> CLEAR_PENDING_EXCEPTION; \
>>>>>> ex->print(); \
>>>>>> ShouldNotReachHere(); \
>>>>>> } (void)(0
>>>>>>
>>>>>> So I think that my macro is not big difference fromother code.
>>>>>>
>>>>>>
>>>>>>>> - Similar crash cases exist. e.g. 6425580 and 7142442.
>>>>>>>> These crashes are different from 6989981. However, I guess that
>>>>>>>> crashed
>>>>>>>> thread had pending exception and we need to fix with similar patch.
>>>>>>>>
>>>>>>>> So I think that new macro is useful later.
>>>>>>>
>>>>>>> Yes, similar problems may come up in other cases as well.
>>>>>>>
>>>>>>> Generally, I don't think it's a good idea to have logging calls
>>>>>>> hidden away in general macros. What we really should do here is
>>>>>>> print some context around the stack trace as well. Something like:
>>>>>>>
>>>>>>> Initializing the attach listener failed with the following
>>>>>>> exception in AttachListener::init when initializing the thread_oop:
>>>>>>>
>>>>>>> This would be possible with the code I suggested, but very hard in a
>>>>>>> general macro.
>>>>>>
>>>>>> Agree.
>>>>>> Should we write code as following?
>>>>>>
>>>>>> if (HAS_PENDING_EXCEPTION) {
>>>>>> tty->print_cr("Exception in VM (AttachListener::init) : ");
>>>>>> java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>>>>>> CLEAR_PENDING_EXCEPTION;
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> I like this way :-)
>>>>>
>>>>> Yes, this is what I'd like to see! Can you update your patch, please?
>>>>>
>>>>> Thanks,
>>>>> /Staffan
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> /Staffan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>> On 2013/09/20 23:05, Staffan Larsen wrote:
>>>>>>>>> I see. CHECK_AND_CLEAR_AND_PRINT? Just kidding... :-)
>>>>>>>>>
>>>>>>>>> Maybe in this case we should not have this as a macro, but
>>>>>>>>> actually add the code after the two calls to call_special?
>>>>>>>>> Something like the code below. I personally think this is more
>>>>>>>>> readable than obscure macros that I have to go look up to
>>>>>>>>> understand what they do.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> /Staffan
>>>>>>>>>
>>>>>>>>> JavaCalls::call_special(&result, thread_oop,
>>>>>>>>> klass,
>>>>>>>>> vmSymbols::object_initializer_name(),
>>>>>>>>>
>>>>>>>>> vmSymbols::threadgroup_string_void_signature(),
>>>>>>>>> thread_group,
>>>>>>>>> string,
>>>>>>>>> THREAD);
>>>>>>>>>
>>>>>>>>> if (HAS_PENDING_EXCEPTION) {
>>>>>>>>> java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>>>>>>>>> CLEAR_PENDING_EXCEPTION;
>>>>>>>>> return;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> KlassHandle group(THREAD, SystemDictionary::ThreadGroup_klass());
>>>>>>>>> JavaCalls::call_special(&result,
>>>>>>>>> thread_group,
>>>>>>>>> group,
>>>>>>>>> vmSymbols::add_method_name(),
>>>>>>>>> vmSymbols::thread_void_signature(),
>>>>>>>>> thread_oop, // ARG 1
>>>>>>>>> THREAD);
>>>>>>>>>
>>>>>>>>> if (HAS_PENDING_EXCEPTION) {
>>>>>>>>> java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>>>>>>>>> CLEAR_PENDING_EXCEPTION;
>>>>>>>>> return;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 20 sep 2013, at 15:53, Yasumasa
>>>>>>>>>> Suenaga<y...@ysfactory.dip.jp> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Staffan,
>>>>>>>>>>
>>>>>>>>>> Thank you for your sponsoring!
>>>>>>>>>>
>>>>>>>>>> "CHECK_AND_CLEAR" is already defined in exceptions.hpp:
>>>>>>>>>> ******************
>>>>>>>>>> #define CHECK_AND_CLEAR THREAD); if
>>>>>>>>>> (HAS_PENDING_EXCEPTION) { CLEAR_PENDING_EXCEPTION; return;
>>>>>>>>>> } (void)(0
>>>>>>>>>> ******************
>>>>>>>>>>
>>>>>>>>>> I think that user wants why serviceability tools are failed.
>>>>>>>>>> So I defined "CHECK_AND_CLEAR" + java_lang_Throwable::print() as
>>>>>>>>>> "CATCH_AND_RETURN" .
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I agree rename this macro.
>>>>>>>>>> Do you have any idea? I don't have a good name :-)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>> On 2013/09/20 20:10, Staffan Larsen wrote:
>>>>>>>>>>> Yasuma,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for finding and fixing this! I have re-opened the bug.
>>>>>>>>>>> Your patch looks good to me, but perhaps CATCH_AND_RETURN should
>>>>>>>>>>> be renamed CHECK_AND_CLEAR?
>>>>>>>>>>>
>>>>>>>>>>> I can sponsor the fix.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> /Staffan
>>>>>>>>>>>
>>>>>>>>>>>> On 20 sep 2013, at 12:41, Yasumasa
>>>>>>>>>>>> Suenaga<y...@ysfactory.dip.jp> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> I encountered this bug:
>>>>>>>>>>>> JDK-6989981: jstack causes "fatal error: ExceptionMark
>>>>>>>>>>>> destructor expects no pending exceptions"
>>>>>>>>>>>>
>>>>>>>>>>>> I read hs_err and attachListener.cpp, Java heap usage is very
>>>>>>>>>>>> high and
>>>>>>>>>>>> it could be OutOfMemoryError in AttachListener::init() .
>>>>>>>>>>>>
>>>>>>>>>>>> If JavaCalls::call_special() in AttachListener::init() fail
>>>>>>>>>>>> which is
>>>>>>>>>>>> caused by OOME, d'tor of EXCEPTION_MARK (~ExceptionMark) will
>>>>>>>>>>>> generate
>>>>>>>>>>>> internal error.
>>>>>>>>>>>>
>>>>>>>>>>>> So I make a patch for avoiding crash and attached in this email
>>>>>>>>>>>> (6989981.patch) .
>>>>>>>>>>>> I'd like to re-open this bug and contribute my patch.
>>>>>>>>>>>> Could you help me?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --- DETAILS ---
>>>>>>>>>>>> CHECK macro is used in JavaCalls::call_special() .
>>>>>>>>>>>>
>>>>>>>>>>>> ******************
>>>>>>>>>>>> void AttachListener::init() {
>>>>>>>>>>>> EXCEPTION_MARK;
>>>>>>>>>>>>
>>>>>>>>>>>> :
>>>>>>>>>>>>
>>>>>>>>>>>> // Initialize thread_oop to put it into the system threadGroup
>>>>>>>>>>>> Handle thread_group (THREAD, Universe::system_thread_group());
>>>>>>>>>>>> JavaValue result(T_VOID);
>>>>>>>>>>>> JavaCalls::call_special(&result, thread_oop,
>>>>>>>>>>>> klass,
>>>>>>>>>>>> vmSymbols::object_initializer_name(),
>>>>>>>>>>>>
>>>>>>>>>>>> vmSymbols::threadgroup_string_void_signature(),
>>>>>>>>>>>> thread_group,
>>>>>>>>>>>> string,
>>>>>>>>>>>> CHECK);
>>>>>>>>>>>>
>>>>>>>>>>>> KlassHandle group(THREAD,
>>>>>>>>>>>> SystemDictionary::ThreadGroup_klass());
>>>>>>>>>>>> JavaCalls::call_special(&result,
>>>>>>>>>>>> thread_group,
>>>>>>>>>>>> group,
>>>>>>>>>>>> vmSymbols::add_method_name(),
>>>>>>>>>>>> vmSymbols::thread_void_signature(),
>>>>>>>>>>>> thread_oop, // ARG 1
>>>>>>>>>>>> CHECK);
>>>>>>>>>>>> ******************
>>>>>>>>>>>>
>>>>>>>>>>>> CHECK macro does not clear pending exception of current thread.
>>>>>>>>>>>> So call_special() fails with runtime exception, d'tor of
>>>>>>>>>>>> ExceptionMark
>>>>>>>>>>>> generates fatal error.
>>>>>>>>>>>>
>>>>>>>>>>>> ******************
>>>>>>>>>>>> ExceptionMark::~ExceptionMark() {
>>>>>>>>>>>> if (_thread->has_pending_exception()) {
>>>>>>>>>>>> Handle exception(_thread, _thread->pending_exception());
>>>>>>>>>>>> _thread->clear_pending_exception(); // Needed to avoid
>>>>>>>>>>>> infinite recursion
>>>>>>>>>>>> if (is_init_completed()) {
>>>>>>>>>>>> exception->print();
>>>>>>>>>>>> fatal("ExceptionMark destructor expects no pending
>>>>>>>>>>>> exceptions");
>>>>>>>>>>>> } else {
>>>>>>>>>>>> vm_exit_during_initialization(exception);
>>>>>>>>>>>> }
>>>>>>>>>>>> }
>>>>>>>>>>>> }
>>>>>>>>>>>> ******************
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --- HOW TO REPRODUCE ---
>>>>>>>>>>>> I also crate testcase of this issue (testcase.tar.gz) . This
>>>>>>>>>>>> testcase contains
>>>>>>>>>>>> two modules.
>>>>>>>>>>>>
>>>>>>>>>>>> - jvmti: JVMTI agent for this issue. This agent traps SIGQUIT and
>>>>>>>>>>>> calls original (in HotSpot) SIGQUIT handler.
>>>>>>>>>>>> This signal handler is invoked, MethodEntry event
>>>>>>>>>>>> callback is
>>>>>>>>>>>> enabled. MethodEntry generates OutOfMemoryError.
>>>>>>>>>>>>
>>>>>>>>>>>> - java : Simple long sleep program.
>>>>>>>>>>>>
>>>>>>>>>>>> I can run this testcase in Fedora18 x86_64. See below.
>>>>>>>>>>>>
>>>>>>>>>>>> -------
>>>>>>>>>>>> $ javac java/LongSleep.java
>>>>>>>>>>>> $ make -C jvmti
>>>>>>>>>>>> make: Entering directory
>>>>>>>>>>>> `/data/share/patch/ExceptionMark/testcase/jvmti'
>>>>>>>>>>>> gcc -I/usr/lib/jvm/java-openjdk/include
>>>>>>>>>>>> -I/usr/lib/jvm/java-openjdk/include/linux -fPIC -c oome.c
>>>>>>>>>>>> gcc -shared -o liboome.so oome.o
>>>>>>>>>>>> make: Leaving directory
>>>>>>>>>>>> `/data/share/patch/ExceptionMark/testcase/jvmti'
>>>>>>>>>>>> $ export JAVA_HOME=</path/to/jre>
>>>>>>>>>>>> $ ./test.sh
>>>>>>>>>>>> -------
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>> <6989981.patch><testcase.tar.gz>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Dmitry Samersoff
>>> Oracle Java development team, Saint Petersburg, Russia
>>> * I would love to change the world, but they won't give me the source code.
>>
