Skip to content

Commit

Permalink
Do not encode out of range dex pcs in the profile.
Browse files Browse the repository at this point in the history
Test: 2271-profile-inline-cache
Bug: 330376053
Change-Id: Ib67013586c60712ccf894405f4fa8254643b273b
  • Loading branch information
Nicolas Geoffray committed Apr 18, 2024
1 parent f3cb22e commit c7bc9f5
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 13 deletions.
34 changes: 31 additions & 3 deletions libprofile/profile/profile_compilation_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
#include "base/unix_file/fd_file.h"
#include "base/utils.h"
#include "base/zip_archive.h"
#include "dex/code_item_accessors-inl.h"
#include "dex/descriptors_names.h"
#include "dex/dex_file_loader.h"

Expand Down Expand Up @@ -676,9 +677,10 @@ ProfileCompilationInfo::ProfileSampleAnnotation ProfileCompilationInfo::GetAnnot

bool ProfileCompilationInfo::AddMethods(const std::vector<ProfileMethodInfo>& methods,
MethodHotness::Flag flags,
const ProfileSampleAnnotation& annotation) {
const ProfileSampleAnnotation& annotation,
bool is_test) {
for (const ProfileMethodInfo& method : methods) {
if (!AddMethod(method, flags, annotation)) {
if (!AddMethod(method, flags, annotation, is_test)) {
return false;
}
}
Expand Down Expand Up @@ -1316,7 +1318,8 @@ ProfileCompilationInfo::ExtraDescriptorIndex ProfileCompilationInfo::AddExtraDes

bool ProfileCompilationInfo::AddMethod(const ProfileMethodInfo& pmi,
MethodHotness::Flag flags,
const ProfileSampleAnnotation& annotation) {
const ProfileSampleAnnotation& annotation,
bool is_test) {
DexFileData* const data = GetOrAddDexFileData(pmi.ref.dex_file, annotation);
if (data == nullptr) { // checksum mismatch
return false;
Expand All @@ -1333,12 +1336,37 @@ bool ProfileCompilationInfo::AddMethod(const ProfileMethodInfo& pmi,
InlineCacheMap* inline_cache = data->FindOrAddHotMethod(pmi.ref.index);
DCHECK(inline_cache != nullptr);

const dex::MethodId& mid = pmi.ref.GetMethodId();
const DexFile& dex_file = *pmi.ref.dex_file;
const dex::ClassDef* class_def = dex_file.FindClassDef(mid.class_idx_);
// If `is_test` is true, we don't try to look at whether dex_pc fit in the
// code item of that method.
uint32_t dex_pc_max = 0u;
if (is_test) {
dex_pc_max = std::numeric_limits<uint32_t>::max();
} else {
if (class_def == nullptr || dex_file.GetClassData(*class_def) == nullptr) {
return true;
}
std::optional<uint32_t> offset = dex_file.GetCodeItemOffset(*class_def, pmi.ref.index);
if (!offset.has_value()) {
return true;
}
CodeItemInstructionAccessor accessor(dex_file, dex_file.GetCodeItem(offset.value()));
dex_pc_max = accessor.InsnsSizeInCodeUnits();
}

for (const ProfileMethodInfo::ProfileInlineCache& cache : pmi.inline_caches) {
if (cache.dex_pc >= std::numeric_limits<uint16_t>::max()) {
// Discard entries that don't fit the encoding. This should only apply to
// inlined inline caches. See also `HInliner::GetInlineCacheAOT`.
continue;
}
if (cache.dex_pc >= dex_pc_max) {
// Discard entries for inlined inline caches. We don't support them in
// profiles yet.
continue;
}
if (cache.is_missing_types) {
FindOrAddDexPc(inline_cache, cache.dex_pc)->SetIsMissingTypes();
continue;
Expand Down
8 changes: 6 additions & 2 deletions libprofile/profile/profile_compilation_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,9 +322,12 @@ class ProfileCompilationInfo {
//
// Note: if an annotation is provided, the methods/classes will be associated with the group
// (dex_file, sample_annotation). Each group keeps its unique set of methods/classes.
// `is_test` should be set to true for unit tests which create artificial dex
// files.
bool AddMethods(const std::vector<ProfileMethodInfo>& methods,
MethodHotness::Flag flags,
const ProfileSampleAnnotation& annotation = ProfileSampleAnnotation::kNone);
const ProfileSampleAnnotation& annotation = ProfileSampleAnnotation::kNone,
bool is_test = false);

// Find a type index in the `dex_file` if there is a `TypeId` for it. Otherwise,
// find or insert the descriptor in "extra descriptors" and return an artificial
Expand Down Expand Up @@ -411,7 +414,8 @@ class ProfileCompilationInfo {
// Note: see AddMethods docs for the handling of annotations.
bool AddMethod(const ProfileMethodInfo& pmi,
MethodHotness::Flag flags,
const ProfileSampleAnnotation& annotation = ProfileSampleAnnotation::kNone);
const ProfileSampleAnnotation& annotation = ProfileSampleAnnotation::kNone,
bool is_test = false);

// Bulk add sampled methods and/or hot methods for a single dex, fast since it only has one
// GetOrAddDexFileData call.
Expand Down
12 changes: 9 additions & 3 deletions libprofile/profile/profile_compilation_info_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1498,10 +1498,16 @@ TEST_F(ProfileCompilationInfoTest, AddMethodsProfileMethodInfoInlineCaches) {

// Add inline caches with the methods. The profile should record only the one for the hot method.
std::vector<TypeReference> types = {};
ProfileMethodInfo::ProfileInlineCache ic(/*dex_pc*/ 0, /*missing_types*/true, types);
ProfileMethodInfo::ProfileInlineCache ic(/*dex_pc=*/ 0, /*missing_types=*/ true, types);
std::vector<ProfileMethodInfo::ProfileInlineCache> inline_caches = {ic};
info.AddMethod(ProfileMethodInfo(hot, inline_caches), Hotness::kFlagHot);
info.AddMethod(ProfileMethodInfo(startup, inline_caches), Hotness::kFlagStartup);
info.AddMethod(ProfileMethodInfo(hot, inline_caches),
Hotness::kFlagHot,
ProfileSampleAnnotation::kNone,
/*is_test=*/ true);
info.AddMethod(ProfileMethodInfo(startup, inline_caches),
Hotness::kFlagStartup,
ProfileSampleAnnotation::kNone,
/*is_test=*/ true);

// Check the hot method's inline cache.
ProfileCompilationInfo::MethodHotness hot_hotness = GetMethod(info, dex1, hot.index);
Expand Down
12 changes: 8 additions & 4 deletions libprofile/profile/profile_test_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,10 @@ class ProfileTestHelper {
uint16_t method_idx,
Hotness::Flag flags,
const ProfileSampleAnnotation& annotation = ProfileSampleAnnotation::kNone) {
return info->AddMethod(
ProfileMethodInfo(MethodReference(dex, method_idx)), flags, annotation);
return info->AddMethod(ProfileMethodInfo(MethodReference(dex, method_idx)),
flags,
annotation,
/*is_test=*/ true);
}

static bool AddMethod(
Expand All @@ -68,8 +70,10 @@ class ProfileTestHelper {
const std::vector<ProfileInlineCache>& inline_caches,
Hotness::Flag flags,
const ProfileSampleAnnotation& annotation = ProfileSampleAnnotation::kNone) {
return info->AddMethod(
ProfileMethodInfo(MethodReference(dex, method_idx), inline_caches), flags, annotation);
return info->AddMethod(ProfileMethodInfo(MethodReference(dex, method_idx), inline_caches),
flags,
annotation,
/*is_test=*/ true);
}

static bool AddClass(ProfileCompilationInfo* info,
Expand Down
5 changes: 4 additions & 1 deletion runtime/jit/profiling_info_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,10 @@ class ProfileCompilationInfoTest : public CommonRuntimeTest {
profile_methods_map->Put(method, pmi);
}

if (!info.AddMethods(profile_methods, flags)
if (!info.AddMethods(profile_methods,
flags,
ProfileCompilationInfo::ProfileSampleAnnotation::kNone,
/*is_test=*/ true)
|| info.GetNumberOfMethods() != profile_methods.size()) {
return false;
}
Expand Down
20 changes: 20 additions & 0 deletions test/2271-profile-inline-cache/src/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public class Main {
private static Method sMethod2 = null;
private static Method sMethod3 = null;
private static Method sMethod4 = null;
private static Method sMethod5 = null;

public static void main(String[] args) throws Exception {
System.loadLibrary(args[0]);
Expand All @@ -39,6 +40,7 @@ public static void main(String[] args) throws Exception {
sMethod2 = Main.class.getDeclaredMethod("$noinline$method2", Base.class);
sMethod3 = Main.class.getDeclaredMethod("$noinline$method3", Base.class);
sMethod4 = Main.class.getDeclaredMethod("$noinline$method4", Base.class);
sMethod5 = Main.class.getDeclaredMethod("$noinline$method5", Base.class);

sFile = createTempFile();
sFile.deleteOnExit();
Expand Down Expand Up @@ -106,6 +108,16 @@ private static void test() throws Exception {
}
ensureMethodJitCompiled(sMethod4);
checkMethodHasInlineCache(sFile, sMethod4, Derived1.class, Derived2.class);

// This method is above the JIT threshold.
ensureJitBaselineCompiled(sMethod5);
try (ScopedAssertNoGc noGc = new ScopedAssertNoGc()) {
$noinline$method5(derived1);
$noinline$method5(derived2);
}
ensureMethodJitCompiled(sMethod5);
// We currently do not encode inlined inline caches.
checkMethodHasNoInlineCache(sFile, sMethod5);
}

private static void reset() {
Expand All @@ -131,6 +143,14 @@ private static void reset() {
obj.f();
}

public static void $noinline$method5(Base obj) {
$inline$callF(obj);
}

public static void $inline$callF(Base obj) {
obj.f();
}

public static class Base {
public void f() {}
}
Expand Down

0 comments on commit c7bc9f5

Please sign in to comment.