ART: Enable bugprone-unused-return-value

Enable bugprone-unused-return-value as an error. This is on top of
the compiler warning for attribute((warn_unused)).

Mark the current occurrences (all unique_ptr.release()) with NOLINT
to signal that we know what we're doing.

Bug: 32619234
Bug: 117926937
Test: WITH_TIDY=1 mmma art
Change-Id: I36659722335eef36acfa5845289104257a393874

12 files changed, 29 insertions, 24 deletions

diff --git a/adbconnection/adbconnection.cc b/adbconnection/adbconnection.cc
index c716d92a9d..205013335b 100644
--- a/adbconnection/adbconnection.cc
+++ b/adbconnection/adbconnection.cc
@@ -251,6 +251,8 @@ void AdbConnectionState::StartDebuggerThreads() {
     runtime->StartThreadBirth();
   }
   ScopedLocalRef<jobject> thr(soa.Env(), CreateAdbConnectionThread(soa.Self()));
+  // Note: Using pthreads instead of std::thread to not abort when the thread cannot be
+  //       created (exception support required).
   pthread_t pthread;
   std::unique_ptr<CallbackData> data(new CallbackData { this, soa.Env()->NewGlobalRef(thr.get()) });
   started_debugger_threads_ = true;
@@ -268,7 +270,7 @@ void AdbConnectionState::StartDebuggerThreads() {
     runtime->EndThreadBirth();
     return;
   }
