From 06cb4a9470c2adaa6ed2a2889be9d4c0fe66156d Mon Sep 17 00:00:00 2001 From: Sebastien Hertz Date: Tue, 28 Apr 2015 15:00:41 +0200 Subject: Fix constructor access check through reflection We must not throw IllegalAccessException if the constructor has been made accessible by a previous call to Constructor.setAccessible, even if the caller cannot access the constructor. Bug: 20639158 (cherry picked from commit 2d2f2a9c665b02ca5139f71e37ca5e08389e4191) Change-Id: Ic5cb54256f11aefcfaa99f2ee85c4a32f30e693a --- runtime/native/java_lang_reflect_Constructor.cc | 4 ++-- test/100-reflect2/src/Main.java | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/runtime/native/java_lang_reflect_Constructor.cc b/runtime/native/java_lang_reflect_Constructor.cc index 810b354..908089e 100644 --- a/runtime/native/java_lang_reflect_Constructor.cc +++ b/runtime/native/java_lang_reflect_Constructor.cc @@ -36,7 +36,7 @@ namespace art { */ static jobject Constructor_newInstance(JNIEnv* env, jobject javaMethod, jobjectArray javaArgs) { ScopedFastNativeObjectAccess soa(env); - mirror::Method* m = soa.Decode(javaMethod); + mirror::Constructor* m = soa.Decode(javaMethod); StackHandleScope<1> hs(soa.Self()); Handle c(hs.NewHandle(m->GetDeclaringClass())); if (UNLIKELY(c->IsAbstract())) { @@ -46,7 +46,7 @@ static jobject Constructor_newInstance(JNIEnv* env, jobject javaMethod, jobjectA return nullptr; } // Verify that we can access the class. - if (!c->IsPublic()) { + if (!m->IsAccessible() && !c->IsPublic()) { auto* caller = GetCallingClass(soa.Self(), 1); // If caller is null, then we called from JNI, just avoid the check since JNI avoids most // access checks anyways. TODO: Investigate if this the correct behavior. diff --git a/test/100-reflect2/src/Main.java b/test/100-reflect2/src/Main.java index 86a5ef8..72e14b1 100644 --- a/test/100-reflect2/src/Main.java +++ b/test/100-reflect2/src/Main.java @@ -266,7 +266,7 @@ class Main { show(ctor.newInstance(new char[] { 'x', 'y', 'z', '!' }, 1, 2)); } - private static void testPackagePrivate() { + private static void testPackagePrivateConstructor() { try { Class c = Class.forName("sub.PPClass"); Constructor cons = c.getConstructor(); @@ -280,10 +280,23 @@ class Main { } } + private static void testPackagePrivateAccessibleConstructor() { + try { + Class c = Class.forName("sub.PPClass"); + Constructor cons = c.getConstructor(); + cons.setAccessible(true); // ensure we prevent IllegalAccessException + cons.newInstance(); + } catch (Exception e) { + // Error. + e.printStackTrace(); + } + } + public static void main(String[] args) throws Exception { testFieldReflection(); testMethodReflection(); testConstructorReflection(); - testPackagePrivate(); + testPackagePrivateConstructor(); + testPackagePrivateAccessibleConstructor(); } } -- cgit v1.1 From 3a0163107e34304ff720bcc34b280aca0ea4a0fc Mon Sep 17 00:00:00 2001 From: David Brazdil Date: Wed, 29 Apr 2015 17:16:07 +0100 Subject: ART: Fix loop information after dead code elimination Compilation failed when only some blocks of a loop were removed during dead code elimination. Bug: 20680703 (cherry picked from commit 69a2804c3bb48cf4fd00a66080f613a4fd96c422) Change-Id: If9988381236e4d8d8c3b508dfce1376b27c20d75 --- compiler/optimizing/dead_code_elimination.cc | 9 +++-- compiler/optimizing/nodes.cc | 15 ++++++++- compiler/optimizing/nodes.h | 11 +++++-- test/480-checker-dead-blocks/src/Main.java | 49 +++++++++++++++++++++++++++- 4 files changed, 76 insertions(+), 8 deletions(-) diff --git a/compiler/optimizing/dead_code_elimination.cc b/compiler/optimizing/dead_code_elimination.cc index 91cd60a..cd427c5 100644 --- a/compiler/optimizing/dead_code_elimination.cc +++ b/compiler/optimizing/dead_code_elimination.cc @@ -65,10 +65,13 @@ void HDeadCodeElimination::RemoveDeadBlocks() { for (HPostOrderIterator it(*graph_); !it.Done(); it.Advance()) { HBasicBlock* block = it.Current(); if (live_blocks.IsBitSet(block->GetBlockId())) { - continue; + // If this block is part of a loop that is being dismantled, we need to + // update its loop information. + block->UpdateLoopInformation(); + } else { + MaybeRecordDeadBlock(block); + block->DisconnectAndDelete(); } - MaybeRecordDeadBlock(block); - block->DisconnectAndDelete(); } // Connect successive blocks created by dead branches. Order does not matter. diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc index 0ff30ea..286ffa8 100644 --- a/compiler/optimizing/nodes.cc +++ b/compiler/optimizing/nodes.cc @@ -383,7 +383,6 @@ bool HLoopInformation::Populate() { } HBasicBlock* HLoopInformation::GetPreHeader() const { - DCHECK_EQ(header_->GetPredecessors().Size(), 2u); return header_->GetDominator(); } @@ -1009,6 +1008,20 @@ void HBasicBlock::DisconnectAndDelete() { SetGraph(nullptr); } +void HBasicBlock::UpdateLoopInformation() { + // Check if loop information points to a dismantled loop. If so, replace with + // the loop information of a larger loop which contains this block, or nullptr + // otherwise. We iterate in case the larger loop has been destroyed too. + while (IsInLoop() && loop_information_->GetBackEdges().IsEmpty()) { + if (IsLoopHeader()) { + HSuspendCheck* suspend_check = loop_information_->GetSuspendCheck(); + DCHECK_EQ(suspend_check->GetBlock(), this); + RemoveInstruction(suspend_check); + } + loop_information_ = loop_information_->GetPreHeader()->GetLoopInformation(); + } +} + void HBasicBlock::MergeWith(HBasicBlock* other) { DCHECK_EQ(GetGraph(), other->GetGraph()); DCHECK(GetDominatedBlocks().Contains(other)); diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 1f58c4d..a5da615 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -592,7 +592,7 @@ class HBasicBlock : public ArenaObject { void RemoveInstructionOrPhi(HInstruction* instruction, bool ensure_safety = true); bool IsLoopHeader() const { - return (loop_information_ != nullptr) && (loop_information_->GetHeader() == this); + return IsInLoop() && (loop_information_->GetHeader() == this); } bool IsLoopPreHeaderFirstPredecessor() const { @@ -611,7 +611,7 @@ class HBasicBlock : public ArenaObject { void SetInLoop(HLoopInformation* info) { if (IsLoopHeader()) { // Nothing to do. This just means `info` is an outer loop. - } else if (loop_information_ == nullptr) { + } else if (!IsInLoop()) { loop_information_ = info; } else if (loop_information_->Contains(*info->GetHeader())) { // Block is currently part of an outer loop. Make it part of this inner loop. @@ -630,6 +630,11 @@ class HBasicBlock : public ArenaObject { loop_information_ = info; } + // Checks if the loop information points to a valid loop. If the loop has been + // dismantled (does not have a back edge any more), loop information is + // removed or replaced with the information of the first valid outer loop. + void UpdateLoopInformation(); + bool IsInLoop() const { return loop_information_ != nullptr; } // Returns wheter this block dominates the blocked passed as parameter. @@ -683,7 +688,7 @@ class HLoopInformationOutwardIterator : public ValueObject { void Advance() { DCHECK(!Done()); - current_ = current_->GetHeader()->GetDominator()->GetLoopInformation(); + current_ = current_->GetPreHeader()->GetLoopInformation(); } HLoopInformation* Current() const { diff --git a/test/480-checker-dead-blocks/src/Main.java b/test/480-checker-dead-blocks/src/Main.java index 560ce95..83dbb26 100644 --- a/test/480-checker-dead-blocks/src/Main.java +++ b/test/480-checker-dead-blocks/src/Main.java @@ -128,7 +128,7 @@ public class Main { // CHECK-DAG: [[Arg:i\d+]] ParameterValue // CHECK-DAG: Return [ [[Arg]] ] - // CHECK-START: int Main.testRemoveLoop(int) dead_code_elimination_final (after) + // CHECK-START: int Main.testDeadLoop(int) dead_code_elimination_final (after) // CHECK-NOT: If // CHECK-NOT: Add @@ -139,9 +139,56 @@ public class Main { return x; } + // CHECK-START: int Main.testUpdateLoopInformation(int) dead_code_elimination_final (before) + // CHECK-DAG: If + // CHECK-DAG: If + // CHECK-DAG: Add + + // CHECK-START: int Main.testUpdateLoopInformation(int) dead_code_elimination_final (after) + // CHECK-DAG: [[Arg:i\d+]] ParameterValue + // CHECK-DAG: Return [ [[Arg]] ] + + // CHECK-START: int Main.testUpdateLoopInformation(int) dead_code_elimination_final (after) + // CHECK-NOT: If + // CHECK-NOT: Add + + public static int testUpdateLoopInformation(int x) { + // Use of Or in the condition generates a dead loop where not all of its + // blocks are removed. This forces DCE to update their loop information. + while (inlineFalse() || !inlineTrue()) { + x++; + } + return x; + } + + // CHECK-START: int Main.testRemoveSuspendCheck(int, int) dead_code_elimination_final (before) + // CHECK: SuspendCheck + // CHECK: SuspendCheck + // CHECK: SuspendCheck + // CHECK-NOT: SuspendCheck + + // CHECK-START: int Main.testRemoveSuspendCheck(int, int) dead_code_elimination_final (after) + // CHECK: SuspendCheck + // CHECK: SuspendCheck + // CHECK-NOT: SuspendCheck + + public static int testRemoveSuspendCheck(int x, int y) { + // Inner loop will leave behind the header with its SuspendCheck. DCE must + // remove it, otherwise the outer loop would end up with two. + while (y > 0) { + while (inlineFalse() || !inlineTrue()) { + x++; + } + y--; + } + return x; + } + public static void main(String[] args) { assertIntEquals(7, testTrueBranch(4, 3)); assertIntEquals(1, testFalseBranch(4, 3)); assertIntEquals(42, testRemoveLoop(42)); + assertIntEquals(23, testUpdateLoopInformation(23)); + assertIntEquals(12, testRemoveSuspendCheck(12, 5)); } } -- cgit v1.1 From 38f2085e53b22762a83c464d91db59a9c0327580 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Tue, 28 Apr 2015 00:52:43 +0100 Subject: Update the remaining input index of phis after deleting an input. bug:20715803 bug:20690906 (cherry picked from commit 5d7b7f81ed5455893f984752c00571ef27cc97c5) Change-Id: Ie55739601b8d6fedc830d6e19d8a053392047d34 --- compiler/optimizing/graph_checker.cc | 30 +++++++++++++++++-- compiler/optimizing/nodes.cc | 3 ++ compiler/optimizing/nodes.h | 5 ++-- test/483-dce-block/expected.txt | 1 + test/483-dce-block/info.txt | 2 ++ test/483-dce-block/src/Main.java | 58 ++++++++++++++++++++++++++++++++++++ 6 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 test/483-dce-block/expected.txt create mode 100644 test/483-dce-block/info.txt create mode 100644 test/483-dce-block/src/Main.java diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc index dc3124b..8ea8f3c 100644 --- a/compiler/optimizing/graph_checker.cc +++ b/compiler/optimizing/graph_checker.cc @@ -170,7 +170,8 @@ void GraphChecker::VisitInstruction(HInstruction* instruction) { } } - // Ensure the uses of `instruction` are defined in a block of the graph. + // Ensure the uses of `instruction` are defined in a block of the graph, + // and the entry in the use list is consistent. for (HUseIterator use_it(instruction->GetUses()); !use_it.Done(); use_it.Advance()) { HInstruction* use = use_it.Current()->GetUser(); @@ -184,6 +185,27 @@ void GraphChecker::VisitInstruction(HInstruction* instruction) { use->GetId(), instruction->GetId())); } + size_t use_index = use_it.Current()->GetIndex(); + if ((use_index >= use->InputCount()) || (use->InputAt(use_index) != instruction)) { + AddError(StringPrintf("User %s:%d of instruction %d has a wrong " + "UseListNode index.", + use->DebugName(), + use->GetId(), + instruction->GetId())); + } + } + + // Ensure the environment uses entries are consistent. + for (HUseIterator use_it(instruction->GetEnvUses()); + !use_it.Done(); use_it.Advance()) { + HEnvironment* use = use_it.Current()->GetUser(); + size_t use_index = use_it.Current()->GetIndex(); + if ((use_index >= use->Size()) || (use->GetInstructionAt(use_index) != instruction)) { + AddError(StringPrintf("Environment user of %s:%d has a wrong " + "UseListNode index.", + instruction->DebugName(), + instruction->GetId())); + } } // Ensure 'instruction' has pointers to its inputs' use entries. @@ -191,7 +213,11 @@ void GraphChecker::VisitInstruction(HInstruction* instruction) { HUserRecord input_record = instruction->InputRecordAt(i); HInstruction* input = input_record.GetInstruction(); HUseListNode* use_node = input_record.GetUseNode(); - if (use_node == nullptr || !input->GetUses().Contains(use_node)) { + size_t use_index = use_node->GetIndex(); + if ((use_node == nullptr) + || !input->GetUses().Contains(use_node) + || (use_index >= e) + || (use_index != i)) { AddError(StringPrintf("Instruction %s:%d has an invalid pointer to use entry " "at input %u (%s:%d).", instruction->DebugName(), diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc index 286ffa8..7fb66c8 100644 --- a/compiler/optimizing/nodes.cc +++ b/compiler/optimizing/nodes.cc @@ -683,6 +683,9 @@ void HPhi::AddInput(HInstruction* input) { void HPhi::RemoveInputAt(size_t index) { RemoveAsUserOfInput(index); inputs_.DeleteAt(index); + for (size_t i = index, e = InputCount(); i < e; ++i) { + InputRecordAt(i).GetUseNode()->SetIndex(i); + } } #define DEFINE_ACCEPT(name, super) \ diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index a5da615..b1294ef 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -801,13 +801,14 @@ class HUseListNode : public ArenaObject { HUseListNode* GetNext() const { return next_; } T GetUser() const { return user_; } size_t GetIndex() const { return index_; } + void SetIndex(size_t index) { index_ = index; } private: HUseListNode(T user, size_t index) : user_(user), index_(index), prev_(nullptr), next_(nullptr) {} T const user_; - const size_t index_; + size_t index_; HUseListNode* prev_; HUseListNode* next_; @@ -1037,7 +1038,7 @@ class HEnvironment : public ArenaObject { GrowableArray > vregs_; - friend HInstruction; + friend class HInstruction; DISALLOW_COPY_AND_ASSIGN(HEnvironment); }; diff --git a/test/483-dce-block/expected.txt b/test/483-dce-block/expected.txt new file mode 100644 index 0000000..ef48625 --- /dev/null +++ b/test/483-dce-block/expected.txt @@ -0,0 +1 @@ +class Main diff --git a/test/483-dce-block/info.txt b/test/483-dce-block/info.txt new file mode 100644 index 0000000..3db88ab --- /dev/null +++ b/test/483-dce-block/info.txt @@ -0,0 +1,2 @@ +Regression test for optimizing that used to crash +compiling the `foo` method. diff --git a/test/483-dce-block/src/Main.java b/test/483-dce-block/src/Main.java new file mode 100644 index 0000000..2f66a74 --- /dev/null +++ b/test/483-dce-block/src/Main.java @@ -0,0 +1,58 @@ +/* + * Copyright (C) 2015 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 { + public static void foo(Object o, int a) { + Object result = null; + if (o instanceof Main) { + // The compiler optimizes the type of `o` by introducing + // a `HBoundType` in this block. + while (a != 3) { + if (a == 2) { + a++; + result = o; + continue; + } else if (willInline()) { + // This block will be detected as dead after inlining. + result = new Object(); + continue; + } + result = new Object(); + } + // The compiler produces a phi at the back edge for `result`. + // Before dead block elimination, the phi has three inputs: + // result = (new Object(), new Object(), HBoundType) + // + // After dead block elimination, the phi has now two inputs: + // result = (new Object(), HBoundType) + // + // Our internal data structure for linking users and inputs expect + // the input index stored in that data structure to be the index + // in the inputs array. So the index before dead block elimination + // of the `HBoundType` would be 2. Dead block elimination must update + // that index to be 1. + } + System.out.println(result.getClass()); + } + + public static boolean willInline() { + return false; + } + + public static void main(String[] args) { + foo(new Main(), 2); + } +} -- cgit v1.1 From 85336e30bb75a5c544d49da83d8d4e7fae5b10ff Mon Sep 17 00:00:00 2001 From: Narayan Kamath Date: Wed, 6 May 2015 14:55:43 +0100 Subject: Fix broken checks in IsValidPartOfMemberNameUtf8Slow. GetUtf16FromUtf8 returns a surrogate pair only if it encounters a 4-byte UTF sequence. Three byte UTF sequences will only return the first or second half of a pair so we need to check for that explicitly. bug: 20844537 Change-Id: Icb660fae77ac8a852fc768e6c1cd5766117e68e4 --- runtime/utf.h | 6 +++--- runtime/utils.cc | 52 ++++++++++++++++++++++++++++++--------------------- runtime/utils_test.cc | 23 +++++++++++++++++++++++ 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/runtime/utf.h b/runtime/utf.h index dd38afa..7f05248 100644 --- a/runtime/utf.h +++ b/runtime/utf.h @@ -87,9 +87,9 @@ size_t ComputeModifiedUtf8Hash(const char* chars); /* * Retrieve the next UTF-16 character or surrogate pair from a UTF-8 string. * single byte, 2-byte and 3-byte UTF-8 sequences result in a single UTF-16 - * character whereas 4-byte UTF-8 sequences result in a surrogate pair. Use - * GetLeadingUtf16Char and GetTrailingUtf16Char to process the return value - * of this function. + * character (possibly one half of a surrogate) whereas 4-byte UTF-8 sequences + * result in a surrogate pair. Use GetLeadingUtf16Char and GetTrailingUtf16Char + * to process the return value of this function. * * Advances "*utf8_data_in" to the start of the next character. * diff --git a/runtime/utils.cc b/runtime/utils.cc index 650214f..7986cdc 100644 --- a/runtime/utils.cc +++ b/runtime/utils.cc @@ -827,14 +827,21 @@ bool IsValidPartOfMemberNameUtf8Slow(const char** pUtf8Ptr) { */ const uint32_t pair = GetUtf16FromUtf8(pUtf8Ptr); - const uint16_t leading = GetLeadingUtf16Char(pair); - const uint32_t trailing = GetTrailingUtf16Char(pair); - if (trailing == 0) { - // Perform follow-up tests based on the high 8 bits of the - // lower surrogate. - switch (leading >> 8) { + // We have a surrogate pair resulting from a valid 4 byte UTF sequence. + // No further checks are necessary because 4 byte sequences span code + // points [U+10000, U+1FFFFF], which are valid codepoints in a dex + // identifier. Furthermore, GetUtf16FromUtf8 guarantees that each of + // the surrogate halves are valid and well formed in this instance. + if (GetTrailingUtf16Char(pair) != 0) { + return true; + } + + + // We've encountered a one, two or three byte UTF-8 sequence. The + // three byte UTF-8 sequence could be one half of a surrogate pair. + switch (leading >> 8) { case 0x00: // It's only valid if it's above the ISO-8859-1 high space (0xa0). return (leading > 0x00a0); @@ -842,9 +849,14 @@ bool IsValidPartOfMemberNameUtf8Slow(const char** pUtf8Ptr) { case 0xd9: case 0xda: case 0xdb: - // It looks like a leading surrogate but we didn't find a trailing - // surrogate if we're here. - return false; + { + // We found a three byte sequence encoding one half of a surrogate. + // Look for the other half. + const uint32_t pair2 = GetUtf16FromUtf8(pUtf8Ptr); + const uint16_t trailing = GetLeadingUtf16Char(pair2); + + return (GetTrailingUtf16Char(pair2) == 0) && (0xdc00 <= trailing && trailing <= 0xdfff); + } case 0xdc: case 0xdd: case 0xde: @@ -855,21 +867,19 @@ bool IsValidPartOfMemberNameUtf8Slow(const char** pUtf8Ptr) { case 0xff: // It's in the range that has spaces, controls, and specials. switch (leading & 0xfff8) { - case 0x2000: - case 0x2008: - case 0x2028: - case 0xfff0: - case 0xfff8: - return false; + case 0x2000: + case 0x2008: + case 0x2028: + case 0xfff0: + case 0xfff8: + return false; } - break; - } - - return true; + return true; + default: + return true; } - // We have a surrogate pair. Check that trailing surrogate is well formed. - return (trailing >= 0xdc00 && trailing <= 0xdfff); + UNREACHABLE(); } /* Return whether the pointed-at modified-UTF-8 encoded character is diff --git a/runtime/utils_test.cc b/runtime/utils_test.cc index 195de0c..869d305 100644 --- a/runtime/utils_test.cc +++ b/runtime/utils_test.cc @@ -521,4 +521,27 @@ TEST_F(UtilsTest, TestSleep) { EXPECT_GT(NanoTime() - start, MsToNs(1000)); } +TEST_F(UtilsTest, IsValidDescriptor) { + std::vector descriptor( + { 'L', 'a', '/', 'b', '$', 0xed, 0xa0, 0x80, 0xed, 0xb0, 0x80, ';', 0x00 }); + EXPECT_TRUE(IsValidDescriptor(reinterpret_cast(&descriptor[0]))); + + std::vector unpaired_surrogate( + { 'L', 'a', '/', 'b', '$', 0xed, 0xa0, 0x80, ';', 0x00 }); + EXPECT_FALSE(IsValidDescriptor(reinterpret_cast(&unpaired_surrogate[0]))); + + std::vector unpaired_surrogate_at_end( + { 'L', 'a', '/', 'b', '$', 0xed, 0xa0, 0x80, 0x00 }); + EXPECT_FALSE(IsValidDescriptor(reinterpret_cast(&unpaired_surrogate_at_end[0]))); + + std::vector invalid_surrogate( + { 'L', 'a', '/', 'b', '$', 0xed, 0xb0, 0x80, ';', 0x00 }); + EXPECT_FALSE(IsValidDescriptor(reinterpret_cast(&invalid_surrogate[0]))); + + std::vector unpaired_surrogate_with_multibyte_sequence( + { 'L', 'a', '/', 'b', '$', 0xed, 0xb0, 0x80, 0xf0, 0x9f, 0x8f, 0xa0, ';', 0x00 }); + EXPECT_FALSE( + IsValidDescriptor(reinterpret_cast(&unpaired_surrogate_with_multibyte_sequence[0]))); +} + } // namespace art -- cgit v1.1 From c2d3221037734567442c5fb9b11a7967b5c87b79 Mon Sep 17 00:00:00 2001 From: Vladimir Marko Date: Thu, 7 May 2015 12:25:40 +0100 Subject: Quick: Abolish kMirOpCheckPart2. The tricks played with kMirOpCheckPart2 are making the native GC map generation unnecessarily complex. They have caused problems in the past and now there is bad interaction with the DCE. Rather than fixing it time and again, remove the pseudo-insn. (The whole purpose of those tricks seems to be to allow the register tracking to be used for the throwing insn before resetting the tracking for the next block. However, it's questionable whether that's better than processing the throwing insn with the subsequent instructions.) Bug: 20736048 (cherry picked from commit e299f167c9559401548eab71678d4b779e46c2fb) Change-Id: I8a60d26c5e6b6b608d68b8bb6b66d411f9a28f90 --- compiler/dex/compiler_enums.h | 1 - compiler/dex/mir_dataflow.cc | 3 --- compiler/dex/mir_graph.cc | 7 ++----- compiler/dex/quick/codegen_util.cc | 16 ---------------- compiler/dex/quick/mir_to_lir.cc | 15 --------------- compiler/dex/quick/quick_compiler.cc | 1 - 6 files changed, 2 insertions(+), 41 deletions(-) diff --git a/compiler/dex/compiler_enums.h b/compiler/dex/compiler_enums.h index 0acdd42..b78b3d7 100644 --- a/compiler/dex/compiler_enums.h +++ b/compiler/dex/compiler_enums.h @@ -172,7 +172,6 @@ enum ExtendedMIROpcode { kMirOpRangeCheck, kMirOpDivZeroCheck, kMirOpCheck, - kMirOpCheckPart2, kMirOpSelect, // Vector opcodes: diff --git a/compiler/dex/mir_dataflow.cc b/compiler/dex/mir_dataflow.cc index b4aec98..a7ba061 100644 --- a/compiler/dex/mir_dataflow.cc +++ b/compiler/dex/mir_dataflow.cc @@ -834,9 +834,6 @@ const uint64_t MIRGraph::oat_data_flow_attributes_[kMirOpLast] = { // 10B MIR_CHECK 0, - // 10C MIR_CHECKPART2 - 0, - // 10D MIR_SELECT DF_DA | DF_UB, diff --git a/compiler/dex/mir_graph.cc b/compiler/dex/mir_graph.cc index 9e3fbbc..1871f07 100644 --- a/compiler/dex/mir_graph.cc +++ b/compiler/dex/mir_graph.cc @@ -52,8 +52,7 @@ const char* MIRGraph::extended_mir_op_names_[kMirOpLast - kMirOpFirst] = { "OpNullCheck", "OpRangeCheck", "OpDivZeroCheck", - "Check1", - "Check2", + "Check", "Select", "ConstVector", "MoveVector", @@ -1508,7 +1507,7 @@ char* MIRGraph::GetDalvikDisassembly(const MIR* mir) { Instruction::Format dalvik_format = Instruction::k10x; // Default to no-operand format. // Handle special cases that recover the original dalvik instruction. - if ((opcode == kMirOpCheck) || (opcode == kMirOpCheckPart2)) { + if (opcode == kMirOpCheck) { str.append(extended_mir_op_names_[opcode - kMirOpFirst]); str.append(": "); // Recover the original Dex instruction. @@ -2517,8 +2516,6 @@ int MIR::DecodedInstruction::FlagsOf() const { return Instruction::kContinue | Instruction::kThrow; case kMirOpCheck: return Instruction::kContinue | Instruction::kThrow; - case kMirOpCheckPart2: - return Instruction::kContinue; case kMirOpSelect: return Instruction::kContinue; case kMirOpConstVector: diff --git a/compiler/dex/quick/codegen_util.cc b/compiler/dex/quick/codegen_util.cc index fb68335..86bb69d 100644 --- a/compiler/dex/quick/codegen_util.cc +++ b/compiler/dex/quick/codegen_util.cc @@ -1391,22 +1391,6 @@ void Mir2Lir::InitReferenceVRegs(BasicBlock* bb, BitVector* references) { } } } - if (bb->block_type != kEntryBlock && bb->first_mir_insn != nullptr && - static_cast(bb->first_mir_insn->dalvikInsn.opcode) == kMirOpCheckPart2) { - // In Mir2Lir::MethodBlockCodeGen() we have artificially moved the throwing - // instruction to the previous block. However, the MIRGraph data used above - // doesn't reflect that, so we still need to process that MIR insn here. - MIR* mir = nullptr; - BasicBlock* pred_bb = bb; - // Traverse empty blocks. - while (mir == nullptr && pred_bb->predecessors.size() == 1u) { - pred_bb = mir_graph_->GetBasicBlock(bb->predecessors[0]); - DCHECK(pred_bb != nullptr); - mir = pred_bb->last_mir_insn; - } - DCHECK(mir != nullptr); - UpdateReferenceVRegsLocal(nullptr, mir, references); - } } bool Mir2Lir::UpdateReferenceVRegsLocal(MIR* mir, MIR* prev_mir, BitVector* references) { diff --git a/compiler/dex/quick/mir_to_lir.cc b/compiler/dex/quick/mir_to_lir.cc index e9e9161..e3e87ec 100644 --- a/compiler/dex/quick/mir_to_lir.cc +++ b/compiler/dex/quick/mir_to_lir.cc @@ -1187,7 +1187,6 @@ void Mir2Lir::HandleExtendedMethodMIR(BasicBlock* bb, MIR* mir) { case kMirOpRangeCheck: case kMirOpDivZeroCheck: case kMirOpCheck: - case kMirOpCheckPart2: // Ignore these known opcodes break; default: @@ -1276,20 +1275,6 @@ bool Mir2Lir::MethodBlockCodeGen(BasicBlock* bb) { head_lir->u.m.def_mask = &kEncodeAll; } - if (opcode == kMirOpCheck) { - // Combine check and work halves of throwing instruction. - MIR* work_half = mir->meta.throw_insn; - mir->dalvikInsn = work_half->dalvikInsn; - mir->optimization_flags = work_half->optimization_flags; - mir->meta = work_half->meta; // Whatever the work_half had, we need to copy it. - opcode = work_half->dalvikInsn.opcode; - SSARepresentation* ssa_rep = work_half->ssa_rep; - work_half->ssa_rep = mir->ssa_rep; - mir->ssa_rep = ssa_rep; - work_half->dalvikInsn.opcode = static_cast(kMirOpCheckPart2); - work_half->meta.throw_insn = mir; - } - if (MIR::DecodedInstruction::IsPseudoMirOp(opcode)) { HandleExtendedMethodMIR(bb, mir); continue; diff --git a/compiler/dex/quick/quick_compiler.cc b/compiler/dex/quick/quick_compiler.cc index 73cfe92..7ca4382 100644 --- a/compiler/dex/quick/quick_compiler.cc +++ b/compiler/dex/quick/quick_compiler.cc @@ -403,7 +403,6 @@ static int kAllOpcodes[] = { kMirOpRangeCheck, kMirOpDivZeroCheck, kMirOpCheck, - kMirOpCheckPart2, kMirOpSelect, }; -- cgit v1.1 From d6ec6511da54e43c0fc0e6e8df93ccde7e4146f8 Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Thu, 21 May 2015 14:06:46 -0700 Subject: ART: Sometimes even empty methods take forever to verify In cases of very high load and/or bad scheduling, the verifier may take longer than the threshold duration to verify an empty method. The LargeMethod detection needs to accept that the code_item may be null. Bug: 21364300 Change-Id: Iceff3e4688cc1a5fe7a836f7a9bf6c49a392b618 --- runtime/verifier/method_verifier.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc index 1b1bc54..6c58d55 100644 --- a/runtime/verifier/method_verifier.cc +++ b/runtime/verifier/method_verifier.cc @@ -287,6 +287,10 @@ MethodVerifier::FailureKind MethodVerifier::VerifyClass(Thread* self, } static bool IsLargeMethod(const DexFile::CodeItem* const code_item) { + if (code_item == nullptr) { + return false; + } + uint16_t registers_size = code_item->registers_size_; uint32_t insns_size = code_item->insns_size_in_code_units_; -- cgit v1.1 From c1956de557ca9934c4f499ab1da18027e442d7cd Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Thu, 11 Jun 2015 16:21:42 -0700 Subject: Fix compaction bug in Class_getDeclaredMethodsUnchecked Added handle to fix the bug. Bug: 21638351 Change-Id: I1c3abea33aa825d3a28c1fc5cb415508686ad93e --- runtime/native/java_lang_Class.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/native/java_lang_Class.cc b/runtime/native/java_lang_Class.cc index 94024ef..67dcc9c 100644 --- a/runtime/native/java_lang_Class.cc +++ b/runtime/native/java_lang_Class.cc @@ -380,8 +380,8 @@ static jobject Class_getDeclaredMethodInternal(JNIEnv* env, jobject javaThis, static jobjectArray Class_getDeclaredMethodsUnchecked(JNIEnv* env, jobject javaThis, jboolean publicOnly) { ScopedFastNativeObjectAccess soa(env); - StackHandleScope<3> hs(soa.Self()); - auto* klass = DecodeClass(soa, javaThis); + StackHandleScope<2> hs(soa.Self()); + auto klass = hs.NewHandle(DecodeClass(soa, javaThis)); size_t num_methods = 0; for (auto& m : klass->GetVirtualMethods(sizeof(void*))) { auto modifiers = m.GetAccessFlags(); -- cgit v1.1 From 603b4c25bb3cc1a67d9722ec7d767210879fd8e0 Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Wed, 17 Jun 2015 16:11:12 -0700 Subject: Fix some java_lang_Class related moving GC bugs There was some missing handles around mirror::Class*. (cherry picked from commit 05b7226787f1470ad93f6f632fed60f70bc8631e Bug: 21898408 Change-Id: Icb754074dfb469473101d20d6873a5bc3274abc5 --- runtime/native/java_lang_Class.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/runtime/native/java_lang_Class.cc b/runtime/native/java_lang_Class.cc index 67dcc9c..a41aed6 100644 --- a/runtime/native/java_lang_Class.cc +++ b/runtime/native/java_lang_Class.cc @@ -282,11 +282,11 @@ static ALWAYS_INLINE inline bool MethodMatchesConstructor(ArtMethod* m, bool pub static jobjectArray Class_getDeclaredConstructorsInternal( JNIEnv* env, jobject javaThis, jboolean publicOnly) { ScopedFastNativeObjectAccess soa(env); - auto* klass = DecodeClass(soa, javaThis); - StackHandleScope<1> hs(soa.Self()); + StackHandleScope<2> hs(soa.Self()); + Handle h_klass = hs.NewHandle(DecodeClass(soa, javaThis)); size_t constructor_count = 0; // Two pass approach for speed. - for (auto& m : klass->GetDirectMethods(sizeof(void*))) { + for (auto& m : h_klass->GetDirectMethods(sizeof(void*))) { constructor_count += MethodMatchesConstructor(&m, publicOnly != JNI_FALSE) ? 1u : 0u; } auto h_constructors = hs.NewHandle(mirror::ObjectArray::Alloc( @@ -296,7 +296,7 @@ static jobjectArray Class_getDeclaredConstructorsInternal( return nullptr; } constructor_count = 0; - for (auto& m : klass->GetDirectMethods(sizeof(void*))) { + for (auto& m : h_klass->GetDirectMethods(sizeof(void*))) { if (MethodMatchesConstructor(&m, publicOnly != JNI_FALSE)) { auto* constructor = mirror::Constructor::CreateFromArtMethod(soa.Self(), &m); if (UNLIKELY(constructor == nullptr)) { @@ -319,16 +319,16 @@ static jobject Class_getDeclaredMethodInternal(JNIEnv* env, jobject javaThis, // were synthesized by the runtime. constexpr uint32_t kSkipModifiers = kAccMiranda | kAccSynthetic; ScopedFastNativeObjectAccess soa(env); - StackHandleScope<4> hs(soa.Self()); + StackHandleScope<3> hs(soa.Self()); auto h_method_name = hs.NewHandle(soa.Decode(name)); if (UNLIKELY(h_method_name.Get() == nullptr)) { ThrowNullPointerException("name == null"); return nullptr; } auto h_args = hs.NewHandle(soa.Decode*>(args)); - auto* klass = DecodeClass(soa, javaThis); + Handle h_klass = hs.NewHandle(DecodeClass(soa, javaThis)); ArtMethod* result = nullptr; - for (auto& m : klass->GetVirtualMethods(sizeof(void*))) { + for (auto& m : h_klass->GetVirtualMethods(sizeof(void*))) { auto* np_method = m.GetInterfaceMethodIfProxy(sizeof(void*)); // May cause thread suspension. mirror::String* np_name = np_method->GetNameAsString(soa.Self()); @@ -347,7 +347,7 @@ static jobject Class_getDeclaredMethodInternal(JNIEnv* env, jobject javaThis, } } if (result == nullptr) { - for (auto& m : klass->GetDirectMethods(sizeof(void*))) { + for (auto& m : h_klass->GetDirectMethods(sizeof(void*))) { auto modifiers = m.GetAccessFlags(); if ((modifiers & kAccConstructor) != 0) { continue; @@ -381,7 +381,7 @@ static jobjectArray Class_getDeclaredMethodsUnchecked(JNIEnv* env, jobject javaT jboolean publicOnly) { ScopedFastNativeObjectAccess soa(env); StackHandleScope<2> hs(soa.Self()); - auto klass = hs.NewHandle(DecodeClass(soa, javaThis)); + Handle klass = hs.NewHandle(DecodeClass(soa, javaThis)); size_t num_methods = 0; for (auto& m : klass->GetVirtualMethods(sizeof(void*))) { auto modifiers = m.GetAccessFlags(); @@ -432,7 +432,7 @@ static jobjectArray Class_getDeclaredMethodsUnchecked(JNIEnv* env, jobject javaT static jobject Class_newInstance(JNIEnv* env, jobject javaThis) { ScopedFastNativeObjectAccess soa(env); StackHandleScope<4> hs(soa.Self()); - auto klass = hs.NewHandle(DecodeClass(soa, javaThis)); + Handle klass = hs.NewHandle(DecodeClass(soa, javaThis)); if (UNLIKELY(klass->GetPrimitiveType() != 0 || klass->IsInterface() || klass->IsArrayClass() || klass->IsAbstract())) { soa.Self()->ThrowNewExceptionF("Ljava/lang/InstantiationException;", -- cgit v1.1