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.
