From 9d68a734cc38a60c64430f85b3dda166aeae1f42 Mon Sep 17 00:00:00 2001 From: Nazim Bhuiyan Date: Mon, 30 Oct 2023 14:39:41 -0400 Subject: [PATCH 1/2] Correct the handling of hidden class field comparisions When using jitFieldsAreSame to compare the fields of two distinct hidden classes, the result can be a false positive if the field names and data types match. This can result in disastrous symref sharing for hidden class stores/loads. Hidden classes generated within the same host class do not have distinct class names, but share the same field names with different field data types and offsets. Therefore, name-based check for whether fields are same can result in false positives when it comes to hidden classes unless the fields are from the same j9class objects. Signed-off-by: Nazim Bhuiyan --- runtime/compiler/env/VMJ9.cpp | 11 +++++++++++ runtime/compiler/env/VMJ9.h | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/runtime/compiler/env/VMJ9.cpp b/runtime/compiler/env/VMJ9.cpp index 0cb55e588a6..79905b00c35 100644 --- a/runtime/compiler/env/VMJ9.cpp +++ b/runtime/compiler/env/VMJ9.cpp @@ -6988,6 +6988,17 @@ TR_J9VMBase::jitFieldsAreSame(TR_ResolvedMethod * method1, I_32 cpIndex1, TR_Res TR::VMAccessCriticalSection jitFieldsAreSame(this); bool result = false; + // Hidden classes generated within the same host class do not have distinct class names, + // but share the same field names with different field data types and offsets. Therefore, + // name-based check for whether fields are same can result in false positives when it comes + // to hidden classes unless the fields are from the same j9class objects. + if (method1->classOfMethod() + && method2->classOfMethod() + && (isHiddenClass(method1->classOfMethod()) + || isHiddenClass(method2->classOfMethod())) + && method1->classOfMethod() != method2->classOfMethod()) + return false; + bool sigSame = true; if (method1->fieldsAreSame(cpIndex1, method2, cpIndex2, sigSame)) result = true; diff --git a/runtime/compiler/env/VMJ9.h b/runtime/compiler/env/VMJ9.h index d7bc124f3a3..204cc5fee72 100644 --- a/runtime/compiler/env/VMJ9.h +++ b/runtime/compiler/env/VMJ9.h @@ -1295,6 +1295,15 @@ class TR_J9VMBase : public TR_FrontEnd virtual bool isAnonymousClass(TR_OpaqueClassBlock *j9clazz) { return (J9_ARE_ALL_BITS_SET(((J9Class*)j9clazz)->romClass->extraModifiers, J9AccClassAnonClass)); } virtual bool isAnonymousClass(J9ROMClass *romClass) { return (J9_ARE_ALL_BITS_SET(romClass->extraModifiers, J9AccClassAnonClass)); } + + /** + * \brief Check whether or not the class object is a hidden class + * + * \param j9clazz the class location + * \return true if class is hidden class, false otherwise + */ + virtual bool isHiddenClass(TR_OpaqueClassBlock *j9clazz) { return (J9_ARE_ALL_BITS_SET(((J9Class*)j9clazz)->romClass->extraModifiers, J9AccClassHidden)); } + virtual int64_t getCpuTimeSpentInCompThread(TR::Compilation * comp); // resolution is 0.5 sec or worse. Returns -1 if unavailable virtual void * getClassLoader(TR_OpaqueClassBlock * classPointer); From a6584f8a294f3a3d3b12b863d20abb4c779670f4 Mon Sep 17 00:00:00 2001 From: Nazim Bhuiyan Date: Fri, 3 Nov 2023 15:23:27 -0400 Subject: [PATCH 2/2] Correct handling of hidden class field comparisions in J9ServerVM Using jitFieldsAreSame to check two different fields can yield false positives if the fields belong to two different hidden class instances within the same host class. By checking if the fields belong to hidden classes, we can prevent false positives when dealing with hidden class fields, which will prevent incorrect field shadow symref sharing. This commit includes the following: * Added TR_J9ServerVM override for isHiddenClass * Added check in TR_J9ServerVM::jitFieldsAreSame using this new helper Signed-off-by: Nazim Bhuiyan --- runtime/compiler/env/VMJ9Server.cpp | 26 +++++++++++++++++++++++++- runtime/compiler/env/VMJ9Server.hpp | 1 + 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/runtime/compiler/env/VMJ9Server.cpp b/runtime/compiler/env/VMJ9Server.cpp index c8ec21dbc28..b54109c81e7 100644 --- a/runtime/compiler/env/VMJ9Server.cpp +++ b/runtime/compiler/env/VMJ9Server.cpp @@ -468,7 +468,21 @@ TR_J9ServerVM::jitFieldsAreSame(TR_ResolvedMethod * method1, I_32 cpIndex1, TR_R { bool result = false; bool sigSame = true; - if (method1->fieldsAreSame(cpIndex1, method2, cpIndex2, sigSame)) + TR_OpaqueClassBlock * classOfMethod1 = static_cast(method1)->classOfMethod(); + TR_OpaqueClassBlock * classOfMethod2 = static_cast(method2)->classOfMethod(); + // Hidden classes generated within the same host class do not have distinct class names, + // but share the same field names with different field data types and offsets. Therefore, + // name-based check for whether fields are same can result in false positives when it comes + // to hidden classes unless the fields are from the same j9class objects. + if (classOfMethod1 + && classOfMethod2 + && (isHiddenClass(classOfMethod1) + || isHiddenClass(classOfMethod2)) + && classOfMethod1 != classOfMethod2) + { + result = false; + } + else if (method1->fieldsAreSame(cpIndex1, method2, cpIndex2, sigSame)) { result = true; } @@ -1943,6 +1957,16 @@ TR_J9ServerVM::isAnonymousClass(TR_OpaqueClassBlock *j9clazz) return (J9_ARE_ALL_BITS_SET(extraModifiers, J9AccClassAnonClass)); } +bool +TR_J9ServerVM::isHiddenClass(TR_OpaqueClassBlock *j9clazz) + { + uintptr_t extraModifiers = 0; + JITServer::ServerStream *stream = _compInfoPT->getMethodBeingCompiled()->_stream; + JITServerHelpers::getAndCacheRAMClassInfo((J9Class *)j9clazz, _compInfoPT->getClientData(), stream, JITServerHelpers::CLASSINFO_ROMCLASS_EXTRAMODIFIERS, (void *)&extraModifiers); + + return (J9_ARE_ALL_BITS_SET(extraModifiers, J9AccClassHidden)); + } + TR_IProfiler * TR_J9ServerVM::getIProfiler() { diff --git a/runtime/compiler/env/VMJ9Server.hpp b/runtime/compiler/env/VMJ9Server.hpp index 43abb35ed2f..98c159c4707 100644 --- a/runtime/compiler/env/VMJ9Server.hpp +++ b/runtime/compiler/env/VMJ9Server.hpp @@ -203,6 +203,7 @@ class TR_J9ServerVM: public TR_J9VM virtual bool transformJlrMethodInvoke(J9Method *callerMethod, J9Class *callerClass) override; using TR_J9VM :: isAnonymousClass; virtual bool isAnonymousClass(TR_OpaqueClassBlock *j9clazz) override; + virtual bool isHiddenClass(TR_OpaqueClassBlock *j9clazz) override; virtual TR_IProfiler *getIProfiler() override; virtual TR_StaticFinalData dereferenceStaticFinalAddress(void *staticAddress, TR::DataType addressType) override; virtual void reserveTrampolineIfNecessary( TR::Compilation *, TR::SymbolReference *symRef, bool inBinaryEncoding) override;