From 72b3e430d880ef57eaa6a34a0822165994052202 Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Tue, 13 May 2014 21:42:05 -0700 Subject: [PATCH] ART: Fix typo in ArtMethod::FindCatchBlock The thrown exception is always resolved, as we have an instance of it. What is potentially not resolved is the catch handler's exception type. The resolution failure will trigger a NoClassDefFoundError, which should replace the original exception. For this, the API has to be changed a little bit to tell callers that there was this change. Change-Id: Id51d54a15c732ed175eb617b3b0331b89cbb2051 --- runtime/catch_block_stack_visitor.cc | 10 +++++- runtime/interpreter/interpreter_common.h | 8 ++++- runtime/mirror/art_method.cc | 13 +++++-- runtime/mirror/art_method.h | 5 ++- test/111-unresolvable-exception/build | 25 +++++++++++++ test/111-unresolvable-exception/expected.txt | 1 + test/111-unresolvable-exception/info.txt | 2 ++ test/111-unresolvable-exception/src/Main.java | 41 ++++++++++++++++++++++ .../src/TestException.java | 18 ++++++++++ 9 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 test/111-unresolvable-exception/build create mode 100644 test/111-unresolvable-exception/expected.txt create mode 100644 test/111-unresolvable-exception/info.txt create mode 100644 test/111-unresolvable-exception/src/Main.java create mode 100644 test/111-unresolvable-exception/src/TestException.java diff --git a/runtime/catch_block_stack_visitor.cc b/runtime/catch_block_stack_visitor.cc index b8202765d..55b330a48 100644 --- a/runtime/catch_block_stack_visitor.cc +++ b/runtime/catch_block_stack_visitor.cc @@ -50,9 +50,17 @@ bool CatchBlockStackVisitor::HandleTryItems(mirror::ArtMethod* method) { } if (dex_pc != DexFile::kDexNoIndex) { bool clear_exception = false; + bool exc_changed = false; StackHandleScope<1> hs(Thread::Current()); Handle to_find(hs.NewHandle((*exception_)->GetClass())); - uint32_t found_dex_pc = method->FindCatchBlock(to_find, dex_pc, &clear_exception); + uint32_t found_dex_pc = method->FindCatchBlock(to_find, dex_pc, &clear_exception, + &exc_changed); + if (UNLIKELY(exc_changed)) { + DCHECK_EQ(DexFile::kDexNoIndex, found_dex_pc); + exception_->Assign(self_->GetException(nullptr)); // TODO: Throw location? + // There is a new context installed, delete it. + delete self_->GetLongJumpContext(); + } exception_handler_->SetClearException(clear_exception); if (found_dex_pc != DexFile::kDexNoIndex) { exception_handler_->SetHandlerDexPc(found_dex_pc); diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h index af8b53445..9b03334b6 100644 --- a/runtime/interpreter/interpreter_common.h +++ b/runtime/interpreter/interpreter_common.h @@ -572,10 +572,16 @@ static inline uint32_t FindNextInstructionFollowingException(Thread* self, ThrowLocation throw_location; mirror::Throwable* exception = self->GetException(&throw_location); bool clear_exception = false; + bool new_exception = false; StackHandleScope<3> hs(self); Handle exception_class(hs.NewHandle(exception->GetClass())); uint32_t found_dex_pc = shadow_frame.GetMethod()->FindCatchBlock(exception_class, dex_pc, - &clear_exception); + &clear_exception, + &new_exception); + if (UNLIKELY(new_exception)) { + // Update the exception. + exception = self->GetException(&throw_location); + } if (found_dex_pc == DexFile::kDexNoIndex) { instrumentation->MethodUnwindEvent(self, this_object, shadow_frame.GetMethod(), dex_pc); diff --git a/runtime/mirror/art_method.cc b/runtime/mirror/art_method.cc index 0632a68af..495ae2d55 100644 --- a/runtime/mirror/art_method.cc +++ b/runtime/mirror/art_method.cc @@ -231,7 +231,7 @@ uintptr_t ArtMethod::ToNativePc(const uint32_t dex_pc) { } uint32_t ArtMethod::FindCatchBlock(Handle& exception_type, uint32_t dex_pc, - bool* has_no_move_exception) { + bool* has_no_move_exception, bool* exc_changed) { MethodHelper mh(this); const DexFile::CodeItem* code_item = mh.GetCodeItem(); // Set aside the exception while we resolve its type. @@ -252,10 +252,17 @@ uint32_t ArtMethod::FindCatchBlock(Handle& exception_type, uint32_t dex_p } // Does this catch exception type apply? Class* iter_exception_type = mh.GetClassFromTypeIdx(iter_type_idx); - if (exception_type.Get() == nullptr) { - self->ClearException(); + if (iter_exception_type == nullptr) { + // Now have a NoClassDefFoundError as exception. + // Note: this is not RI behavior. RI would have failed when loading the class. + *exc_changed = true; + + // TODO: Add old exception as suppressed. LOG(WARNING) << "Unresolved exception class when finding catch block: " << mh.GetTypeDescriptorFromTypeIdx(iter_type_idx); + + // Return immediately. + return DexFile::kDexNoIndex; } else if (iter_exception_type->IsAssignableFrom(exception_type.Get())) { found_dex_pc = it.GetHandlerAddress(); break; diff --git a/runtime/mirror/art_method.h b/runtime/mirror/art_method.h index 27a10be2f..3950a980d 100644 --- a/runtime/mirror/art_method.h +++ b/runtime/mirror/art_method.h @@ -381,8 +381,11 @@ class MANAGED ArtMethod : public Object { // Find the catch block for the given exception type and dex_pc. When a catch block is found, // indicates whether the found catch block is responsible for clearing the exception or whether // a move-exception instruction is present. + // In the process of finding a catch block we might trigger resolution errors. This is flagged + // by exc_changed, which indicates that a different exception is now stored in the thread and + // should be reloaded. uint32_t FindCatchBlock(Handle& exception_type, uint32_t dex_pc, - bool* has_no_move_exception) + bool* has_no_move_exception, bool* exc_changed) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); static void SetClass(Class* java_lang_reflect_ArtMethod); diff --git a/test/111-unresolvable-exception/build b/test/111-unresolvable-exception/build new file mode 100644 index 000000000..c21a9eff8 --- /dev/null +++ b/test/111-unresolvable-exception/build @@ -0,0 +1,25 @@ +#!/bin/bash +# +# Copyright (C) 2014 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. + +# Stop if something fails. +set -e + +mkdir classes +${JAVAC} -d classes `find src -name '*.java'` +rm classes/TestException.class + +${DX} -JXmx256m --debug --dex --dump-to=classes.lst --output=classes.dex classes +zip $TEST_NAME.jar classes.dex diff --git a/test/111-unresolvable-exception/expected.txt b/test/111-unresolvable-exception/expected.txt new file mode 100644 index 000000000..052dd74e7 --- /dev/null +++ b/test/111-unresolvable-exception/expected.txt @@ -0,0 +1 @@ +Caught class java.lang.NoClassDefFoundError diff --git a/test/111-unresolvable-exception/info.txt b/test/111-unresolvable-exception/info.txt new file mode 100644 index 000000000..5ba373334 --- /dev/null +++ b/test/111-unresolvable-exception/info.txt @@ -0,0 +1,2 @@ +Test that we do not segfault when we check a catch handler +for an unresolvable exception. diff --git a/test/111-unresolvable-exception/src/Main.java b/test/111-unresolvable-exception/src/Main.java new file mode 100644 index 000000000..ba07ee1bf --- /dev/null +++ b/test/111-unresolvable-exception/src/Main.java @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2014 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. + */ + +public class Main { + static public void main(String[] args) throws Exception { + try { + check(false); + } catch (Throwable t) { // Should catch the NoClassDefFoundError + System.out.println("Caught " + t.getClass()); + } + } + + private static void check(boolean b) { + try { + if (b) { // Need this to not be dead code, but also not be invoked. + throwsTestException(); // TestException is checked, so we need something potentially + // throwing it. + } + throw new RuntimeException(); // Trigger exception handling. + } catch (TestException e) { // This handler will have an unresolvable class. + } catch (Exception e) { // General-purpose handler + System.out.println("Should not get here!"); + } + } + + // This avoids having to construct one explicitly, which won't work. + private static native void throwsTestException() throws TestException; +} diff --git a/test/111-unresolvable-exception/src/TestException.java b/test/111-unresolvable-exception/src/TestException.java new file mode 100644 index 000000000..2d8b23402 --- /dev/null +++ b/test/111-unresolvable-exception/src/TestException.java @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2014 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. + */ + +public class TestException extends Exception { +} -- 2.11.0