[Utility] Simplify and generalize the CleanUp helper, NFC

[vsk]

Summary

Removing the template arguments and most of the mutating methods from
CleanUp makes it easier to understand and reuse.

In its present state, CleanUp would be too cumbersome to adapt to cases
where multiple objects need to be released. Take for example this change
in swift-lldb:

https://github.com/apple/swift-lldb/pull/334/files#diff-6f474df750f75c8ba675f2a8408a5629R219

This change is simple to express with the new CleanUp, but not so simple
with the old version.

[zturner]

Couldn't we just add a creator helper so that you can say:

auto cleanup = makeCleanupRAII([&] {
    process_sp->Destroy(/*force_kill=*/false);
    debugger.StopEventHandlerThread();
  });

[labath]

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

This would be a generalization of your syntax, so you could still use a lambda when needed, but you could avoid the lambda-capture dance for the simple cases.

[labath]

Btw, I think we should also move the CleanUp class out of the lldb_utility and into lldb_private namespace. we have been slowly getting rid of these...

[vsk]

Couldn't we just add a creator helper so that you can say:

auto cleanup = makeCleanupRAII([&] {

  process_sp->Destroy(/*force_kill=*/false);
  debugger.StopEventHandlerThread();
});

@zturner In this version of the patch, CleanUp accepts a std::function, which gets rid of the lifetime issue with llvm::function_ref. So with this you should bee able to write "auto cleanup = CleanUp([&] { ... })". Does that work for you?

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

@labath I find the current version of the code more readable, since the behavior of the cleanup is apparent in less time. And I'd prefer to keep the CleanUp utility very small, without any scope for expansion. I don't think the ergonomics of the new syntax outweigh the advantages of a simpler approach.

we should also move the CleanUp class out of the lldb_utility and into lldb_private namespace

Sounds good, I'll update the patch once we hash out the interface.

[labath]

In D43662#1016939, @vsk wrote:

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

@labath I find the current version of the code more readable, since the behavior of the cleanup is apparent in less time. And I'd prefer to keep the CleanUp utility very small, without any scope for expansion. I don't think the ergonomics of the new syntax outweigh the advantages of a simpler approach.

I don't think this would complicate anything. It should literally be a matter of replacing your constructor with:

