[RFA,04/14] Introduce gdb_dlopen_up

Message ID 20170408201208.2672-5-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey April 8, 2017, 8:11 p.m. UTC
  This introduces gdb_dlopen_up, a unique_ptr that can close a dlopen'd
library.  All the functions working with dlopen handles are updated to
use this new type.

I did not try to build this on Windows.

2017-04-07  Tom Tromey  <tom@tromey.com>

	* jit.c (struct jit_reader): Declare separately.  Add constructor
	and destructor.  Change type of "handle".
	(loaded_jit_reader): Define separately.
	(jit_reader_load): Update.  New "new".
	(jit_reader_unload_command): Use "delete".
	* gdb-dlfcn.h (struct dlclose_deleter): New.
	(gdb_dlopen_up): New typedef.
	(gdb_dlopen, gdb_dlsym): Update types.
	(gdb_dlclose): Remove.
	* gdb-dlfcn.c (gdb_dlopen): Return a gdb_dlopen_up.
	(gdb_dlsym): Change type of "handle".
	(make_cleanup_dlclose): Remove.
	(dlclose_deleter::operator()): Rename from gdb_dlclose.
	* compile/compile-c-support.c (load_libcc): Update.
---
 gdb/ChangeLog                   | 17 ++++++++++++++++
 gdb/compile/compile-c-support.c |  6 ++++--
 gdb/gdb-dlfcn.c                 | 45 ++++++++++++-----------------------------
 gdb/gdb-dlfcn.h                 | 24 ++++++++++++----------
 gdb/jit.c                       | 42 +++++++++++++++++++++-----------------
 5 files changed, 70 insertions(+), 64 deletions(-)
  

Comments

Simon Marchi April 10, 2017, 3:13 a.m. UTC | #1
On 2017-04-08 16:11, Tom Tromey wrote:
> This introduces gdb_dlopen_up, a unique_ptr that can close a dlopen'd
> library.  All the functions working with dlopen handles are updated to
> use this new type.
> 
> I did not try to build this on Windows.

Well, apparently the build on Windows was broken by the guy that pushed 
the ptid patch last week, can you believe it?  With that fixed (pushing 
a the fix in a moment) and your patch, it builds fine with 
--host=x86_64-w64-mingw32.

Otherwise, LGTM.

Thanks,

Simon
  
Pedro Alves April 10, 2017, 9:26 a.m. UTC | #2
Hi!

On 04/08/2017 09:11 PM, Tom Tromey wrote:
> This introduces gdb_dlopen_up, a unique_ptr that can close a dlopen'd
> library.  All the functions working with dlopen handles are updated to
> use this new type.
> 

gdb_dlopen_up would sound to me like something that owns a
pointer to dlopen?  Since this is a unique pointer that instead
owns a _handle_ that is returned by dlopen, I'd suggest naming
it dlhandle_up or gdb_dlhandle_up instead.

Thanks,
Pedro Alves
  
Tom Tromey April 10, 2017, 11:19 p.m. UTC | #3
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> gdb_dlopen_up would sound to me like something that owns a
Pedro> pointer to dlopen?  Since this is a unique pointer that instead
Pedro> owns a _handle_ that is returned by dlopen, I'd suggest naming
Pedro> it dlhandle_up or gdb_dlhandle_up instead.

I made this change.

Tom
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9451af1..296030c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,22 @@ 
 2017-04-07  Tom Tromey  <tom@tromey.com>
 
+	* jit.c (struct jit_reader): Declare separately.  Add constructor
+	and destructor.  Change type of "handle".
+	(loaded_jit_reader): Define separately.
+	(jit_reader_load): Update.  New "new".
+	(jit_reader_unload_command): Use "delete".
+	* gdb-dlfcn.h (struct dlclose_deleter): New.
+	(gdb_dlopen_up): New typedef.
+	(gdb_dlopen, gdb_dlsym): Update types.
+	(gdb_dlclose): Remove.
+	* gdb-dlfcn.c (gdb_dlopen): Return a gdb_dlopen_up.
+	(gdb_dlsym): Change type of "handle".
+	(make_cleanup_dlclose): Remove.
+	(dlclose_deleter::operator()): Rename from gdb_dlclose.
+	* compile/compile-c-support.c (load_libcc): Update.
+
+2017-04-07  Tom Tromey  <tom@tromey.com>
+
 	* symtab.h (find_pcs_for_symtab_line): Change return type.
 	* symtab.c (find_pcs_for_symtab_line): Change return type.
 	* python/py-linetable.c (build_line_table_tuple_from_pcs): Change
diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c
index c969b42..a10296a 100644
--- a/gdb/compile/compile-c-support.c
+++ b/gdb/compile/compile-c-support.c
@@ -75,12 +75,11 @@  c_get_range_decl_name (const struct dynamic_prop *prop)
 static gcc_c_fe_context_function *
 load_libcc (void)
 {
-  void *handle;
   gcc_c_fe_context_function *func;
 
    /* gdb_dlopen will call error () on an error, so no need to check
       value.  */
-  handle = gdb_dlopen (STRINGIFY (GCC_C_FE_LIBCC));
+  gdb_dlopen_up handle = gdb_dlopen (STRINGIFY (GCC_C_FE_LIBCC));
   func = (gcc_c_fe_context_function *) gdb_dlsym (handle,
 						  STRINGIFY (GCC_C_FE_CONTEXT));
 
@@ -88,6 +87,9 @@  load_libcc (void)
     error (_("could not find symbol %s in library %s"),
 	   STRINGIFY (GCC_C_FE_CONTEXT),
 	   STRINGIFY (GCC_C_FE_LIBCC));
+
+  /* Leave the library open.  */
+  handle.release ();
   return func;
 }
 
diff --git a/gdb/gdb-dlfcn.c b/gdb/gdb-dlfcn.c
index a11e396..11571d6 100644
--- a/gdb/gdb-dlfcn.c
+++ b/gdb/gdb-dlfcn.c
@@ -31,27 +31,20 @@ 
 
 #ifdef NO_SHARED_LIB
 
-void *
+gdb_dlopen_up
 gdb_dlopen (const char *filename)
 {
   gdb_assert_not_reached ("gdb_dlopen should not be called on this platform.");
 }
 
 void *
-gdb_dlsym (void *handle, const char *symbol)
+gdb_dlsym (const gdb_dlopen_up &handle, const char *symbol)
 {
   gdb_assert_not_reached ("gdb_dlsym should not be called on this platform.");
 }
 
-struct cleanup *
-make_cleanup_dlclose (void *handle)
-{
-  gdb_assert_not_reached ("make_cleanup_dlclose should not be called on this "
-                          "platform.");
-}
-
-int
-gdb_dlclose (void *handle)
+void
+dlclose_deleter::operator() (void *handle) const
 {
   gdb_assert_not_reached ("gdb_dlclose should not be called on this platform.");
 }
@@ -64,7 +57,7 @@  is_dl_available (void)
 
 #else /* NO_SHARED_LIB */
 
-void *
+gdb_dlopen_up
 gdb_dlopen (const char *filename)
 {
   void *result;
@@ -74,7 +67,7 @@  gdb_dlopen (const char *filename)
   result = (void *) LoadLibrary (filename);
 #endif
   if (result != NULL)
-    return result;
+    return gdb_dlopen_up (result);
 
 #ifdef HAVE_DLFCN_H
   error (_("Could not load %s: %s"), filename, dlerror());
@@ -97,37 +90,25 @@  gdb_dlopen (const char *filename)
 }
 
 void *
-gdb_dlsym (void *handle, const char *symbol)
+gdb_dlsym (const gdb_dlopen_up &handle, const char *symbol)
 {
 #ifdef HAVE_DLFCN_H
-  return dlsym (handle, symbol);
+  return dlsym (handle.get (), symbol);
 #elif __MINGW32__
-  return (void *) GetProcAddress ((HMODULE) handle, symbol);
+  return (void *) GetProcAddress ((HMODULE) handle.get (), symbol);
 #endif
 }
 
-int
-gdb_dlclose (void *handle)
+void
+dlclose_deleter::operator() (void *handle) const
 {
 #ifdef HAVE_DLFCN_H
-  return dlclose (handle);
+  dlclose (handle);
 #elif __MINGW32__
-  return !((int) FreeLibrary ((HMODULE) handle));
+  FreeLibrary ((HMODULE) handle);
 #endif
 }
 
-static void
-do_dlclose_cleanup (void *handle)
-{
-  gdb_dlclose (handle);
-}
-
-struct cleanup *
-make_cleanup_dlclose (void *handle)
-{
-  return make_cleanup (do_dlclose_cleanup, handle);
-}
-
 int
 is_dl_available (void)
 {
diff --git a/gdb/gdb-dlfcn.h b/gdb/gdb-dlfcn.h
index 31709a9..a9435ed 100644
--- a/gdb/gdb-dlfcn.h
+++ b/gdb/gdb-dlfcn.h
@@ -20,26 +20,28 @@ 
 #ifndef GDB_DLFCN_H
 #define GDB_DLFCN_H
 
+/* A deleter that closes an open dynamic library.  */
+
+struct dlclose_deleter
+{
+  void operator() (void *handle) const;
+};
+
+/* A unique pointer that points to a dynamic library.  */
+
+typedef std::unique_ptr<void, dlclose_deleter> gdb_dlopen_up;
+
 /* Load the dynamic library file named FILENAME, and return a handle
    for that dynamic library.  Return NULL if the loading fails for any
    reason.  */
 
-void *gdb_dlopen (const char *filename);
+gdb_dlopen_up gdb_dlopen (const char *filename);
 
 /* Return the address of the symbol named SYMBOL inside the shared
    library whose handle is HANDLE.  Return NULL when the symbol could
    not be found.  */
 
-void *gdb_dlsym (void *handle, const char *symbol);
-
-/* Install a cleanup routine which closes the handle HANDLE.  */
-
-struct cleanup *make_cleanup_dlclose (void *handle);
-
-/* Cleanup the shared object pointed to by HANDLE. Return 0 on success
-   and nonzero on failure.  */
-
-int gdb_dlclose (void *handle);
+void *gdb_dlsym (const gdb_dlopen_up &handle, const char *symbol);
 
 /* Return non-zero if the dynamic library functions are available on
    this platform.  */
diff --git a/gdb/jit.c b/gdb/jit.c
index 158d6d8..7523a46 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -151,14 +151,29 @@  bfd_open_from_target_memory (CORE_ADDR addr, ULONGEST size, char *target)
 			      mem_bfd_iovec_stat);
 }
 
+struct jit_reader
+{
+  jit_reader (struct gdb_reader_funcs *f, gdb_dlopen_up &&h)
+    : functions (f), handle (std::move (h))
+  {
+  }
+
+  ~jit_reader ()
+  {
+    functions->destroy (functions);
+  }
+
+  jit_reader (const jit_reader &) = delete;
+  jit_reader &operator= (const jit_reader &) = delete;
+
+  struct gdb_reader_funcs *functions;
+  gdb_dlopen_up handle;
+};
+
 /* One reader that has been loaded successfully, and can potentially be used to
    parse debug info.  */
 
-static struct jit_reader
-{
-  struct gdb_reader_funcs *functions;
-  void *handle;
-} *loaded_jit_reader = NULL;
+static struct jit_reader *loaded_jit_reader = NULL;
 
 typedef struct gdb_reader_funcs * (reader_init_fn_type) (void);
 static const char *reader_init_fn_sym = "gdb_init_reader";
@@ -168,17 +183,13 @@  static const char *reader_init_fn_sym = "gdb_init_reader";
 static struct jit_reader *
 jit_reader_load (const char *file_name)
 {
-  void *so;
   reader_init_fn_type *init_fn;
-  struct jit_reader *new_reader = NULL;
   struct gdb_reader_funcs *funcs = NULL;
-  struct cleanup *old_cleanups;
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog, _("Opening shared object %s.\n"),
                         file_name);
-  so = gdb_dlopen (file_name);
-  old_cleanups = make_cleanup_dlclose (so);
+  gdb_dlopen_up so = gdb_dlopen (file_name);
 
   init_fn = (reader_init_fn_type *) gdb_dlsym (so, reader_init_fn_sym);
   if (!init_fn)
@@ -192,12 +203,7 @@  jit_reader_load (const char *file_name)
   if (funcs->reader_version != GDB_READER_INTERFACE_VERSION)
     error (_("Reader version does not match GDB version."));
 
-  new_reader = XCNEW (struct jit_reader);
-  new_reader->functions = funcs;
-  new_reader->handle = so;
-
-  discard_cleanups (old_cleanups);
-  return new_reader;
+  return new jit_reader (funcs, std::move (so));
 }
 
 /* Provides the jit-reader-load command.  */
@@ -240,10 +246,8 @@  jit_reader_unload_command (char *args, int from_tty)
 
   reinit_frame_cache ();
   jit_inferior_exit_hook (current_inferior ());
-  loaded_jit_reader->functions->destroy (loaded_jit_reader->functions);
 
-  gdb_dlclose (loaded_jit_reader->handle);
-  xfree (loaded_jit_reader);
+  delete loaded_jit_reader;
   loaded_jit_reader = NULL;
 }