From da9badb9edea5e0d18cd9f97eff0d0937ad48310 Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Fri, 5 Jun 2015 20:22:12 -0700 Subject: [PATCH] ART: Check long and double register pairs in invokes For invokes, ensure that long and double parameters are actually in registers pairs. We were testing the pair, but skipping the actual high parameter register. Bug: 17410612 Change-Id: I8f4c3335ea8b7dc3cf252bee52a5a706ae8905f8 --- compiler/optimizing/builder.cc | 12 +++++------- compiler/optimizing/optimizing_compiler_stats.h | 15 ++++++++------ runtime/verifier/method_verifier.cc | 26 ++++++++++++++++++++----- test/800-smali/expected.txt | 1 + test/800-smali/smali/b_17410612.smali | 14 +++++++++++++ test/800-smali/src/Main.java | 2 ++ 6 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 test/800-smali/smali/b_17410612.smali diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc index f98029da0..e5fe6c38b 100644 --- a/compiler/optimizing/builder.cc +++ b/compiler/optimizing/builder.cc @@ -748,13 +748,11 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction, for (size_t i = start_index; i < number_of_vreg_arguments; i++, argument_index++) { Primitive::Type type = Primitive::GetType(descriptor[descriptor_index++]); bool is_wide = (type == Primitive::kPrimLong) || (type == Primitive::kPrimDouble); - if (!is_range && is_wide && args[i] + 1 != args[i + 1]) { - LOG(WARNING) << "Non sequential register pair in " << dex_compilation_unit_->GetSymbol() - << " at " << dex_pc; - // We do not implement non sequential register pair. - MaybeRecordStat(MethodCompilationStat::kNotCompiledNonSequentialRegPair); - return false; - } + // Longs and doubles should be in pairs, that is, sequential registers. The verifier should + // reject any class where this is violated. + DCHECK(is_range || !is_wide || (args[i] + 1 == args[i + 1])) + << "Non sequential register pair in " << dex_compilation_unit_->GetSymbol() + << " at " << dex_pc; HInstruction* arg = LoadLocal(is_range ? register_index + i : args[i], type); invoke->SetArgumentAt(argument_index, arg); if (is_wide) { diff --git a/compiler/optimizing/optimizing_compiler_stats.h b/compiler/optimizing/optimizing_compiler_stats.h index b6b1bb1ca..b988813f7 100644 --- a/compiler/optimizing/optimizing_compiler_stats.h +++ b/compiler/optimizing/optimizing_compiler_stats.h @@ -19,6 +19,7 @@ #include #include +#include #include "atomic.h" @@ -38,7 +39,6 @@ enum MethodCompilationStat { kNotCompiledHugeMethod, kNotCompiledLargeMethodNoBranches, kNotCompiledNoCodegen, - kNotCompiledNonSequentialRegPair, kNotCompiledPathological, kNotCompiledSpaceFilter, kNotCompiledUnhandledInstruction, @@ -84,14 +84,15 @@ class OptimizingCompilerStats { for (int i = 0; i < kLastStat; i++) { if (compile_stats_[i] != 0) { - LOG(INFO) << PrintMethodCompilationStat(i) << ": " << compile_stats_[i]; + LOG(INFO) << PrintMethodCompilationStat(static_cast(i)) << ": " + << compile_stats_[i]; } } } } private: - std::string PrintMethodCompilationStat(int stat) const { + std::string PrintMethodCompilationStat(MethodCompilationStat stat) const { switch (stat) { case kAttemptCompilation : return "kAttemptCompilation"; case kCompiledBaseline : return "kCompiledBaseline"; @@ -106,7 +107,6 @@ class OptimizingCompilerStats { case kNotCompiledHugeMethod : return "kNotCompiledHugeMethod"; case kNotCompiledLargeMethodNoBranches : return "kNotCompiledLargeMethodNoBranches"; case kNotCompiledNoCodegen : return "kNotCompiledNoCodegen"; - case kNotCompiledNonSequentialRegPair : return "kNotCompiledNonSequentialRegPair"; case kNotCompiledPathological : return "kNotCompiledPathological"; case kNotCompiledSpaceFilter : return "kNotCompiledSpaceFilter"; case kNotCompiledUnhandledInstruction : return "kNotCompiledUnhandledInstruction"; @@ -120,9 +120,12 @@ class OptimizingCompilerStats { case kRemovedCheckedCast: return "kRemovedCheckedCast"; case kRemovedDeadInstruction: return "kRemovedDeadInstruction"; case kRemovedNullCheck: return "kRemovedNullCheck"; - default: LOG(FATAL) << "invalid stat"; + + case kLastStat: break; // Invalid to print out. } - return ""; + LOG(FATAL) << "invalid stat " + << static_cast::type>(stat); + UNREACHABLE(); } AtomicInteger compile_stats_[kLastStat]; diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc index 4d8822795..b86a7ee96 100644 --- a/runtime/verifier/method_verifier.cc +++ b/runtime/verifier/method_verifier.cc @@ -3372,11 +3372,27 @@ ArtMethod* MethodVerifier::VerifyInvocationArgsFromIterator( << " but expected " << reg_type; return nullptr; } - } else if (!work_line_->VerifyRegisterType(this, get_reg, reg_type)) { - // Continue on soft failures. We need to find possible hard failures to avoid problems in the - // compiler. - if (have_pending_hard_failure_) { - return nullptr; + } else { + if (!work_line_->VerifyRegisterType(this, get_reg, reg_type)) { + // Continue on soft failures. We need to find possible hard failures to avoid problems in + // the compiler. + if (have_pending_hard_failure_) { + return nullptr; + } + } else if (reg_type.IsLongOrDoubleTypes()) { + // Check that registers are consecutive (for non-range invokes). Invokes are the only + // instructions not specifying register pairs by the first component, but require them + // nonetheless. Only check when there's an actual register in the parameters. If there's + // none, this will fail below. + if (!is_range && sig_registers + 1 < expected_args) { + uint32_t second_reg = arg[sig_registers + 1]; + if (second_reg != get_reg + 1) { + Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "Rejecting invocation, long or double parameter " + "at index " << sig_registers << " is not a pair: " << get_reg << " + " + << second_reg << "."; + return nullptr; + } + } } } sig_registers += reg_type.IsLongOrDoubleTypes() ? 2 : 1; diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt index a6b216bf3..85656374c 100644 --- a/test/800-smali/expected.txt +++ b/test/800-smali/expected.txt @@ -16,4 +16,5 @@ MoveExc MoveExceptionOnEntry EmptySparseSwitch b/20224106 +b/17410612 Done! diff --git a/test/800-smali/smali/b_17410612.smali b/test/800-smali/smali/b_17410612.smali new file mode 100644 index 000000000..17718cbf6 --- /dev/null +++ b/test/800-smali/smali/b_17410612.smali @@ -0,0 +1,14 @@ +.class public LB17410612; + +# Test that an invoke with a long parameter has the long parameter in +# a pair. This should fail in the verifier and not an abort in the compiler. + +.super Ljava/lang/Object; + +.method public static run()V + .registers 4 + const-wide v0, 0 # Make (v0, v1) a long + const-wide v2, 0 # Make (v2, v3) a long + invoke-static {v0, v3}, Ljava/lang/Long;->valueOf(J)Ljava/lang/Long; + return-void +.end method diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java index 3e8836408..33df06d87 100644 --- a/test/800-smali/src/Main.java +++ b/test/800-smali/src/Main.java @@ -81,6 +81,8 @@ public class Main { null)); testCases.add(new TestCase("b/20224106", "B20224106", "run", null, new VerifyError(), 0)); + testCases.add(new TestCase("b/17410612", "B17410612", "run", null, new VerifyError(), + 0)); } public void runTests() { -- 2.11.0