From 7929d1d111e7cc0a2025b97f59e7e04ea09b3ff4 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Fri, 10 May 2024 07:50:01 +0000 Subject: Revert "Workaround for b/336842546" This reverts commit a4ac01044c50f4da02c40b8da5520d2eb65b41d9. Bug: 336842546 Bug: 73760543 Reason for revert: Fix for the bug has been submitted. CL also has an issue with thread suspension. Change-Id: I06785d58f3e473a13e18876e481fff9118851f53 --- compiler/dex/inline_method_analyser.cc | 5 +--- compiler/optimizing/optimizing_compiler.cc | 8 ++--- dex2oat/linker/oat_writer.cc | 11 ++++--- runtime/class_linker-inl.h | 25 ++++------------ runtime/class_linker.h | 4 +-- runtime/entrypoints/entrypoint_utils-inl.h | 34 ++++++++-------------- .../quick/quick_trampoline_entrypoints.cc | 7 ++--- 7 files changed, 32 insertions(+), 62 deletions(-) diff --git a/compiler/dex/inline_method_analyser.cc b/compiler/dex/inline_method_analyser.cc index 0567fe12c8..85cf83c099 100644 --- a/compiler/dex/inline_method_analyser.cc +++ b/compiler/dex/inline_method_analyser.cc @@ -147,11 +147,8 @@ ArtMethod* GetTargetConstructor(ArtMethod* method, const Instruction* invoke_dir accessor.RegistersSize() - accessor.InsSize()); } uint32_t method_index = invoke_direct->VRegB_35c(); - StackHandleScope<2> hs(Thread::Current()); - Handle h_dex_cache = hs.NewHandle(method->GetDexCache()); - Handle h_class_loader = hs.NewHandle(method->GetClassLoader()); ArtMethod* target_method = Runtime::Current()->GetClassLinker()->LookupResolvedMethod( - method_index, h_dex_cache, h_class_loader); + method_index, method->GetDexCache(), method->GetClassLoader()); if (kIsDebugBuild && target_method != nullptr) { CHECK(!target_method->IsStatic()); CHECK(target_method->IsConstructor()); diff --git a/compiler/optimizing/optimizing_compiler.cc b/compiler/optimizing/optimizing_compiler.cc index a75ac9239b..45d534a9ec 100644 --- a/compiler/optimizing/optimizing_compiler.cc +++ b/compiler/optimizing/optimizing_compiler.cc @@ -1183,14 +1183,14 @@ CompiledMethod* OptimizingCompiler::JniCompile(uint32_t access_flags, const CompilerOptions& compiler_options = GetCompilerOptions(); if (compiler_options.IsBootImage()) { ScopedObjectAccess soa(Thread::Current()); - VariableSizedHandleScope handles(soa.Self()); - ScopedNullHandle class_loader; // null means boot class path loader. - ArtMethod* method = - runtime->GetClassLinker()->LookupResolvedMethod(method_idx, dex_cache, class_loader); + ArtMethod* method = runtime->GetClassLinker()->LookupResolvedMethod( + method_idx, dex_cache.Get(), /*class_loader=*/ nullptr); // Try to compile a fully intrinsified implementation. Do not try to do this for // signature polymorphic methods as the InstructionBuilder cannot handle them; // and it would be useless as they always have a slow path for type conversions. if (method != nullptr && UNLIKELY(method->IsIntrinsic()) && !method->IsSignaturePolymorphic()) { + VariableSizedHandleScope handles(soa.Self()); + ScopedNullHandle class_loader; // null means boot class path loader. Handle compiling_class = handles.NewHandle(method->GetDeclaringClass()); DexCompilationUnit dex_compilation_unit( class_loader, diff --git a/dex2oat/linker/oat_writer.cc b/dex2oat/linker/oat_writer.cc index 7c2f8d6249..68e0690766 100644 --- a/dex2oat/linker/oat_writer.cc +++ b/dex2oat/linker/oat_writer.cc @@ -1837,12 +1837,11 @@ class OatWriter::WriteCodeMethodVisitor : public OrderedMethodVisitor { ArtMethod* GetTargetMethod(const LinkerPatch& patch) REQUIRES_SHARED(Locks::mutator_lock_) { MethodReference ref = patch.TargetMethod(); - StackHandleScope<3> hs(Thread::Current()); - Handle h_dex_cache = hs.NewHandle(GetDexCache(ref.dex_file)); - // We must save both ObjPtr since they might be obsolete after LookupResolvedMethod. - HandleWrapperObjPtr h_original_dex_cache(hs.NewHandleWrapper(&dex_cache_)); - HandleWrapperObjPtr h_class_loader(hs.NewHandleWrapper(&class_loader_)); - ArtMethod* method = class_linker_->LookupResolvedMethod(ref.index, h_dex_cache, h_class_loader); + ObjPtr dex_cache = + (dex_file_ == ref.dex_file) ? dex_cache_ : class_linker_->FindDexCache( + Thread::Current(), *ref.dex_file); + ArtMethod* method = + class_linker_->LookupResolvedMethod(ref.index, dex_cache, class_loader_); CHECK(method != nullptr); return method; } diff --git a/runtime/class_linker-inl.h b/runtime/class_linker-inl.h index 1f5272c7ce..6951e35791 100644 --- a/runtime/class_linker-inl.h +++ b/runtime/class_linker-inl.h @@ -271,31 +271,16 @@ inline bool ClassLinker::CheckInvokeClassMismatch(ObjPtr dex_c } inline ArtMethod* ClassLinker::LookupResolvedMethod(uint32_t method_idx, - Handle dex_cache, - Handle class_loader) { - DCHECK(dex_cache->GetClassLoader() == class_loader.Get()); + ObjPtr dex_cache, + ObjPtr class_loader) { + DCHECK(dex_cache->GetClassLoader() == class_loader); ArtMethod* resolved = dex_cache->GetResolvedMethod(method_idx); if (resolved == nullptr) { const DexFile& dex_file = *dex_cache->GetDexFile(); const dex::MethodId& method_id = dex_file.GetMethodId(method_idx); - ObjPtr klass = - LookupResolvedType(method_id.class_idx_, dex_cache.Get(), class_loader.Get()); - - if (UNLIKELY(klass == nullptr)) { - // We normaly should not end up here. However the verifier currently doesn't guarantee - // the invariant of having the klass in the class table. b/73760543 b/336842546 - klass = ResolveType(method_id.class_idx_, dex_cache, class_loader); - if (UNLIKELY(klass == nullptr)) { - // This can only happen if the current thread is not allowed to load - // classes. - DCHECK(!Thread::Current()->CanLoadClasses()); - DCHECK(Thread::Current()->IsExceptionPending()); - return nullptr; - } - } - + ObjPtr klass = LookupResolvedType(method_id.class_idx_, dex_cache, class_loader); if (klass != nullptr) { - resolved = FindResolvedMethod(klass, dex_cache.Get(), class_loader.Get(), method_idx); + resolved = FindResolvedMethod(klass, dex_cache, class_loader, method_idx); } } return resolved; diff --git a/runtime/class_linker.h b/runtime/class_linker.h index 7f53ca6e93..eabfb4926a 100644 --- a/runtime/class_linker.h +++ b/runtime/class_linker.h @@ -339,8 +339,8 @@ class EXPORT ClassLinker { // Look up a previously resolved method with the given index. ArtMethod* LookupResolvedMethod(uint32_t method_idx, - Handle dex_cache, - Handle class_loader) + ObjPtr dex_cache, + ObjPtr class_loader) REQUIRES_SHARED(Locks::mutator_lock_); // Find a method with the given index from class `klass`, and update the dex cache. diff --git a/runtime/entrypoints/entrypoint_utils-inl.h b/runtime/entrypoints/entrypoint_utils-inl.h index e1efa4a0ac..b0d0ab4b07 100644 --- a/runtime/entrypoints/entrypoint_utils-inl.h +++ b/runtime/entrypoints/entrypoint_utils-inl.h @@ -131,60 +131,50 @@ inline ArtMethod* GetResolvedMethod(ArtMethod* outer_method, ClassLinker* class_linker = Runtime::Current()->GetClassLinker(); ArtMethod* method = outer_method; for (InlineInfo inline_info : inline_infos) { - StackHandleScope<2> hs(Thread::Current()); - Handle h_dex_cache; - Handle h_class_loader; DCHECK(!inline_info.EncodesArtMethod()); DCHECK_NE(inline_info.GetDexPc(), static_cast(-1)); MethodInfo method_info = code_info.GetMethodInfoOf(inline_info); uint32_t method_index = method_info.GetMethodIndex(); const uint32_t dex_file_index = method_info.GetDexFileIndex(); ArtMethod* inlined_method = nullptr; + ObjPtr dex_cache = nullptr; if (method_info.HasDexFileIndex()) { if (method_info.GetDexFileIndexKind() == MethodInfo::kKindBCP) { ArrayRef bcp_dex_files(class_linker->GetBootClassPath()); DCHECK_LT(dex_file_index, bcp_dex_files.size()) << "OOB access to bcp_dex_files. Dumping info: " - << GetResolvedMethodErrorString(class_linker, - inlined_method, - method, - outer_method, - /*dex_cache=*/ nullptr, - method_info); + << GetResolvedMethodErrorString( + class_linker, inlined_method, method, outer_method, dex_cache, method_info); const DexFile* dex_file = bcp_dex_files[dex_file_index]; DCHECK_NE(dex_file, nullptr); - h_dex_cache = hs.NewHandle(class_linker->FindDexCache(Thread::Current(), *dex_file)); + dex_cache = class_linker->FindDexCache(Thread::Current(), *dex_file); } else { ArrayRef oat_dex_files( outer_method->GetDexFile()->GetOatDexFile()->GetOatFile()->GetOatDexFiles()); DCHECK_LT(dex_file_index, oat_dex_files.size()) << "OOB access to oat_dex_files. Dumping info: " - << GetResolvedMethodErrorString(class_linker, - inlined_method, - method, - outer_method, - /*dex_cache=*/ nullptr, - method_info); + << GetResolvedMethodErrorString( + class_linker, inlined_method, method, outer_method, dex_cache, method_info); const OatDexFile* odf = oat_dex_files[dex_file_index]; DCHECK_NE(odf, nullptr); - h_dex_cache = hs.NewHandle(class_linker->FindDexCache(Thread::Current(), *odf)); + dex_cache = class_linker->FindDexCache(Thread::Current(), *odf); } } else { - h_dex_cache = hs.NewHandle(outer_method->GetDexCache()); + dex_cache = outer_method->GetDexCache(); } - h_class_loader = hs.NewHandle(h_dex_cache->GetClassLoader()); - inlined_method = class_linker->LookupResolvedMethod(method_index, h_dex_cache, h_class_loader); + inlined_method = + class_linker->LookupResolvedMethod(method_index, dex_cache, dex_cache->GetClassLoader()); if (UNLIKELY(inlined_method == nullptr)) { LOG(FATAL) << GetResolvedMethodErrorString( - class_linker, inlined_method, method, outer_method, h_dex_cache.Get(), method_info); + class_linker, inlined_method, method, outer_method, dex_cache, method_info); UNREACHABLE(); } DCHECK(!inlined_method->IsRuntimeMethod()); DCHECK_EQ(inlined_method->GetDexFile() == outer_method->GetDexFile(), dex_file_index == MethodInfo::kSameDexFile) << GetResolvedMethodErrorString( - class_linker, inlined_method, method, outer_method, h_dex_cache.Get(), method_info); + class_linker, inlined_method, method, outer_method, dex_cache, method_info); method = inlined_method; } diff --git a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc index 85e5d4e1f4..b53339acd9 100644 --- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc +++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc @@ -1114,10 +1114,9 @@ static void DumpB74410240DebugData(ArtMethod** sp) REQUIRES_SHARED(Locks::mutato caller = WellKnownClasses::java_lang_String_charAt; CHECK_EQ(caller->GetDexMethodIndex(), method_index); } else { - StackHandleScope<2> hs(Thread::Current()); - Handle h_dex_cache(hs.NewHandle(caller->GetDexCache())); - Handle h_class_loader(hs.NewHandle(caller->GetClassLoader())); - caller = class_linker->LookupResolvedMethod(method_index, h_dex_cache, h_class_loader); + ObjPtr dex_cache = caller->GetDexCache(); + ObjPtr class_loader = caller->GetClassLoader(); + caller = class_linker->LookupResolvedMethod(method_index, dex_cache, class_loader); CHECK(caller != nullptr); } } -- cgit v1.2.3-59-g8ed1b