template<typename F, typename... Args>
CleanUp(F &&f, Args &&..args)
    : PerformCleanup(true), Clean(std::bind(std::forward<F>(f), std::forward<Args>(args)...) {}

Although I can see how you may find the other usage syntax more understandable, so I am not adamant about this...

[vsk]

In D43662#1016950, @labath wrote:

In D43662#1016939, @vsk wrote:

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

@labath I find the current version of the code more readable, since the behavior of the cleanup is apparent in less time. And I'd prefer to keep the CleanUp utility very small, without any scope for expansion. I don't think the ergonomics of the new syntax outweigh the advantages of a simpler approach.

I don't think this would complicate anything. It should literally be a matter of replacing your constructor with:

template<typename F, typename... Args>
CleanUp(F &&f, Args &&..args)
    : PerformCleanup(true), Clean(std::bind(std::forward<F>(f), std::forward<Args>(args)...) {}

Although I can see how you may find the other usage syntax more understandable, so I am not adamant about this...

Thanks for sharing this (I haven't reached for parameter packs before :). I'll still maintain that having the lambda written-out explicitly requires less effort from prospective readers, but am happy to be outvoted on this one. Pavel, others, wdyt?

[labath]

While we wait for the verdict on the template stuff, it occurred to me that you don't need the PerformCleanup bool member as std::function has an "empty" state. So the destructor can just do if(Cleanup) Cleanup() and the disable function can reset the cleanup object. (If you want to make sure the object is always created with a non-empty std::function initially, you can add an assert in the constructor).

[JDevlieghere]

I agree with Pavel, I prefer having a slightly more complex constructor if we can make the call sites simpler for the simple common cases, especially since we don't lose any generality by doing so. It's also not an uncommon pattern in C++, e.g. std::async does something very similar.

Also +1 on just calling operator bool on the function itself.

[vsk]

Thanks for the feedback.

I've added support for forwarding arguments to the cleanup function though the constructor. It uses std::bind instead of a lambda because I couldn't figure out a simpler way to support reference arguments. (Btw, llvm::ThreadPool does the same thing.)

I've also added a unit test. PTAL.

[labath]

This looks great, thanks. I've been wanting to do something about the CleanUp class for a long time. I have just a couple of nits, but no need to go through review again.

unittests/Utility/CleanUpTest.cpp

1 ↗

(On Diff #135671)

s/test/Test/

13 ↗

(On Diff #135671)

Looks like this is left-over from testing.

[vsk]

Thanks! I'll clean up the issues you pointed out before committing.

[amccarth]

Nit: As a single word, "cleanup" is a noun (or an adjective). As two words, "clean up" is a verb. Given that, I'd expect to see classes and objects with names like Cleanup or cleanup and functions to contain CleanUp. When I see an identifier like CleanUpTest, I expect it's a function that cleans up after a test, not a test of a class called CleanUp.

lldb/trunk/include/lldb/Utility/CleanUp.h

 //===-- CleanUp.h -----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_CleanUp_h_
 #define liblldb_CleanUp_h_
 
 #include "lldb/lldb-public.h"
 #include <functional>
 
-namespace lldb_utility {
+namespace lldb_private {
 
-//----------------------------------------------------------------------
-// Templated class that guarantees that a cleanup callback function will
-// be called. The cleanup function will be called once under the
-// following conditions:
-// - when the object goes out of scope
-// - when the user explicitly calls clean.
-// - the current value will be cleaned up when a new value is set using
-//   set(T value) as long as the current value hasn't already been cleaned.
-//
-// This class is designed to be used with simple types for type T (like
-// file descriptors, opaque handles, pointers, etc). If more complex
-// type T objects are desired, we need to probably specialize this class
-// to take "const T&" for all input T parameters. Yet if a type T is
-// complex already it might be better to build the cleanup functionality
-// into T.
-//
-// The cleanup function must take one argument that is of type T.
-// The calback function return type is R. The return value is currently
-// needed for "CallbackType". If there is an easy way to get around the
-// need for the return value we can change this class.
-//
-// The two template parameters are:
-//    T - The variable type of value that will be stored and used as the
-//      sole argument for the cleanup callback.
-//    R - The return type for the cleanup function.
-//
-// EXAMPLES
-//  // Use with file handles that get opened where you want to close
-//  // them. Below we use "int open(const char *path, int oflag, ...)"
-//  // which returns an integer file descriptor. -1 is the invalid file
-//  // descriptor so to make an object that will call "int close(int fd)"
-//  // automatically we can use:
-//
-//  CleanUp <int, int> fd(open("/tmp/a.txt", O_RDONLY, 0), -1, close);
-//
-//  // malloc/free example
-//  CleanUp <void *, void> malloced_bytes(malloc(32), NULL, free);
-//----------------------------------------------------------------------
-template <typename T, typename R = void> class CleanUp {
-public:
-  typedef T value_type;
-  typedef std::function<R(value_type)> CallbackType;
-
-  //----------------------------------------------------------------------
-  // Constructor that sets the current value only. No values are
-  // considered to be invalid and the cleanup function will be called
-  // regardless of the value of m_current_value.
-  //----------------------------------------------------------------------
-  CleanUp(value_type value, CallbackType callback)
-      : m_current_value(value), m_invalid_value(), m_callback(callback),
-        m_callback_called(false), m_invalid_value_is_valid(false) {}
-
-  //----------------------------------------------------------------------
-  // Constructor that sets the current value and also the invalid value.
-  // The cleanup function will be called on "m_value" as long as it isn't
-  // equal to "m_invalid_value".
-  //----------------------------------------------------------------------
-  CleanUp(value_type value, value_type invalid, CallbackType callback)
-      : m_current_value(value), m_invalid_value(invalid), m_callback(callback),
-        m_callback_called(false), m_invalid_value_is_valid(true) {}
-
-  //----------------------------------------------------------------------
-  // Automatically cleanup when this object goes out of scope.
-  //----------------------------------------------------------------------
-  ~CleanUp() { clean(); }
-
-  //----------------------------------------------------------------------
-  // Access the value stored in this class
-  //----------------------------------------------------------------------
-  value_type get() { return m_current_value; }
-
-  //----------------------------------------------------------------------
-  // Access the value stored in this class
-  //----------------------------------------------------------------------
-  const value_type get() const { return m_current_value; }
-
-  //----------------------------------------------------------------------
-  // Reset the owned value to "value". If a current value is valid and
-  // the cleanup callback hasn't been called, the previous value will
-  // be cleaned up (see void CleanUp::clean()).
-  //----------------------------------------------------------------------
-  void set(const value_type value) {
-    // Cleanup the current value if needed
-    clean();
-    // Now set the new value and mark our callback as not called
-    m_callback_called = false;
-    m_current_value = value;
-  }
-
-  //----------------------------------------------------------------------
-  // Checks is "m_current_value" is valid. The value is considered valid
-  // no invalid value was supplied during construction of this object or
-  // if an invalid value was supplied and "m_current_value" is not equal
-  // to "m_invalid_value".
-  //
-  // Returns true if "m_current_value" is valid, false otherwise.
-  //----------------------------------------------------------------------
-  bool is_valid() const {
-    if (m_invalid_value_is_valid)
-      return m_current_value != m_invalid_value;
-    return true;
-  }
-
-  //----------------------------------------------------------------------
-  // This function will call the cleanup callback provided in the
-  // constructor one time if the value is considered valid (See is_valid()).
-  // This function sets m_callback_called to true so we don't call the
-  // cleanup callback multiple times on the same value.
-  //----------------------------------------------------------------------
-  void clean() {
-    if (m_callback && !m_callback_called) {
-      m_callback_called = true;
-      if (is_valid())
-        m_callback(m_current_value);
-    }
-  }
+/// Run a cleanup function on scope exit unless it's explicitly disabled.
+class CleanUp {
+  std::function<void()> Clean;
 
-  //----------------------------------------------------------------------
-  // Cancels the cleanup that would have been called on "m_current_value"
-  // if it was valid. This function can be used to release the value
-  // contained in this object so ownership can be transferred to the caller.
-  //----------------------------------------------------------------------
-  value_type release() {
-    m_callback_called = true;
-    return m_current_value;
-  }
-
-private:
-  value_type m_current_value;
-  const value_type m_invalid_value;
-  CallbackType m_callback;
-  bool m_callback_called;
-  bool m_invalid_value_is_valid;
-
-  // Outlaw default constructor, copy constructor and the assignment operator
-  DISALLOW_COPY_AND_ASSIGN(CleanUp);
-};
-
-template <typename T, typename R, typename A0> class CleanUp2 {
 public:
-  typedef T value_type;
-  typedef std::function<R(value_type, A0)> CallbackType;
-
-  //----------------------------------------------------------------------
-  // Constructor that sets the current value only. No values are
-  // considered to be invalid and the cleanup function will be called
-  // regardless of the value of m_current_value.
-  //----------------------------------------------------------------------
-  CleanUp2(value_type value, CallbackType callback, A0 arg)
-      : m_current_value(value), m_invalid_value(), m_callback(callback),
-        m_callback_called(false), m_invalid_value_is_valid(false),
-        m_argument(arg) {}
-
-  //----------------------------------------------------------------------
-  // Constructor that sets the current value and also the invalid value.
-  // The cleanup function will be called on "m_value" as long as it isn't
-  // equal to "m_invalid_value".
-  //----------------------------------------------------------------------
-  CleanUp2(value_type value, value_type invalid, CallbackType callback, A0 arg)
-      : m_current_value(value), m_invalid_value(invalid), m_callback(callback),
-        m_callback_called(false), m_invalid_value_is_valid(true),
-        m_argument(arg) {}
-
-  //----------------------------------------------------------------------
-  // Automatically cleanup when this object goes out of scope.
-  //----------------------------------------------------------------------
-  ~CleanUp2() { clean(); }
-
-  //----------------------------------------------------------------------
-  // Access the value stored in this class
-  //----------------------------------------------------------------------
-  value_type get() { return m_current_value; }
+  /// 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)...)) {}
 
-  //----------------------------------------------------------------------
-  // Access the value stored in this class
-  //----------------------------------------------------------------------
-  const value_type get() const { return m_current_value; }
-
-  //----------------------------------------------------------------------
-  // Reset the owned value to "value". If a current value is valid and
-  // the cleanup callback hasn't been called, the previous value will
-  // be cleaned up (see void CleanUp::clean()).
-  //----------------------------------------------------------------------
-  void set(const value_type value) {
-    // Cleanup the current value if needed
-    clean();
-    // Now set the new value and mark our callback as not called
-    m_callback_called = false;
-    m_current_value = value;
-  }
-
-  //----------------------------------------------------------------------
-  // Checks is "m_current_value" is valid. The value is considered valid
-  // no invalid value was supplied during construction of this object or
-  // if an invalid value was supplied and "m_current_value" is not equal
-  // to "m_invalid_value".
-  //
-  // Returns true if "m_current_value" is valid, false otherwise.
-  //----------------------------------------------------------------------
-  bool is_valid() const {
-    if (m_invalid_value_is_valid)
-      return m_current_value != m_invalid_value;
-    return true;
-  }
-
-  //----------------------------------------------------------------------
-  // This function will call the cleanup callback provided in the
-  // constructor one time if the value is considered valid (See is_valid()).
-  // This function sets m_callback_called to true so we don't call the
-  // cleanup callback multiple times on the same value.
-  //----------------------------------------------------------------------
-  void clean() {
-    if (m_callback && !m_callback_called) {
-      m_callback_called = true;
-      if (is_valid())
-        m_callback(m_current_value, m_argument);
-    }
+  ~CleanUp() {
+    if (Clean)
+      Clean();
   }
 
-  //----------------------------------------------------------------------
-  // Cancels the cleanup that would have been called on "m_current_value"
-  // if it was valid. This function can be used to release the value
-  // contained in this object so ownership can be transferred to the caller.
-  //----------------------------------------------------------------------
-  value_type release() {
-    m_callback_called = true;
-    return m_current_value;
-  }
+  /// Disable the cleanup.
+  void disable() { Clean = nullptr; }
 
-private:
-  value_type m_current_value;
-  const value_type m_invalid_value;
-  CallbackType m_callback;
-  bool m_callback_called;
-  bool m_invalid_value_is_valid;
-  A0 m_argument;
-
-  // Outlaw default constructor, copy constructor and the assignment operator
-  DISALLOW_COPY_AND_ASSIGN(CleanUp2);
+  // Prevent cleanups from being run more than once.
+  DISALLOW_COPY_AND_ASSIGN(CleanUp);
 };
 
-} // namespace lldb_utility
+} // namespace lldb_private
 
 #endif // #ifndef liblldb_CleanUp_h_

lldb/trunk/lldb.xcodeproj/project.pbxproj

/* Begin PBXBuildFile section */
     6D762BEE1B1605D2006C929D /* LLDBServerUtilities.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D762BEC1B1605CD006C929D /* LLDBServerUtilities.cpp */; };
     6D86CEA01B440F8500A7FBFA /* CommandObjectBugreport.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D86CE9E1B440F6B00A7FBFA /* CommandObjectBugreport.cpp */; };
     6D95DC001B9DC057000E318A /* DIERef.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D95DBFD1B9DC057000E318A /* DIERef.cpp */; };
     6D95DC011B9DC057000E318A /* HashedNameToDIE.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D95DBFE1B9DC057000E318A /* HashedNameToDIE.cpp */; };
     6D95DC021B9DC057000E318A /* SymbolFileDWARFDwo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D95DBFF1B9DC057000E318A /* SymbolFileDWARFDwo.cpp */; };
     6D99A3631BBC2F3200979793 /* ArmUnwindInfo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D99A3621BBC2F3200979793 /* ArmUnwindInfo.cpp */; };
     6D9AB3DD1BB2B74E003F2289 /* TypeMap.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D9AB3DC1BB2B74E003F2289 /* TypeMap.cpp */; };
     6DEC6F391BD66D750091ABA6 /* TaskPool.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6DEC6F381BD66D750091ABA6 /* TaskPool.cpp */; };
