[Utility] Replace Cleanup by llvm::scope_exit

[JDevlieghere]

Summary

This removes the CleanUp class and replaces its usages with llvm's ScopeExit which has similar semantics.

[jingham]

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

901–903

This seems an undesirable cleanup. Now it is going to be hard to just put a breakpoint on the cleanup code, which seems worth the one extra line (and not related to replacing CleanUp with scope_exit. Ditto the one on 826. Is clang-format doing this for you?

[JDevlieghere]

Add a comment so you can easily break on the cleanup code.

[jingham]

This pattern repeated a couple of times north or the one you fixed. Can you get those too?

[JDevlieghere]

Add more comments to keep the original formatting.

[JDevlieghere]

In D67378#1664070, @jingham wrote:

This pattern repeated a couple of times north or the one you fixed. Can you get those too?

Thanks, I missed those. I believe I covered all the cases that were multi-line originally.

[vsk]

Thanks a lot!

lldb/include/lldb/Utility/CleanUp.h

-//===-- CleanUp.h -----------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef liblldb_CleanUp_h_
-#define liblldb_CleanUp_h_
-
-#include "lldb/lldb-public.h"
-#include <functional>
-
-namespace lldb_private {
-
-/// Run a cleanup function on scope exit unless it's explicitly disabled.
-class CleanUp {
-  std::function<void()> Clean;
-
-public:
-  /// Register a cleanup function which applies \p Func to a list of arguments.
-  /// Use caution with arguments which are references: they will be copied.
-  template <typename F, typename... Args>
-  CleanUp(F &&Func, Args &&... args)
-      : Clean(std::bind(std::forward<F>(Func), std::forward<Args>(args)...)) {}
-
-  ~CleanUp() {
-    if (Clean)
-      Clean();
-  }
-
-  /// Disable the cleanup.
-  void disable() { Clean = nullptr; }
-
-  // Prevent cleanups from being run more than once.
-  DISALLOW_COPY_AND_ASSIGN(CleanUp);
-};
-
-} // namespace lldb_private
-
-#endif // #ifndef liblldb_CleanUp_h_

lldb/source/Host/macosx/objcxx/Host.mm

 #include <unistd.h>
 
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/ProcessInfo.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-defines.h"
 
-#include "llvm/Support/FileSystem.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include "../cfcpp/CFCBundle.h"
 #include "../cfcpp/CFCMutableArray.h"
 #include "../cfcpp/CFCMutableDictionary.h"
 #include "../cfcpp/CFCReleaser.h"
 #include "../cfcpp/CFCString.h"
 
 #include <objc/objc-auto.h>
static Status LaunchProcessPosixSpawn(const char *exe_path,
   error.SetError(::posix_spawnattr_init(&attr), eErrorTypePOSIX);
 
   if (error.Fail()) {
     LLDB_LOG(log, "error: {0}, ::posix_spawnattr_init ( &attr )", error);
     return error;
   }
 
   // Make sure we clean up the posix spawn attributes before exiting this scope.
-  CleanUp cleanup_attr(posix_spawnattr_destroy, &attr);
+  auto cleanup_attr =
+      llvm::make_scope_exit([&]() { posix_spawnattr_destroy(&attr); });
 
   sigset_t no_signals;
   sigset_t all_signals;
   sigemptyset(&no_signals);
   sigfillset(&all_signals);
   ::posix_spawnattr_setsigmask(&attr, &no_signals);
   ::posix_spawnattr_setsigdefault(&attr, &all_signals);
 
if (num_file_actions > 0) {
     if (error.Fail()) {
       LLDB_LOG(log,
                "error: {0}, ::posix_spawn_file_actions_init ( &file_actions )",
                error);
       return error;
     }
 
     // Make sure we clean up the posix file actions before exiting this scope.
-    CleanUp cleanup_fileact(posix_spawn_file_actions_destroy, &file_actions);
+    auto cleanup_fileact = llvm::make_scope_exit(
+        [&]() { posix_spawn_file_actions_destroy(&file_actions); });
 
     for (size_t i = 0; i < num_file_actions; ++i) {
       const FileAction *launch_file_action =
           launch_info.GetFileActionAtIndex(i);
       if (launch_file_action) {
         if (!AddPosixSpawnFileAction(&file_actions, launch_file_action, log,
                                      error))
           return error;

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Thread.h"
-#include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
+#include "llvm/ADT/ScopeExit.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 /// Default Constructor
 PlatformPOSIX::PlatformPOSIX(bool is_host)
     : RemoteAwarePlatform(is_host), // This is the local host platform
       m_option_group_platform_rsync(new OptionGroupPlatformRSync()),
lldb::addr_t path_addr = process->AllocateMemory(path_len,
                                                    utility_error);
   if (path_addr == LLDB_INVALID_ADDRESS) {
     error.SetErrorStringWithFormat("dlopen error: could not allocate memory"
                                     "for path: %s", utility_error.AsCString());
     return LLDB_INVALID_IMAGE_TOKEN;
   }
   
   // Make sure we deallocate the input string memory:
-  CleanUp path_cleanup([process, path_addr] { 
-      process->DeallocateMemory(path_addr); 
-  });
-  
+  auto path_cleanup = llvm::make_scope_exit(
+      [process, path_addr] { process->DeallocateMemory(path_addr); });
   process->WriteMemory(path_addr, path.c_str(), path_len, utility_error);
   if (utility_error.Fail()) {
     error.SetErrorStringWithFormat("dlopen error: could not write path string:"
                                     " %s", utility_error.AsCString());
     return LLDB_INVALID_IMAGE_TOKEN;
   }
   
   // Make space for our return structure.  It is two pointers big: the token
   // and the error string.
   const uint32_t addr_size = process->GetAddressByteSize();
   lldb::addr_t return_addr = process->CallocateMemory(2*addr_size,
                                                       permissions,
                                                       utility_error);
   if (utility_error.Fail()) {
     error.SetErrorStringWithFormat("dlopen error: could not allocate memory"
                                     "for path: %s", utility_error.AsCString());
     return LLDB_INVALID_IMAGE_TOKEN;
   }
   
   // Make sure we deallocate the result structure memory
-  CleanUp return_cleanup([process, return_addr] {
-      process->DeallocateMemory(return_addr);
-  });
-  
+  auto return_cleanup = llvm::make_scope_exit(
+      [process, return_addr] { process->DeallocateMemory(return_addr); });
   // This will be the address of the storage for paths, if we are using them,
   // or nullptr to signal we aren't.
   lldb::addr_t path_array_addr = 0x0;
-  llvm::Optional<CleanUp> path_array_cleanup;
+  llvm::Optional<llvm::detail::scope_exit<std::function<void()>>>
+      path_array_cleanup;
 
   // This is the address to a buffer large enough to hold the largest path
   // conjoined with the library name we're passing in.  This is a convenience 
   // to avoid having to call malloc in the dlopen function.
   lldb::addr_t buffer_addr = 0x0;
-  llvm::Optional<CleanUp> buffer_cleanup;
-  
+  llvm::Optional<llvm::detail::scope_exit<std::function<void()>>>
+      buffer_cleanup;
   // Set the values into our args and write them to the target:
   if (paths != nullptr) {
     // First insert the paths into the target.  This is expected to be a 
     // continuous buffer with the strings laid out null terminated and
     // end to end with an empty string terminating the buffer.
     // We also compute the buffer's required size as we go.
     size_t buffer_size = 0;
     std::string path_array;
if (paths != nullptr) {
     if (buffer_addr == LLDB_INVALID_ADDRESS) {
       error.SetErrorStringWithFormat("dlopen error: could not allocate memory"
                                       "for buffer: %s", 
                                       utility_error.AsCString());
       return LLDB_INVALID_IMAGE_TOKEN;
     }
   
     // Make sure we deallocate the buffer memory:
-    buffer_cleanup.emplace([process, buffer_addr] { 
-        process->DeallocateMemory(buffer_addr); 
-    });
+    buffer_cleanup.emplace(
+        [process, buffer_addr] { process->DeallocateMemory(buffer_addr); });
   }
     
   arguments.GetValueAtIndex(0)->GetScalar() = path_addr;
   arguments.GetValueAtIndex(1)->GetScalar() = path_array_addr;
   arguments.GetValueAtIndex(2)->GetScalar() = buffer_addr;
   arguments.GetValueAtIndex(3)->GetScalar() = return_addr;
 
   lldb::addr_t func_args_addr = LLDB_INVALID_ADDRESS;
   
   diagnostics.Clear();
   if (!do_dlopen_function->WriteFunctionArguments(exe_ctx, 
                                                  func_args_addr,
                                                  arguments,
                                                  diagnostics)) {
     error.SetErrorStringWithFormat("dlopen error: could not write function "
                                    "arguments: %s", 
                                    diagnostics.GetString().c_str());
     return LLDB_INVALID_IMAGE_TOKEN;
   }
   
   // Make sure we clean up the args structure.  We can't reuse it because the
   // Platform lives longer than the process and the Platforms don't get a
   // signal to clean up cached data when a process goes away.
-  CleanUp args_cleanup([do_dlopen_function, &exe_ctx, func_args_addr] {
+  auto args_cleanup =
+      llvm::make_scope_exit([do_dlopen_function, &exe_ctx, func_args_addr] {
         do_dlopen_function->DeallocateFunctionResults(exe_ctx, func_args_addr);
       });
-  
   // Now run the caller:
   EvaluateExpressionOptions options;
   options.SetExecutionPolicy(eExecutionPolicyAlways);
   options.SetLanguage(eLanguageTypeC_plus_plus);
   options.SetIgnoreBreakpoints(true);
   options.SetUnwindOnError(true);
   options.SetTrapExceptions(false); // dlopen can't throw exceptions, so
                                     // don't do the work to trap them.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 #include "lldb/Target/ABI.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/ThreadPlanCallFunction.h"
 #include "lldb/Utility/Args.h"
-#include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
 #include "GDBRemoteRegisterContext.h"
 #include "Plugins/Platform/MacOSX/PlatformRemoteiOS.h"
 #include "Plugins/Process/Utility/GDBRemoteSignals.h"
 #include "Plugins/Process/Utility/InferiorCallPOSIX.h"
 #include "Plugins/Process/Utility/StopInfoMachException.h"
 #include "ProcessGDBRemote.h"
 #include "ProcessGDBRemoteLog.h"
 #include "ThreadGDBRemote.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
 #define DEBUGSERVER_BASENAME "debugserver"
 using namespace llvm;
 using namespace lldb;
 using namespace lldb_private;
#ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
     int sockets[2]; /* the pair of socket descriptors */
     if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) == -1) {
       error.SetErrorToErrno();
       return error;
     }
 
     int our_socket = sockets[0];
     int gdb_socket = sockets[1];
-    CleanUp cleanup_our(close, our_socket);
-    CleanUp cleanup_gdb(close, gdb_socket);
+    auto cleanup_our = llvm::make_scope_exit([&]() { close(our_socket); });
+    auto cleanup_gdb = llvm::make_scope_exit([&]() { close(gdb_socket); });
 
     // Don't let any child processes inherit our communication socket
     SetCloexecFlag(our_socket);
     communication_fd = gdb_socket;
 #endif
 
     error = m_gdb_comm.StartDebugserverProcess(
         nullptr, GetTarget().GetPlatform().get(), debugserver_launch_info,
         nullptr, nullptr, communication_fd);
 
     if (error.Success())
       m_debugserver_pid = debugserver_launch_info.GetProcessID();
     else
       m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
 
     if (m_debugserver_pid != LLDB_INVALID_PROCESS_ID) {
 #ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
       // Our process spawned correctly, we can now set our connection to use
       // our end of the socket pair
-      cleanup_our.disable();
+      cleanup_our.release();
       m_gdb_comm.SetConnection(new ConnectionFileDescriptor(our_socket, true));
 #endif
       StartAsyncThread();
     }
 
     if (error.Fail()) {
       Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
 

lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

 #include "Host/macosx/cfcpp/CFCData.h"
 #include "Host/macosx/cfcpp/CFCReleaser.h"
 #include "Host/macosx/cfcpp/CFCString.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 #include "lldb/Utility/UUID.h"
 #include "mach/machine.h"
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static CFURLRef (*g_dlsym_DBGCopyFullDSYMURLForUUID)(CFUUIDRef uuid, CFURLRef exec_url) = nullptr;
 static CFDictionaryRef (*g_dlsym_DBGCopyDSYMPropertyLists)(CFURLRef dsym_url) = nullptr;
 
FileSpec Symbols::FindSymbolFileInBundle(const FileSpec &dsym_bundle_fspec,
 
   ::strncat(path, "/Contents/Resources/DWARF", sizeof(path) - strlen(path) - 1);
 
   DIR *dirp = opendir(path);
   if (!dirp)
     return {};
 
   // Make sure we close the directory before exiting this scope.
-  CleanUp cleanup_dir(closedir, dirp);
+  auto cleanup_dir = llvm::make_scope_exit([&]() { closedir(dirp); });
 
   FileSpec dsym_fspec;
   dsym_fspec.GetDirectory().SetCString(path);
   struct dirent *dp;
   while ((dp = readdir(dirp)) != NULL) {
     // Only search directories
     if (dp->d_type == DT_DIR || dp->d_type == DT_UNKNOWN) {
       if (dp->d_namlen == 1 && dp->d_name[0] == '.')

lldb/tools/lldb-test/lldb-test.cpp

 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 
 #include "llvm/ADT/IntervalMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/ScopeExit.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
 #include <cstdio>
 #include <thread>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
int main(int argc, const char *argv[]) {
   SystemLifetimeManager DebuggerLifetime;
   if (auto e = DebuggerLifetime.Initialize(
           std::make_unique<SystemInitializerTest>(), nullptr)) {
     WithColor::error() << "initialization failed: " << toString(std::move(e))
                        << '\n';
     return 1;
   }
 
-  CleanUp TerminateDebugger([&] { DebuggerLifetime.Terminate(); });
+  auto TerminateDebugger =
+      llvm::make_scope_exit([&] { DebuggerLifetime.Terminate(); });
 
   auto Dbg = lldb_private::Debugger::CreateInstance();
   ModuleList::GetGlobalModuleListProperties().SetEnableExternalLookup(false);
   CommandReturnObject Result;
   Dbg->GetCommandInterpreter().HandleCommand(
       "settings set plugin.process.gdb-remote.packet-timeout 60",
       /*add_to_history*/ eLazyBoolNo, Result);
 

lldb/unittests/Utility/CMakeLists.txt

 add_lldb_unittest(UtilityTests
   AnsiTerminalTest.cpp
   ArgsTest.cpp
   OptionsWithRawTest.cpp
   ArchSpecTest.cpp
   BroadcasterTest.cpp
-  CleanUpTest.cpp
   ConstStringTest.cpp
   CompletionRequestTest.cpp
   DataExtractorTest.cpp
   EnvironmentTest.cpp
   EventTest.cpp
   FileSpecTest.cpp
   FlagsTest.cpp
   JSONTest.cpp

lldb/unittests/Utility/CleanUpTest.cpp

-//===-- CleanUpTest.cpp -----------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "lldb/Utility/CleanUp.h"
-#include "gtest/gtest.h"
-
-using namespace lldb_private;
-
-TEST(CleanUpTest, no_args) {
-  bool f = false;
-  {
-    CleanUp cleanup([&] { f = true; });
-  }
-  ASSERT_TRUE(f);
-}
-
-TEST(CleanUpTest, multiple_args) {
-  bool f1 = false;
-  bool f2 = false;
-  bool f3 = false;
-  {
-    CleanUp cleanup(
-        [](bool arg1, bool *arg2, bool &arg3) {
-          ASSERT_FALSE(arg1);
-          *arg2 = true;
-          arg3 = true;
-        },
-        f1, &f2, f3);
-  }
-  ASSERT_TRUE(f2);
-  ASSERT_FALSE(f3);
-}
-
-TEST(CleanUpTest, disable) {
-  bool f = false;
-  {
-    CleanUp cleanup([&] { f = true; });
-    cleanup.disable();
-  }
-  ASSERT_FALSE(f);
-}