diff options
author | 2025-02-17 14:21:07 +0000 | |
---|---|---|
committer | 2025-02-18 07:19:47 -0800 | |
commit | cb3c3b2819f326d54a6bd960a9a82c5290aa69b0 (patch) | |
tree | 1b71adef87aeebc9bdf92f6c66330c3a31fa1877 | |
parent | b4d4a7c4c368272c4368db35ddffbd8fcab544fb (diff) |
Do not inline a method that was marked as un-compilable.
The compiler may have internally mark a method as not compilable. Do not
clear the verification results from the compiler callbacks, we still
need them during compilation.
Test: 860-vdex-failure
Bug: 395243275
Change-Id: I79ba61eb8a7ba6729b22c4c27fa83d8373fce03a
-rw-r--r-- | compiler/optimizing/inliner.cc | 10 | ||||
-rw-r--r-- | dex2oat/common_compiler_driver_test.cc | 18 | ||||
-rw-r--r-- | dex2oat/common_compiler_driver_test.h | 1 | ||||
-rw-r--r-- | dex2oat/common_transaction_test.cc | 1 | ||||
-rw-r--r-- | dex2oat/dex/quick_compiler_callbacks.cc | 7 | ||||
-rw-r--r-- | dex2oat/dex/quick_compiler_callbacks.h | 1 | ||||
-rw-r--r-- | dex2oat/dex2oat.cc | 1 | ||||
-rw-r--r-- | dex2oat/verifier_deps_test.cc | 1 | ||||
-rw-r--r-- | runtime/compiler_callbacks.h | 1 | ||||
-rw-r--r-- | runtime/noop_compiler_callbacks.h | 1 | ||||
-rw-r--r-- | test/860-vdex-failure/expected-stderr.txt | 0 | ||||
-rw-r--r-- | test/860-vdex-failure/expected-stdout.txt | 0 | ||||
-rw-r--r-- | test/860-vdex-failure/info.txt | 2 | ||||
-rw-r--r-- | test/860-vdex-failure/run.py | 20 | ||||
-rw-r--r-- | test/860-vdex-failure/smali/Caller.smali | 22 | ||||
-rw-r--r-- | test/860-vdex-failure/smali/TestCase.smali | 22 | ||||
-rw-r--r-- | test/860-vdex-failure/src/Main.java | 30 |
17 files changed, 117 insertions, 21 deletions
diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc index be8bc69de0..251867d512 100644 --- a/compiler/optimizing/inliner.cc +++ b/compiler/optimizing/inliner.cc @@ -22,6 +22,7 @@ #include "builder.h" #include "class_linker.h" #include "class_root-inl.h" +#include "compiler_callbacks.h" #include "constant_folding.h" #include "data_type-inl.h" #include "dead_code_elimination.h" @@ -407,10 +408,15 @@ static bool IsMethodVerified(ArtMethod* method) // At runtime, we know this is cold code if the class is not verified, so don't // bother analyzing. if (Runtime::Current()->IsAotCompiler()) { - if (method->GetDeclaringClass()->IsVerifiedNeedsAccessChecks() || - method->GetDeclaringClass()->ShouldVerifyAtRuntime()) { + if (method->GetDeclaringClass()->IsVerifiedNeedsAccessChecks()) { + DCHECK(!Runtime::Current()->GetCompilerCallbacks()->IsUncompilableMethod( + MethodReference(method->GetDexFile(), method->GetDexMethodIndex()))); return true; } + if (method->GetDeclaringClass()->ShouldVerifyAtRuntime()) { + return !Runtime::Current()->GetCompilerCallbacks()->IsUncompilableMethod( + MethodReference(method->GetDexFile(), method->GetDexMethodIndex())); + } } return false; } diff --git a/dex2oat/common_compiler_driver_test.cc b/dex2oat/common_compiler_driver_test.cc index 81e06b2345..7ce5e94bcd 100644 --- a/dex2oat/common_compiler_driver_test.cc +++ b/dex2oat/common_compiler_driver_test.cc @@ -28,7 +28,7 @@ namespace art { -CommonCompilerDriverTest::CommonCompilerDriverTest() : inaccessible_page_(nullptr) {} +CommonCompilerDriverTest::CommonCompilerDriverTest() {} CommonCompilerDriverTest::~CommonCompilerDriverTest() {} void CommonCompilerDriverTest::CompileAll(jobject class_loader, @@ -44,13 +44,7 @@ void CommonCompilerDriverTest::CompileAll(jobject class_loader, timings, &compiler_options_->image_classes_); - // Verification results in the `callback_` should not be used during compilation. - down_cast<QuickCompilerCallbacks*>(callbacks_.get())->SetVerificationResults( - reinterpret_cast<VerificationResults*>(inaccessible_page_)); compiler_driver_->CompileAll(class_loader, dex_files, timings); - down_cast<QuickCompilerCallbacks*>(callbacks_.get())->SetVerificationResults( - verification_results_.get()); - compiler_driver_->FreeThreadPools(); } @@ -107,19 +101,9 @@ void CommonCompilerDriverTest::SetUp() { CommonCompilerTest::SetUp(); CreateCompilerDriver(); - - // Note: We cannot use MemMap because some tests tear down the Runtime and destroy - // the gMaps, so when destroying the MemMap, the test would crash. - const size_t page_size = MemMap::GetPageSize(); - inaccessible_page_ = mmap(nullptr, page_size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - CHECK(inaccessible_page_ != MAP_FAILED) << strerror(errno); } void CommonCompilerDriverTest::TearDown() { - if (inaccessible_page_ != nullptr) { - munmap(inaccessible_page_, MemMap::GetPageSize()); - inaccessible_page_ = nullptr; - } image_reservation_.Reset(); compiler_driver_.reset(); verification_results_.reset(); diff --git a/dex2oat/common_compiler_driver_test.h b/dex2oat/common_compiler_driver_test.h index b49d18fa10..36090c0644 100644 --- a/dex2oat/common_compiler_driver_test.h +++ b/dex2oat/common_compiler_driver_test.h @@ -68,7 +68,6 @@ class CommonCompilerDriverTest : public CommonCompilerTest { private: MemMap image_reservation_; - void* inaccessible_page_; }; } // namespace art diff --git a/dex2oat/common_transaction_test.cc b/dex2oat/common_transaction_test.cc index d24acd575d..cd0671de4d 100644 --- a/dex2oat/common_transaction_test.cc +++ b/dex2oat/common_transaction_test.cc @@ -33,6 +33,7 @@ class CommonTransactionTestCompilerCallbacks : public CompilerCallbacks { void AddUncompilableMethod([[maybe_unused]] MethodReference ref) override {} void AddUncompilableClass([[maybe_unused]] ClassReference ref) override {} void ClassRejected([[maybe_unused]] ClassReference ref) override {} + bool IsUncompilableMethod([[maybe_unused]] MethodReference ref) override { return false; } verifier::VerifierDeps* GetVerifierDeps() const override { return nullptr; } diff --git a/dex2oat/dex/quick_compiler_callbacks.cc b/dex2oat/dex/quick_compiler_callbacks.cc index 6261cc27c5..ee055fbfd9 100644 --- a/dex2oat/dex/quick_compiler_callbacks.cc +++ b/dex2oat/dex/quick_compiler_callbacks.cc @@ -45,6 +45,13 @@ void QuickCompilerCallbacks::ClassRejected(ClassReference ref) { } } +bool QuickCompilerCallbacks::IsUncompilableMethod(MethodReference ref) { + if (verification_results_ != nullptr) { + return verification_results_->IsUncompilableMethod(ref); + } + return false; +} + ClassStatus QuickCompilerCallbacks::GetPreviousClassState(ClassReference ref) { // If we don't have class unloading enabled in the compiler, we will never see class that were // previously verified. Return false to avoid overhead from the lookup in the compiler driver. diff --git a/dex2oat/dex/quick_compiler_callbacks.h b/dex2oat/dex/quick_compiler_callbacks.h index bb5bed38a2..9e82f2b99b 100644 --- a/dex2oat/dex/quick_compiler_callbacks.h +++ b/dex2oat/dex/quick_compiler_callbacks.h @@ -37,6 +37,7 @@ class QuickCompilerCallbacks final : public CompilerCallbacks { void AddUncompilableMethod(MethodReference ref) override; void AddUncompilableClass(ClassReference ref) override; + bool IsUncompilableMethod(MethodReference ref) override; void ClassRejected(ClassReference ref) override; diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc index 0f377719a7..344c9406f2 100644 --- a/dex2oat/dex2oat.cc +++ b/dex2oat/dex2oat.cc @@ -1990,7 +1990,6 @@ class Dex2Oat final { dex_files, timings_, &compiler_options_->image_classes_); - callbacks_->SetVerificationResults(nullptr); // Should not be needed anymore. driver_->CompileAll(class_loader, dex_files, timings_); driver_->FreeThreadPools(); return class_loader; diff --git a/dex2oat/verifier_deps_test.cc b/dex2oat/verifier_deps_test.cc index 828691d3e2..bffb3bccc5 100644 --- a/dex2oat/verifier_deps_test.cc +++ b/dex2oat/verifier_deps_test.cc @@ -55,6 +55,7 @@ class VerifierDepsCompilerCallbacks : public CompilerCallbacks { void AddUncompilableMethod([[maybe_unused]] MethodReference ref) override {} void AddUncompilableClass([[maybe_unused]] ClassReference ref) override {} void ClassRejected([[maybe_unused]] ClassReference ref) override {} + bool IsUncompilableMethod([[maybe_unused]] MethodReference ref) override { return false; } verifier::VerifierDeps* GetVerifierDeps() const override { return deps_; } void SetVerifierDeps(verifier::VerifierDeps* deps) override { deps_ = deps; } diff --git a/runtime/compiler_callbacks.h b/runtime/compiler_callbacks.h index e196b0b921..40692af5ca 100644 --- a/runtime/compiler_callbacks.h +++ b/runtime/compiler_callbacks.h @@ -54,6 +54,7 @@ class CompilerCallbacks { virtual void AddUncompilableMethod(MethodReference ref) = 0; virtual void AddUncompilableClass(ClassReference ref) = 0; + virtual bool IsUncompilableMethod(MethodReference ref) = 0; virtual void ClassRejected(ClassReference ref) = 0; virtual verifier::VerifierDeps* GetVerifierDeps() const = 0; diff --git a/runtime/noop_compiler_callbacks.h b/runtime/noop_compiler_callbacks.h index 2a4a45af17..9432c539a5 100644 --- a/runtime/noop_compiler_callbacks.h +++ b/runtime/noop_compiler_callbacks.h @@ -35,6 +35,7 @@ class NoopCompilerCallbacks final : public CompilerCallbacks { void AddUncompilableMethod([[maybe_unused]] MethodReference ref) override {} void AddUncompilableClass([[maybe_unused]] ClassReference ref) override {} + bool IsUncompilableMethod([[maybe_unused]] MethodReference ref) override { return false; } void ClassRejected([[maybe_unused]] ClassReference ref) override {} verifier::VerifierDeps* GetVerifierDeps() const override { return nullptr; } diff --git a/test/860-vdex-failure/expected-stderr.txt b/test/860-vdex-failure/expected-stderr.txt new file mode 100644 index 0000000000..e69de29bb2 --- /dev/null +++ b/test/860-vdex-failure/expected-stderr.txt diff --git a/test/860-vdex-failure/expected-stdout.txt b/test/860-vdex-failure/expected-stdout.txt new file mode 100644 index 0000000000..e69de29bb2 --- /dev/null +++ b/test/860-vdex-failure/expected-stdout.txt diff --git a/test/860-vdex-failure/info.txt b/test/860-vdex-failure/info.txt new file mode 100644 index 0000000000..aeab3b8aee --- /dev/null +++ b/test/860-vdex-failure/info.txt @@ -0,0 +1,2 @@ +Regression test for the optimizing compiler, which used to try to inline a +method which failed verification. diff --git a/test/860-vdex-failure/run.py b/test/860-vdex-failure/run.py new file mode 100644 index 0000000000..91e6651059 --- /dev/null +++ b/test/860-vdex-failure/run.py @@ -0,0 +1,20 @@ +#!/bin/bash +# +# 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. + + +def run(ctx, args): + # Pass speed as filter to ensure compilation. + ctx.default_run(args, vdex=True, vdex_filter="speed") diff --git a/test/860-vdex-failure/smali/Caller.smali b/test/860-vdex-failure/smali/Caller.smali new file mode 100644 index 0000000000..ca85cb6126 --- /dev/null +++ b/test/860-vdex-failure/smali/Caller.smali @@ -0,0 +1,22 @@ +# 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. + +.class public LCaller; +.super Ljava/lang/Object; + +.method public static test()V + .registers 0 + invoke-static {}, LTestCase;->test()V + return-void +.end method diff --git a/test/860-vdex-failure/smali/TestCase.smali b/test/860-vdex-failure/smali/TestCase.smali new file mode 100644 index 0000000000..51b4132606 --- /dev/null +++ b/test/860-vdex-failure/smali/TestCase.smali @@ -0,0 +1,22 @@ +# 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. + +.class public LTestCase; +.super Ljava/lang/Object; + +.method public static test()V + .registers 1 + move-result-object v0 + return-void +.end method diff --git a/test/860-vdex-failure/src/Main.java b/test/860-vdex-failure/src/Main.java new file mode 100644 index 0000000000..99a300f838 --- /dev/null +++ b/test/860-vdex-failure/src/Main.java @@ -0,0 +1,30 @@ +/* + * 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. + */ + +import java.lang.reflect.InvocationTargetException; + +public class Main { + public static void main(String[] args) throws Exception { + try { + Class.forName("Caller").getDeclaredMethod("test").invoke(null); + throw new Error("Expected InvocationTargetException"); + } catch (InvocationTargetException e) { + if (!(e.getCause() instanceof VerifyError)) { + throw new Error("Expected VerifyError, got " + e.getCause()); + } + } + } +} |