gdb::handle_eintr, remove need to specify return type (Re: [PATCH v2 01/10] Add handle_eintr to wrap EINTR handling in syscalls)

Message ID 75070000-03a2-eeed-4f72-e29f199291af@palves.net
State New
Headers show
Series
  • gdb::handle_eintr, remove need to specify return type (Re: [PATCH v2 01/10] Add handle_eintr to wrap EINTR handling in syscalls)
Related show

Commit Message

Pedro Alves Oct. 12, 2020, 5:56 p.m.
Hi!

On 9/7/20 3:59 PM, Kamil Rytarowski wrote:
> On 07.09.2020 16:06, Simon Marchi wrote:

>> On 2020-09-03 8:28 p.m., Kamil Rytarowski wrote:


>>> +#ifndef GDBSUPPORT_EINTR_H

>>> +#define GDBSUPPORT_EINTR_H

>>> +

>>> +#include <cerrno>

>>> +

>>> +namespace gdb

>>> +{

>>> +template <typename Ret, typename Fun, typename... Args>

>>> +inline Ret handle_eintr (const Ret &R, const Fun &F, const Args &... A)

>>> +{

>>> +  Ret ret;

>>> +  do

>>> +    {

>>> +      errno = 0;

>>> +      ret = F (A...);

>>> +    }

>>> +  while (ret == R && errno == EINTR);

>>> +  return ret;

>>> +}

>>> +}

>>

>> Can you document this function a little bit, including a usage example?  I tried

>> to apply it in gdb/linux-nat.c, function async_file_mark, but I'm not sure I'm

>> doing it correctly.  In particular, what should I pass as the `R` parameter?  Does

>> that make sense?

>>

> 

> I'm going to add an example and some documentation.

> 

>>   gdb::handle_eintr (-1, ::write, linux_nat_event_pipe[1], "+", 1);

>>

> 

> gdb::handle_eintr<ssize_t> (-1, ::write, linux_nat_event_pipe[1], "+", 1);


Going through my email backlog I noticed this.

This requirement to explicitly specify the return type at each caller
seems unnecessary.  We can make the compiler deduce it for us based on
the return type of the wrapped function.  I gave it a shot -- do you see
an issue with the change below?

I adjusted the NetBSD files, but I don't have an easy way to confirm
they still build OK, though I expect they do.

I converted Linux's my_waitpid to use the adjusted handle_eintr, and
it worked OK.

From 0c2cf066f3f34a9aa3de7aab7bc2bca80b557b45 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>

Date: Tue, 8 Sep 2020 17:34:41 +0100
Subject: [PATCH] gdb::handle_eintr, remove need to specify return type

This eliminates the need to specify the return type when using
handle_eintr.  We let the compiler deduce it for us.

Also, use lowercase for function parameter names.  Uppercase should
only be used on template parameters.

gdb/ChangeLog:

	* nat/linux-waitpid.c: Include "gdbsupport/eintr.h".
	(my_waitpid): Use gdb::handle_eintr.

gdbserver/ChangeLog:

	* netbsd-low.cc (netbsd_waitpid, netbsd_process_target::kill)
	(netbsd_qxfer_libraries_svr4): Use gdb::handle_eintr without
	explicit type.

gdbsupport/ChangeLog:

	* eintr.h: Include "gdbsupport/traits.h".
	(handle_eintr): Replace Ret template parameter with ErrorValType.
	Use it as type of the failure value.  Deduce the function's return
	type using gdb::return_type.  Use lowercase for function parameter
	names.
	* traits.h (struct return_type): New.
---
 gdb/nat/linux-waitpid.c | 11 ++---------
 gdbserver/netbsd-low.cc | 10 +++++-----
 gdbsupport/eintr.h      | 29 ++++++++++++++++++-----------
 gdbsupport/traits.h     | 11 +++++++++++
 4 files changed, 36 insertions(+), 25 deletions(-)


base-commit: 87a37e5e078f506fa9905b74e9238593c537fcd5
-- 
2.14.5

Comments

Kamil Rytarowski Oct. 13, 2020, 1:43 p.m. | #1
On 12.10.2020 19:56, Pedro Alves wrote:
> Hi!

> 


Hi Pedro,

> On 9/7/20 3:59 PM, Kamil Rytarowski wrote:

>> On 07.09.2020 16:06, Simon Marchi wrote:

>>> On 2020-09-03 8:28 p.m., Kamil Rytarowski wrote:

> 

>>>> +#ifndef GDBSUPPORT_EINTR_H

>>>> +#define GDBSUPPORT_EINTR_H

>>>> +

