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

[Pedro Alves <pedro@palves.net>]

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

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

base-commit: 87a37e5e078f506fa9905b74e9238593c537fcd5
-- 
2.14.5