ART: Fix things for valgrind

Wire up valgrind gtests. Add valgrind-test-art-host, currently
only depending on valgrind-test-art-host-gtest32.

Fix an Alloc setting to allow running valgrind.

Refactor the fault handler to manage (and correctly release) the
handlers.

Fix minor failure-case leaks exposed by tests.

Failing tests:

The optimizing compiler is leaking non-arena-ed structures
(e.g., assembler buffers), as code generators are not destroyed.
The solution has been moved to a follow-up CL.

Note: All 64b tests are failing as we cannot allocate a heap.

Change-Id: I7f854cfd098d9f68107ce492363e7dba9a82b9fa

15 files changed, 96 insertions, 53 deletions

diff --git a/Android.mk b/Android.mk
index 669939b..b3cc2d0 100644
--- a/Android.mk
+++ b/Android.mk
@@ -229,6 +229,11 @@ test-art-host-interpreter$(2ND_ART_PHONY_TEST_HOST_SUFFIX): test-art-host-run-te
 	$(hide) $(call ART_TEST_PREREQ_FINISHED,$@)
 endif
 
+# Valgrind. Currently only 32b gtests.
+.PHONY: valgrind-test-art-host
+valgrind-test-art-host: valgrind-test-art-host-gtest32
+	$(hide) $(call ART_TEST_PREREQ_FINISHED,$@)
+
 ########################################################################
 # target test rules
 
diff --git a/build/Android.common_test.mk b/build/Android.common_test.mk
index 20c5a21..3897401 100644
--- a/build/Android.common_test.mk
+++ b/build/Android.common_test.mk
@@ -41,6 +41,18 @@ ART_TEST_KNOWN_BROKEN := \
   test-art-host-run-test-gcstress-interpreter-prebuild-114-ParallelGC64 \
   test-art-host-run-test-gcstress-optimizing-prebuild-114-ParallelGC64
 
+# Failing valgrind tests.
+# Note: *all* 64b tests involving the runtime do not work currently. b/15170219.
+
+# Optimizing compiler codegen is not destructed and can leak non-arena-ed structures.
+ART_TEST_KNOWN_BROKEN += \
+  valgrind-test-art-host-gtest-codegen_test32 \
+  valgrind-test-art-host-gtest-find_loops_test32 \
+  valgrind-test-art-host-gtest-linearize_test32 \
+  valgrind-test-art-host-gtest-live_ranges_test32 \
+  valgrind-test-art-host-gtest-liveness_test32 \
+  valgrind-test-art-host-gtest-register_allocator_test32
+
 # List of known failing tests that when executed won't cause test execution to not finish.
 # The test name must be the full rule name such as test-art-host-oat-optimizing-HelloWorld64.
 ART_TEST_KNOWN_FAILING :=
diff --git a/build/Android.gtest.mk b/build/Android.gtest.mk
index 352938e..700bcf0 100644
--- a/build/Android.gtest.mk
+++ b/build/Android.gtest.mk
@@ -442,7 +442,14 @@ define define-test-art-gtest-combination
   endif
 
   rule_name := $(3)test-art-$(1)-gtest$(4)
-  dependencies := $$(ART_TEST_$(2)_GTEST$(4)_RULES)
+  ifeq ($(3),valgrind-)
+    ifneq ($(1),host)
+      $$(error valgrind tests only wired up for the host)
+    endif
+    dependencies := $$(ART_TEST_$(2)_VALGRIND_GTEST$(4)_RULES)
+  else
+    dependencies := $$(ART_TEST_$(2)_GTEST$(4)_RULES)
+  endif
 
 .PHONY: $$(rule_name)
 $$(rule_name): $$(dependencies)
diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc
index 32a7676..db6a01e 100644
--- a/compiler/driver/compiler_driver.cc
+++ b/compiler/driver/compiler_driver.cc
@@ -981,7 +981,7 @@ bool CompilerDriver::CanEmbedReferenceTypeInCode(ClassReference* ref,
 
   ScopedObjectAccess soa(Thread::Current());
   mirror::Class* reference_class = mirror::Reference::GetJavaLangRefReference();
-  bool is_initialized;
+  bool is_initialized = false;
   bool unused_finalizable;
   // Make sure we have a finished Reference class object before attempting to use it.
   if (!CanEmbedTypeInCode(*reference_class->GetDexCache()->GetDexFile(),
diff --git a/compiler/oat_test.cc b/compiler/oat_test.cc
index e858a7b..80d7b98 100644
--- a/compiler/oat_test.cc
+++ b/compiler/oat_test.cc
@@ -197,13 +197,13 @@ TEST_F(OatTest, OatHeaderIsValid) {
     std::vector<const DexFile*> dex_files;
     uint32_t image_file_location_oat_checksum = 0;
     uint32_t image_file_location_oat_begin = 0;
-    OatHeader* oat_header = OatHeader::Create(instruction_set,
-                                              instruction_set_features,
-                                              &dex_files,
-                                              image_file_location_oat_checksum,
-                                              image_file_location_oat_begin,
-                                              nullptr);
-    ASSERT_NE(oat_header, nullptr);
+    std::unique_ptr<OatHeader> oat_header(OatHeader::Create(instruction_set,
+                                                            instruction_set_features,
+                                                            &dex_files,
+                                                            image_file_location_oat_checksum,
+                                                            image_file_location_oat_begin,
+                                                            nullptr));
+    ASSERT_NE(oat_header.get(), nullptr);
     ASSERT_TRUE(oat_header->IsValid());
 
     char* magic = const_cast<char*>(oat_header->GetMagic());
diff --git a/compiler/utils/assembler.cc b/compiler/utils/assembler.cc
index 68b784a..e3045e1 100644
--- a/compiler/utils/assembler.cc
+++ b/compiler/utils/assembler.cc
@@ -92,6 +92,7 @@ void AssemblerBuffer::ExtendCapacity() {
 
   // Compute the relocation delta and switch to the new contents area.
   ptrdiff_t delta = new_contents - contents_;
+  delete[] contents_;
   contents_ = new_contents;
 
   // Update the cursor and recompute the limit.
diff --git a/compiler/utils/x86_64/assembler_x86_64.h b/compiler/utils/x86_64/assembler_x86_64.h
index 3f9f007..763dafe 100644
--- a/compiler/utils/x86_64/assembler_x86_64.h
+++ b/compiler/utils/x86_64/assembler_x86_64.h
@@ -253,7 +253,7 @@ class Address : public Operand {
 
 class X86_64Assembler FINAL : public Assembler {
  public:
-  X86_64Assembler() {}
+  X86_64Assembler() : cfi_cfa_offset_(0), cfi_pc_(0) {}
   virtual ~X86_64Assembler() {}
 
   /*
diff --git a/compiler/utils/x86_64/assembler_x86_64_test.cc b/compiler/utils/x86_64/assembler_x86_64_test.cc
index 4ed7b20..7a48b63 100644
--- a/compiler/utils/x86_64/assembler_x86_64_test.cc
+++ b/compiler/utils/x86_64/assembler_x86_64_test.cc
@@ -16,6 +16,7 @@
 
 #include "assembler_x86_64.h"
 
+#include "base/stl_util.h"
 #include "utils/assembler_test.h"
 
 namespace art {
@@ -62,6 +63,11 @@ class AssemblerX86_64Test : public AssemblerTest<x86_64::X86_64Assembler, x86_64
     }
   }
 
+  void TearDown() OVERRIDE {
+    AssemblerTest::TearDown();
+    STLDeleteElements(&registers_);
+  }
+
   std::vector<x86_64::CpuRegister*> GetRegisters() OVERRIDE {
     return registers_;
   }
@@ -219,6 +225,7 @@ std::string setcc_test_fn(x86_64::X86_64Assembler* assembler) {
     }
   }
 
+  STLDeleteElements(&registers);
   return str.str();
 }
 
diff --git a/runtime/class_linker-inl.h b/runtime/class_linker-inl.h
index 1306546..875efbb 100644
--- a/runtime/class_linker-inl.h
+++ b/runtime/class_linker-inl.h
@@ -156,7 +156,7 @@ inline mirror::ArtField* ClassLinker::ResolveField(uint32_t field_idx, mirror::A
 }
 
 inline mirror::Object* ClassLinker::AllocObject(Thread* self) {
-  return GetClassRoot(kJavaLangObject)->Alloc<false, false>(self,
+  return GetClassRoot(kJavaLangObject)->Alloc<true, false>(self,
       Runtime::Current()->GetHeap()->GetCurrentAllocator());
 }
 
diff --git a/runtime/dex_file.cc b/runtime/dex_file.cc
index ed3592c..3bb47d4 100644
--- a/runtime/dex_file.cc
+++ b/runtime/dex_file.cc
@@ -206,19 +206,20 @@ const DexFile* DexFile::OpenFile(int fd, const char* location, bool verify,
 
   const Header* dex_header = reinterpret_cast<const Header*>(map->Begin());
 
-  const DexFile* dex_file = OpenMemory(location, dex_header->checksum_, map.release(), error_msg);
-  if (dex_file == nullptr) {
+  std::unique_ptr<const DexFile> dex_file(OpenMemory(location, dex_header->checksum_, map.release(),
+                                                     error_msg));
+  if (dex_file.get() == nullptr) {
     *error_msg = StringPrintf("Failed to open dex file '%s' from memory: %s", location,
                               error_msg->c_str());
     return nullptr;
   }
 
-  if (verify && !DexFileVerifier::Verify(dex_file, dex_file->Begin(), dex_file->Size(), location,
-                                         error_msg)) {
+  if (verify && !DexFileVerifier::Verify(dex_file.get(), dex_file->Begin(), dex_file->Size(),
+                                         location, error_msg)) {
     return nullptr;
   }
 
-  return dex_file;
+  return dex_file.release();
 }
 
 const char* DexFile::kClassesDex = "classes.dex";
diff --git a/runtime/fault_handler.cc b/runtime/fault_handler.cc
index 47696f9..fede2f8 100644
--- a/runtime/fault_handler.cc
+++ b/runtime/fault_handler.cc
@@ -19,6 +19,7 @@
 #include <setjmp.h>
 #include <sys/mman.h>
 #include <sys/ucontext.h>
+#include "base/stl_util.h"
 #include "mirror/art_method.h"
 #include "mirror/class.h"
 #include "sigchain.h"
@@ -115,13 +116,23 @@ void FaultManager::Init() {
   initialized_ = true;
 }
 
-void FaultManager::Shutdown() {
+void FaultManager::Release() {
   if (initialized_) {
     UnclaimSignalChain(SIGSEGV);
     initialized_ = false;
   }
 }
 
+void FaultManager::Shutdown() {
+  if (initialized_) {
+    Release();
+
+    // Free all handlers.
+    STLDeleteElements(&generated_code_handlers_);
+    STLDeleteElements(&other_handlers_);
+  }
+}
+
 void FaultManager::HandleFault(int sig, siginfo_t* info, void* context) {
   // BE CAREFUL ALLOCATING HERE INCLUDING USING LOG(...)
   //
@@ -156,9 +167,9 @@ void FaultManager::HandleFault(int sig, siginfo_t* info, void* context) {
 
   // Now set up the nested signal handler.
 
-  // Shutdown the fault manager so that it will remove the signal chain for
+  // Release the fault manager so that it will remove the signal chain for
   // SIGSEGV and we call the real sigaction.
-  fault_manager.Shutdown();
+  fault_manager.Release();
 
   // The action for SIGSEGV should be the default handler now.
 
@@ -226,6 +237,7 @@ void FaultManager::HandleFault(int sig, siginfo_t* info, void* context) {
 }
 
 void FaultManager::AddHandler(FaultHandler* handler, bool generated_code) {
+  DCHECK(initialized_);
   if (generated_code) {
     generated_code_handlers_.push_back(handler);
   } else {
diff --git a/runtime/fault_handler.h b/runtime/fault_handler.h
index bb26780..8b66a6f 100644
--- a/runtime/fault_handler.h
+++ b/runtime/fault_handler.h
@@ -39,10 +39,17 @@ class FaultManager {
   ~FaultManager();
 
   void Init();
+
+  // Unclaim signals.
+  void Release();
+
+  // Unclaim signals and delete registered handlers.
   void Shutdown();
 
   void HandleFault(int sig, siginfo_t* info, void* context);
   void HandleNestedSignal(int sig, siginfo_t* info, void* context);
+
+  // Added handlers are owned by the fault handler and will be freed on Shutdown().
   void AddHandler(FaultHandler* handler, bool generated_code);
   void RemoveHandler(FaultHandler* handler);
 
diff --git a/runtime/mem_map_test.cc b/runtime/mem_map_test.cc
index 69f618c..e54d0e0 100644
--- a/runtime/mem_map_test.cc
+++ b/runtime/mem_map_test.cc
@@ -18,6 +18,8 @@
 
 #include <memory>
 
+#include <valgrind.h>
+
 #include "gtest/gtest.h"
 
 namespace art {
@@ -198,17 +200,20 @@ TEST_F(MemMapTest, RemapAtEnd32bit) {
 #endif
 
 TEST_F(MemMapTest, MapAnonymousExactAddr32bitHighAddr) {
-  uintptr_t start_addr = ART_BASE_ADDRESS + 0x1000000;
-  std::string error_msg;
-  std::unique_ptr<MemMap> map(MemMap::MapAnonymous("MapAnonymousExactAddr32bitHighAddr",
-                                             reinterpret_cast<byte*>(start_addr),
-                                             0x21000000,
-                                             PROT_READ | PROT_WRITE,
-                                             true,
-                                             &error_msg));
-  ASSERT_TRUE(map.get() != nullptr) << error_msg;
-  ASSERT_TRUE(error_msg.empty());
-  ASSERT_EQ(reinterpret_cast<uintptr_t>(BaseBegin(map.get())), start_addr);
+  // This test may not work under valgrind.
+  if (RUNNING_ON_VALGRIND == 0) {
+    uintptr_t start_addr = ART_BASE_ADDRESS + 0x1000000;
+    std::string error_msg;
+    std::unique_ptr<MemMap> map(MemMap::MapAnonymous("MapAnonymousExactAddr32bitHighAddr",
+                                                     reinterpret_cast<byte*>(start_addr),
+                                                     0x21000000,
+                                                     PROT_READ | PROT_WRITE,
+                                                     true,
+                                                     &error_msg));
+    ASSERT_TRUE(map.get() != nullptr) << error_msg;
+    ASSERT_TRUE(error_msg.empty());
+    ASSERT_EQ(reinterpret_cast<uintptr_t>(BaseBegin(map.get())), start_addr);
+  }
 }
 
 TEST_F(MemMapTest, MapAnonymousOverflow) {
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 89ad505..4b90324 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -139,9 +139,6 @@ Runtime::Runtime()
       system_class_loader_(nullptr),
       dump_gc_performance_on_shutdown_(false),
       preinitialization_transaction_(nullptr),
-      null_pointer_handler_(nullptr),
-      suspend_handler_(nullptr),
-      stack_overflow_handler_(nullptr),
       verify_(false),
       target_sdk_version_(0),
       implicit_null_checks_(false),
@@ -199,10 +196,6 @@ Runtime::~Runtime() {
   // TODO: acquire a static mutex on Runtime to avoid racing.
   CHECK(instance_ == nullptr || instance_ == this);
   instance_ = nullptr;
-
-  delete null_pointer_handler_;
-  delete suspend_handler_;
-  delete stack_overflow_handler_;
 }
 
 struct AbortState {
@@ -733,7 +726,8 @@ bool Runtime::Init(const RuntimeOptions& raw_options, bool ignore_unrecognized)
     case kArm64:
     case kX86_64:
       implicit_null_checks_ = true;
-      implicit_so_checks_ = true;
+      // Installing stack protection does not play well with valgrind.
+      implicit_so_checks_ = (RUNNING_ON_VALGRIND == 0);
       break;
     default:
       // Keep the defaults.
@@ -745,16 +739,19 @@ bool Runtime::Init(const RuntimeOptions& raw_options, bool ignore_unrecognized)
 
     // These need to be in a specific order.  The null point check handler must be
     // after the suspend check and stack overflow check handlers.
+    //
+    // Note: the instances attach themselves to the fault manager and are handled by it. The manager
+    //       will delete the instance on Shutdown().
     if (implicit_suspend_checks_) {
-      suspend_handler_ = new SuspensionHandler(&fault_manager);
+      new SuspensionHandler(&fault_manager);
     }
 
     if (implicit_so_checks_) {
-      stack_overflow_handler_ = new StackOverflowHandler(&fault_manager);
+      new StackOverflowHandler(&fault_manager);
     }
 
     if (implicit_null_checks_) {
-      null_pointer_handler_ = new NullPointerHandler(&fault_manager);
+      new NullPointerHandler(&fault_manager);
     }
 
     if (kEnableJavaStackTraceHandler) {
diff --git a/runtime/runtime.h b/runtime/runtime.h
index a0993ca..9df1453 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -463,16 +463,8 @@ class Runtime {
 
   void AddCurrentRuntimeFeaturesAsDex2OatArguments(std::vector<std::string>* arg_vector) const;
 
-  bool ExplicitNullChecks() const {
-    return null_pointer_handler_ == nullptr;
-  }
-
-  bool ExplicitSuspendChecks() const {
-    return suspend_handler_ == nullptr;
-  }
-
   bool ExplicitStackOverflowChecks() const {
-    return stack_overflow_handler_ == nullptr;
+    return !implicit_so_checks_;
   }
 
   bool IsVerificationEnabled() const {
@@ -636,9 +628,6 @@ class Runtime {
 
   // Transaction used for pre-initializing classes at compilation time.
   Transaction* preinitialization_transaction_;
-  NullPointerHandler* null_pointer_handler_;
-  SuspensionHandler* suspend_handler_;
-  StackOverflowHandler* stack_overflow_handler_;
 
   // If false, verification is disabled. True by default.
   bool verify_;