>>>> +#include <cerrno>

>>>> +

>>>> +namespace gdb

>>>> +{

>>>> +template <typename Ret, typename Fun, typename... Args>

>>>> +inline Ret handle_eintr (const Ret &R, const Fun &F, const Args &... A)

>>>> +{

>>>> +  Ret ret;

>>>> +  do

>>>> +    {

>>>> +      errno = 0;

>>>> +      ret = F (A...);

>>>> +    }

>>>> +  while (ret == R && errno == EINTR);

>>>> +  return ret;

>>>> +}

>>>> +}

>>>

>>> Can you document this function a little bit, including a usage example?  I tried

>>> to apply it in gdb/linux-nat.c, function async_file_mark, but I'm not sure I'm

>>> doing it correctly.  In particular, what should I pass as the `R` parameter?  Does

>>> that make sense?

>>>

>>

>> I'm going to add an example and some documentation.

>>

>>>   gdb::handle_eintr (-1, ::write, linux_nat_event_pipe[1], "+", 1);

>>>

>>

>> gdb::handle_eintr<ssize_t> (-1, ::write, linux_nat_event_pipe[1], "+", 1);

> 

> Going through my email backlog I noticed this.

> 

> This requirement to explicitly specify the return type at each caller

> seems unnecessary.  We can make the compiler deduce it for us based on

> the return type of the wrapped function.  I gave it a shot -- do you see

> an issue with the change below?

> 


This patch applied on trunk gives me:

gmake[2]: Wejście do katalogu '/public/binutils-gdb-netbsd/build/gdbserver'
  CXX    netbsd-low.o
../../gdbserver/netbsd-low.cc: In function ‘bool elf_64_file_p(const
char*)’:
../../gdbserver/netbsd-low.cc:1152:57: error: no matching function for
call to ‘handle_eintr(int, int (&)(const char*, int, ...), const char*&,
int)’
 1152 |   int fd = gdb::handle_eintr (-1, ::open, file, O_RDONLY);
      |                                                         ^
In file included from ../../gdbserver/netbsd-low.cc:37:
../../gdbserver/../gdbsupport/eintr.h:58:1: note: candidate:
‘template<class ErrorValType, class Fun, class ... Args> typename
gdb::return_type<Fun>::type gdb::handle_eintr(ErrorValType, const Fun&,
const Args& ...)’
   58 | handle_eintr (ErrorValType errval, const Fun &f, const Args &...
args)
      | ^~~~~~~~~~~~
../../gdbserver/../gdbsupport/eintr.h:58:1: note:   template argument
deduction/substitution failed:
../../gdbserver/../gdbsupport/eintr.h: In substitution of
‘template<class ErrorValType, class Fun, class ... Args> typename
gdb::return_type<Fun>::type gdb::handle_eintr(ErrorValType, const Fun&,
const Args& ...) [with ErrorValType = int; Fun = int(const char*, int,
...); Args = {const char*, int}]’:
../../gdbserver/netbsd-low.cc:1152:57:   required from here
../../gdbserver/../gdbsupport/eintr.h:58:1: error: invalid use of
incomplete type ‘struct gdb::return_type<int(const char*, int, ...)>’
In file included from ../../gdbserver/../gdbsupport/poison.h:23,
                 from ../../gdbserver/../gdbsupport/common-utils.h:26,
                 from ../../gdbserver/../gdbsupport/common-defs.h:125,
                 from ../../gdbserver/server.h:22,
                 from ../../gdbserver/netbsd-low.cc:18:
../../gdbserver/../gdbsupport/traits.h:49:8: note: declaration of
‘struct gdb::return_type<int(const char*, int, ...)>’
   49 | struct return_type;
      |        ^~~~~~~~~~~
gmake[2]: *** [Makefile:551: netbsd-low.o] Błąd 1

Patch

diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
index f50e0c7bcff..d066239220a 100644
--- a/gdb/nat/linux-waitpid.c
+++ b/gdb/nat/linux-waitpid.c
@@ -22,6 +22,7 @@ 
 #include "linux-nat.h"
 #include "linux-waitpid.h"
 #include "gdbsupport/gdb_wait.h"
+#include "gdbsupport/eintr.h"
 
 /* Convert wait status STATUS to a string.  Used for printing debug
    messages only.  */