-  data.release();
+  data.release();  // NOLINT pthreads API.
 }
 
 static bool FlagsSet(int16_t data, int16_t flags) {
diff --git a/build/Android.bp b/build/Android.bp
index 9797268680..47a540d521 100644
--- a/build/Android.bp
+++ b/build/Android.bp
@@ -21,6 +21,7 @@ art_clang_tidy_errors = [
     "bugprone-argument-comment",
     "bugprone-lambda-function-name",
     "bugprone-unused-raii",  // Protect scoped things like MutexLock.
+    "bugprone-unused-return-value",
     "bugprone-virtual-near-miss",
     "modernize-use-bool-literals",
     "modernize-use-nullptr",
@@ -37,6 +38,7 @@ art_clang_tidy_errors = [
 art_clang_tidy_errors_str = "bugprone-argument-comment"
         + ",bugprone-lambda-function-name"
         + ",bugprone-unused-raii"
+        + ",bugprone-unused-return-value"
         + ",bugprone-virtual-near-miss"
         + ",modernize-redundant-void-arg"
         + ",modernize-use-bool-literals"
diff --git a/compiler/dex/verification_results.cc b/compiler/dex/verification_results.cc
index 1e0b94de81..dd947d90b7 100644
--- a/compiler/dex/verification_results.cc
+++ b/compiler/dex/verification_results.cc
@@ -79,7 +79,7 @@ void VerificationResults::ProcessVerifiedMethod(verifier::MethodVerifier* method
   if (inserted) {
     // Successfully added, release the unique_ptr since we no longer have ownership.
     DCHECK_EQ(GetVerifiedMethod(ref), verified_method.get());
-    verified_method.release();
+    verified_method.release();  // NOLINT b/117926937
   } else {
     // TODO: Investigate why are we doing the work again for this method and try to avoid it.
     LOG(WARNING) << "Method processed more than once: " << ref.PrettyMethod();
@@ -117,7 +117,7 @@ void VerificationResults::CreateVerifiedMethodFor(MethodReference ref) {
                                       /*expected*/ nullptr,
                                       verified_method.get()) ==
           AtomicMap::InsertResult::kInsertResultSuccess) {
-    verified_method.release();
+    verified_method.release();  // NOLINT b/117926937
   }
 }
 
diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc
index 47b434d58d..f0f2b3e397 100644
--- a/dex2oat/dex2oat.cc
+++ b/dex2oat/dex2oat.cc
@@ -662,21 +662,21 @@ class Dex2Oat final {
     if (!kIsDebugBuild && !(kRunningOnMemoryTool && kMemoryToolDetectsLeaks)) {
       // We want to just exit on non-debug builds, not bringing the runtime down
       // in an orderly fashion. So release the following fields.
-      driver_.release();
-      image_writer_.release();
+      driver_.release();                // NOLINT
+      image_writer_.release();          // NOLINT
       for (std::unique_ptr<const DexFile>& dex_file : opened_dex_files_) {
-        dex_file.release();
+        dex_file.release();             // NOLINT
       }
       new std::vector<MemMap>(std::move(opened_dex_files_maps_));  // Leak MemMaps.
       for (std::unique_ptr<File>& vdex_file : vdex_files_) {
-        vdex_file.release();
+        vdex_file.release();            // NOLINT
       }
       for (std::unique_ptr<File>& oat_file : oat_files_) {
-        oat_file.release();
+        oat_file.release();             // NOLINT
       }
-      runtime_.release();
-      verification_results_.release();
-      key_value_store_.release();
+      runtime_.release();               // NOLINT
+      verification_results_.release();  // NOLINT
+      key_value_store_.release();       // NOLINT
     }
   }
 
diff --git a/dexlayout/dexlayout.cc b/dexlayout/dexlayout.cc
index db6945fbaa..8905aa31c4 100644
--- a/dexlayout/dexlayout.cc
+++ b/dexlayout/dexlayout.cc
@@ -1554,7 +1554,7 @@ void DexLayout::LayoutClassDefsAndClassData(const DexFile* dex_file) {
       // Overwrite the existing vector with the new ordering, note that the sets of objects are
       // equivalent, but the order changes. This is why this is not a memory leak.
       // TODO: Consider cleaning this up with a shared_ptr.
-      class_datas[class_data_index].release();
+      class_datas[class_data_index].release();  // NOLINT b/117926937
       class_datas[class_data_index].reset(class_data);
       ++class_data_index;
     }
@@ -1570,7 +1570,7 @@ void DexLayout::LayoutClassDefsAndClassData(const DexFile* dex_file) {
       // Overwrite the existing vector with the new ordering, note that the sets of objects are
       // equivalent, but the order changes. This is why this is not a memory leak.
       // TODO: Consider cleaning this up with a shared_ptr.
-      class_defs[i].release();
+      class_defs[i].release();  // NOLINT b/117926937
       class_defs[i].reset(new_class_def_order[i]);
     }
   }
@@ -1671,7 +1671,7 @@ void DexLayout::LayoutStringData(const DexFile* dex_file) {
   // Now we know what order we want the string data, reorder them.
   size_t data_index = 0;
   for (dex_ir::StringId* string_id : string_ids) {
-    string_datas[data_index].release();
+    string_datas[data_index].release();  // NOLINT b/117926937
     string_datas[data_index].reset(string_id->DataItem());
     ++data_index;
   }
diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index db2b143022..96cc68d080 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -1288,7 +1288,7 @@ bool Redefiner::FinishAllRemainingAllocations(RedefinitionDataHolder& holder) {
 }
 
 void Redefiner::ClassRedefinition::ReleaseDexFile() {
-  dex_file_.release();
+  dex_file_.release();  // NOLINT b/117926937
 }
 
 void Redefiner::ReleaseAllDexFiles() {
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index a0e5b5c926..33909f7957 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -812,7 +812,7 @@ jvmtiError ThreadUtil::RunAgentThread(jvmtiEnv* jvmti_env,
     runtime->EndThreadBirth();
     return ERR(INTERNAL);
   }
-  data.release();
+  data.release();  // NOLINT pthreads API.
 
   return ERR(NONE);
 }
diff --git a/runtime/class_loader_context.cc b/runtime/class_loader_context.cc
index 4da0091884..5c8d68527b 100644
--- a/runtime/class_loader_context.cc
+++ b/runtime/class_loader_context.cc
@@ -64,10 +64,10 @@ ClassLoaderContext::~ClassLoaderContext() {
     // make sure we do not de-allocate them.
     for (ClassLoaderInfo& info : class_loader_chain_) {
       for (std::unique_ptr<OatFile>& oat_file : info.opened_oat_files) {
-        oat_file.release();
+        oat_file.release();  // NOLINT b/117926937
       }
       for (std::unique_ptr<const DexFile>& dex_file : info.opened_dex_files) {
-        dex_file.release();
+        dex_file.release();  // NOLINT b/117926937
       }
     }
   }
diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc
index 0a54e38698..d31f166869 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -1344,13 +1344,14 @@ static JdwpError ER_Set(JdwpState* state, Request* request, ExpandBuf* pReply)
   VLOG(jdwp) << StringPrintf("    --> event requestId=%#x", requestId);
 
   /* add it to the list */
+  // TODO: RegisterEvent() should take std::unique_ptr<>.
   JdwpError err = state->RegisterEvent(pEvent.get());
   if (err != ERR_NONE) {
     /* registration failed, probably because event is bogus */
     LOG(WARNING) << "WARNING: event request rejected";
     return err;
   }
-  pEvent.release();
+  pEvent.release();  // NOLINT b/117926937
   return ERR_NONE;
 }
 
diff --git a/runtime/native/dalvik_system_DexFile.cc b/runtime/native/dalvik_system_DexFile.cc
index 36f9b1aaeb..6becd36bf1 100644
--- a/runtime/native/dalvik_system_DexFile.cc
+++ b/runtime/native/dalvik_system_DexFile.cc
@@ -112,7 +112,7 @@ static jlongArray ConvertDexFilesToJavaArray(JNIEnv* env,
 
   // Now release all the unique_ptrs.
   for (auto& dex_file : vec) {
-    dex_file.release();
+    dex_file.release();  // NOLINT
   }
 
   return long_array;
@@ -295,7 +295,7 @@ static jobject DexFile_openDexFileNative(JNIEnv* env,
       ScopedObjectAccess soa(env);
       for (auto& dex_file : dex_files) {
         if (linker->IsDexFileRegistered(soa.Self(), *dex_file)) {
-          dex_file.release();
+          dex_file.release();  // NOLINT
         }
       }
     }
diff --git a/runtime/oat_file_manager.cc b/runtime/oat_file_manager.cc
index a9ef9a3fa9..b9e9d384c2 100644
--- a/runtime/oat_file_manager.cc
+++ b/runtime/oat_file_manager.cc
@@ -84,7 +84,7 @@ void OatFileManager::UnRegisterAndDeleteOatFile(const OatFile* oat_file) {
   auto it = oat_files_.find(compare);
   CHECK(it != oat_files_.end());
   oat_files_.erase(it);
-  compare.release();
+  compare.release();  // NOLINT b/117926937
 }
 
 const OatFile* OatFileManager::FindOpenedOatFileFromDexLocation(
@@ -567,7 +567,7 @@ std::vector<std::unique_ptr<const DexFile>> OatFileManager::OpenDexFilesFromOat(
           if (added_image_space) {
             // Successfully added image space to heap, release the map so that it does not get
             // freed.
-            image_space.release();
+            image_space.release();  // NOLINT b/117926937
 
             // Register for tracking.
             for (const auto& dex_file : dex_files) {
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 51775c69a2..b3492e1e31 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -729,7 +729,7 @@ void Thread::CreateNativeThread(JNIEnv* env, jobject java_peer, size_t stack_siz
       // JNIEnvExt we created.
       // Note: we can't check for tmp_jni_env == nullptr, as that would require synchronization
       //       between the threads.
-      child_jni_env_ext.release();
+      child_jni_env_ext.release();  // NOLINT pthreads API.
       return;
     }
   }