From 0237ac82184e8123dcb24151e273a63a514a3283 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Sat, 1 Mar 2025 01:26:25 +0000 Subject: Pass the dex register array to NterpGetInstanceField. To lookup the field in the holder's class. This reduces by half the number of slow lookups. Also do a dex cache lookup in the fast path. This reduces by 10% the number of slow lookups. Test: test.py Change-Id: If9ae991a4e5b82e45ea961b9a777be9f4dc6df2d --- runtime/interpreter/interpreter_common.h | 17 +++++++---- runtime/interpreter/interpreter_switch_impl-inl.h | 6 ++-- runtime/interpreter/mterp/arm64ng/object.S | 2 ++ runtime/interpreter/mterp/armng/main.S | 15 +++++++++- runtime/interpreter/mterp/nterp.cc | 35 ++++++++++++++++++++--- runtime/interpreter/mterp/riscv64/object.S | 4 +++ runtime/interpreter/mterp/x86_64ng/main.S | 4 +++ runtime/interpreter/mterp/x86ng/main.S | 22 +++++++++++++- 8 files changed, 91 insertions(+), 14 deletions(-) diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h index ce6c412ba8..89900671b8 100644 --- a/runtime/interpreter/interpreter_common.h +++ b/runtime/interpreter/interpreter_common.h @@ -290,22 +290,29 @@ LIBART_PROTECTED extern "C" uint32_t NterpGetInstanceFieldOffset(Thread* self, ArtMethod* caller, const uint16_t* dex_pc_ptr, - size_t resolve_field_type); + size_t resolve_field_type, + uint32_t* registers); static inline void GetFieldInfo(Thread* self, - ArtMethod* caller, + ShadowFrame& shadow_frame, const uint16_t* dex_pc_ptr, bool is_static, bool resolve_field_type, ArtField** field, bool* is_volatile, - MemberOffset* offset) { + MemberOffset* offset) + REQUIRES_SHARED(Locks::mutator_lock_) { size_t tls_value = 0u; if (!self->GetInterpreterCache()->Get(self, dex_pc_ptr, &tls_value)) { if (is_static) { - tls_value = NterpGetStaticField(self, caller, dex_pc_ptr, resolve_field_type); + tls_value = NterpGetStaticField( + self, shadow_frame.GetMethod(), dex_pc_ptr, resolve_field_type); } else { - tls_value = NterpGetInstanceFieldOffset(self, caller, dex_pc_ptr, resolve_field_type); + tls_value = NterpGetInstanceFieldOffset(self, + shadow_frame.GetMethod(), + dex_pc_ptr, + resolve_field_type, + shadow_frame.GetVRegAddr(0)); } if (self->IsExceptionPending()) { diff --git a/runtime/interpreter/interpreter_switch_impl-inl.h b/runtime/interpreter/interpreter_switch_impl-inl.h index 7915a10094..791e9df04d 100644 --- a/runtime/interpreter/interpreter_switch_impl-inl.h +++ b/runtime/interpreter/interpreter_switch_impl-inl.h @@ -73,7 +73,7 @@ ALWAYS_INLINE bool DoFieldGet(Thread* self, MemberOffset offset(0u); bool is_volatile; GetFieldInfo(self, - shadow_frame.GetMethod(), + shadow_frame, reinterpret_cast(inst), is_static, /*resolve_field_type=*/ false, @@ -157,7 +157,7 @@ ALWAYS_INLINE bool DoFieldGet(Thread* self, // Returns true on success, otherwise throws an exception and returns false. template ALWAYS_INLINE bool DoFieldPut(Thread* self, - const ShadowFrame& shadow_frame, + ShadowFrame& shadow_frame, const Instruction* inst, uint16_t inst_data, const instrumentation::Instrumentation* instrumentation) @@ -172,7 +172,7 @@ ALWAYS_INLINE bool DoFieldPut(Thread* self, MemberOffset offset(0u); bool is_volatile; GetFieldInfo(self, - shadow_frame.GetMethod(), + shadow_frame, reinterpret_cast(inst), is_static, resolve_field_type, diff --git a/runtime/interpreter/mterp/arm64ng/object.S b/runtime/interpreter/mterp/arm64ng/object.S index ade39d5e89..f51ab6026c 100644 --- a/runtime/interpreter/mterp/arm64ng/object.S +++ b/runtime/interpreter/mterp/arm64ng/object.S @@ -193,6 +193,7 @@ ldr x1, [sp] mov x2, xPC mov x3, #0 + mov x4, xFP EXPORT_PC bl nterp_get_instance_field_offset // Zero extension (nterp_get_instance_field_offset returns uint32_t) of the return value is @@ -275,6 +276,7 @@ .else mov x3, #0 .endif + mov x4, xFP EXPORT_PC bl nterp_get_instance_field_offset // Zero extension (nterp_get_instance_field_offset returns uint32_t) of the return value is diff --git a/runtime/interpreter/mterp/armng/main.S b/runtime/interpreter/mterp/armng/main.S index 5298840f92..38f7647dd2 100644 --- a/runtime/interpreter/mterp/armng/main.S +++ b/runtime/interpreter/mterp/armng/main.S @@ -1950,7 +1950,6 @@ END nterp_f2l_doconv // Entrypoints into runtime. NTERP_TRAMPOLINE nterp_get_static_field, NterpGetStaticField -NTERP_TRAMPOLINE nterp_get_instance_field_offset, NterpGetInstanceFieldOffset NTERP_TRAMPOLINE nterp_filled_new_array, NterpFilledNewArray NTERP_TRAMPOLINE nterp_filled_new_array_range, NterpFilledNewArrayRange NTERP_TRAMPOLINE nterp_get_class, NterpGetClass @@ -1959,6 +1958,20 @@ NTERP_TRAMPOLINE nterp_get_method, NterpGetMethod NTERP_TRAMPOLINE nterp_hot_method, NterpHotMethod NTERP_TRAMPOLINE nterp_load_object, NterpLoadObject +ENTRY nterp_get_instance_field_offset + SETUP_SAVE_REFS_ONLY_FRAME ip + INCREASE_FRAME 8 + str rFP, [sp] + bl NterpGetInstanceFieldOffset + DECREASE_FRAME 8 + RESTORE_SAVE_REFS_ONLY_FRAME + REFRESH_MARKING_REGISTER + ldr ip, [rSELF, #THREAD_EXCEPTION_OFFSET] @ Get exception field. + cmp ip, #0 + bne nterp_deliver_pending_exception + bx lr +END nterp_get_instance_field_offset + ENTRY nterp_deliver_pending_exception DELIVER_PENDING_EXCEPTION END nterp_deliver_pending_exception diff --git a/runtime/interpreter/mterp/nterp.cc b/runtime/interpreter/mterp/nterp.cc index 95cfe7fb7d..f08f70143b 100644 --- a/runtime/interpreter/mterp/nterp.cc +++ b/runtime/interpreter/mterp/nterp.cc @@ -391,8 +391,12 @@ extern "C" size_t NterpGetMethod(Thread* self, ArtMethod* caller, const uint16_t } } +template ALWAYS_INLINE FLATTEN -static ArtField* FindFieldFast(ArtMethod* caller, uint16_t field_index) +static ArtField* FindFieldFast(ArtMethod* caller, + uint16_t field_index, + uint32_t* registers, + const Instruction* inst) REQUIRES_SHARED(Locks::mutator_lock_) { if (caller->IsObsolete()) { return nullptr; @@ -405,6 +409,28 @@ static ArtField* FindFieldFast(ArtMethod* caller, uint16_t field_index) return cls->FindDeclaredField(field_index); } + if (!kStatic) { + mirror::Object* obj = reinterpret_cast32(registers[inst->VRegB_22c()]); + if (obj != nullptr) { + mirror::Class* obj_cls = obj->GetClass(); + if (obj_cls->GetDexTypeIndex() == field_id.class_idx_ && + obj_cls->GetDexCache() == cls->GetDexCache() && + obj_cls->IsPublic()) { + ArtField* resolved_field = obj_cls->FindDeclaredField(field_index); + if (caller->SkipAccessChecks() || + (resolved_field != nullptr && resolved_field->IsPublic())) { + return resolved_field; + } + } + } + } + + ArtField* field = caller->GetDexCache()->GetResolvedField(field_index); + if (caller->SkipAccessChecks() || + (field != nullptr && field->IsPublic() && field->GetDeclaringClass()->IsPublic())) { + return field; + } + return nullptr; } @@ -435,7 +461,7 @@ extern "C" size_t NterpGetStaticField(Thread* self, uint16_t field_index = inst->VRegB_21c(); Instruction::Code opcode = inst->Opcode(); - ArtField* resolved_field = FindFieldFast(caller, field_index); + ArtField* resolved_field = FindFieldFast(caller, field_index, nullptr, inst); if (resolved_field == nullptr || !resolved_field->IsStatic()) { resolved_field = FindFieldSlow( self, caller, field_index, /*is_static=*/ true, IsInstructionSPut(opcode)); @@ -492,13 +518,14 @@ LIBART_PROTECTED extern "C" uint32_t NterpGetInstanceFieldOffset(Thread* self, ArtMethod* caller, const uint16_t* dex_pc_ptr, - size_t resolve_field_type) // Resolve if not zero + size_t resolve_field_type, // Resolve if not zero + uint32_t* registers) REQUIRES_SHARED(Locks::mutator_lock_) { const Instruction* inst = Instruction::At(dex_pc_ptr); uint16_t field_index = inst->VRegC_22c(); Instruction::Code opcode = inst->Opcode(); - ArtField* resolved_field = FindFieldFast(caller, field_index); + ArtField* resolved_field = FindFieldFast(caller, field_index, registers, inst); if (resolved_field == nullptr || resolved_field->IsStatic()) { resolved_field = FindFieldSlow( self, caller, field_index, /*is_static=*/ false, IsInstructionIPut(opcode)); diff --git a/runtime/interpreter/mterp/riscv64/object.S b/runtime/interpreter/mterp/riscv64/object.S index 43bb31f1e9..ebaf550932 100644 --- a/runtime/interpreter/mterp/riscv64/object.S +++ b/runtime/interpreter/mterp/riscv64/object.S @@ -290,6 +290,7 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, zero + mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset @@ -367,6 +368,7 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, zero + mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset @@ -452,6 +454,7 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, zero + mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset @@ -535,6 +538,7 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, $value + mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset diff --git a/runtime/interpreter/mterp/x86_64ng/main.S b/runtime/interpreter/mterp/x86_64ng/main.S index 80753c4fc2..7126c9f5ae 100644 --- a/runtime/interpreter/mterp/x86_64ng/main.S +++ b/runtime/interpreter/mterp/x86_64ng/main.S @@ -1547,6 +1547,7 @@ END_FUNCTION \name movq 0(%rsp), %rsi movq rPC, %rdx movq $$0, %rcx + movq rFP, %r8 call nterp_get_instance_field_offset testl %eax, %eax jns 1b @@ -1581,6 +1582,7 @@ END_FUNCTION \name movq 0(%rsp), %rsi movq rPC, %rdx movq $$0, %rcx + movq rFP, %r8 call nterp_get_instance_field_offset testl %eax, %eax jns 1b @@ -1944,6 +1946,7 @@ NterpPutObjectInstanceField: movq 0(%rsp), %rsi movq rPC, %rdx // %rcx is already set. + movq rFP, %r8 call nterp_get_instance_field_offset // Reload the value as it may have moved. GET_VREG %ecx, %rbp # ecx <- v[A] @@ -1987,6 +1990,7 @@ NterpGetObjectInstanceField: movq 0(%rsp), %rsi movq rPC, %rdx movq $$0, %rcx + movq rFP, %r8 call nterp_get_instance_field_offset testl %eax, %eax jns 1b diff --git a/runtime/interpreter/mterp/x86ng/main.S b/runtime/interpreter/mterp/x86ng/main.S index d2f4271f99..dabdd4233a 100644 --- a/runtime/interpreter/mterp/x86ng/main.S +++ b/runtime/interpreter/mterp/x86ng/main.S @@ -2321,7 +2321,6 @@ SYMBOL(EndExecuteNterpImpl): // Entrypoints into runtime. NTERP_TRAMPOLINE nterp_get_static_field, NterpGetStaticField -NTERP_TRAMPOLINE nterp_get_instance_field_offset, NterpGetInstanceFieldOffset NTERP_TRAMPOLINE nterp_filled_new_array, NterpFilledNewArray NTERP_TRAMPOLINE nterp_filled_new_array_range, NterpFilledNewArrayRange NTERP_TRAMPOLINE nterp_get_class, NterpGetClass @@ -2330,6 +2329,27 @@ NTERP_TRAMPOLINE nterp_get_method, NterpGetMethod NTERP_TRAMPOLINE nterp_hot_method, NterpHotMethod NTERP_TRAMPOLINE nterp_load_object, NterpLoadObject +// Special case nterp_get_instance_field_offset as it requires 5 arguments. +DEFINE_FUNCTION nterp_get_instance_field_offset + movd %ebx, %xmm0 + SETUP_SAVE_REFS_ONLY_FRAME ebx + movd %xmm0, %ebx + INCREASE_FRAME 12 + PUSH_ARG edi + PUSH_ARG ebx + PUSH_ARG edx + PUSH_ARG ecx + PUSH_ARG eax + call NterpGetInstanceFieldOffset + DECREASE_FRAME 32 + RESTORE_IBASE_WITH_CFA + FETCH_INST_CLEAR_OPCODE + RESTORE_SAVE_REFS_ONLY_FRAME + cmpl LITERAL(0), %fs:THREAD_EXCEPTION_OFFSET + jne nterp_deliver_pending_exception + ret +END_FUNCTION nterp_get_instance_field_offset + DEFINE_FUNCTION nterp_deliver_pending_exception DELIVER_PENDING_EXCEPTION END_FUNCTION nterp_deliver_pending_exception -- cgit v1.2.3-59-g8ed1b From 1cdb151ce10e05140f59e184d3dc8896d3ec8434 Mon Sep 17 00:00:00 2001 From: Lokesh Gidra Date: Sun, 9 Mar 2025 08:47:14 +0000 Subject: Ensure for any object in old-gen its class is also in there Like in black-dense case, during young collections also we need to make sure that for any object in old-gen, its class is also in old-gen. Otherwise, we run the risk that if some first-object of a page in old-gen has its class pointer updated, then we'd be reading wrong data from the class object. Bug: 343220989 Bug: 401488879 Test: ATP test infra Change-Id: I518dd732ab3e2ccb91bf5b5578474ebf90665f81 --- runtime/gc/collector/mark_compact.cc | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/runtime/gc/collector/mark_compact.cc b/runtime/gc/collector/mark_compact.cc index 57ec48a3de..95c33d0540 100644 --- a/runtime/gc/collector/mark_compact.cc +++ b/runtime/gc/collector/mark_compact.cc @@ -1247,12 +1247,14 @@ bool MarkCompact::PrepareForCompaction() { [&first_obj](mirror::Object* obj) { first_obj = obj; }); } if (first_obj != nullptr) { + mirror::Object* compacted_obj; if (reinterpret_cast(first_obj) >= old_gen_end_) { // post-compact address of the first live object in young-gen. - first_obj = PostCompactOldObjAddr(first_obj); - DCHECK_LT(reinterpret_cast(first_obj), post_compact_end_); + compacted_obj = PostCompactOldObjAddr(first_obj); + DCHECK_LT(reinterpret_cast(compacted_obj), post_compact_end_); } else { DCHECK(!young_gen_); + compacted_obj = first_obj; } // It's important to page-align mid-gen boundary. However, that means // there could be an object overlapping that boundary. We will deal with @@ -1260,7 +1262,32 @@ bool MarkCompact::PrepareForCompaction() { // to ensure that we don't de-promote an object from old-gen back to // young-gen. Otherwise, we may skip dirtying card for such an object if // it contains native-roots to young-gen. - mid_gen_end_ = AlignUp(reinterpret_cast(first_obj), gPageSize); + mid_gen_end_ = AlignUp(reinterpret_cast(compacted_obj), gPageSize); + // We need to ensure that for any object in old-gen, its class is also in + // there (for the same reason as mentioned above in the black-dense case). + // So adjust mid_gen_end_ accordingly, in the worst case all the way up + // to post_compact_end_. + auto iter = class_after_obj_map_.lower_bound(ObjReference::FromMirrorPtr(first_obj)); + for (; iter != class_after_obj_map_.end(); iter++) { + // 'mid_gen_end_' is now post-compact, so need to compare with + // post-compact addresses. + compacted_obj = + PostCompactAddress(iter->second.AsMirrorPtr(), old_gen_end_, moving_space_end_); + // We cannot update the map with post-compact addresses yet as compaction-phase + // expects pre-compacted addresses. So we will update in FinishPhase(). + if (reinterpret_cast(compacted_obj) < mid_gen_end_) { + mirror::Object* klass = iter->first.AsMirrorPtr(); + DCHECK_LT(reinterpret_cast(klass), black_allocations_begin_); + klass = PostCompactAddress(klass, old_gen_end_, moving_space_end_); + // We only need to make sure that the class object doesn't move during + // compaction, which can be ensured by just making its first word be + // consumed in to the old-gen. + mid_gen_end_ = + std::max(mid_gen_end_, reinterpret_cast(klass) + kObjectAlignment); + mid_gen_end_ = AlignUp(mid_gen_end_, gPageSize); + } + } + CHECK_LE(mid_gen_end_, post_compact_end_); } else { // Young-gen is empty. mid_gen_end_ = post_compact_end_; -- cgit v1.2.3-59-g8ed1b From 71fcdfac2ffde37e60f12dc4e07ad56cd8344614 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Mon, 10 Mar 2025 06:32:05 -0800 Subject: Test1963: remove unnecessary JNI. Other CTS tests already require a working memfd_create(), and ART is no longer running tests locally on fugu (whose ancient kernel didn't support memfd_create()). Change-Id: If442de7f62a3b2a38206223f15fdb2f62429523d --- .../check_memfd_create.cc | 67 ---------------------- .../src/Main.java | 6 -- test/Android.bp | 1 - 3 files changed, 74 deletions(-) delete mode 100644 test/1963-add-to-dex-classloader-in-memory/check_memfd_create.cc diff --git a/test/1963-add-to-dex-classloader-in-memory/check_memfd_create.cc b/test/1963-add-to-dex-classloader-in-memory/check_memfd_create.cc deleted file mode 100644 index 70a64d71ee..0000000000 --- a/test/1963-add-to-dex-classloader-in-memory/check_memfd_create.cc +++ /dev/null @@ -1,67 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - - -#include -#include -#include - -#include "jvmti.h" - -#include "base/logging.h" -#include "base/globals.h" -#include "base/memfd.h" - -#ifdef __linux__ -#include -#endif - -namespace art { -namespace Test1963AddToDexClassLoaderInMemory { - -extern "C" JNIEXPORT jboolean JNICALL Java_Main_hasWorkingMemfdCreate(JNIEnv*, jclass) { - // We should always have a working version if we're on normal buildbots. - if (!art::kIsTargetBuild) { - return true; - } -#ifdef __linux__ - struct utsname name; - if (uname(&name) >= 0) { - std::istringstream version(name.release); - std::string major_str; - std::string minor_str; - std::getline(version, major_str, '.'); - std::getline(version, minor_str, '.'); - int major = std::stoi(major_str); - int minor = std::stoi(minor_str); - if (major >= 4 || (major == 3 && minor >= 17)) { - // memfd_create syscall was added in 3.17 - return true; - } - } -#endif - int res = memfd_create_compat("TEST THAT MEMFD CREATE WORKS", 0); - if (res < 0) { - PLOG(ERROR) << "Unable to call memfd_create_compat successfully!"; - return false; - } else { - close(res); - return true; - } -} - -} // namespace Test1963AddToDexClassLoaderInMemory -} // namespace art diff --git a/test/1963-add-to-dex-classloader-in-memory/src/Main.java b/test/1963-add-to-dex-classloader-in-memory/src/Main.java index 1825e4faab..9c4cf57739 100644 --- a/test/1963-add-to-dex-classloader-in-memory/src/Main.java +++ b/test/1963-add-to-dex-classloader-in-memory/src/Main.java @@ -17,16 +17,10 @@ public class Main { public static void main(String[] args) throws Exception { try { - if (!hasWorkingMemfdCreate()) { - System.out.println("---NO memfd_create---"); - } art.Test1963.run(); } catch (Throwable t) { System.out.println(t); t.printStackTrace(System.out); - return; } } - - public static native boolean hasWorkingMemfdCreate(); } diff --git a/test/Android.bp b/test/Android.bp index 63840648ca..d3084fe7d2 100644 --- a/test/Android.bp +++ b/test/Android.bp @@ -725,7 +725,6 @@ art_cc_defaults { "1959-redefine-object-instrument/fake_redef_object.cc", "1960-obsolete-jit-multithread-native/native_say_hi.cc", "1964-add-to-dex-classloader-file/add_to_loader.cc", - "1963-add-to-dex-classloader-in-memory/check_memfd_create.cc", "2012-structural-redefinition-failures-jni-id/set-jni-id-used.cc", "2031-zygote-compiled-frame-deopt/native-wait.cc", "2038-hiddenapi-jvmti-ext/hiddenapi_ext.cc", -- cgit v1.2.3-59-g8ed1b From 7e0bfd62fa99fa13156bebeb0c72247954ae31fd Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Mon, 10 Mar 2025 23:17:43 -0700 Subject: Revert "Pass the dex register array to NterpGetInstanceField." This reverts commit 0237ac82184e8123dcb24151e273a63a514a3283. Reason for revert: b/402253994 Change-Id: I78da473f76753899956cbee2e8d228ca54b34d85 --- runtime/interpreter/interpreter_common.h | 17 ++++------- runtime/interpreter/interpreter_switch_impl-inl.h | 6 ++-- runtime/interpreter/mterp/arm64ng/object.S | 2 -- runtime/interpreter/mterp/armng/main.S | 15 +--------- runtime/interpreter/mterp/nterp.cc | 35 +++-------------------- runtime/interpreter/mterp/riscv64/object.S | 4 --- runtime/interpreter/mterp/x86_64ng/main.S | 4 --- runtime/interpreter/mterp/x86ng/main.S | 22 +------------- 8 files changed, 14 insertions(+), 91 deletions(-) diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h index 89900671b8..ce6c412ba8 100644 --- a/runtime/interpreter/interpreter_common.h +++ b/runtime/interpreter/interpreter_common.h @@ -290,29 +290,22 @@ LIBART_PROTECTED extern "C" uint32_t NterpGetInstanceFieldOffset(Thread* self, ArtMethod* caller, const uint16_t* dex_pc_ptr, - size_t resolve_field_type, - uint32_t* registers); + size_t resolve_field_type); static inline void GetFieldInfo(Thread* self, - ShadowFrame& shadow_frame, + ArtMethod* caller, const uint16_t* dex_pc_ptr, bool is_static, bool resolve_field_type, ArtField** field, bool* is_volatile, - MemberOffset* offset) - REQUIRES_SHARED(Locks::mutator_lock_) { + MemberOffset* offset) { size_t tls_value = 0u; if (!self->GetInterpreterCache()->Get(self, dex_pc_ptr, &tls_value)) { if (is_static) { - tls_value = NterpGetStaticField( - self, shadow_frame.GetMethod(), dex_pc_ptr, resolve_field_type); + tls_value = NterpGetStaticField(self, caller, dex_pc_ptr, resolve_field_type); } else { - tls_value = NterpGetInstanceFieldOffset(self, - shadow_frame.GetMethod(), - dex_pc_ptr, - resolve_field_type, - shadow_frame.GetVRegAddr(0)); + tls_value = NterpGetInstanceFieldOffset(self, caller, dex_pc_ptr, resolve_field_type); } if (self->IsExceptionPending()) { diff --git a/runtime/interpreter/interpreter_switch_impl-inl.h b/runtime/interpreter/interpreter_switch_impl-inl.h index 791e9df04d..7915a10094 100644 --- a/runtime/interpreter/interpreter_switch_impl-inl.h +++ b/runtime/interpreter/interpreter_switch_impl-inl.h @@ -73,7 +73,7 @@ ALWAYS_INLINE bool DoFieldGet(Thread* self, MemberOffset offset(0u); bool is_volatile; GetFieldInfo(self, - shadow_frame, + shadow_frame.GetMethod(), reinterpret_cast(inst), is_static, /*resolve_field_type=*/ false, @@ -157,7 +157,7 @@ ALWAYS_INLINE bool DoFieldGet(Thread* self, // Returns true on success, otherwise throws an exception and returns false. template ALWAYS_INLINE bool DoFieldPut(Thread* self, - ShadowFrame& shadow_frame, + const ShadowFrame& shadow_frame, const Instruction* inst, uint16_t inst_data, const instrumentation::Instrumentation* instrumentation) @@ -172,7 +172,7 @@ ALWAYS_INLINE bool DoFieldPut(Thread* self, MemberOffset offset(0u); bool is_volatile; GetFieldInfo(self, - shadow_frame, + shadow_frame.GetMethod(), reinterpret_cast(inst), is_static, resolve_field_type, diff --git a/runtime/interpreter/mterp/arm64ng/object.S b/runtime/interpreter/mterp/arm64ng/object.S index f51ab6026c..ade39d5e89 100644 --- a/runtime/interpreter/mterp/arm64ng/object.S +++ b/runtime/interpreter/mterp/arm64ng/object.S @@ -193,7 +193,6 @@ ldr x1, [sp] mov x2, xPC mov x3, #0 - mov x4, xFP EXPORT_PC bl nterp_get_instance_field_offset // Zero extension (nterp_get_instance_field_offset returns uint32_t) of the return value is @@ -276,7 +275,6 @@ .else mov x3, #0 .endif - mov x4, xFP EXPORT_PC bl nterp_get_instance_field_offset // Zero extension (nterp_get_instance_field_offset returns uint32_t) of the return value is diff --git a/runtime/interpreter/mterp/armng/main.S b/runtime/interpreter/mterp/armng/main.S index 38f7647dd2..5298840f92 100644 --- a/runtime/interpreter/mterp/armng/main.S +++ b/runtime/interpreter/mterp/armng/main.S @@ -1950,6 +1950,7 @@ END nterp_f2l_doconv // Entrypoints into runtime. NTERP_TRAMPOLINE nterp_get_static_field, NterpGetStaticField +NTERP_TRAMPOLINE nterp_get_instance_field_offset, NterpGetInstanceFieldOffset NTERP_TRAMPOLINE nterp_filled_new_array, NterpFilledNewArray NTERP_TRAMPOLINE nterp_filled_new_array_range, NterpFilledNewArrayRange NTERP_TRAMPOLINE nterp_get_class, NterpGetClass @@ -1958,20 +1959,6 @@ NTERP_TRAMPOLINE nterp_get_method, NterpGetMethod NTERP_TRAMPOLINE nterp_hot_method, NterpHotMethod NTERP_TRAMPOLINE nterp_load_object, NterpLoadObject -ENTRY nterp_get_instance_field_offset - SETUP_SAVE_REFS_ONLY_FRAME ip - INCREASE_FRAME 8 - str rFP, [sp] - bl NterpGetInstanceFieldOffset - DECREASE_FRAME 8 - RESTORE_SAVE_REFS_ONLY_FRAME - REFRESH_MARKING_REGISTER - ldr ip, [rSELF, #THREAD_EXCEPTION_OFFSET] @ Get exception field. - cmp ip, #0 - bne nterp_deliver_pending_exception - bx lr -END nterp_get_instance_field_offset - ENTRY nterp_deliver_pending_exception DELIVER_PENDING_EXCEPTION END nterp_deliver_pending_exception diff --git a/runtime/interpreter/mterp/nterp.cc b/runtime/interpreter/mterp/nterp.cc index f08f70143b..95cfe7fb7d 100644 --- a/runtime/interpreter/mterp/nterp.cc +++ b/runtime/interpreter/mterp/nterp.cc @@ -391,12 +391,8 @@ extern "C" size_t NterpGetMethod(Thread* self, ArtMethod* caller, const uint16_t } } -template ALWAYS_INLINE FLATTEN -static ArtField* FindFieldFast(ArtMethod* caller, - uint16_t field_index, - uint32_t* registers, - const Instruction* inst) +static ArtField* FindFieldFast(ArtMethod* caller, uint16_t field_index) REQUIRES_SHARED(Locks::mutator_lock_) { if (caller->IsObsolete()) { return nullptr; @@ -409,28 +405,6 @@ static ArtField* FindFieldFast(ArtMethod* caller, return cls->FindDeclaredField(field_index); } - if (!kStatic) { - mirror::Object* obj = reinterpret_cast32(registers[inst->VRegB_22c()]); - if (obj != nullptr) { - mirror::Class* obj_cls = obj->GetClass(); - if (obj_cls->GetDexTypeIndex() == field_id.class_idx_ && - obj_cls->GetDexCache() == cls->GetDexCache() && - obj_cls->IsPublic()) { - ArtField* resolved_field = obj_cls->FindDeclaredField(field_index); - if (caller->SkipAccessChecks() || - (resolved_field != nullptr && resolved_field->IsPublic())) { - return resolved_field; - } - } - } - } - - ArtField* field = caller->GetDexCache()->GetResolvedField(field_index); - if (caller->SkipAccessChecks() || - (field != nullptr && field->IsPublic() && field->GetDeclaringClass()->IsPublic())) { - return field; - } - return nullptr; } @@ -461,7 +435,7 @@ extern "C" size_t NterpGetStaticField(Thread* self, uint16_t field_index = inst->VRegB_21c(); Instruction::Code opcode = inst->Opcode(); - ArtField* resolved_field = FindFieldFast(caller, field_index, nullptr, inst); + ArtField* resolved_field = FindFieldFast(caller, field_index); if (resolved_field == nullptr || !resolved_field->IsStatic()) { resolved_field = FindFieldSlow( self, caller, field_index, /*is_static=*/ true, IsInstructionSPut(opcode)); @@ -518,14 +492,13 @@ LIBART_PROTECTED extern "C" uint32_t NterpGetInstanceFieldOffset(Thread* self, ArtMethod* caller, const uint16_t* dex_pc_ptr, - size_t resolve_field_type, // Resolve if not zero - uint32_t* registers) + size_t resolve_field_type) // Resolve if not zero REQUIRES_SHARED(Locks::mutator_lock_) { const Instruction* inst = Instruction::At(dex_pc_ptr); uint16_t field_index = inst->VRegC_22c(); Instruction::Code opcode = inst->Opcode(); - ArtField* resolved_field = FindFieldFast(caller, field_index, registers, inst); + ArtField* resolved_field = FindFieldFast(caller, field_index); if (resolved_field == nullptr || resolved_field->IsStatic()) { resolved_field = FindFieldSlow( self, caller, field_index, /*is_static=*/ false, IsInstructionIPut(opcode)); diff --git a/runtime/interpreter/mterp/riscv64/object.S b/runtime/interpreter/mterp/riscv64/object.S index ebaf550932..43bb31f1e9 100644 --- a/runtime/interpreter/mterp/riscv64/object.S +++ b/runtime/interpreter/mterp/riscv64/object.S @@ -290,7 +290,6 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, zero - mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset @@ -368,7 +367,6 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, zero - mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset @@ -454,7 +452,6 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, zero - mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset @@ -538,7 +535,6 @@ ld a1, (sp) // a1 := caller ArtMethod* mv a2, xPC mv a3, $value - mv a4, xFP EXPORT_PC call nterp_get_instance_field_offset // result a0 := field_offset diff --git a/runtime/interpreter/mterp/x86_64ng/main.S b/runtime/interpreter/mterp/x86_64ng/main.S index 7126c9f5ae..80753c4fc2 100644 --- a/runtime/interpreter/mterp/x86_64ng/main.S +++ b/runtime/interpreter/mterp/x86_64ng/main.S @@ -1547,7 +1547,6 @@ END_FUNCTION \name movq 0(%rsp), %rsi movq rPC, %rdx movq $$0, %rcx - movq rFP, %r8 call nterp_get_instance_field_offset testl %eax, %eax jns 1b @@ -1582,7 +1581,6 @@ END_FUNCTION \name movq 0(%rsp), %rsi movq rPC, %rdx movq $$0, %rcx - movq rFP, %r8 call nterp_get_instance_field_offset testl %eax, %eax jns 1b @@ -1946,7 +1944,6 @@ NterpPutObjectInstanceField: movq 0(%rsp), %rsi movq rPC, %rdx // %rcx is already set. - movq rFP, %r8 call nterp_get_instance_field_offset // Reload the value as it may have moved. GET_VREG %ecx, %rbp # ecx <- v[A] @@ -1990,7 +1987,6 @@ NterpGetObjectInstanceField: movq 0(%rsp), %rsi movq rPC, %rdx movq $$0, %rcx - movq rFP, %r8 call nterp_get_instance_field_offset testl %eax, %eax jns 1b diff --git a/runtime/interpreter/mterp/x86ng/main.S b/runtime/interpreter/mterp/x86ng/main.S index dabdd4233a..d2f4271f99 100644 --- a/runtime/interpreter/mterp/x86ng/main.S +++ b/runtime/interpreter/mterp/x86ng/main.S @@ -2321,6 +2321,7 @@ SYMBOL(EndExecuteNterpImpl): // Entrypoints into runtime. NTERP_TRAMPOLINE nterp_get_static_field, NterpGetStaticField +NTERP_TRAMPOLINE nterp_get_instance_field_offset, NterpGetInstanceFieldOffset NTERP_TRAMPOLINE nterp_filled_new_array, NterpFilledNewArray NTERP_TRAMPOLINE nterp_filled_new_array_range, NterpFilledNewArrayRange NTERP_TRAMPOLINE nterp_get_class, NterpGetClass @@ -2329,27 +2330,6 @@ NTERP_TRAMPOLINE nterp_get_method, NterpGetMethod NTERP_TRAMPOLINE nterp_hot_method, NterpHotMethod NTERP_TRAMPOLINE nterp_load_object, NterpLoadObject -// Special case nterp_get_instance_field_offset as it requires 5 arguments. -DEFINE_FUNCTION nterp_get_instance_field_offset - movd %ebx, %xmm0 - SETUP_SAVE_REFS_ONLY_FRAME ebx - movd %xmm0, %ebx - INCREASE_FRAME 12 - PUSH_ARG edi - PUSH_ARG ebx - PUSH_ARG edx - PUSH_ARG ecx - PUSH_ARG eax - call NterpGetInstanceFieldOffset - DECREASE_FRAME 32 - RESTORE_IBASE_WITH_CFA - FETCH_INST_CLEAR_OPCODE - RESTORE_SAVE_REFS_ONLY_FRAME - cmpl LITERAL(0), %fs:THREAD_EXCEPTION_OFFSET - jne nterp_deliver_pending_exception - ret -END_FUNCTION nterp_get_instance_field_offset - DEFINE_FUNCTION nterp_deliver_pending_exception DELIVER_PENDING_EXCEPTION END_FUNCTION nterp_deliver_pending_exception -- cgit v1.2.3-59-g8ed1b From 45ef50aad24ddd225da8f45702b40d1f894e1059 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Thu, 13 Feb 2025 15:53:48 -0500 Subject: cpplint: disable new categories for upgrade preparation We're updating cpplint to 2.0.0 which triggers a bunch of warnings here. Disable them until someone can clean things up. Bug: 399719956 Test: parallel ../tools/repohooks/tools/cpplint.py --quiet -- `find -name '*.cc' -o -name '*.h'` (cherry picked from https://android-review.googlesource.com/q/commit:ce10dc6fbdeaef31f0e7ff6e80e0678cb3e13e94) Merged-In: I4b8dc957c3e76d529ac7e506311763a6393a35df Change-Id: I4b8dc957c3e76d529ac7e506311763a6393a35df --- CPPLINT.cfg | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CPPLINT.cfg b/CPPLINT.cfg index 83288421e2..27feb34455 100644 --- a/CPPLINT.cfg +++ b/CPPLINT.cfg @@ -26,8 +26,15 @@ linelength=100 # Ignore the following categories of errors, as specified by the filter: # (the filter settings are concatenated together) filter=-build/c++11 +filter=-build/c++17 filter=-build/include filter=-readability/function,-readability/streams,-readability/todo filter=-runtime/printf,-runtime/references,-runtime/sizeof,-runtime/threadsafe_fn # TODO: this should be re-enabled. filter=-whitespace/line_length +filter=-whitespace/braces +filter=-whitespace/indent_namespace +filter=-whitespace/newline +filter=-readability/casting +# TODO: https://github.com/cpplint/cpplint/issues/298 +filter=-readability/nolint -- cgit v1.2.3-59-g8ed1b From 9f360eb65f3d191fd24c5864cb43628e5e3078e7 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Thu, 13 Feb 2025 15:53:48 -0500 Subject: cpplint: disable new categories for upgrade preparation We're updating cpplint to 2.0.0 which triggers a bunch of warnings here. Disable them until someone can clean things up. Bug: 399719956 Test: parallel ../tools/repohooks/tools/cpplint.py --quiet -- `find -name '*.cc' -o -name '*.h'` (cherry picked from https://android-review.googlesource.com/q/commit:ce10dc6fbdeaef31f0e7ff6e80e0678cb3e13e94) Merged-In: I4b8dc957c3e76d529ac7e506311763a6393a35df Change-Id: I4b8dc957c3e76d529ac7e506311763a6393a35df --- CPPLINT.cfg | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CPPLINT.cfg b/CPPLINT.cfg index 83288421e2..27feb34455 100644 --- a/CPPLINT.cfg +++ b/CPPLINT.cfg @@ -26,8 +26,15 @@ linelength=100 # Ignore the following categories of errors, as specified by the filter: # (the filter settings are concatenated together) filter=-build/c++11 +filter=-build/c++17 filter=-build/include filter=-readability/function,-readability/streams,-readability/todo filter=-runtime/printf,-runtime/references,-runtime/sizeof,-runtime/threadsafe_fn # TODO: this should be re-enabled. filter=-whitespace/line_length +filter=-whitespace/braces +filter=-whitespace/indent_namespace +filter=-whitespace/newline +filter=-readability/casting +# TODO: https://github.com/cpplint/cpplint/issues/298 +filter=-readability/nolint -- cgit v1.2.3-59-g8ed1b From abeeacd902042cb2e4941ad66608f8bc526613d4 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Tue, 11 Mar 2025 02:57:35 -0700 Subject: Fix SELinux denial on GMS Core's symlinks to secondary dex files. Bug: 401662336 Bug: 391895923 Test: Presubmit Flag: EXEMPT bugfix Change-Id: Iaa9a716cfe262897e313b994db92855721e1dfcc --- .../java/com/android/server/art/DexUseManagerLocal.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java b/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java index 704dabe034..9c7f45de24 100644 --- a/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java +++ b/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java @@ -67,6 +67,8 @@ import java.io.OutputStream; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Collections; @@ -116,10 +118,8 @@ public class DexUseManagerLocal { // Impose a limit on the input accepted by notifyDexContainersLoaded per owning package. /** @hide */ @VisibleForTesting public static final int MAX_PATH_LENGTH = 4096; - /** @hide */ @VisibleForTesting public static final int MAX_CLASS_LOADER_CONTEXT_LENGTH = 10000; - /** @hide */ private static final int MAX_SECONDARY_DEX_FILES_PER_OWNER = 500; @@ -669,14 +669,18 @@ public class DexUseManagerLocal { @NonNull String classLoaderContext, @NonNull String abiName, long lastUsedAtMs) { DexLoader loader = DexLoader.create(loadingPackageName, isolatedProcess); // This is to avoid a loading package from using up the SecondaryDexUse entries for another - // package (up to the MAX_SECONDARY_DEX_FILES_PER_OWNER limit). We don't care about the - // loading package messing up its own SecondaryDexUse entries. + // package (up to the MAX_SECONDARY_DEX_FILES_PER_OWNER limit). // Note that we are using system_server's permission to check the existence. This is fine // with the assumption that the file must be world readable to be used by other apps. // We could use artd's permission to check the existence, and then there wouldn't be any // permission issue, but that requires bringing up the artd service, which may be too // expensive. // TODO(jiakaiz): Check if the assumption is true. + // This doesn't apply to secondary dex files that aren't used by other apps, but we + // don't care about the loading package messing up its own SecondaryDexUse + // entries. + // Also note that the check doesn't follow symlinks because GMSCore creates symlinks to + // its secondary dex files, while system_server doesn't have the permission to follow them. if (isLoaderOtherApp(loader, owningPackageName) && !mInjector.pathExists(dexPath)) { AsLog.w("Not recording non-existent secondary dex file '" + dexPath + "'"); return; @@ -1399,7 +1403,7 @@ public class DexUseManagerLocal { } public boolean pathExists(String path) { - return new File(path).exists(); + return Files.exists(Paths.get(path), LinkOption.NOFOLLOW_LINKS); } @NonNull -- cgit v1.2.3-59-g8ed1b From 4b88637ae7a8fa459bcd3a22452770ec4858772f Mon Sep 17 00:00:00 2001 From: Mythri Alle Date: Tue, 4 Mar 2025 13:53:38 +0000 Subject: Add a parser for processing long running method traces Bug: 352518093 Test: Manual testing Change-Id: Ic16b9a571cfe18dc3291f7e6aaa8b72e50b91e01 --- tools/trace_parser/Android.bp | 41 ++++ .../long_running_method_trace_parser.cc | 252 +++++++++++++++++++++ 2 files changed, 293 insertions(+) create mode 100644 tools/trace_parser/Android.bp create mode 100644 tools/trace_parser/long_running_method_trace_parser.cc diff --git a/tools/trace_parser/Android.bp b/tools/trace_parser/Android.bp new file mode 100644 index 0000000000..f28af64c7f --- /dev/null +++ b/tools/trace_parser/Android.bp @@ -0,0 +1,41 @@ +// +// Copyright (C) 2025 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "art_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["art_license"], +} + +art_cc_binary { + name: "long_running_method_trace_parser", + defaults: ["art_debug_defaults"], + host_supported: true, + device_supported: false, + srcs: [ + "long_running_method_trace_parser.cc", + ], + static_libs: [ + "libbase", + "libartbase", + "liblog", + "libz", + "libziparchive", + ], +} diff --git a/tools/trace_parser/long_running_method_trace_parser.cc b/tools/trace_parser/long_running_method_trace_parser.cc new file mode 100644 index 0000000000..9645a445f8 --- /dev/null +++ b/tools/trace_parser/long_running_method_trace_parser.cc @@ -0,0 +1,252 @@ +/* + * Copyright (C) 2025 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +#include "base/leb128.h" +#include "base/os.h" +#include "base/unix_file/fd_file.h" + +namespace art { + +// These constants are defined in the ART sources in the following files: +// +// - art/runtime/trace.h +// - art/runtime/trace_profile.cc +static const int kThreadInfoHeaderV2 = 0; +static const int kMethodInfoHeaderV2 = 1; +static const int kEntryHeaderV2 = 2; +static const int kMethodEntry = 0; +static const int kMethodExit = 1; +static const int kAlwaysOnMethodInfoHeaderSize = 11; +static const int kAlwaysOnTraceHeaderSize = 12; + +uint64_t ReadNumber(int num_bytes, uint8_t* header) { + uint64_t number = 0; + for (int i = 0; i < num_bytes; i++) { + uint64_t c = header[i]; + number += c << (i * 8); + } + return number; +} + +bool ProcessMethodInfo(std::unique_ptr& file, std::map& name_map) { + // The first byte that specified the type of the packet is already read in + // ParseLongRunningMethodTrace. + uint8_t header[kAlwaysOnMethodInfoHeaderSize - 1]; + if (!file->ReadFully(&header, sizeof(header))) { + printf("Couldn't read header\n"); + return false; + } + uint64_t id = ReadNumber(8, header); + int length = ReadNumber(2, header + 8); + + std::unique_ptr name(new char[length]); + if (!file->ReadFully(name.get(), length)) { + return false; + } + std::string str(name.get(), length); + std::replace(str.begin(), str.end(), '\t', ' '); + if (str[str.length() - 1] == '\n') { + str.erase(str.length() - 1); + } + name_map.emplace(id, str); + return true; +} + +void PrintTraceEntry(const std::string& method_name, + int event_type, + int* current_depth, + size_t timestamp) { + std::string entry; + for (int i = 0; i < *current_depth; i++) { + entry.push_back('.'); + } + if (event_type == kMethodEntry) { + *current_depth += 1; + entry.append(".>> "); + } else if (event_type == kMethodExit) { + *current_depth -= 1; + entry.append("<< "); + } else { + entry.append("?? "); + } + entry.append(" "); + entry.append(method_name); + entry.append(" "); + entry.append(std::to_string(timestamp)); + entry.append("\n"); + printf("%s", entry.c_str()); +} + +bool SkipTraceEntries(std::unique_ptr& file) { + // The first byte that specified the type of the packet is already read in + // ParseLongRunningMethodTrace. + uint8_t header[kAlwaysOnTraceHeaderSize - 1]; + if (!file->ReadFully(header, sizeof(header))) { + return false; + } + + // Read thread id + ReadNumber(4, header); + int offset = 4; + // Read number of records + ReadNumber(3, header + offset); + offset += 3; + int total_size = ReadNumber(4, header + offset); + std::unique_ptr buffer(new uint8_t[total_size]); + if (!file->ReadFully(buffer.get(), total_size)) { + return false; + } + return true; +} + +bool ProcessLongRunningMethodTraceEntries(std::unique_ptr& file, + std::map& current_depth_map, + std::map& method_map) { + // The first byte that specified the type of the packet is already read in + // ParseLongRunningMethodTrace. + uint8_t header[kAlwaysOnTraceHeaderSize - 1]; + if (!file->ReadFully(header, sizeof(header))) { + return false; + } + + uint32_t thread_id = ReadNumber(4, header); + int offset = 4; + int num_records = ReadNumber(3, header + offset); + offset += 3; + int total_size = ReadNumber(4, header + offset); + if (total_size == 0) { + return true; + } + std::unique_ptr buffer(new uint8_t[total_size]); + if (!file->ReadFully(buffer.get(), total_size)) { + return false; + } + + printf("Thread: %d\n", thread_id); + int current_depth = 0; + if (current_depth_map.find(thread_id) != current_depth_map.end()) { + // Get the current call stack depth. If it is the first method we are seeing on this thread + // then this map wouldn't have an entry, and we start with the depth of 0. + current_depth = current_depth_map[thread_id]; + } + + const uint8_t* current_buffer_ptr = buffer.get(); + const uint8_t* end_ptr = buffer.get() + total_size; + uint64_t prev_method_id = 0; + int64_t prev_timestamp_and_action = 0; + for (int i = 0; i < num_records; i++) { + // Read timestamp and action + int64_t ts_diff = 0; + if (!DecodeSignedLeb128Checked(¤t_buffer_ptr, end_ptr, &ts_diff)) { + LOG(FATAL) << "Reading past the buffer when decoding timestamp"; + } + int64_t timestamp_and_action = prev_timestamp_and_action + ts_diff; + prev_timestamp_and_action = timestamp_and_action; + bool is_method_exit = timestamp_and_action & 0x1; + + uint64_t method_id; + std::string method_name; + if (!is_method_exit) { + int64_t method_diff = 0; + if (!DecodeSignedLeb128Checked(¤t_buffer_ptr, end_ptr, &method_diff)) { + LOG(FATAL) << "Reading past the buffer when decoding method id"; + } + method_id = prev_method_id + method_diff; + prev_method_id = method_id; + if (method_map.find(method_id) == method_map.end()) { + LOG(FATAL) << "No entry for method " << std::hex << method_id; + } + method_name = method_map[method_id]; + } + + PrintTraceEntry(method_name, + is_method_exit? kMethodExit: kMethodEntry, + ¤t_depth, + timestamp_and_action & ~0x1); + } + current_depth_map[thread_id] = current_depth; + return true; +} + +void ParseLongRunningMethodTrace(char* file_name) { + std::unique_ptr file(OS::OpenFileForReading(file_name)); + if (file == nullptr) { + printf("Couldn't open file\n"); + return; + } + + // Map to maintain information about threads and methods + std::map method_map; + + // Map to Maintain the current depth of the method in the call stack. Used to + // correctly indent when printing the trace events. + std::map current_depth_map; + + // First parse metadata. To keep the implementation of dumping the data + // simple, we don't ensure that the information about methods is dumped before the + // methods. This is also good if the ANR report got truncated. We will then + // have information about how long the methods took and we can infer some of + // the method names from the stack trace. + while (true) { + uint8_t entry_header; + if (!file->ReadFully(&entry_header, sizeof(entry_header))) { + break; + } + if (entry_header == kEntryHeaderV2) { + if (!SkipTraceEntries(file)) { + break; + } + } else { + DCHECK_EQ(entry_header, kMethodInfoHeaderV2); + if (!ProcessMethodInfo(file, method_map)) { + break; + } + } + } + + // Reset file + file->ResetOffset(); + + while (true) { + uint8_t entry_header; + if (!file->ReadFully(&entry_header, sizeof(entry_header))) { + break; + } + if (entry_header != kEntryHeaderV2) { + break; + } + if (!ProcessLongRunningMethodTraceEntries(file, current_depth_map, method_map)) { + break; + } + } +} + +} // namespace art + +int main(int argc, char **argv) { + if (argc < 1) { + printf("Usage trace "); + return -1; + } + + art::ParseLongRunningMethodTrace(argv[1]); + return 0; +} -- cgit v1.2.3-59-g8ed1b From a6a75dba00303a88642a1c57ad2ed390797adf47 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Mon, 3 Mar 2025 22:45:53 +0000 Subject: Remove SDM status from dump. The SDM status has two fields: sdm-status and sdm-signature. sdm-status can actually be inferred from the compiler filter and the compilation reason, as SDM files should contain special compilation reasons. sdm-signature is no longer necessary after we added install-time check on SDM files, making SDM files with the wrong signature rejected. This change essentially reverts https://r.android.com/3342909. Bug: 377474232 Test: atest ArtServiceTests Change-Id: I079f21715afd4039bb801d3c8b79ec8a49b3c687 --- .../com/android/server/art/ArtManagerLocal.java | 29 +---- .../com/android/server/art/ArtShellCommand.java | 21 +--- .../java/com/android/server/art/DumpHelper.java | 93 +------------- .../com/android/server/art/DumpHelperTest.java | 139 +-------------------- 4 files changed, 13 insertions(+), 269 deletions(-) diff --git a/libartservice/service/java/com/android/server/art/ArtManagerLocal.java b/libartservice/service/java/com/android/server/art/ArtManagerLocal.java index d8a508eb4e..735da25fbb 100644 --- a/libartservice/service/java/com/android/server/art/ArtManagerLocal.java +++ b/libartservice/service/java/com/android/server/art/ArtManagerLocal.java @@ -1002,19 +1002,8 @@ public final class ArtManagerLocal { @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) public void dump( @NonNull PrintWriter pw, @NonNull PackageManagerLocal.FilteredSnapshot snapshot) { - dump(pw, snapshot, false /* verifySdmSignatures */); - } - - /** - * Same as above, but allows to specify options. - * - * @hide - */ - @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) - public void dump(@NonNull PrintWriter pw, - @NonNull PackageManagerLocal.FilteredSnapshot snapshot, boolean verifySdmSignatures) { try (var pin = mInjector.createArtdPin()) { - new DumpHelper(this, verifySdmSignatures).dump(pw, snapshot); + new DumpHelper(this).dump(pw, snapshot); } } @@ -1030,21 +1019,9 @@ public final class ArtManagerLocal { @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) public void dumpPackage(@NonNull PrintWriter pw, @NonNull PackageManagerLocal.FilteredSnapshot snapshot, @NonNull String packageName) { - dumpPackage(pw, snapshot, packageName, false /* verifySdmSignatures */); - } - - /** - * Same as above, but allows to specify options. - * - * @hide - */ - @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) - public void dumpPackage(@NonNull PrintWriter pw, - @NonNull PackageManagerLocal.FilteredSnapshot snapshot, @NonNull String packageName, - boolean verifySdmSignatures) { try (var pin = mInjector.createArtdPin()) { - new DumpHelper(this, verifySdmSignatures) - .dumpPackage(pw, snapshot, Utils.getPackageStateOrThrow(snapshot, packageName)); + new DumpHelper(this).dumpPackage( + pw, snapshot, Utils.getPackageStateOrThrow(snapshot, packageName)); } } diff --git a/libartservice/service/java/com/android/server/art/ArtShellCommand.java b/libartservice/service/java/com/android/server/art/ArtShellCommand.java index 0bbb6213d2..f83f5cf083 100644 --- a/libartservice/service/java/com/android/server/art/ArtShellCommand.java +++ b/libartservice/service/java/com/android/server/art/ArtShellCommand.java @@ -171,23 +171,11 @@ public final class ArtShellCommand extends BasicShellCommandHandler { return 0; } case "dump": { - boolean verifySdmSignatures = false; - - String opt; - while ((opt = getNextOption()) != null) { - switch (opt) { - case "--verify-sdm-signatures": - verifySdmSignatures = true; - break; - } - } - String packageName = getNextArg(); if (packageName != null) { - mInjector.getArtManagerLocal().dumpPackage( - pw, snapshot, packageName, verifySdmSignatures); + mInjector.getArtManagerLocal().dumpPackage(pw, snapshot, packageName); } else { - mInjector.getArtManagerLocal().dump(pw, snapshot, verifySdmSignatures); + mInjector.getArtManagerLocal().dump(pw, snapshot); } return 0; } @@ -1084,13 +1072,10 @@ public final class ArtShellCommand extends BasicShellCommandHandler { pw.println(" Cleanup obsolete files, such as dexopt artifacts that are outdated or"); pw.println(" correspond to dex container files that no longer exist."); pw.println(); - pw.println(" dump [--verify-sdm-signatures] [PACKAGE_NAME]"); + pw.println(" dump [PACKAGE_NAME]"); pw.println(" Dump the dexopt state in text format to stdout."); pw.println(" If PACKAGE_NAME is empty, the command is for all packages. Otherwise, it"); pw.println(" is for the given package."); - pw.println(" Options:"); - pw.println(" --verify-sdm-signatures Also verify SDM file signatures and include"); - pw.println(" their statuses."); pw.println(); pw.println(" dexopt-packages -r REASON"); pw.println(" Run batch dexopt for the given reason."); diff --git a/libartservice/service/java/com/android/server/art/DumpHelper.java b/libartservice/service/java/com/android/server/art/DumpHelper.java index 77cc37f4b3..9c3bb3f087 100644 --- a/libartservice/service/java/com/android/server/art/DumpHelper.java +++ b/libartservice/service/java/com/android/server/art/DumpHelper.java @@ -20,12 +20,7 @@ import static com.android.server.art.DexUseManagerLocal.CheckedSecondaryDexInfo; import static com.android.server.art.DexUseManagerLocal.DexLoader; import static com.android.server.art.model.DexoptStatus.DexContainerFileDexoptStatus; -import android.annotation.FlaggedApi; import android.annotation.NonNull; -import android.annotation.SuppressLint; -import android.content.pm.PackageManager; -import android.content.pm.SigningInfo; -import android.content.pm.SigningInfoException; import android.os.Build; import android.os.RemoteException; import android.os.ServiceSpecificException; @@ -39,7 +34,6 @@ import com.android.server.pm.pkg.PackageState; import dalvik.system.VMRuntime; -import java.io.File; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Comparator; @@ -60,16 +54,14 @@ import java.util.stream.Collectors; @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) public class DumpHelper { @NonNull private final Injector mInjector; - private final boolean mVerifySdmSignatures; - public DumpHelper(@NonNull ArtManagerLocal artManagerLocal, boolean verifySdmSignatures) { - this(new Injector(artManagerLocal), verifySdmSignatures); + public DumpHelper(@NonNull ArtManagerLocal artManagerLocal) { + this(new Injector(artManagerLocal)); } @VisibleForTesting - public DumpHelper(@NonNull Injector injector, boolean verifySdmSignatures) { + public DumpHelper(@NonNull Injector injector) { mInjector = injector; - mVerifySdmSignatures = verifySdmSignatures; } /** Handles {@link ArtManagerLocal#dump(PrintWriter, PackageManagerLocal.FilteredSnapshot)}. */ @@ -205,9 +197,6 @@ public class DumpHelper { fileStatus.isPrimaryAbi() ? " [primary-abi]" : ""); ipw.increaseIndent(); ipw.printf("[location is %s]\n", fileStatus.getLocationDebugString()); - if (fileStatus.isPrimaryDex()) { - dumpSdmStatus(ipw, fileStatus.getDexContainerFile(), isa); - } ipw.decreaseIndent(); } } @@ -228,69 +217,6 @@ public class DumpHelper { } } - private void dumpSdmStatus( - @NonNull IndentingPrintWriter ipw, @NonNull String dexPath, @NonNull String isa) { - if (!android.content.pm.Flags.cloudCompilationPm()) { - return; - } - - String sdmPath = getSdmPath(dexPath, isa); - String status = ""; - String signature = "skipped"; - if (mInjector.fileExists(sdmPath)) { - // "Pending" means yet to be picked up by dexopt. For now, "pending" is the only status - // because SDM files are not supported yet. - status = "pending"; - // This operation is expensive, so hide it behind a flag. - if (mVerifySdmSignatures) { - signature = getSdmSignatureStatus(dexPath, sdmPath); - } - } - if (!status.isEmpty()) { - ipw.printf("sdm: [sdm-status=%s] [sdm-signature=%s]\n", status, signature); - } - } - - // The new API usage is safe because it's guarded by a flag. The "NewApi" lint is wrong because - // it's meaningless (b/380891026). We have to work around the lint error because there is no - // `isAtLeastB` to check yet. - // TODO(jiakaiz): Remove this workaround, change @FlaggedApi to @RequiresApi here, and check - // `isAtLeastB` at the call site after B SDK is finalized. - @FlaggedApi(android.content.pm.Flags.FLAG_CLOUD_COMPILATION_PM) - @SuppressLint("NewApi") - @NonNull - private String getSdmSignatureStatus(@NonNull String dexPath, @NonNull String sdmPath) { - SigningInfo sdmSigningInfo; - try { - sdmSigningInfo = - mInjector.getVerifiedSigningInfo(sdmPath, SigningInfo.VERSION_SIGNING_BLOCK_V3); - } catch (SigningInfoException e) { - AsLog.w("Failed to verify SDM signature", e); - return "invalid-sdm-signature"; - } - - SigningInfo apkSigningInfo; - try { - apkSigningInfo = - mInjector.getVerifiedSigningInfo(dexPath, SigningInfo.VERSION_SIGNING_BLOCK_V3); - } catch (SigningInfoException e) { - AsLog.w("Failed to verify SDM signature", e); - return "invalid-apk-signature"; - } - - if (!sdmSigningInfo.signersMatchExactly(apkSigningInfo)) { - return "mismatched-signers"; - } - - return "verified"; - } - - @NonNull - private static String getSdmPath(@NonNull String dexPath, @NonNull String isa) { - return Utils.replaceFileExtension( - dexPath, "." + isa + ArtConstants.SECURE_DEX_METADATA_FILE_EXT); - } - @NonNull private String getLoaderState( @NonNull PackageManagerLocal.FilteredSnapshot snapshot, @NonNull DexLoader loader) { @@ -326,18 +252,5 @@ public class DumpHelper { public DexUseManagerLocal getDexUseManager() { return GlobalInjector.getInstance().getDexUseManager(); } - - public boolean fileExists(@NonNull String path) { - return new File(path).exists(); - } - - // TODO(jiakaiz): See another comment about "NewApi" above. - @FlaggedApi(android.content.pm.Flags.FLAG_CLOUD_COMPILATION_PM) - @SuppressLint("NewApi") - @NonNull - public SigningInfo getVerifiedSigningInfo( - @NonNull String path, int minAppSigningSchemeVersion) throws SigningInfoException { - return PackageManager.getVerifiedSigningInfo(path, minAppSigningSchemeVersion); - } } } diff --git a/libartservice/service/javatests/com/android/server/art/DumpHelperTest.java b/libartservice/service/javatests/com/android/server/art/DumpHelperTest.java index 9dbecaf49c..55224b93f7 100644 --- a/libartservice/service/javatests/com/android/server/art/DumpHelperTest.java +++ b/libartservice/service/javatests/com/android/server/art/DumpHelperTest.java @@ -24,19 +24,12 @@ import static com.android.server.art.model.DexoptStatus.DexContainerFileDexoptSt import static com.google.common.truth.Truth.assertThat; import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.argThat; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import android.annotation.NonNull; -import android.annotation.SuppressLint; -import android.content.pm.SigningInfo; -import android.content.pm.SigningInfoException; import android.os.SystemProperties; import androidx.test.filters.SmallTest; @@ -77,8 +70,8 @@ public class DumpHelperTest { @Mock private ArtManagerLocal mArtManagerLocal; @Mock private DexUseManagerLocal mDexUseManagerLocal; @Mock private PackageManagerLocal.FilteredSnapshot mSnapshot; - @Mock private SigningInfo mSigningInfoA; - @Mock private SigningInfo mSigningInfoB; + + private DumpHelper mDumpHelper; @Before public void setUp() throws Exception { @@ -106,10 +99,7 @@ public class DumpHelperTest { setUpForBar(); setUpForSdk(); - lenient().when(mSigningInfoA.signersMatchExactly(mSigningInfoA)).thenReturn(true); - lenient().when(mSigningInfoA.signersMatchExactly(mSigningInfoB)).thenReturn(false); - lenient().when(mSigningInfoB.signersMatchExactly(mSigningInfoB)).thenReturn(true); - lenient().when(mSigningInfoB.signersMatchExactly(mSigningInfoA)).thenReturn(false); + mDumpHelper = new DumpHelper(mInjector); } @Test @@ -157,122 +147,10 @@ public class DumpHelperTest { + "Current GC: CollectorTypeCMC\n"; var stringWriter = new StringWriter(); - createDumpHelper(false /* verifySdmSignatures */) - .dump(new PrintWriter(stringWriter), mSnapshot); + mDumpHelper.dump(new PrintWriter(stringWriter), mSnapshot); assertThat(stringWriter.toString()).isEqualTo(expected); } - @Test - public void testDumpSdmStatusNotFound() throws Exception { - when(mInjector.fileExists(any())).thenReturn(false); - - var stringWriter = new StringWriter(); - createDumpHelper(true /* verifySdmSignatures */) - .dumpPackage( - new PrintWriter(stringWriter), mSnapshot, getPackageState(PKG_NAME_BAR)); - assertThat(stringWriter.toString()).doesNotContain("sdm:"); - } - - @Test - public void testDumpSdmStatusInvalidSdmSignature() throws Exception { - doReturn(false).when(mInjector).fileExists("/somewhere/app/bar/base.arm.sdm"); - doReturn(true).when(mInjector).fileExists("/somewhere/app/bar/base.arm64.sdm"); - when(mInjector.getVerifiedSigningInfo(eq("/somewhere/app/bar/base.arm64.sdm"), anyInt())) - .thenThrow(SigningInfoException.class); - - var stringWriter = new StringWriter(); - createDumpHelper(true /* verifySdmSignatures */) - .dumpPackage( - new PrintWriter(stringWriter), mSnapshot, getPackageState(PKG_NAME_BAR)); - assertThat(stringWriter.toString()) - .contains("sdm: [sdm-status=pending] [sdm-signature=invalid-sdm-signature]"); - } - - @Test - public void testDumpSdmStatusInvalidApkSignature() throws Exception { - doReturn(false).when(mInjector).fileExists("/somewhere/app/bar/base.arm.sdm"); - doReturn(true).when(mInjector).fileExists("/somewhere/app/bar/base.arm64.sdm"); - doReturn(mSigningInfoA) - .when(mInjector) - .getVerifiedSigningInfo(eq("/somewhere/app/bar/base.arm64.sdm"), anyInt()); - doThrow(SigningInfoException.class) - .when(mInjector) - .getVerifiedSigningInfo(eq("/somewhere/app/bar/base.apk"), anyInt()); - - var stringWriter = new StringWriter(); - createDumpHelper(true /* verifySdmSignatures */) - .dumpPackage( - new PrintWriter(stringWriter), mSnapshot, getPackageState(PKG_NAME_BAR)); - assertThat(stringWriter.toString()) - .contains("sdm: [sdm-status=pending] [sdm-signature=invalid-apk-signature]"); - } - - @Test - public void testDumpSdmStatusSignersNotMatch() throws Exception { - doReturn(false).when(mInjector).fileExists("/somewhere/app/bar/base.arm.sdm"); - doReturn(true).when(mInjector).fileExists("/somewhere/app/bar/base.arm64.sdm"); - doReturn(mSigningInfoA) - .when(mInjector) - .getVerifiedSigningInfo(eq("/somewhere/app/bar/base.arm64.sdm"), anyInt()); - doReturn(mSigningInfoB) - .when(mInjector) - .getVerifiedSigningInfo(eq("/somewhere/app/bar/base.apk"), anyInt()); - - var stringWriter = new StringWriter(); - createDumpHelper(true /* verifySdmSignatures */) - .dumpPackage( - new PrintWriter(stringWriter), mSnapshot, getPackageState(PKG_NAME_BAR)); - assertThat(stringWriter.toString()) - .contains("sdm: [sdm-status=pending] [sdm-signature=mismatched-signers]"); - } - - @Test - public void testDumpSdmStatusVerified() throws Exception { - doReturn(false).when(mInjector).fileExists("/somewhere/app/bar/base.arm.sdm"); - doReturn(true).when(mInjector).fileExists("/somewhere/app/bar/base.arm64.sdm"); - doReturn(mSigningInfoA) - .when(mInjector) - .getVerifiedSigningInfo(eq("/somewhere/app/bar/base.arm64.sdm"), anyInt()); - doReturn(mSigningInfoA) - .when(mInjector) - .getVerifiedSigningInfo(eq("/somewhere/app/bar/base.apk"), anyInt()); - - var stringWriter = new StringWriter(); - createDumpHelper(true /* verifySdmSignatures */) - .dumpPackage( - new PrintWriter(stringWriter), mSnapshot, getPackageState(PKG_NAME_BAR)); - assertThat(stringWriter.toString()) - .containsMatch(" \\Qpath: /somewhere/app/bar/base.apk\\E\n" - + " arm:.*\n" - + " .*\n" - + " arm64:.*\n" - + " .*\n" - + " \\Qsdm: [sdm-status=pending] " - + "[sdm-signature=verified]\\E\n"); - } - - @Test - public void testDumpSdmStatusMultiArch() throws Exception { - doReturn(true).when(mInjector).fileExists("/somewhere/app/bar/base.arm.sdm"); - doReturn(true).when(mInjector).fileExists("/somewhere/app/bar/base.arm64.sdm"); - doReturn(mSigningInfoA).when(mInjector).getVerifiedSigningInfo(any(), anyInt()); - - var stringWriter = new StringWriter(); - createDumpHelper(true /* verifySdmSignatures */) - .dumpPackage( - new PrintWriter(stringWriter), mSnapshot, getPackageState(PKG_NAME_BAR)); - assertThat(stringWriter.toString()) - .containsMatch(" \\Qpath: /somewhere/app/bar/base.apk\\E\n" - + " arm:.*\n" - + " .*\n" - + " \\Qsdm: [sdm-status=pending] " - + "[sdm-signature=verified]\\E\n" - + " arm64:.*\n" - + " .*\n" - + " \\Qsdm: [sdm-status=pending] " - + "[sdm-signature=verified]\\E\n"); - } - private PackageState createPackageState(@NonNull String packageName, int appId, boolean isApex, boolean hasPackage, @NonNull String primaryAbi, @NonNull String secondaryAbi) { var pkgState = mock(PackageState.class); @@ -440,13 +318,4 @@ public class DumpHelperTest { PKG_NAME_SDK, "/somewhere/app/sdk/base.apk")) .thenReturn(Set.of()); } - - @SuppressLint("DirectInvocationOnMock") - private PackageState getPackageState(String packageName) { - return mSnapshot.getPackageState(packageName); - } - - private DumpHelper createDumpHelper(boolean verifySdmSignatures) { - return new DumpHelper(mInjector, verifySdmSignatures); - } } -- cgit v1.2.3-59-g8ed1b From 4370b86853f0fdf16f0e957365c6b600e5b32fe2 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Fri, 7 Mar 2025 17:13:16 +0000 Subject: Ensure oat checksum determinism across hosts and devices. For Cloud Compilation, we need to enable hosts to generate a boot image that has exactly the same checksum as generated on device. In other words, we need to make the oat checksum deterministic across hosts and devices. To achieve that, when writing the oat header, the non-deterministic fields are padded to their length limits and excluded from the oat checksum computation. Bug: 372868052 Test: m test-art-host-gtest Test: Generate a boot image on host. See the checksum being identical to the one on device. Change-Id: I9f73a28b349b673c25909b61a0c7ae8cf883c014 --- dex2oat/dex2oat.cc | 21 +++---- dex2oat/linker/image_test.h | 9 ++- dex2oat/linker/oat_writer.cc | 49 ++++++++++++--- dex2oat/linker/oat_writer.h | 27 +++++++- dex2oat/linker/oat_writer_test.cc | 85 ++++++++++++++++++++++--- oatdump/oatdump.cc | 5 +- runtime/oat/oat.cc | 128 +++++++++++++++++++++++--------------- runtime/oat/oat.h | 63 ++++++++++++++++++- 8 files changed, 293 insertions(+), 94 deletions(-) diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc index 7564bf0b68..39b0d826b4 100644 --- a/dex2oat/dex2oat.cc +++ b/dex2oat/dex2oat.cc @@ -500,15 +500,6 @@ class ThreadLocalHashOverride { Handle old_field_value_; }; -class OatKeyValueStore : public SafeMap { - public: - using SafeMap::Put; - - iterator Put(const std::string& k, bool v) { - return SafeMap::Put(k, v ? OatHeader::kTrueValue : OatHeader::kFalseValue); - } -}; - class Dex2Oat final { public: explicit Dex2Oat(TimingLogger* timings) @@ -893,7 +884,7 @@ class Dex2Oat final { } // Fill some values into the key-value store for the oat header. - key_value_store_.reset(new OatKeyValueStore()); + key_value_store_.reset(new linker::OatKeyValueStore()); // Automatically force determinism for the boot image and boot image extensions in a host build. if (!kIsTargetBuild && (IsBootImage() || IsBootImageExtension())) { @@ -978,7 +969,8 @@ class Dex2Oat final { } oss << argv[i]; } - key_value_store_->Put(OatHeader::kDex2OatCmdLineKey, oss.str()); + key_value_store_->PutNonDeterministic( + OatHeader::kDex2OatCmdLineKey, oss.str(), /*allow_truncation=*/true); } key_value_store_->Put(OatHeader::kDebuggableKey, compiler_options_->debuggable_); key_value_store_->Put(OatHeader::kNativeDebuggableKey, @@ -1696,7 +1688,10 @@ class Dex2Oat final { CompilerFilter::DependsOnImageChecksum(original_compiler_filter)) { std::string versions = apex_versions_argument_.empty() ? runtime->GetApexVersions() : apex_versions_argument_; - key_value_store_->Put(OatHeader::kApexVersionsKey, versions); + if (!key_value_store_->PutNonDeterministic(OatHeader::kApexVersionsKey, versions)) { + LOG(ERROR) << "Cannot store apex versions string because it's too long"; + return dex2oat::ReturnCode::kOther; + } } // Now that we have adjusted whether we generate an image, encode it in the @@ -2915,7 +2910,7 @@ class Dex2Oat final { std::unique_ptr compiler_options_; - std::unique_ptr key_value_store_; + std::unique_ptr key_value_store_; std::unique_ptr verification_results_; diff --git a/dex2oat/linker/image_test.h b/dex2oat/linker/image_test.h index 3706b685fa..349462e098 100644 --- a/dex2oat/linker/image_test.h +++ b/dex2oat/linker/image_test.h @@ -231,13 +231,12 @@ inline void ImageTest::DoCompile(ImageHeader::StorageMode storage_mode, CompileAll(class_loader, class_path, &timings); TimingLogger::ScopedTiming t("WriteElf", &timings); - SafeMap key_value_store; + OatKeyValueStore key_value_store; key_value_store.Put(OatHeader::kBootClassPathKey, android::base::Join(out_helper.dex_file_locations, ':')); - key_value_store.Put(OatHeader::kApexVersionsKey, Runtime::Current()->GetApexVersions()); - key_value_store.Put( - OatHeader::kConcurrentCopying, - compiler_options_->EmitReadBarrier() ? OatHeader::kTrueValue : OatHeader::kFalseValue); + key_value_store.PutNonDeterministic(OatHeader::kApexVersionsKey, + Runtime::Current()->GetApexVersions()); + key_value_store.Put(OatHeader::kConcurrentCopying, compiler_options_->EmitReadBarrier()); std::vector> elf_writers; std::vector> oat_writers; diff --git a/dex2oat/linker/oat_writer.cc b/dex2oat/linker/oat_writer.cc index 20787ddc8f..6cdfe7bb40 100644 --- a/dex2oat/linker/oat_writer.cc +++ b/dex2oat/linker/oat_writer.cc @@ -111,6 +111,33 @@ inline uint32_t CodeAlignmentSize(uint32_t header_offset, const CompiledMethod& } // anonymous namespace +bool OatKeyValueStore::PutNonDeterministic(const std::string& k, + const std::string& v, + bool allow_truncation) { + size_t length = OatHeader::GetNonDeterministicFieldLength(k); + DCHECK_GT(length, 0u); + if (v.length() <= length) { + map_.Put(k, v); + return true; + } + if (allow_truncation) { + LOG(WARNING) << "Key value store field " << k << "too long. Truncating"; + map_.Put(k, v.substr(length)); + return true; + } + return false; +} + +void OatKeyValueStore::Put(const std::string& k, const std::string& v) { + DCHECK(OatHeader::IsDeterministicField(k)); + map_.Put(k, v); +} + +void OatKeyValueStore::Put(const std::string& k, bool v) { + DCHECK(OatHeader::IsDeterministicField(k)); + map_.Put(k, v ? OatHeader::kTrueValue : OatHeader::kFalseValue); +} + // .bss mapping offsets used for BCP DexFiles. struct OatWriter::BssMappingInfo { // Offsets set in PrepareLayout. @@ -550,7 +577,7 @@ bool OatWriter::WriteAndOpenDexFiles( bool OatWriter::StartRoData(const std::vector& dex_files, OutputStream* oat_rodata, - SafeMap* key_value_store) { + OatKeyValueStore* key_value_store) { CHECK(write_state_ == WriteState::kStartRoData); // Record the ELF rodata section offset, i.e. the beginning of the OAT data. @@ -1972,9 +1999,18 @@ bool OatWriter::VisitDexMethods(DexMethodVisitor* visitor) { return true; } -size_t OatWriter::InitOatHeader(uint32_t num_dex_files, - SafeMap* key_value_store) { +size_t OatWriter::InitOatHeader(uint32_t num_dex_files, OatKeyValueStore* key_value_store) { TimingLogger::ScopedTiming split("InitOatHeader", timings_); + + // `key_value_store` only exists in the first oat file in a multi-image boot image. + if (key_value_store != nullptr) { + // Add non-deterministic fields if they don't exist. These fields should always exist with fixed + // lengths. + for (auto [field, length] : OatHeader::kNonDeterministicFieldsAndLengths) { + key_value_store->map_.FindOrAdd(std::string(field)); + } + } + // Check that oat version when runtime was compiled matches the oat version // when dex2oat was compiled. We have seen cases where they got out of sync. constexpr std::array dex2oat_oat_version = OatHeader::kOatVersion; @@ -1982,7 +2018,7 @@ size_t OatWriter::InitOatHeader(uint32_t num_dex_files, oat_header_ = OatHeader::Create(GetCompilerOptions().GetInstructionSet(), GetCompilerOptions().GetInstructionSetFeatures(), num_dex_files, - key_value_store, + key_value_store != nullptr ? &key_value_store->map_ : nullptr, oat_data_offset_); size_oat_header_ += sizeof(OatHeader); size_oat_header_key_value_store_ += oat_header_->GetHeaderSize() - sizeof(OatHeader); @@ -2751,10 +2787,7 @@ bool OatWriter::WriteHeader(OutputStream* out) { // Update checksum with header data. DCHECK_EQ(oat_header_->GetChecksum(), 0u); // For checksum calculation. - const uint8_t* header_begin = reinterpret_cast(oat_header_); - const uint8_t* header_end = oat_header_->GetKeyValueStore() + oat_header_->GetKeyValueStoreSize(); - uint32_t old_checksum = oat_checksum_; - oat_checksum_ = adler32(old_checksum, header_begin, header_end - header_begin); + oat_header_->ComputeChecksum(&oat_checksum_); oat_header_->SetChecksum(oat_checksum_); const size_t file_offset = oat_data_offset_; diff --git a/dex2oat/linker/oat_writer.h b/dex2oat/linker/oat_writer.h index 5a775fa517..4f50b1a39c 100644 --- a/dex2oat/linker/oat_writer.h +++ b/dex2oat/linker/oat_writer.h @@ -70,6 +70,29 @@ enum class CopyOption { kOnlyIfCompressed }; +class OatKeyValueStore { + public: + // Puts a key value pair whose key is in `OatHeader::kNonDeterministicFieldsAndLengths`. + bool PutNonDeterministic(const std::string& k, + const std::string& v, + bool allow_truncation = false); + + // Puts a key value pair whose key is in `OatHeader::kDeterministicFields`. + void Put(const std::string& k, const std::string& v); + + // Puts a key value pair whose key is in `OatHeader::kDeterministicFields`. + void Put(const std::string& k, bool v); + + // Makes sure calls with `const char*` falls into the overload for `std::string`, not the one for + // `bool`. + void Put(const std::string& k, const char* v) { Put(k, std::string(v)); } + + private: + SafeMap map_; + + friend class OatWriter; +}; + // OatHeader variable length with count of D OatDexFiles // // TypeLookupTable[0] one descriptor to class def index hash table for each OatDexFile. @@ -167,7 +190,7 @@ class OatWriter { // Start writing .rodata, including supporting data structures for dex files. bool StartRoData(const std::vector& dex_files, OutputStream* oat_rodata, - SafeMap* key_value_store); + OatKeyValueStore* key_value_store); // Initialize the writer with the given parameters. void Initialize(const CompilerDriver* compiler_driver, const VerificationResults* verification_results, @@ -291,7 +314,7 @@ class OatWriter { void WriteVerifierDeps(verifier::VerifierDeps* verifier_deps, /*out*/std::vector* buffer); - size_t InitOatHeader(uint32_t num_dex_files, SafeMap* key_value_store); + size_t InitOatHeader(uint32_t num_dex_files, OatKeyValueStore* key_value_store); size_t InitClassOffsets(size_t offset); size_t InitOatClasses(size_t offset); size_t InitOatMaps(size_t offset); diff --git a/dex2oat/linker/oat_writer_test.cc b/dex2oat/linker/oat_writer_test.cc index f3b598fe77..538c1abc9a 100644 --- a/dex2oat/linker/oat_writer_test.cc +++ b/dex2oat/linker/oat_writer_test.cc @@ -14,8 +14,11 @@ * limitations under the License. */ -#include "android-base/stringprintf.h" +#include "oat_writer.h" +#include + +#include "android-base/stringprintf.h" #include "arch/instruction_set_features.h" #include "art_method-inl.h" #include "base/file_utils.h" @@ -35,6 +38,7 @@ #include "driver/compiler_driver.h" #include "driver/compiler_options.h" #include "entrypoints/quick/quick_entrypoints.h" +#include "gtest/gtest.h" #include "linker/elf_writer.h" #include "linker/elf_writer_quick.h" #include "linker/multi_oat_relative_patcher.h" @@ -104,7 +108,7 @@ class OatTest : public CommonCompilerDriverTest { bool WriteElf(File* vdex_file, File* oat_file, const std::vector& dex_files, - SafeMap& key_value_store, + OatKeyValueStore& key_value_store, bool verify) { TimingLogger timings("WriteElf", false, false); ClearBootImageOption(); @@ -124,7 +128,7 @@ class OatTest : public CommonCompilerDriverTest { bool WriteElf(File* vdex_file, File* oat_file, const std::vector& dex_filenames, - SafeMap& key_value_store, + OatKeyValueStore& key_value_store, bool verify, CopyOption copy, ProfileCompilationInfo* profile_compilation_info) { @@ -143,7 +147,7 @@ class OatTest : public CommonCompilerDriverTest { File* oat_file, File&& dex_file_fd, const char* location, - SafeMap& key_value_store, + OatKeyValueStore& key_value_store, bool verify, CopyOption copy, ProfileCompilationInfo* profile_compilation_info = nullptr) { @@ -159,7 +163,7 @@ class OatTest : public CommonCompilerDriverTest { bool DoWriteElf(File* vdex_file, File* oat_file, OatWriter& oat_writer, - SafeMap& key_value_store, + OatKeyValueStore& key_value_store, bool verify, CopyOption copy) { std::unique_ptr elf_writer = CreateElfWriterQuick( @@ -426,7 +430,7 @@ TEST_F(OatTest, WriteRead) { } ScratchFile tmp_base, tmp_oat(tmp_base, ".oat"), tmp_vdex(tmp_base, ".vdex"); - SafeMap key_value_store; + OatKeyValueStore key_value_store; key_value_store.Put(OatHeader::kBootClassPathChecksumsKey, "testkey"); bool success = WriteElf(tmp_vdex.GetFile(), tmp_oat.GetFile(), @@ -494,6 +498,67 @@ TEST_F(OatTest, WriteRead) { } } +TEST_F(OatTest, ChecksumDeterminism) { + ClassLinker* class_linker = Runtime::Current()->GetClassLinker(); + SetupCompiler(/*compiler_options=*/{}); + + if (kCompile) { + TimingLogger timings("OatTest::ChecksumDeterminism", /*precise=*/false, /*verbose=*/false); + CompileAll(/*class_loader=*/nullptr, class_linker->GetBootClassPath(), &timings); + } + + auto write_elf_and_get_checksum = [&](OatKeyValueStore& key_value_store, + /*out*/ uint32_t* checksum) { + ScratchFile tmp_base, tmp_oat(tmp_base, ".oat"), tmp_vdex(tmp_base, ".vdex"); + + bool success = WriteElf(tmp_vdex.GetFile(), + tmp_oat.GetFile(), + class_linker->GetBootClassPath(), + key_value_store, + /*verify=*/false); + ASSERT_TRUE(success); + + std::string error_msg; + std::unique_ptr oat_file(OatFile::Open(/*zip_fd=*/-1, + tmp_oat.GetFilename(), + tmp_oat.GetFilename(), + /*executable=*/false, + /*low_4gb=*/true, + &error_msg)); + ASSERT_TRUE(oat_file.get() != nullptr) << error_msg; + const OatHeader& oat_header = oat_file->GetOatHeader(); + ASSERT_TRUE(oat_header.IsValid()); + *checksum = oat_header.GetChecksum(); + }; + + uint32_t checksum_1, checksum_2, checksum_3; + + { + OatKeyValueStore key_value_store; + key_value_store.Put(OatHeader::kBootClassPathChecksumsKey, "testkey"); + ASSERT_NO_FATAL_FAILURE(write_elf_and_get_checksum(key_value_store, &checksum_1)); + } + + { + // Put non-deterministic fields. This should not affect the checksum. + OatKeyValueStore key_value_store; + key_value_store.Put(OatHeader::kBootClassPathChecksumsKey, "testkey"); + key_value_store.PutNonDeterministic(OatHeader::kDex2OatCmdLineKey, "cmdline"); + key_value_store.PutNonDeterministic(OatHeader::kApexVersionsKey, "apex-versions"); + ASSERT_NO_FATAL_FAILURE(write_elf_and_get_checksum(key_value_store, &checksum_2)); + EXPECT_EQ(checksum_1, checksum_2); + } + + { + // Put deterministic fields. This should affect the checksum. + OatKeyValueStore key_value_store; + key_value_store.Put(OatHeader::kBootClassPathChecksumsKey, "testkey"); + key_value_store.Put(OatHeader::kClassPathKey, "classpath"); + ASSERT_NO_FATAL_FAILURE(write_elf_and_get_checksum(key_value_store, &checksum_3)); + EXPECT_NE(checksum_1, checksum_3); + } +} + TEST_F(OatTest, OatHeaderSizeCheck) { // If this test is failing and you have to update these constants, // it is time to update OatHeader::kOatVersion @@ -548,7 +613,7 @@ TEST_F(OatTest, EmptyTextSection) { CompileAll(class_loader, dex_files, &timings); ScratchFile tmp_base, tmp_oat(tmp_base, ".oat"), tmp_vdex(tmp_base, ".vdex"); - SafeMap key_value_store; + OatKeyValueStore key_value_store; bool success = WriteElf(tmp_vdex.GetFile(), tmp_oat.GetFile(), dex_files, @@ -617,7 +682,7 @@ void OatTest::TestDexFileInput(bool verify, bool low_4gb, bool use_profile) { input_dexfiles.push_back(std::move(dex_file2_data)); scratch_files.push_back(&dex_file2); - SafeMap key_value_store; + OatKeyValueStore key_value_store; { // Test using the AddDexFileSource() interface with the dex files. ScratchFile tmp_base, tmp_oat(tmp_base, ".oat"), tmp_vdex(tmp_base, ".vdex"); @@ -746,7 +811,7 @@ void OatTest::TestZipFileInput(bool verify, CopyOption copy) { success = zip_builder.Finish(); ASSERT_TRUE(success) << strerror(errno); - SafeMap key_value_store; + OatKeyValueStore key_value_store; { // Test using the AddDexFileSource() interface with the zip file. std::vector input_filenames = { zip_file.GetFilename().c_str() }; @@ -865,7 +930,7 @@ void OatTest::TestZipFileInputWithEmptyDex() { success = zip_builder.Finish(); ASSERT_TRUE(success) << strerror(errno); - SafeMap key_value_store; + OatKeyValueStore key_value_store; std::vector input_filenames = { zip_file.GetFilename().c_str() }; ScratchFile oat_file, vdex_file(oat_file, ".vdex"); std::unique_ptr profile_compilation_info(new ProfileCompilationInfo()); diff --git a/oatdump/oatdump.cc b/oatdump/oatdump.cc index c567a5e730..f7320765a7 100644 --- a/oatdump/oatdump.cc +++ b/oatdump/oatdump.cc @@ -490,12 +490,11 @@ class OatDumper { // Print the key-value store. { os << "KEY VALUE STORE:\n"; - size_t index = 0; + uint32_t offset = 0; const char* key; const char* value; - while (oat_header.GetStoreKeyValuePairByIndex(index, &key, &value)) { + while (oat_header.GetNextStoreKeyValuePair(&offset, &key, &value)) { os << key << " = " << value << "\n"; - index++; } os << "\n"; } diff --git a/runtime/oat/oat.cc b/runtime/oat/oat.cc index 9a5dd9dfdf..840825cea8 100644 --- a/runtime/oat/oat.cc +++ b/runtime/oat/oat.cc @@ -17,9 +17,10 @@ #include "oat.h" #include +#include +#include "android-base/logging.h" #include "android-base/stringprintf.h" - #include "arch/instruction_set.h" #include "arch/instruction_set_features.h" #include "base/bit_utils.h" @@ -37,6 +38,13 @@ static size_t ComputeOatHeaderSize(const SafeMap* vari for ( ; it != end; ++it) { estimate += it->first.length() + 1; estimate += it->second.length() + 1; + + size_t non_deterministic_field_length = OatHeader::GetNonDeterministicFieldLength(it->first); + if (non_deterministic_field_length > 0u) { + DCHECK_LE(it->second.length(), non_deterministic_field_length); + size_t padding = non_deterministic_field_length - it->second.length(); + estimate += padding; + } } } return sizeof(OatHeader) + estimate; @@ -367,67 +375,79 @@ const uint8_t* OatHeader::GetKeyValueStore() const { const char* OatHeader::GetStoreValueByKey(const char* key) const { std::string_view key_view(key); - const char* ptr = reinterpret_cast(&key_value_store_); - const char* end = ptr + key_value_store_size_; - - while (ptr < end) { - // Scan for a closing zero. - const char* str_end = reinterpret_cast(memchr(ptr, 0, end - ptr)); - if (UNLIKELY(str_end == nullptr)) { - LOG(WARNING) << "OatHeader: Unterminated key in key value store."; - return nullptr; - } - const char* value_start = str_end + 1; - const char* value_end = - reinterpret_cast(memchr(value_start, 0, end - value_start)); - if (UNLIKELY(value_end == nullptr)) { - LOG(WARNING) << "OatHeader: Unterminated value in key value store."; - return nullptr; - } - if (key_view == std::string_view(ptr, str_end - ptr)) { + + uint32_t offset = 0; + const char* current_key; + const char* value; + while (GetNextStoreKeyValuePair(&offset, ¤t_key, &value)) { + if (key_view == current_key) { // Same as key. - return value_start; + return value; } - // Different from key. Advance over the value. - ptr = value_end + 1; } + // Not found. return nullptr; } -bool OatHeader::GetStoreKeyValuePairByIndex(size_t index, - const char** key, - const char** value) const { - const char* ptr = reinterpret_cast(&key_value_store_); - const char* end = ptr + key_value_store_size_; - size_t counter = index; +bool OatHeader::GetNextStoreKeyValuePair(/*inout*/ uint32_t* offset, + /*out*/ const char** key, + /*out*/ const char** value) const { + if (*offset >= key_value_store_size_) { + return false; + } - while (ptr < end) { - // Scan for a closing zero. - const char* str_end = reinterpret_cast(memchr(ptr, 0, end - ptr)); - if (UNLIKELY(str_end == nullptr)) { - LOG(WARNING) << "OatHeader: Unterminated key in key value store."; - return false; - } - const char* value_start = str_end + 1; - const char* value_end = - reinterpret_cast(memchr(value_start, 0, end - value_start)); - if (UNLIKELY(value_end == nullptr)) { - LOG(WARNING) << "OatHeader: Unterminated value in key value store."; + const char* start = reinterpret_cast(&key_value_store_); + const char* ptr = start + *offset; + const char* end = start + key_value_store_size_; + + // Scan for a closing zero. + const char* str_end = reinterpret_cast(memchr(ptr, 0, end - ptr)); + if (UNLIKELY(str_end == nullptr)) { + LOG(WARNING) << "OatHeader: Unterminated key in key value store."; + return false; + } + const char* value_start = str_end + 1; + const char* value_end = reinterpret_cast(memchr(value_start, 0, end - value_start)); + if (UNLIKELY(value_end == nullptr)) { + LOG(WARNING) << "OatHeader: Unterminated value in key value store."; + return false; + } + + *key = ptr; + *value = value_start; + + // Advance over the value. + size_t key_len = str_end - ptr; + size_t value_len = value_end - value_start; + size_t non_deterministic_field_length = GetNonDeterministicFieldLength(*key); + if (non_deterministic_field_length > 0u) { + if (UNLIKELY(value_len > non_deterministic_field_length)) { + LOG(WARNING) << "OatHeader: Non-deterministic field too long in key value store."; return false; } - if (counter == 0) { - *key = ptr; - *value = value_start; - return true; - } else { - --counter; + *offset += key_len + 1 + non_deterministic_field_length + 1; + } else { + *offset += key_len + 1 + value_len + 1; + } + + return true; +} + +void OatHeader::ComputeChecksum(/*inout*/ uint32_t* checksum) const { + *checksum = adler32(*checksum, reinterpret_cast(this), sizeof(OatHeader)); + + uint32_t last_offset = 0; + uint32_t offset = 0; + const char* key; + const char* value; + while (GetNextStoreKeyValuePair(&offset, &key, &value)) { + if (IsDeterministicField(key)) { + // Update the checksum. + *checksum = adler32(*checksum, GetKeyValueStore() + last_offset, offset - last_offset); } - // Advance over the value. - ptr = value_end + 1; + last_offset = offset; } - // Not found. - return false; } size_t OatHeader::GetHeaderSize() const { @@ -478,6 +498,14 @@ void OatHeader::Flatten(const SafeMap* key_value_store data_ptr += it->first.length() + 1; strlcpy(data_ptr, it->second.c_str(), it->second.length() + 1); data_ptr += it->second.length() + 1; + + size_t non_deterministic_field_length = GetNonDeterministicFieldLength(it->first); + if (non_deterministic_field_length > 0u) { + DCHECK_LE(it->second.length(), non_deterministic_field_length); + size_t padding = non_deterministic_field_length - it->second.length(); + memset(data_ptr, 0, padding); + data_ptr += padding; + } } } key_value_store_size_ = data_ptr - reinterpret_cast(&key_value_store_); diff --git a/runtime/oat/oat.h b/runtime/oat/oat.h index 53f9497d1f..0061c90da9 100644 --- a/runtime/oat/oat.h +++ b/runtime/oat/oat.h @@ -18,6 +18,9 @@ #define ART_RUNTIME_OAT_OAT_H_ #include +#include +#include +#include #include #include "base/compiler_filter.h" @@ -44,8 +47,8 @@ std::ostream& operator<<(std::ostream& stream, StubType stub_type); class EXPORT PACKED(4) OatHeader { public: static constexpr std::array kOatMagic { { 'o', 'a', 't', '\n' } }; - // Last oat version changed reason: Restore to 16 KB ELF alignment. - static constexpr std::array kOatVersion{{'2', '5', '8', '\0'}}; + // Last oat version changed reason: Ensure oat checksum determinism across hosts and devices. + static constexpr std::array kOatVersion{{'2', '5', '9', '\0'}}; static constexpr const char* kDex2OatCmdLineKey = "dex2oat-cmdline"; static constexpr const char* kDebuggableKey = "debuggable"; @@ -59,6 +62,34 @@ class EXPORT PACKED(4) OatHeader { static constexpr const char* kCompilationReasonKey = "compilation-reason"; static constexpr const char* kRequiresImage = "requires-image"; + // Fields listed here are key value store fields that are deterministic across hosts and devices, + // meaning they should have exactly the same value when the oat file is generated on different + // hosts and devices for the same app / boot image and for the same device model with the same + // compiler options. If you are adding a new field that doesn't hold this property, put it in + // `kNonDeterministicFieldsAndLengths` and assign a length limit. + // + // When writing the oat header, the non-deterministic fields are padded to their length limits and + // excluded from the oat checksum computation. This makes the oat checksum deterministic across + // hosts and devices, which is important for Cloud Compilation, where we generate an oat file on a + // host and use it on a device. + static constexpr std::array kDeterministicFields{ + kDebuggableKey, + kNativeDebuggableKey, + kCompilerFilter, + kClassPathKey, + kBootClassPathKey, + kBootClassPathChecksumsKey, + kConcurrentCopying, + kCompilationReasonKey, + kRequiresImage, + }; + + static constexpr std::array, 2> + kNonDeterministicFieldsAndLengths{ + std::make_pair(kDex2OatCmdLineKey, 2048), + std::make_pair(kApexVersionsKey, 1024), + }; + static constexpr const char kTrueValue[] = "true"; static constexpr const char kFalseValue[] = "false"; @@ -70,6 +101,24 @@ class EXPORT PACKED(4) OatHeader { uint32_t base_oat_offset = 0u); static void Delete(OatHeader* header); + static constexpr bool IsDeterministicField(std::string_view key) { + for (std::string_view field : kDeterministicFields) { + if (field == key) { + return true; + } + } + return false; + } + + static constexpr size_t GetNonDeterministicFieldLength(std::string_view key) { + for (auto [field, length] : kNonDeterministicFieldsAndLengths) { + if (field == key) { + return length; + } + } + return 0; + } + bool IsValid() const; std::string GetValidationErrorMessage() const; static void CheckOatVersion(std::array version); @@ -116,7 +165,13 @@ class EXPORT PACKED(4) OatHeader { uint32_t GetKeyValueStoreSize() const; const uint8_t* GetKeyValueStore() const; const char* GetStoreValueByKey(const char* key) const; - bool GetStoreKeyValuePairByIndex(size_t index, const char** key, const char** value) const; + + // Returns the next key-value pair, at the given offset. On success, updates `offset`. + // The expected use case is to start the iteration with an offset initialized to zero and + // repeatedly call this function with the same offset pointer, until the function returns false. + bool GetNextStoreKeyValuePair(/*inout*/ uint32_t* offset, + /*out*/ const char** key, + /*out*/ const char** value) const; size_t GetHeaderSize() const; bool IsDebuggable() const; @@ -127,6 +182,8 @@ class EXPORT PACKED(4) OatHeader { const uint8_t* GetOatAddress(StubType type) const; + void ComputeChecksum(/*inout*/ uint32_t* checksum) const; + private: bool KeyHasValue(const char* key, const char* value, size_t value_size) const; -- cgit v1.2.3-59-g8ed1b From ca2362c05e2fc05f9211e39ecb74857195b27780 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Mon, 10 Mar 2025 06:39:58 -0800 Subject: Remove memfd_create_compat(). Multiple places were already unconditionally calling memfd_create(), and ART no longer runs tests on fugu (whose ancient kernel didn't have memfd_create()). Change-Id: I8ca96d75a9e6f4fe5395b210f60d9920808bb26c --- compiler/common_compiler_test.cc | 2 +- libartbase/base/memfd.cc | 23 ----------------------- libartbase/base/memfd.h | 4 ---- openjdkjvmti/ti_search.cc | 2 +- runtime/gc/space/image_space.cc | 6 +++--- 5 files changed, 5 insertions(+), 32 deletions(-) diff --git a/compiler/common_compiler_test.cc b/compiler/common_compiler_test.cc index e54c85f747..ef915e4152 100644 --- a/compiler/common_compiler_test.cc +++ b/compiler/common_compiler_test.cc @@ -63,7 +63,7 @@ class CommonCompilerTestImpl::CodeAndMetadata { const uint32_t capacity = RoundUp(code_offset + code_size, page_size); // Create a memfd handle with sufficient capacity. - android::base::unique_fd mem_fd(art::memfd_create_compat("test code", /*flags=*/ 0)); + android::base::unique_fd mem_fd(art::memfd_create("test code", /*flags=*/ 0)); CHECK_GE(mem_fd.get(), 0); int err = ftruncate(mem_fd, capacity); CHECK_EQ(err, 0); diff --git a/libartbase/base/memfd.cc b/libartbase/base/memfd.cc index 3b9872b295..4530f8199b 100644 --- a/libartbase/base/memfd.cc +++ b/libartbase/base/memfd.cc @@ -64,29 +64,6 @@ int memfd_create([[maybe_unused]] const char* name, [[maybe_unused]] unsigned in #endif // __NR_memfd_create -// This is a wrapper that will attempt to simulate memfd_create if normal running fails. -int memfd_create_compat(const char* name, unsigned int flags) { - int res = memfd_create(name, flags); - if (res >= 0) { - return res; - } -#if !defined(_WIN32) - // Try to create an anonymous file with tmpfile that we can use instead. - if (flags == 0) { - FILE* file = tmpfile(); - if (file != nullptr) { - // We want the normal 'dup' semantics since memfd_create without any flags isn't CLOEXEC. - // Unfortunately on some android targets we will compiler error if we use dup directly and so - // need to use fcntl. - int nfd = fcntl(fileno(file), F_DUPFD, /*lowest allowed fd*/ 0); - fclose(file); - return nfd; - } - } -#endif - return res; -} - #if defined(__BIONIC__) static bool IsSealFutureWriteSupportedInternal() { diff --git a/libartbase/base/memfd.h b/libartbase/base/memfd.h index 3c27dcb9e3..b288f7bc37 100644 --- a/libartbase/base/memfd.h +++ b/libartbase/base/memfd.h @@ -69,10 +69,6 @@ namespace art { // check for safety on older kernels (b/116769556).. int memfd_create(const char* name, unsigned int flags); -// Call memfd(2) if available on platform and return result. Try to give us an unlinked FD in some -// other way if memfd fails or isn't supported. -int memfd_create_compat(const char* name, unsigned int flags); - // Return whether the kernel supports sealing future writes of a memfd. bool IsSealFutureWriteSupported(); diff --git a/openjdkjvmti/ti_search.cc b/openjdkjvmti/ti_search.cc index 30a889aaa5..e441010939 100644 --- a/openjdkjvmti/ti_search.cc +++ b/openjdkjvmti/ti_search.cc @@ -278,7 +278,7 @@ jvmtiError SearchUtil::AddToDexClassLoaderInMemory(jvmtiEnv* jvmti_env, // lot of code as well. // Create a memfd - art::File file(art::memfd_create_compat("JVMTI InMemory Added dex file", 0), /*check-usage*/true); + art::File file(art::memfd_create("JVMTI InMemory Added dex file", 0), /*check-usage*/true); if (file.Fd() < 0) { char* reason = strerror(errno); JVMTI_LOG(ERROR, jvmti_env) << "Unable to create memfd due to " << reason; diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc index 3d55c04828..97512bc79c 100644 --- a/runtime/gc/space/image_space.cc +++ b/runtime/gc/space/image_space.cc @@ -1967,9 +1967,9 @@ bool ImageSpace::BootImageLayout::CompileBootclasspathElements( std::string art_filename = ExpandLocation(base_filename, bcp_index); std::string vdex_filename = ImageHeader::GetVdexLocationFromImageLocation(art_filename); std::string oat_filename = ImageHeader::GetOatLocationFromImageLocation(art_filename); - android::base::unique_fd art_fd(memfd_create_compat(art_filename.c_str(), /*flags=*/ 0)); - android::base::unique_fd vdex_fd(memfd_create_compat(vdex_filename.c_str(), /*flags=*/ 0)); - android::base::unique_fd oat_fd(memfd_create_compat(oat_filename.c_str(), /*flags=*/ 0)); + android::base::unique_fd art_fd(memfd_create(art_filename.c_str(), /*flags=*/ 0)); + android::base::unique_fd vdex_fd(memfd_create(vdex_filename.c_str(), /*flags=*/ 0)); + android::base::unique_fd oat_fd(memfd_create(oat_filename.c_str(), /*flags=*/ 0)); if (art_fd.get() == -1 || vdex_fd.get() == -1 || oat_fd.get() == -1) { *error_msg = StringPrintf("Failed to create memfd handles for compiling bootclasspath for %s", boot_class_path_locations_[bcp_index].c_str()); -- cgit v1.2.3-59-g8ed1b From 11a5d95861c061ca65ecbf28597e2176a5448c8b Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Tue, 11 Mar 2025 14:39:13 +0000 Subject: Fix --oat-location for mainline boot image extension. dex2oat uses the basename of the oat location to determine the soname of the oat file in the ELF header. - If the output is specified by "--oat-file", dex2oat takes the oat location from "--oat-file" and uses it as is. - If the output is specified by "--oat-fd", dex2oat takes the oat location from "--oat-location", and for boot image extensions, dex2oat expends the oat location with the name of the first input dex file. For the mainline boot image extension specifically, before this change, odrefresh passes "--oat-location=/data/misc/.../boot-framework-adservices.oat", so dex2oat expends it to "boot-framework-adservices-framework-adservices.oat". This is unexpected, and it causes a discrepancy between odrefresh and Cloud Compilation. Cloud Compilation is typically performed on host, with the output specified by "--oat-file", so the soname of the mainline boot image extension is "boot-framework-adservices.oat". This discrepancy causes all the offsets in the oat file to differ. After this change, odrefresh passes "--oat-location=/data/misc/.../boot.oat", so the soname after expansion is "boot-framework-adservices.oat", which is correct. Bug: 372868052 Test: Generate a mainline boot image extension on host. See the checksum being identical to the one on device. Change-Id: I9bfd165a9505e889c3da7d4bc408f9a4f2e52714 --- odrefresh/odrefresh.cc | 10 +++++++++- odrefresh/odrefresh_test.cc | 30 +++++++++++++++++++++--------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc index fc782bc697..92912b7bfa 100644 --- a/odrefresh/odrefresh.cc +++ b/odrefresh/odrefresh.cc @@ -1762,7 +1762,6 @@ WARN_UNUSED CompilationResult OnDeviceRefresh::RunDex2oat( } } - args.Add("--oat-location=%s", artifacts.OatPath()); std::pair location_kind_pairs[] = { std::make_pair(artifacts.ImagePath(), artifacts.ImageKind()), std::make_pair(artifacts.OatPath(), "oat"), @@ -1910,9 +1909,16 @@ OnDeviceRefresh::RunDex2oatForBootClasspath(const std::string& staging_dir, preloaded_classes_file, strerror(errno))); } + args.Add("--oat-location=%s", OdrArtifacts::ForBootImage(output_path).OatPath()); } else { // Mainline extension. args.Add("--compiler-filter=%s", kMainlineCompilerFilter); + // For boot image extensions, dex2oat takes the oat location of the primary boot image and + // expends it with the name of the first input dex file. + args.Add("--oat-location=%s", + OdrArtifacts::ForBootImage( + GetPrimaryBootImagePath(/*on_system=*/false, /*minimal=*/false, isa)) + .OatPath()); } const OdrSystemProperties& system_properties = config_.GetSystemProperties(); @@ -2080,6 +2086,8 @@ WARN_UNUSED CompilationResult OnDeviceRefresh::RunDex2oatForSystemServer( args.Add("--class-loader-context-fds=%s", Join(fds, ':')); } + args.Add("--oat-location=%s", OdrArtifacts::ForSystemServer(output_path).OatPath()); + const OdrSystemProperties& system_properties = config_.GetSystemProperties(); args.AddRuntimeIfNonEmpty("-Xms%s", system_properties.GetOrEmpty("dalvik.vm.dex2oat-Xms")) .AddRuntimeIfNonEmpty("-Xmx%s", system_properties.GetOrEmpty("dalvik.vm.dex2oat-Xmx")); diff --git a/odrefresh/odrefresh_test.cc b/odrefresh/odrefresh_test.cc index b19a4225b4..7f4e990ffe 100644 --- a/odrefresh/odrefresh_test.cc +++ b/odrefresh/odrefresh_test.cc @@ -48,12 +48,14 @@ namespace art { namespace odrefresh { +using ::android::base::Basename; using ::android::base::Split; using ::android::modules::sdklevel::IsAtLeastU; using ::testing::_; using ::testing::AllOf; using ::testing::Contains; using ::testing::ElementsAre; +using ::testing::EndsWith; using ::testing::Not; using ::testing::ResultOf; using ::testing::Return; @@ -336,7 +338,7 @@ TEST_F(OdRefreshTest, BootImageMainlineExtension) { FdOf(framework_jar_), FdOf(conscrypt_jar_), FdOf(framework_wifi_jar_)))), - Contains(Flag("--oat-location=", dalvik_cache_dir_ + "/x86_64/boot-conscrypt.oat")), + Contains(Flag("--oat-location=", dalvik_cache_dir_ + "/x86_64/boot.oat")), Not(Contains(Flag("--base=", _))), Contains(Flag("--boot-image=", _)), Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_)))))) @@ -435,11 +437,15 @@ TEST_F(OdRefreshTest, BootClasspathJarsFallback) { } TEST_F(OdRefreshTest, AllSystemServerJars) { - EXPECT_CALL(*mock_exec_utils_, - DoExecAndReturnCode(AllOf(Contains(Flag("--dex-file=", location_provider_jar_)), - Contains("--class-loader-context=PCL[]"), - Not(Contains(Flag("--class-loader-context-fds=", _))), - Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_)))))) + EXPECT_CALL( + *mock_exec_utils_, + DoExecAndReturnCode(AllOf( + Contains(Flag("--dex-file=", location_provider_jar_)), + Contains("--class-loader-context=PCL[]"), + Not(Contains(Flag("--class-loader-context-fds=", _))), + Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_))), + Contains(Flag("--oat-location=", + EndsWith("@" + Basename(location_provider_jar_) + "@classes.odex")))))) .WillOnce(Return(0)); EXPECT_CALL( *mock_exec_utils_, @@ -447,7 +453,9 @@ TEST_F(OdRefreshTest, AllSystemServerJars) { Contains(Flag("--dex-file=", services_jar_)), Contains(Flag("--class-loader-context=", ART_FORMAT("PCL[{}]", location_provider_jar_))), Contains(Flag("--class-loader-context-fds=", FdOf(location_provider_jar_))), - Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_)))))) + Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_))), + Contains( + Flag("--oat-location=", EndsWith("@" + Basename(services_jar_) + "@classes.odex")))))) .WillOnce(Return(0)); EXPECT_CALL( *mock_exec_utils_, @@ -457,7 +465,9 @@ TEST_F(OdRefreshTest, AllSystemServerJars) { ART_FORMAT("PCL[];PCL[{}:{}]", location_provider_jar_, services_jar_))), Contains(ListFlag("--class-loader-context-fds=", ElementsAre(FdOf(location_provider_jar_), FdOf(services_jar_)))), - Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_)))))) + Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_))), + Contains(Flag("--oat-location=", + EndsWith("@" + Basename(services_foo_jar_) + "@classes.odex")))))) .WillOnce(Return(0)); EXPECT_CALL( *mock_exec_utils_, @@ -467,7 +477,9 @@ TEST_F(OdRefreshTest, AllSystemServerJars) { ART_FORMAT("PCL[];PCL[{}:{}]", location_provider_jar_, services_jar_))), Contains(ListFlag("--class-loader-context-fds=", ElementsAre(FdOf(location_provider_jar_), FdOf(services_jar_)))), - Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_)))))) + Contains(Flag("--cache-info-fd=", FdOf(cache_info_xml_))), + Contains(Flag("--oat-location=", + EndsWith("@" + Basename(services_bar_jar_) + "@classes.odex")))))) .WillOnce(Return(0)); EXPECT_EQ( -- cgit v1.2.3-59-g8ed1b