@@ -54,13 +55,5 @@  status_to_str (int status)
 int
 my_waitpid (int pid, int *status, int flags)
 {
-  int ret;
-
-  do
-    {
-      ret = waitpid (pid, status, flags);
-    }
-  while (ret == -1 && errno == EINTR);
-
-  return ret;
+  return gdb::handle_eintr (-1, ::waitpid, pid, status, flags);
 }
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index 30028d3a384..519614d0473 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -245,7 +245,7 @@  netbsd_waitpid (ptid_t ptid, struct target_waitstatus *ourstatus,
   int options = (target_options & TARGET_WNOHANG) ? WNOHANG : 0;
 
   pid_t pid
-    = gdb::handle_eintr<int> (-1, ::waitpid, ptid.pid (), &status, options);
+    = gdb::handle_eintr (-1, ::waitpid, ptid.pid (), &status, options);
 
   if (pid == -1)
     perror_with_name (_("Child process unexpectedly missing"));
@@ -456,7 +456,7 @@  netbsd_process_target::kill (process_info *process)
     return -1;
 
   int status;
-  if (gdb::handle_eintr<int> (-1, ::waitpid, pid, &status, 0) == -1)
+  if (gdb::handle_eintr (-1, ::waitpid, pid, &status, 0) == -1)
     return -1;
   mourn (process);
   return 0;
@@ -1149,15 +1149,15 @@  netbsd_qxfer_libraries_svr4 (const pid_t pid, const char *annex,
 static bool
 elf_64_file_p (const char *file)
 {
-  int fd = gdb::handle_eintr<int> (-1, ::open, file, O_RDONLY);
+  int fd = gdb::handle_eintr (-1, ::open, file, O_RDONLY);
   if (fd < 0)
     perror_with_name (("open"));
 
   Elf64_Ehdr header;
-  ssize_t ret = gdb::handle_eintr<ssize_t> (-1, ::read, fd, &header, sizeof (header));
+  ssize_t ret = gdb::handle_eintr (-1, ::read, fd, &header, sizeof (header));
   if (ret == -1)
     perror_with_name (("read"));
-  gdb::handle_eintr<int> (-1, ::close, fd);
+  gdb::handle_eintr (-1, ::close, fd);
   if (ret != sizeof (header))
     error ("Cannot read ELF file header: %s", file);
 
diff --git a/gdbsupport/eintr.h b/gdbsupport/eintr.h
index 64ff5940b75..ab8310e561c 100644
--- a/gdbsupport/eintr.h
+++ b/gdbsupport/eintr.h
@@ -22,6 +22,8 @@ 
 
 #include <cerrno>
 
+#include "gdbsupport/traits.h"
+
 namespace gdb
 {
 /* Repeat a system call interrupted with a signal.
@@ -43,25 +45,30 @@  namespace gdb
 
    You could wrap it by writing the wrapped form:
 
-   ssize_t ret = gdb::handle_eintr<ssize_t> (-1, ::write, pipe[1], "+", 1);
+   ssize_t ret = gdb::handle_eintr (-1, ::write, pipe[1], "+", 1);
 
-   The RET typename specifies the return type of the wrapped system call, which
-   is typically int or ssize_t.  The R argument specifies the failure value
-   indicating the interrupted syscall when calling the F function with
-   the A... arguments.  */
+   ERRVAL specifies the failure value indicating that the call to the
+   F function with ARGS... arguments was possibly interrupted with a
+   signal.  */
 
-template <typename Ret, typename Fun, typename... Args>
-inline Ret handle_eintr (const Ret &R, const Fun &F, const Args &... A)
+template <typename ErrorValType,
+	  typename Fun,
+	  typename... Args>
+inline typename return_type<Fun>::type
+handle_eintr (ErrorValType errval, const Fun &f, const Args &... args)
 {
-  Ret ret;
+  typename return_type<Fun>::type ret;
+
   do
     {
       errno = 0;
-      ret = F (A...);
+      ret = f (args...);
     }
-  while (ret == R && errno == EINTR);
+  while (ret == errval && errno == EINTR);
+
   return ret;
 }
-}
+
+} /* namespace gdb */
 
 #endif /* GDBSUPPORT_EINTR_H */
diff --git a/gdbsupport/traits.h b/gdbsupport/traits.h
index 93b609ac109..f161ca00fa6 100644
--- a/gdbsupport/traits.h
+++ b/gdbsupport/traits.h
@@ -43,6 +43,17 @@ 
 
 namespace gdb {
 
+/* Use partial specialization to get access a function's return
+   type.  */
+template<class Signature>
+struct return_type;
+
+template<typename Res, typename... Args>
+struct return_type<Res (Args...)>
+{
+  typedef Res type;
+};
+
 /* Pre C++14-safe (CWG 1558) version of C++17's std::void_t.  See
    <http://en.cppreference.com/w/cpp/types/void_t>.  */