vsk updated this revision to Diff 135671.
vsk added a comment.
Herald added a subscriber: mgorny.
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.
https://reviews.llvm.org/D43662
Files:
include/lldb/Utility/CleanUp.h
lldb.xcodeproj/project.pbxproj
source/Host/macosx/Host.mm
source/Host/macosx/Symbols.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
unittests/Utility/CMakeLists.txt
unittests/Utility/CleanUpTest.cpp
Index: unittests/Utility/CleanUpTest.cpp
===================================================================
--- /dev/null
+++ unittests/Utility/CleanUpTest.cpp
@@ -0,0 +1,49 @@
+//===-- 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"
+
+#include <iostream>
+
+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);
+}
Index: unittests/Utility/CMakeLists.txt
===================================================================
--- unittests/Utility/CMakeLists.txt
+++ unittests/Utility/CMakeLists.txt
@@ -1,5 +1,6 @@
add_lldb_unittest(UtilityTests
ArchSpecTest.cpp
+ CleanUpTest.cpp
ConstStringTest.cpp
EnvironmentTest.cpp
JSONTest.cpp
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3316,27 +3316,22 @@
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 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);
+
// 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(
@@ -3352,8 +3347,8 @@
#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();
}
Index: source/Host/macosx/Symbols.cpp
===================================================================
--- source/Host/macosx/Symbols.cpp
+++ source/Host/macosx/Symbols.cpp
@@ -240,52 +240,53 @@
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);
- if (dsym_bundle_fspec.GetPath(path, sizeof(path))) {
- ::strncat(path, "/Contents/Resources/DWARF",
- sizeof(path) - strlen(path) - 1);
-
- 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;
-
- if (dp->d_namlen == 2 && dp->d_name[0] == '.' && dp->d_name[1] == '.')
- continue;
- }
+ DIR *dirp = opendir(path);
+ if (!dirp)
+ return {};
- 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;
- }
- }
+ // Make sure we close the directory before exiting this scope.
+ CleanUp cleanup_dir(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] == '.')
+ continue;
+
+ 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;
+
+ return {};
}
static bool GetModuleSpecInfoFromUUIDDictionary(CFDictionaryRef uuid_dict,
Index: source/Host/macosx/Host.mm
===================================================================
--- source/Host/macosx/Host.mm
+++ source/Host/macosx/Host.mm
@@ -1277,10 +1277,8 @@
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;
@@ -1382,11 +1380,8 @@
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 =
Index: lldb.xcodeproj/project.pbxproj
===================================================================
--- lldb.xcodeproj/project.pbxproj
+++ lldb.xcodeproj/project.pbxproj
@@ -770,6 +770,7 @@
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 */; };
@@ -1242,6 +1243,7 @@
dstPath = "$(DEVELOPER_INSTALL_DIR)/usr/share/man/man1";
dstSubfolderSpec = 0;
files = (
+ 7F94D7182040A13A006EE3EA /* CleanUpTest.cpp in CopyFiles */,
AF90106515AB7D3600FF120D /* lldb.1 in CopyFiles */,
);
runOnlyForDeploymentPostprocessing = 1;
@@ -2672,6 +2674,7 @@
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>"; };
@@ -3419,6 +3422,7 @@
2321F9421BDD343A00BA9A93 /* Utility */ = {
isa = PBXGroup;
children = (
+ 7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */,
23E2E5161D903689006F38BB /* ArchSpecTest.cpp */,
9A3D43C81F3150D200EB767C /* ConstStringTest.cpp */,
9A3D43C71F3150D200EB767C /* LogTest.cpp */,
Index: include/lldb/Utility/CleanUp.h
===================================================================
--- include/lldb/Utility/CleanUp.h
+++ include/lldb/Utility/CleanUp.h
@@ -13,249 +13,31 @@
#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(); }
+ /// 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
- //----------------------------------------------------------------------
- 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;
+ ~CleanUp() {
+ if (Clean)
+ Clean();
}
- //----------------------------------------------------------------------
- // 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);
- }
- }
+ /// Disable the cleanup.
+ void disable() { Clean = nullptr; }
- //----------------------------------------------------------------------
- // 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;
- 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-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
