Skip to content

Commit

Permalink
Don't assume resolved classses are filled in for ResolvedFieldAccessTest
Browse files Browse the repository at this point in the history
The assumption that the dex cache type for class of the field is
resolved is not correct since FindClass does not fill in the
resolved type array. This resulted in crashes if dex_access_to ==
nullptr.

Also implemented for ResolvedMethodAccessTest

Bug: 31277064

Test: Launch ZeroTouch with profile guided compilation.

Change-Id: I40cfb15f3ae4fbfe941fd01ea46211bd86b6e6f7
  • Loading branch information
Mathieu Chartier committed Sep 9, 2016
1 parent 0e4a2f0 commit c90450a
Show file tree
Hide file tree
Showing 6 changed files with 101 additions and 5 deletions.
3 changes: 2 additions & 1 deletion build/Android.gtest.mk
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ GTEST_DEX_DIRECTORIES := \
MyClassNatives \
Nested \
NonStaticLeafMethods \
Packages \
ProtoCompare \
ProtoCompare2 \
ProfileTestMultiDex \
Expand Down Expand Up @@ -69,7 +70,7 @@ ART_GTEST_dex2oat_environment_tests_DEX_DEPS := Main MainStripped MultiDex Multi

ART_GTEST_class_linker_test_DEX_DEPS := Interfaces MultiDex MyClass Nested Statics StaticsFromCode
ART_GTEST_compiler_driver_test_DEX_DEPS := AbstractMethod StaticLeafMethods ProfileTestMultiDex
ART_GTEST_dex_cache_test_DEX_DEPS := Main
ART_GTEST_dex_cache_test_DEX_DEPS := Main Packages
ART_GTEST_dex_file_test_DEX_DEPS := GetMethodSignature Main Nested
ART_GTEST_dex2oat_test_DEX_DEPS := $(ART_GTEST_dex2oat_environment_tests_DEX_DEPS)
ART_GTEST_exception_test_DEX_DEPS := ExceptionHandle
Expand Down
34 changes: 30 additions & 4 deletions runtime/mirror/class-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,21 @@ inline bool Class::ResolvedFieldAccessTest(Class* access_to, ArtField* field,
// class rather than the declaring class itself.
DexCache* referrer_dex_cache = use_referrers_cache ? this->GetDexCache() : dex_cache;
uint32_t class_idx = referrer_dex_cache->GetDexFile()->GetFieldId(field_idx).class_idx_;
// The referenced class has already been resolved with the field, get it from the dex cache.
Class* dex_access_to = referrer_dex_cache->GetResolvedType(class_idx);
// The referenced class has already been resolved with the field, but may not be in the dex
// cache. Using ResolveType here without handles in the caller should be safe since there
// should be no thread suspension due to the class being resolved.
// TODO: Clean this up to use handles in the caller.
Class* dex_access_to;
{
StackHandleScope<2> hs(Thread::Current());
Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(referrer_dex_cache));
Handle<mirror::ClassLoader> h_class_loader(hs.NewHandle(access_to->GetClassLoader()));
dex_access_to = Runtime::Current()->GetClassLinker()->ResolveType(
*referrer_dex_cache->GetDexFile(),
class_idx,
h_dex_cache,
h_class_loader);
}
DCHECK(dex_access_to != nullptr);
if (UNLIKELY(!this->CanAccess(dex_access_to))) {
if (throw_on_failure) {
Expand Down Expand Up @@ -398,8 +411,21 @@ inline bool Class::ResolvedMethodAccessTest(Class* access_to, ArtMethod* method,
// class rather than the declaring class itself.
DexCache* referrer_dex_cache = use_referrers_cache ? this->GetDexCache() : dex_cache;
uint32_t class_idx = referrer_dex_cache->GetDexFile()->GetMethodId(method_idx).class_idx_;
// The referenced class has already been resolved with the method, get it from the dex cache.
Class* dex_access_to = referrer_dex_cache->GetResolvedType(class_idx);
// The referenced class has already been resolved with the method, but may not be in the dex
// cache. Using ResolveType here without handles in the caller should be safe since there
// should be no thread suspension due to the class being resolved.
// TODO: Clean this up to use handles in the caller.
Class* dex_access_to;
{
StackHandleScope<2> hs(Thread::Current());
Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(referrer_dex_cache));
Handle<mirror::ClassLoader> h_class_loader(hs.NewHandle(access_to->GetClassLoader()));
dex_access_to = Runtime::Current()->GetClassLinker()->ResolveType(
*referrer_dex_cache->GetDexFile(),
class_idx,
h_dex_cache,
h_class_loader);
}
DCHECK(dex_access_to != nullptr);
if (UNLIKELY(!this->CanAccess(dex_access_to))) {
if (throw_on_failure) {
Expand Down
1 change: 1 addition & 0 deletions runtime/mirror/class.h
Original file line number Diff line number Diff line change
Expand Up @@ -1476,6 +1476,7 @@ class MANAGED Class FINAL : public Object {
// java.lang.Class
static GcRoot<Class> java_lang_Class_;

ART_FRIEND_TEST(DexCacheTest, TestResolvedFieldAccess); // For ResolvedFieldAccessTest
friend struct art::ClassOffsets; // for verifying offset information
friend class Object; // For VisitReferences
DISALLOW_IMPLICIT_CONSTRUCTORS(Class);
Expand Down
28 changes: 28 additions & 0 deletions runtime/mirror/dex_cache_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,33 @@ TEST_F(DexCacheTest, LinearAlloc) {
EXPECT_TRUE(linear_alloc->Contains(klass->GetDexCache()->GetResolvedMethods()));
}

TEST_F(DexCacheTest, TestResolvedFieldAccess) {
ScopedObjectAccess soa(Thread::Current());
jobject jclass_loader(LoadDex("Packages"));
ASSERT_TRUE(jclass_loader != nullptr);
Runtime* const runtime = Runtime::Current();
ClassLinker* const class_linker = runtime->GetClassLinker();
StackHandleScope<3> hs(soa.Self());
Handle<mirror::ClassLoader> class_loader(hs.NewHandle(
soa.Decode<mirror::ClassLoader*>(jclass_loader)));
Handle<mirror::Class> klass1 =
hs.NewHandle(class_linker->FindClass(soa.Self(), "Lpackage1/Package1;", class_loader));
ASSERT_TRUE(klass1.Get() != nullptr);
Handle<mirror::Class> klass2 =
hs.NewHandle(class_linker->FindClass(soa.Self(), "Lpackage2/Package2;", class_loader));
ASSERT_TRUE(klass2.Get() != nullptr);
EXPECT_EQ(klass1->GetDexCache(), klass2->GetDexCache());

EXPECT_NE(klass1->NumStaticFields(), 0u);
for (ArtField& field : klass2->GetSFields()) {
EXPECT_FALSE((
klass1->ResolvedFieldAccessTest</*throw_on_failure*/ false,
/*use_referrers_cache*/ false>(klass2.Get(),
&field,
field.GetDexFieldIndex(),
klass1->GetDexCache())));
}
}

} // namespace mirror
} // namespace art
20 changes: 20 additions & 0 deletions test/Packages/Package1.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright (C) 2016 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 package1;
class Package1 {
static int someField;
}
20 changes: 20 additions & 0 deletions test/Packages/Package2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright (C) 2016 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 package2;
class Package2 {
static int someField;
}

0 comments on commit c90450a

Please sign in to comment.