On 2/10/2013 12:11 AM, Yasumasa Suenaga wrote:
Hi David,
Other than printing a message on the console it pretends that the init
has succeeded!
No.
signal_thread_entry() @ hotspot/src/share/vm/runtime/os.cpp
--------
250 case SIGBREAK: {
251 // Check if the signal is a trigger to start the
Attach Listener - in that
252 // case don't print stack traces.
253 if (!DisableAttachMechanism &&
AttachListener::is_init_trigger()) {
254 continue;
255 }
--------
AttachListener::init() is called through is_init_trigger() (line 505).
AttachListener::is_init_trigger() @
hotspot/src/share/vm/services/attachListener.cpp
--------
501 if (ret == 0) {
502 // simple check to avoid starting the attach mechanism when
503 // a bogus user creates the file
504 if (st.st_uid == geteuid()) {
505 init();
506 return true;
507 }
508 }
--------
If exception occurs in AttachListner::init(),
AttachListner::is_init_trigger() returns "true" .
Process of SIGBREAK handler will be stopped (caused by continue
statement) and signal_thread_entry()
will return top of loop.
In my patch, pending exception which occurs in AttachListener
initialization is cleared.
Thus Signal Dispatcher has no damage.
But as I said the init() fails and we pretend it succeeded with respect
to the caller. Now for the signal case if the init fails we can simply
do nothing - the VM is running, this particular action failed, so the VM
continues - that's okay. So I'm okay with the fix in that regard.
In general I think AttachListener::init should return a boolean to
indicate success or failure so that the caller can decide what needs to
be done eg at VM startup it seems reasonable to call
vm_exit_during_initialization because we can't pre-start the listener as
we are required to do.
Though as Staffan pointed out init() incorrectly calls
vm_exit_during_initialization in other places. :(
What are the consequences of doing this?
If AttachListener::init() cannot work due to exception, it cannot create
UNIX domain socket to accept from java tools (e.g. jstack) .
Target VM which tried to invoke AttachListner continues to run.
(However, runtime exception such as OOME occurs, target VM will be
stopped immediately :-)
On the other hand, java tools which tried to attach target VM will fail
because it cannot connect UNIX domain socket.
Will the tool fail differently now compared to the VM aborting? Purely
from a testing perspective we tend to notice VM aborts but the console
print about the exception may well go unnoticed.
Thanks,
David
Please run my testcase. It's actual behavior.
Thanks,
Yasumasa
On 2013/10/01 22:15, David Holmes 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?
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.
