NPE when compiling ConcurrentLinkedQueue.offer with JITServer and Java11 #8370
Comments
|
Attn: @harryyu1994 |
|
@mpirvu Could you try assigning this issue to me? |
|
I examined the code and I am pretty sure that those two missing break statements need to be there. |
|
Got a simple recreate for the NPE in case it helps in further analysis: Start the jitserver and then run the command: Without |
/**
* Inserts the specified element at the tail of this queue.
* As the queue is unbounded, this method will never return {@code false}.
*
* @return {@code true} (as specified by {@link Queue#offer})
* @throws NullPointerException if the specified element is null
*/
public boolean offer(E e) {
final Node<E> newNode = new Node<E>(Objects.requireNonNull(e));
for (Node<E> t = tail, p = t;;) {
Node<E> q = p.next;
if (q == null) {
// p is last node
if (NEXT.compareAndSet(p, null, newNode)) {
// Successful CAS is the linearization point
// for e to become an element of this queue,
// and for newNode to become "live".
if (p != t) // hop two nodes at a time; failure is OK
TAIL.weakCompareAndSet(this, t, newNode);
return true;
}
// Lost CAS race to another thread; re-read next
}
else if (p == q)
// We have fallen off list. If tail is unchanged, it
// will also be off-list, in which case we need to
// jump to head, from which all live nodes are always
// reachable. Else the new tail is a better bet.
p = (t != (t = tail)) ? t : head;
else
// Check for tail updates after two hops.
p = (p != t && t != (t = tail)) ? t : q;
}
}
/**
* Inserts the specified element at the tail of this queue.
* As the queue is unbounded, this method will never throw
* {@link IllegalStateException} or return {@code false}.
*
* @return {@code true} (as specified by {@link Collection#add})
* @throws NullPointerException if the specified element is null
*/
public boolean add(E e) {
return offer(e);
} |
Reduced it even further import java.util.concurrent.ConcurrentLinkedQueue;
public class TestCase {
public static void main(String args[]) {
ConcurrentLinkedQueue<Method> l = new ConcurrentLinkedQueue<Method>();
l.offer(new Method());
}
}
class Method {
}Code Path On JITServer |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Pre IlGenOpt Trees n46n BBStart <block_4> [0x7f4573005810] bci=[-1,32,361] rc=0 vc=0 vn=- li=- udi=- nc=0
n50n ResolveCHK [#298] [0x7f4573005950] bci=[-1,32,361] rc=0 vc=0 vn=- li=- udi=- nc=1
n49n aload java/util/concurrent/ConcurrentLinkedQueue.NEXT Ljava/lang/invoke/VarHandle;[#370 unresolved notAccessed volatile Static] [flags 0x2307 0x0 ] [0x7f4573005900] bci=[-1,32,361] rc=3 vc=0 vn=- li=- udi=- nc=0
n56n NULLCHK on n49n [#32] [0x7f4573080140] bci=[-1,39,361] rc=0 vc=0 vn=- li=- udi=- nc=1
n55n icalli java/lang/invoke/VarHandleInternal.compareAndSet_impl(Ljava/util/concurrent/ConcurrentLinkedQueue$Node;Ljava/lang/Void;Ljava/util/concurrent/ConcurrentLinkedQueue$Node;)Z[#375 native virtual Method -128] [flags 0x500 0x0 ] () [0x7f45730800f0] bci=[-1,39,361] rc=2 vc=0 vn=- li=- udi=- nc=5 flg=0x20
n54n aloadi <vft-symbol>[#293 Shadow] [flags 0x18607 0x0 ] [0x7f45730800a0] bci=[-1,39,361] rc=1 vc=0 vn=- li=- udi=- nc=1
n49n ==>aload
n49n ==>aload
n51n aload p<auto slot 4>[#367 Auto] [flags 0x7 0x0 ] [0x7f45730059a0] bci=[-1,35,361] rc=1 vc=0 vn=- li=- udi=- nc=0
n52n aconst NULL [0x7f4573080000] bci=[-1,37,361] rc=1 vc=0 vn=- li=- udi=- nc=0
n53n aload newNode<auto slot 2>[#364 Auto] [flags 0x7 0x0 ] [0x7f4573080050] bci=[-1,38,361] rc=1 vc=0 vn=- li=- udi=- nc=0
n60n ificmpeq --> block_18 BBStart at n3n () [0x7f4573080280] bci=[-1,42,361] rc=0 vc=0 vn=- li=- udi=- nc=2 flg=0x20
n55n ==>icalli
n57n iconst 0 [0x7f4573080190] bci=[-1,42,361] rc=1 vc=0 vn=- li=- udi=- nc=0
n47n BBEnd </block_4> ===== [0x7f4573005860] bci=[-1,42,361] rc=0 vc=0 vn=- li=- udi=- nc=0JITServer n46n BBStart <block_4> [0x7f5392862860] bci=[-1,32,361] rc=0 vc=0 vn=- li=- udi=- nc=0
n50n ResolveCHK [#298] [0x7f53928629a0] bci=[-1,32,361] rc=0 vc=0 vn=- li=- udi=- nc=1
n49n aload java/util/concurrent/ConcurrentLinkedQueue.NEXT Ljava/lang/invoke/VarHandle;[#370 unresolved notAccessed volatile Static] [flags 0x82307 0x0 ] [0x7f5392862950] bci=[-1,32,361] rc=2 vc=0 vn=- li=- udi=- nc=0
n56n NULLCHK on n52n [#32] [0x7f53928dd140] bci=[-1,39,361] rc=0 vc=0 vn=- li=- udi=- nc=1
n55n calli java/lang/invoke/VarHandleInternal.compareAndSet_impl(Ljava/lang/Object;)V[#373 native virtual Method -128] [flags 0x500 0x0 ] () [0x7f53928dd0f0] bci=[-1,39,361] rc=1 vc=0 vn=- li=- udi=- nc=3 flg=0x20
n54n aloadi <vft-symbol>[#293 Shadow] [flags 0x18607 0x0 ] [0x7f53928dd0a0] bci=[-1,39,361] rc=1 vc=0 vn=- li=- udi=- nc=1
n52n aconst NULL [0x7f53928dd000] bci=[-1,37,361] rc=2 vc=0 vn=- li=- udi=- nc=0
n52n ==>aconst NULL
n53n aload newNode<auto slot 2>[#364 Auto] [flags 0x7 0x0 ] [0x7f53928dd050] bci=[-1,38,361] rc=1 vc=0 vn=- li=- udi=- nc=0
n60n astore <pending push temp 0>[#374 Auto] [flags 0x7 0x800 ] [0x7f53928dd280] bci=[-1,42,361] rc=0 vc=0 vn=- li=- udi=- nc=1
n49n ==>aload
n63n ificmpeq --> block_18 BBStart at n3n () [0x7f53928dd370] bci=[-1,42,361] rc=0 vc=0 vn=- li=- udi=- nc=2 flg=0x20 |
|
JITServer has wrong number of parameters and return type (it returns void instead of bool). if (NEXT.compareAndSet(p, null, newNode)) |
|
JITServer should have done this but didn't:
if (((TR_ResolvedJ9Method*)resolvedMethod)->isVarHandleAccessMethod())
{
// VarHandle access methods are resolved to *_impl()V, restore their signatures to obtain function correctness in the Walker
J9ROMMethodRef *romMethodRef = (J9ROMMethodRef *)(cp()->romConstantPool + cpIndex);
J9ROMNameAndSignature *nameAndSig = J9ROMFIELDREF_NAMEANDSIGNATURE(romMethodRef);
int32_t signatureLength;
char *signature = utf8Data(J9ROMNAMEANDSIGNATURE_SIGNATURE(nameAndSig), signatureLength);
((TR_ResolvedJ9Method *)resolvedMethod)->setSignature(signature, signatureLength, comp->trMemory());
}
TR_ResolvedMethod *
TR_ResolvedJ9JITServerMethod::getResolvedPossiblyPrivateVirtualMethod(TR::Compilation * comp, I_32 cpIndex, bool ignoreRtResolve, bool * unresolvedInCP)
{
TR_ASSERT(cpIndex != -1, "cpIndex shouldn't be -1");
#if TURN_OFF_INLINING
return 0;
#else
auto compInfoPT = (TR::CompilationInfoPerThreadRemote *) _fe->_compInfoPT;
TR_ResolvedMethod *resolvedMethod = NULL;
if (compInfoPT->getCachedResolvedMethod(compInfoPT->getResolvedMethodKey(TR_ResolvedMethodType::VirtualFromCP, (TR_OpaqueClassBlock *) _ramClass, cpIndex), this,
&resolvedMethod, unresolvedInCP))
{
if (resolvedMethod == NULL)
{
TR::DebugCounter::incStaticDebugCounter(comp, "resources.resolvedMethods/virtual/null");
if (unresolvedInCP)
handleUnresolvedVirtualMethodInCP(cpIndex, unresolvedInCP);
}
return resolvedMethod;
}
// See if the constant pool entry is already resolved or not
if (unresolvedInCP)
*unresolvedInCP = true;
if (!((_fe->_compInfoPT->getClientData()->getRtResolve()) &&
!comp->ilGenRequest().details().isMethodHandleThunk() && // cmvc 195373
performTransformation(comp, "Setting as unresolved virtual call cpIndex=%d\n",cpIndex) ) || ignoreRtResolve)
{
_stream->write(JITServer::MessageType::ResolvedMethod_getResolvedPossiblyPrivateVirtualMethodAndMirror, (TR_ResolvedMethod *) _remoteMirror, literals(), cpIndex);
auto recv = _stream->read<J9Method *, UDATA, bool, TR_ResolvedJ9JITServerMethodInfo>();
J9Method *ramMethod = std::get<0>(recv);
UDATA vTableIndex = std::get<1>(recv);
bool isUnresolvedInCP = std::get<2>(recv);
if (unresolvedInCP)
*unresolvedInCP = isUnresolvedInCP;
bool createResolvedMethod = true;
if (comp->compileRelocatableCode() && ramMethod && comp->getOption(TR_UseSymbolValidationManager))
{
if (!comp->getSymbolValidationManager()->addVirtualMethodFromCPRecord((TR_OpaqueMethodBlock *)ramMethod, cp(), cpIndex))
createResolvedMethod = false;
}
if (vTableIndex)
{
TR_AOTInliningStats *aotStats = NULL;
if (comp->getOption(TR_EnableAOTStats))
aotStats = & (((TR_JitPrivateConfig *)_fe->_jitConfig->privateConfig)->aotStats->virtualMethods);
TR_ResolvedJ9JITServerMethodInfo &methodInfo = std::get<3>(recv);
// call constructor without making a new query
if (createResolvedMethod)
{
resolvedMethod = createResolvedMethodFromJ9Method(comp, cpIndex, vTableIndex, ramMethod, unresolvedInCP, aotStats, methodInfo);
compInfoPT->cacheResolvedMethod(compInfoPT->getResolvedMethodKey(TR_ResolvedMethodType::VirtualFromCP, (TR_OpaqueClassBlock *) _ramClass, cpIndex), (TR_OpaqueMethodBlock *) ramMethod, (uint32_t) vTableIndex, methodInfo);
}
}
}
if (resolvedMethod == NULL)
{
TR::DebugCounter::incStaticDebugCounter(comp, "resources.resolvedMethods/virtual/null");
if (unresolvedInCP)
handleUnresolvedVirtualMethodInCP(cpIndex, unresolvedInCP);
}
else
{
if (((TR_ResolvedJ9Method*)resolvedMethod)->isVarHandleAccessMethod())
{
// VarHandle access methods are resolved to *_impl()V, restore their signatures to obtain function correctness in the Walker
J9ROMConstantPoolItem *cpItem = (J9ROMConstantPoolItem *)romLiterals();
J9ROMMethodRef *romMethodRef = (J9ROMMethodRef *)(cpItem + cpIndex);
int32_t signatureLength = 0;
char *signature = getROMString(signatureLength, romMethodRef,
{
offsetof(J9ROMMethodRef, nameAndSignature),
offsetof(J9ROMNameAndSignature, signature)
});
((TR_ResolvedJ9Method *)resolvedMethod)->setSignature(signature, signatureLength, comp->trMemory());
}
TR::DebugCounter::incStaticDebugCounter(comp, "resources.resolvedMethods/virtual");
TR::DebugCounter::incStaticDebugCounter(comp, "resources.resolvedMethods/virtual:#bytes", sizeof(TR_ResolvedJ9Method));
}
return resolvedMethod;
#endif
}
There are 4 calls to |
|
The issue is confirmed to be resolved method caching related. |
|
I am guessing that we cached a NULL entry meaning the method was unresolved. The assumption was that it's ok to pretend methods are unresolved because this only has a performance implication. This is why the cache only lives for the duration of a compilation. |
|
Some information I've gathered, still a few things confuse me atm. In this case, the signature is supposed to be: if (((TR_ResolvedJ9Method*)resolvedMethod)->isVarHandleAccessMethod())
{
// VarHandle access methods are resolved to *_impl()V, restore their signatures to obtain function correctness in the Walker
J9ROMConstantPoolItem *cpItem = (J9ROMConstantPoolItem *)romLiterals();
J9ROMMethodRef *romMethodRef = (J9ROMMethodRef *)(cpItem + cpIndex);
int32_t signatureLength = 0;
char *signature = getROMString(signatureLength, romMethodRef,
{
offsetof(J9ROMMethodRef, nameAndSignature),
offsetof(J9ROMNameAndSignature, signature)
});
((TR_ResolvedJ9Method *)resolvedMethod)->setSignature(signature, signatureLength, comp->trMemory());
}So now I need to figure out if we can actually cache VarHandle type resolvedMethods. |
|
It turns out we are simply missing an extra step to restore the method signature for VarHandles after we create the resolvedMethod from the cached entry. if (compInfoPT->getCachedResolvedMethod(compInfoPT->getResolvedMethodKey(TR_ResolvedMethodType::VirtualFromCP, (TR_OpaqueClassBlock *) _ramClass, cpIndex), this,
&resolvedMethod, unresolvedInCP))
{
if (resolvedMethod == NULL)
{
TR::DebugCounter::incStaticDebugCounter(comp, "resources.resolvedMethods/virtual/null");
if (unresolvedInCP)
handleUnresolvedVirtualMethodInCP(cpIndex, unresolvedInCP);
}
// needs to restore method signature for VarHandles here !!!
return resolvedMethod;
} |
harryyu1994
added a commit
to harryyu1994/openj9
that referenced
this issue
Jan 24, 2020
After creating the resolvedMethod in TR_ResolvedJ9JITServerMethod::getResolvedPossiblyPrivateVirtualMethod, we forgot to restore method signatures for VarHandle access methods. Fixes: eclipse-openj9#8370 Signed-off-by: Harry Yu <harryyu1994@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The NPE has the following stack trace and is seen in several Java11 sanity tests:
It happens even if we compile with optLevel=noOpt.
A simple command line to reproduce it:
"/home/mpirvu/FullJava11/openj9-openjdk-jdk11/build/linux-x86_64-normal-server-release/images/jdk/bin/java" -Xcompressedrefs -XX:-EnableHCR -Xjit:"count=0,optLevel=noopt,verbose,vlog=vlogutilc.txt,limit={java/util/concurrent/ConcurrentLinkedQueue.offer*},{java/util/concurrent/ConcurrentLinkedQueue.offer*}(traceFull,log=log.txt)" -XX:+UseJITServer -XXjitdirectory=/home/mpirvu/FullJava11/openj9-openjdk-jdk11/.. -cp "/home/mpirvu/FullJava11/openj9-openjdk-jdk11/openj9/test/TKG/../../jvmtest/TestConfig/resources:/home/mpirvu/FullJava11/openj9-openjdk-jdk11/openj9/test/TKG/../TKG/lib/testng.jar:/home/mpirvu/FullJava11/openj9-openjdk-jdk11/openj9/test/TKG/../TKG/lib/jcommander.jar:/home/mpirvu/FullJava11/openj9-openjdk-jdk11/openj9/test/TKG/../../jvmtest/functional/JIT_Test/jitt.jar" org.testng.TestNG -d "/home/mpirvu/FullJava11/openj9-openjdk-jdk11/openj9/test/TKG/../TKG/test_output_15796621567907/StringPeepholeTest_0" "/home/mpirvu/FullJava11/openj9-openjdk-jdk11/openj9/test/TKG/../../jvmtest/functional/JIT_Test/testng.xml" -testnames StringPeepholeTest -groups level.sanity -excludegroups d.*.linux_x86-64_cmprssptrs,d.*.arch.x86,d.*.os.linux,d.*.bits.64,d.*.generic-allThe text was updated successfully, but these errors were encountered: