[1/3] gdbsupport: change scoped_fd::to_file to a free function

Message ID 20210722193714.1778717-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • [1/3] gdbsupport: change scoped_fd::to_file to a free function
Related show

Commit Message

Philippe Waroquiers via Gdb-patches July 22, 2021, 7:37 p.m.
The following patch wants to chagne gdb_fopen_cloexec to return a
scoped_fd.  Doing this causes a cyclic include between scoped_fd.h and
filestuff.h.  scoped_fd.h includes filestuff.h because of the
scoped_fd::to_file method.  To fix that, change scoped_fd::to_file to
be a free function in filestuff.h.

Change-Id: Ic82a48914b2aacee8f14af535b7469245f88b93d
---
 gdb/dwarf2/index-write.c            |  2 +-
 gdb/source.c                        |  2 +-
 gdb/unittests/scoped_fd-selftests.c |  2 +-
 gdbsupport/filestuff.h              | 17 +++++++++++++++++
 gdbsupport/scoped_fd.h              | 13 -------------
 5 files changed, 20 insertions(+), 16 deletions(-)

-- 
2.32.0

Comments

Tom Tromey July 30, 2021, 1:47 p.m. | #1
Simon> The following patch wants to chagne gdb_fopen_cloexec to return a

"change"
Also I think that should be gdb_mkostemp_cloexec?

Simon> scoped_fd.  Doing this causes a cyclic include between scoped_fd.h and
Simon> filestuff.h.  scoped_fd.h includes filestuff.h because of the
Simon> scoped_fd::to_file method.  To fix that, change scoped_fd::to_file to
Simon> be a free function in filestuff.h.

I guess that, while I don't really object, it seems like it would be
better to break the dependency some other way.  For instance would
introducing a new header that only declares gdb_file_up work?  The idea
behind this is not to let the oddities of C++ header file management
influence how the code itself is spelled -- that seems like a kind of
inversion to me.  Going along with this is the idea that a method is the
clearest way to write to_file.  These are all opinions of course, maybe
there's a reason to prefer a free function.

Tom
Philippe Waroquiers via Gdb-patches Sept. 30, 2021, 4:17 p.m. | #2
On 2021-07-30 09:47, Tom Tromey wrote:
> Simon> The following patch wants to chagne gdb_fopen_cloexec to return a

> 

> "change"

> Also I think that should be gdb_mkostemp_cloexec?


Both, actually.

> 

> Simon> scoped_fd.  Doing this causes a cyclic include between scoped_fd.h and

> Simon> filestuff.h.  scoped_fd.h includes filestuff.h because of the

> Simon> scoped_fd::to_file method.  To fix that, change scoped_fd::to_file to

> Simon> be a free function in filestuff.h.

> 

> I guess that, while I don't really object, it seems like it would be

> better to break the dependency some other way.  For instance would

> introducing a new header that only declares gdb_file_up work?  The idea

> behind this is not to let the oddities of C++ header file management

> influence how the code itself is spelled -- that seems like a kind of

> inversion to me.  Going along with this is the idea that a method is the

> clearest way to write to_file.  These are all opinions of course, maybe

> there's a reason to prefer a free function.


There's three way to do this:

 - scoped_fd::to_file method
 - gdb_file::from_fd static method
 - a free function

I never know which is best, which object should know about the other
one, or if no object should no about the other (and hence use a free
function).

But I agree with you here.  See the updated 1/3 patch below.

Patch 2 changes trivially to update the includes and a to_file call.


From 7a281de194629ba00546c2d34cc41d2f77ccc5b3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Thu, 22 Jul 2021 11:34:57 -0400
Subject: [PATCH] gdbsupport: move gdb_file_up to its own file

The following patches wants to change gdb_fopen_cloexec and
gdb_mkostemp_cloexec to return a scoped_fd.  Doing this causes a cyclic
include between scoped_fd.h and filestuff.h, that both want to include
each other.  scoped_fd.h includes filestuff.h because of the
scoped_fd::to_file method's return value.  filestuff.h would then
include scoped_fd.h for gdb_fopen_cloexec's and gdb_mkostemp_cloexec's
return values.

To fix that, move gdb_file_up to its own file, gdb_file.h.

Change-Id: Ic82a48914b2aacee8f14af535b7469245f88b93d
---
 gdbsupport/filestuff.h | 13 +------------
 gdbsupport/gdb_file.h  | 37 +++++++++++++++++++++++++++++++++++++
 gdbsupport/scoped_fd.h |  2 +-
 3 files changed, 39 insertions(+), 13 deletions(-)
 create mode 100644 gdbsupport/gdb_file.h

diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index 7f4cfb2388b1..aa15f89aa920 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -21,6 +21,7 @@
 
 #include <dirent.h>
 #include <fcntl.h>
+#include "gdb_file.h"
 
 /* Note all the file descriptors which are open when this is called.
    These file descriptors will not be closed by close_most_fds.  */
@@ -69,18 +70,6 @@ gdb_open_cloexec (const std::string &filename, int flags,
   return gdb_open_cloexec (filename.c_str (), flags, mode);
 }
 
-struct gdb_file_deleter
-{
-  void operator() (FILE *file) const
-  {
-    fclose (file);
-  }
-};
-
-/* A unique pointer to a FILE.  */
-
-typedef std::unique_ptr<FILE, gdb_file_deleter> gdb_file_up;
-
 /* Like 'fopen', but ensures that the returned file descriptor has the
    close-on-exec flag set.  */
 
diff --git a/gdbsupport/gdb_file.h b/gdbsupport/gdb_file.h
new file mode 100644
index 000000000000..2f5b399f6e87
--- /dev/null
+++ b/gdbsupport/gdb_file.h
@@ -0,0 +1,37 @@
+/* gdb_file_up, an RAII wrapper around FILE.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDBSUPPORT_GDB_FILE
+#define GDBSUPPORT_GDB_FILE
+
+#include <memory>
+#include <stdio.h>
+
+struct gdb_file_deleter
+{
+  void operator() (FILE *file) const
+  {
+    fclose (file);
+  }
+};
+
+/* A unique pointer to a FILE.  */
+
+typedef std::unique_ptr<FILE, gdb_file_deleter> gdb_file_up;
+
+#endif
diff --git a/gdbsupport/scoped_fd.h b/gdbsupport/scoped_fd.h
index a1aad8493998..f16e811a2cef 100644
--- a/gdbsupport/scoped_fd.h
+++ b/gdbsupport/scoped_fd.h
@@ -21,7 +21,7 @@
 #define COMMON_SCOPED_FD_H
 
 #include <unistd.h>
-#include "filestuff.h"
+#include "gdb_file.h"
 
 /* A smart-pointer-like class to automatically close a file descriptor.  */
 
-- 
2.33.0
Tom Tromey Sept. 30, 2021, 5:48 p.m. | #3
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:


Simon> But I agree with you here.  See the updated 1/3 patch below.

Looks good to me, thank you.

Tom
Philippe Waroquiers via Gdb-patches Sept. 30, 2021, 7:29 p.m. | #4
On 2021-09-30 13:48, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Simon> But I agree with you here.  See the updated 1/3 patch below.

> 

> Looks good to me, thank you.

> 

> Tom

> 


Thanks, pushed.

Simon

Patch

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 4e00c716d910..59a6aa4264d5 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1544,7 +1544,7 @@  struct index_wip_file
     if (out_file_fd.get () == -1)
       perror_with_name (("mkstemp"));
 
-    out_file = out_file_fd.to_file ("wb");
+    out_file = to_file (out_file_fd, "wb");
 
     if (out_file == nullptr)
       error (_("Can't open `%s' for writing"), filename_temp.data ());
diff --git a/gdb/source.c b/gdb/source.c
index 7d1934bd6c9b..0e3308048dc3 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1620,7 +1620,7 @@  search_command_helper (const char *regex, int from_tty, bool forward)
   if (lseek (desc.get (), (*offsets)[line - 1], 0) < 0)
     perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
 
-  gdb_file_up stream = desc.to_file (FDOPEN_MODE);
+  gdb_file_up stream = to_file (desc, FDOPEN_MODE);
   clearerr (stream.get ());
 
   gdb::def_vector<char> buf;
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
index d1803aaf8da2..1a361f00a2e3 100644
--- a/gdb/unittests/scoped_fd-selftests.c
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -76,7 +76,7 @@  test_to_file ()
 
   unlink (filename);
   
-  gdb_file_up file = sfd.to_file ("rw");
+  gdb_file_up file = to_file (sfd, "rw");
   SELF_CHECK (file != nullptr);
   SELF_CHECK (sfd.get () == -1);
 }
diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index 7f4cfb2388b1..658e1df4e558 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -21,6 +21,7 @@ 
 
 #include <dirent.h>
 #include <fcntl.h>
+#include "scoped_fd.h"
 
 /* Note all the file descriptors which are open when this is called.
    These file descriptors will not be closed by close_most_fds.  */
@@ -96,6 +97,22 @@  gdb_fopen_cloexec (const std::string &filename, const char *opentype)
   return gdb_fopen_cloexec (filename.c_str (), opentype);
 }
 
+/* Converts FD into a gdb_file_up.
+
+   On success, the file descriptor owned by FD is released, meaning that FD no
+   longer contains nor owns it.  */
+
+static inline
+gdb_file_up to_file (scoped_fd &fd, const char *mode) noexcept
+{
+  gdb_file_up result (fdopen (fd.get (), mode));
+
+  if (result != nullptr)
+    int unused ATTRIBUTE_UNUSED = fd.release ();
+
+  return result;
+}
+
 /* Like 'socketpair', but ensures that the returned file descriptors
    have the close-on-exec flag set.  */
 
diff --git a/gdbsupport/scoped_fd.h b/gdbsupport/scoped_fd.h
index a1aad8493998..a571729c951c 100644
--- a/gdbsupport/scoped_fd.h
+++ b/gdbsupport/scoped_fd.h
@@ -21,7 +21,6 @@ 
 #define COMMON_SCOPED_FD_H
 
 #include <unistd.h>
-#include "filestuff.h"
 
 /* A smart-pointer-like class to automatically close a file descriptor.  */
 
@@ -63,18 +62,6 @@  class scoped_fd
     return fd;
   }
 
-  /* Like release, but return a gdb_file_up that owns the file
-     descriptor.  On success, this scoped_fd will be released.  On
-     failure, return NULL and leave this scoped_fd in possession of
-     the fd.  */
-  gdb_file_up to_file (const char *mode) noexcept
-  {
-    gdb_file_up result (fdopen (m_fd, mode));
-    if (result != nullptr)
-      m_fd = -1;
-    return result;
-  }
-
   int get () const noexcept
   {
     return m_fd;