[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

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`.


Repository:
  rL LLVM

https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325964: [Utility] Simplify and generalize the CleanUp 
helper, NFC (authored by vedantk, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43662?vs=135671=135698#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43662

Files:
  lldb/trunk/include/lldb/Utility/CleanUp.h
  lldb/trunk/lldb.xcodeproj/project.pbxproj
  lldb/trunk/source/Host/macosx/Host.mm
  lldb/trunk/source/Host/macosx/Symbols.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/trunk/unittests/Utility/CMakeLists.txt
  lldb/trunk/unittests/Utility/CleanUpTest.cpp

Index: lldb/trunk/lldb.xcodeproj/project.pbxproj
===
--- lldb/trunk/lldb.xcodeproj/project.pbxproj
+++ lldb/trunk/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 = ""; };
 		6DEC6F381BD66D750091ABA6 /* TaskPool.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = TaskPool.cpp; path = source/Host/common/TaskPool.cpp; sourceTree = ""; };
 		6DEC6F3A1BD66D950091ABA6 /* TaskPool.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = TaskPool.h; path = include/lldb/Host/TaskPool.h; sourceTree = ""; };
+		7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CleanUpTest.cpp; sourceTree = ""; };
 		8C26C4241C3EA4340031DF7C /* TSanRuntime.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = TSanRuntime.cpp; path = TSan/TSanRuntime.cpp; sourceTree = ""; };
 		8C26C4251C3EA4340031DF7C /* TSanRuntime.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = TSanRuntime.h; path = TSan/TSanRuntime.h; sourceTree = ""; };
 		8C2D6A52197A1EAF006989C9 /* MemoryHistory.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = MemoryHistory.cpp; path = source/Target/MemoryHistory.cpp; sourceTree = ""; };
@@ -3419,6 +3422,7 @@
 		2321F9421BDD343A00BA9A93 /* Utility */ = {
 			isa = PBXGroup;
 			children = (
+7F94D7172040A13A006EE3EA /* CleanUpTest.cpp */,
 23E2E5161D903689006F38BB /* ArchSpecTest.cpp */,
 9A3D43C81F3150D200EB767C /* ConstStringTest.cpp */,
 9A3D43C71F3150D200EB767C /* LogTest.cpp */,
Index: lldb/trunk/unittests/Utility/CMakeLists.txt
===
--- lldb/trunk/unittests/Utility/CMakeLists.txt
+++ lldb/trunk/unittests/Utility/CMakeLists.txt
@@ -1,5 +1,6 @@
 add_lldb_unittest(UtilityTests
   ArchSpecTest.cpp
+  CleanUpTest.cpp
   ConstStringTest.cpp
   EnvironmentTest.cpp
   JSONTest.cpp
Index: lldb/trunk/unittests/Utility/CleanUpTest.cpp
===
--- lldb/trunk/unittests/Utility/CleanUpTest.cpp
+++ lldb/trunk/unittests/Utility/CleanUpTest.cpp
@@ -0,0 +1,47 @@
+//===-- 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 

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

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


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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.




Comment at: unittests/Utility/CleanUpTest.cpp:1
+//===-- CleanUptest.cpp -*- C++ 
-*-===//
+//

s/test/Test/



Comment at: unittests/Utility/CleanUpTest.cpp:13
+
+#include 
+

Looks like this is left-over from testing.


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Vedant Kumar via Phabricator via lldb-commits]

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 
+
+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 ) {
+  ASSERT_FALSE(arg1);
+  *arg2 = true;
+  arg3 = true;
+},
+f1, , 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 our_socket(-1, -1, close);
-lldb_utility::CleanUp 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",
-

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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).


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

In https://reviews.llvm.org/D43662#1016950, @labath wrote:

> In https://reviews.llvm.org/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
>   CleanUp(F &, Args &&..args)
>   : PerformCleanup(true), Clean(std::bind(std::forward(f), 
> std::forward(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?


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Zachary Turner via lldb-commits]

I wouldn’t use std::bind though, just make a lambda.
On Thu, Feb 22, 2018 at 6:06 PM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> In https://reviews.llvm.org/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
>   CleanUp(F &, Args &&..args)
>   : PerformCleanup(true), Clean(std::bind(std::forward(f),
> std::forward(args)...) {}
>
> Although I can see how you may find the other usage syntax more
> understandable, so I am not adamant about this...
>
>
> https://reviews.llvm.org/D43662
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/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
  CleanUp(F &, Args &&..args)
  : PerformCleanup(true), Clean(std::bind(std::forward(f), 
std::forward(args)...) {}

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


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

> 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.


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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...


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

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

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


https://reviews.llvm.org/D43662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

[Vedant Kumar via Phabricator via lldb-commits]

vsk created this revision.
vsk added reviewers: zturner, labath, davide, JDevlieghere.

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.


https://reviews.llvm.org/D43662

Files:
  include/lldb/Utility/CleanUp.h
  source/Host/macosx/Host.mm
  source/Host/macosx/Symbols.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.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 our_socket(-1, -1, close);
-lldb_utility::CleanUp 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];
+lldb_utility::CleanUp cleanup_our([our_socket] { close(our_socket); });
+lldb_utility::CleanUp cleanup_gdb([gdb_socket] { 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 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 {
-