+    7F94D7182040A13A006EE3EA /* CleanUpTest.cpp in CopyFiles */ = {isa = PBXBuildFile; fileRef = 7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */; };
     8C26C4261C3EA5F90031DF7C /* TSanRuntime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C26C4241C3EA4340031DF7C /* TSanRuntime.cpp */; };
     8C2D6A53197A1EAF006989C9 /* MemoryHistory.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C2D6A52197A1EAF006989C9 /* MemoryHistory.cpp */; };
     8C2D6A5E197A250F006989C9 /* MemoryHistoryASan.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C2D6A5A197A1FDC006989C9 /* MemoryHistoryASan.cpp */; };
     8C3BD9961EF45DA50016C343 /* MainThreadCheckerRuntime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C3BD9951EF45D9B0016C343 /* MainThreadCheckerRuntime.cpp */; };
     8C3BD9A01EF5D1FF0016C343 /* JSONTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C3BD99F1EF5D1B50016C343 /* JSONTest.cpp */; };
     8CCB017E19BA28A80009FD44 /* ThreadCollection.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CCB017A19BA283D0009FD44 /* ThreadCollection.cpp */; };
     8CCB018219BA4E270009FD44 /* SBThreadCollection.h in Headers */ = {isa = PBXBuildFile; fileRef = 8CCB018119BA4E210009FD44 /* SBThreadCollection.h */; settings = {ATTRIBUTES = (Public, ); }; };
     8CCB018319BA51BF0009FD44 /* SBThreadCollection.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CCB017F19BA4DD00009FD44 /* SBThreadCollection.cpp */; };
942829BE1A89835300521B30 /* CopyFiles */ = {
       runOnlyForDeploymentPostprocessing = 1;
     };
     AF90106415AB7D2900FF120D /* CopyFiles */ = {
       isa = PBXCopyFilesBuildPhase;
       buildActionMask = 8;
       dstPath = "$(DEVELOPER_INSTALL_DIR)/usr/share/man/man1";
       dstSubfolderSpec = 0;
       files = (
+        7F94D7182040A13A006EE3EA /* CleanUpTest.cpp in CopyFiles */,
         AF90106515AB7D3600FF120D /* lldb.1 in CopyFiles */,
       );
       runOnlyForDeploymentPostprocessing = 1;
     };
     AFF87C85150FF5CC000E1742 /* CopyFiles */ = {
       isa = PBXCopyFilesBuildPhase;
       buildActionMask = 8;
       dstPath = /Developer/Library/Lockdown/ServiceAgents/;

     6D95DC031B9DC06F000E318A /* DIERef.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DIERef.h; sourceTree = "<group>"; };
     6D95DC041B9DC06F000E318A /* SymbolFileDWARFDwo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SymbolFileDWARFDwo.h; sourceTree = "<group>"; };
     6D99A3611BBC2F1600979793 /* ArmUnwindInfo.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ArmUnwindInfo.h; path = include/lldb/Symbol/ArmUnwindInfo.h; sourceTree = "<group>"; };
     6D99A3621BBC2F3200979793 /* ArmUnwindInfo.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = ArmUnwindInfo.cpp; path = source/Symbol/ArmUnwindInfo.cpp; sourceTree = "<group>"; };
     6D9AB3DC1BB2B74E003F2289 /* TypeMap.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = TypeMap.cpp; path = source/Symbol/TypeMap.cpp; sourceTree = "<group>"; };
     6D9AB3DE1BB2B76B003F2289 /* TypeMap.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = TypeMap.h; path = include/lldb/Symbol/TypeMap.h; sourceTree = "<group>"; };
     6DEC6F381BD66D750091ABA6 /* TaskPool.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = TaskPool.cpp; path = source/Host/common/TaskPool.cpp; sourceTree = "<group>"; };
     6DEC6F3A1BD66D950091ABA6 /* TaskPool.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = TaskPool.h; path = include/lldb/Host/TaskPool.h; sourceTree = "<group>"; };
+    7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CleanUpTest.cpp; sourceTree = "<group>"; };
     8C26C4241C3EA4340031DF7C /* TSanRuntime.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = TSanRuntime.cpp; path = TSan/TSanRuntime.cpp; sourceTree = "<group>"; };
     8C26C4251C3EA4340031DF7C /* TSanRuntime.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = TSanRuntime.h; path = TSan/TSanRuntime.h; sourceTree = "<group>"; };
     8C2D6A52197A1EAF006989C9 /* MemoryHistory.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = MemoryHistory.cpp; path = source/Target/MemoryHistory.cpp; sourceTree = "<group>"; };
     8C2D6A54197A1EBE006989C9 /* MemoryHistory.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = MemoryHistory.h; path = include/lldb/Target/MemoryHistory.h; sourceTree = "<group>"; };
     8C2D6A5A197A1FDC006989C9 /* MemoryHistoryASan.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MemoryHistoryASan.cpp; sourceTree = "<group>"; };
     8C2D6A5B197A1FDC006989C9 /* MemoryHistoryASan.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MemoryHistoryASan.h; sourceTree = "<group>"; };
     8C3BD9931EF45D9B0016C343 /* MainThreadCheckerRuntime.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MainThreadCheckerRuntime.h; sourceTree = "<group>"; };
     8C3BD9951EF45D9B0016C343 /* MainThreadCheckerRuntime.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = MainThreadCheckerRuntime.cpp; sourceTree = "<group>"; };
2321F93F1BDD33D800BA9A93 /* ScriptInterpreter */ = {
         2321F94B1BDD35D500BA9A93 /* Python */,
       );
       path = ScriptInterpreter;
       sourceTree = "<group>";
     };
     2321F9421BDD343A00BA9A93 /* Utility */ = {
       isa = PBXGroup;
       children = (
+        7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */,
         23E2E5161D903689006F38BB /* ArchSpecTest.cpp */,
         9A3D43C81F3150D200EB767C /* ConstStringTest.cpp */,
         9A3D43C71F3150D200EB767C /* LogTest.cpp */,
         9A3D43CB1F3150D200EB767C /* NameMatchesTest.cpp */,
         9A3D43C61F3150D200EB767C /* StatusTest.cpp */,
         9A3D43CA1F3150D200EB767C /* StructuredDataTest.cpp */,
         9A3D43C91F3150D200EB767C /* TildeExpressionResolverTest.cpp */,
         9A3D43CC1F3150D200EB767C /* TimeoutTest.cpp */,

lldb/trunk/source/Host/macosx/Host.mm

static Status LaunchProcessPosixSpawn(const char *exe_path,
   posix_spawnattr_t attr;
   error.SetError(::posix_spawnattr_init(&attr), eErrorTypePOSIX);
 
   if (error.Fail()) {
     LLDB_LOG(log, "error: {0}, ::posix_spawnattr_init ( &attr )", error);
     return error;
   }
 
-  // Make a quick class that will cleanup the posix spawn attributes in case
-  // we return in the middle of this function.
-  lldb_utility::CleanUp<posix_spawnattr_t *, int> posix_spawnattr_cleanup(
-      &attr, posix_spawnattr_destroy);
+  // Make sure we clean up the posix spawn attributes before exiting this scope.
+  CleanUp cleanup_attr(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);
 
error.SetError(::posix_spawn_file_actions_init(&file_actions),
                    eErrorTypePOSIX);
     if (error.Fail()) {
       LLDB_LOG(log,
                "error: {0}, ::posix_spawn_file_actions_init ( &file_actions )",
                error);
       return error;
     }
 
-    // Make a quick class that will cleanup the posix spawn attributes in case
-    // we return in the middle of this function.
-    lldb_utility::CleanUp<posix_spawn_file_actions_t *, int>
-        posix_spawn_file_actions_cleanup(&file_actions,
-                                         posix_spawn_file_actions_destroy);
+    // Make sure we clean up the posix file actions before exiting this scope.
+    CleanUp cleanup_fileact(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/trunk/source/Host/macosx/Symbols.cpp

#endif // #if !defined (__arm__) && !defined (__arm64__) && !defined
 
   return items_found;
 }
 
 FileSpec Symbols::FindSymbolFileInBundle(const FileSpec &dsym_bundle_fspec,
                                          const lldb_private::UUID *uuid,
                                          const ArchSpec *arch) {
   char path[PATH_MAX];
+  if (dsym_bundle_fspec.GetPath(path, sizeof(path)) == 0)
+    return {};
 
-  FileSpec dsym_fspec;
+  ::strncat(path, "/Contents/Resources/DWARF", sizeof(path) - strlen(path) - 1);
+
+  DIR *dirp = opendir(path);
+  if (!dirp)
+    return {};
 
-  if (dsym_bundle_fspec.GetPath(path, sizeof(path))) {
-    ::strncat(path, "/Contents/Resources/DWARF",
-              sizeof(path) - strlen(path) - 1);
+  // Make sure we close the directory before exiting this scope.
+  CleanUp cleanup_dir(closedir, dirp);
 
-    lldb_utility::CleanUp<DIR *, int> dirp(opendir(path), NULL, closedir);
-    if (dirp.is_valid()) {
-      dsym_fspec.GetDirectory().SetCString(path);
-      struct dirent *dp;
-      while ((dp = readdir(dirp.get())) != NULL) {
-        // Only search directories
-        if (dp->d_type == DT_DIR || dp->d_type == DT_UNKNOWN) {
-          if (dp->d_namlen == 1 && dp->d_name[0] == '.')
-            continue;
+  FileSpec dsym_fspec;
+  while ((dp = readdir(dirp)) != NULL) {
 
-          if (dp->d_namlen == 2 && dp->d_name[0] == '.' && dp->d_name[1] == '.')
-            continue;
-        }
 
-        if (dp->d_type == DT_REG || dp->d_type == DT_UNKNOWN) {
-          dsym_fspec.GetFilename().SetCString(dp->d_name);
-          ModuleSpecList module_specs;
-          if (ObjectFile::GetModuleSpecifications(dsym_fspec, 0, 0,
-                                                  module_specs)) {
-            ModuleSpec spec;
-            for (size_t i = 0; i < module_specs.GetSize(); ++i) {
-              bool got_spec = module_specs.GetModuleSpecAtIndex(i, spec);
-              UNUSED_IF_ASSERT_DISABLED(got_spec);
-              assert(got_spec);
-              if ((uuid == NULL ||
-                   (spec.GetUUIDPtr() && spec.GetUUID() == *uuid)) &&
-                  (arch == NULL ||
-                   (spec.GetArchitecturePtr() &&
-                    spec.GetArchitecture().IsCompatibleMatch(*arch)))) {
-                return dsym_fspec;
-              }
-            }
-          }
-        }
-      }
-    }
-  }
-  dsym_fspec.Clear();
-  return dsym_fspec;
+      if (ObjectFile::GetModuleSpecifications(dsym_fspec, 0, 0, module_specs)) {
+
+  return {};
 }
 
 static bool GetModuleSpecInfoFromUUIDDictionary(CFDictionaryRef uuid_dict,
                                                 ModuleSpec &module_spec) {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
   bool success = false;
   if (uuid_dict != NULL && CFGetTypeID(uuid_dict) == CFDictionaryGetTypeID()) {
     std::string str;

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

if (m_debugserver_pid == LLDB_INVALID_PROCESS_ID) {
     const std::weak_ptr<ProcessGDBRemote> this_wp =
         std::static_pointer_cast<ProcessGDBRemote>(shared_from_this());
     debugserver_launch_info.SetMonitorProcessCallback(
         std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3, _4), false);
     debugserver_launch_info.SetUserID(process_info.GetUserID());
 
     int communication_fd = -1;
 #ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
-    // Auto close the sockets we might open up unless everything goes OK. This
-    // helps us not leak file descriptors when things go wrong.
-    lldb_utility::CleanUp<int, int> our_socket(-1, -1, close);
-    lldb_utility::CleanUp<int, int> gdb_socket(-1, -1, close);
-
     // Use a socketpair on non-Windows systems for security and performance
     // reasons.
-    {
-      int sockets[2]; /* the pair of socket descriptors */
-      if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) == -1) {
-        error.SetErrorToErrno();
-        return error;
-      }
 
-      our_socket.set(sockets[0]);
-      gdb_socket.set(sockets[1]);
-    }
+    int our_socket = sockets[0];
+    int gdb_socket = sockets[1];
+    CleanUp cleanup_our(close, our_socket);
+    CleanUp cleanup_gdb(close, gdb_socket);
 
     // Don't let any child processes inherit our communication socket
-    SetCloexecFlag(our_socket.get());
-    communication_fd = gdb_socket.get();
+    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
-      m_gdb_comm.SetConnection(
-          new ConnectionFileDescriptor(our_socket.release(), true));
+      cleanup_our.disable();
+      m_gdb_comm.SetConnection(new ConnectionFileDescriptor(our_socket, true));
 #endif
       StartAsyncThread();
     }
 
     if (error.Fail()) {
       Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
 
       if (log)

lldb/trunk/unittests/Utility/CMakeLists.txt

 add_lldb_unittest(UtilityTests
   ArchSpecTest.cpp
+  CleanUpTest.cpp
   ConstStringTest.cpp
   EnvironmentTest.cpp
   JSONTest.cpp
   LogTest.cpp
   NameMatchesTest.cpp
   StatusTest.cpp
   StringExtractorTest.cpp
   StructuredDataTest.cpp

lldb/trunk/unittests/Utility/CleanUpTest.cpp

+//===-- CleanUpTest.cpp -----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#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);
+}