[v2] gdb::handle_eintr, remove need to specify return type

Message ID 223cf7ec-36b5-fe52-7d0f-e19c335be534@palves.net
State New
Headers show
Series
  • [v2] gdb::handle_eintr, remove need to specify return type
Related show

Commit Message

Pedro Alves Oct. 13, 2020, 2:17 p.m.
On 10/13/20 2:43 PM, Kamil Rytarowski wrote:

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

>       |        ^~~~~~~~~~~


Huh, Fun is clearly a function, yet the compiler doesn't pick
the partial specialization.

I could reproduce it here, seems to be related to open being varargs.

Anyhow, we don't really need the new trait.  We can just 
use decltype and a trailing return.  I don't know why I didn't
think of it in the first place.

Here's an updated patch.  I believe this one should work for you too.


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

Date: Tue, 8 Sep 2020 17:34:41 +0100
Subject: [PATCH v2] 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 (handle_eintr): Replace Ret template parameter with
	ErrorValType.  Use it as type of the failure value.  Deduce the
	function's return type using decltype.  Use lowercase for function
	parameter names.
---
 gdb/nat/linux-waitpid.c | 11 ++---------
 gdbserver/netbsd-low.cc | 10 +++++-----
 gdbsupport/eintr.h      | 32 +++++++++++++++++++-------------
 3 files changed, 26 insertions(+), 27 deletions(-)


base-commit: 87a37e5e078f506fa9905b74e9238593c537fcd5
-- 
2.14.5

Comments

Kamil Rytarowski Oct. 13, 2020, 2:54 p.m. | #1
On 13.10.2020 16:17, Pedro Alves wrote:
> On 10/13/20 2:43 PM, Kamil Rytarowski wrote:

> 

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

>>       |        ^~~~~~~~~~~

> 

> Huh, Fun is clearly a function, yet the compiler doesn't pick

> the partial specialization.

> 

> I could reproduce it here, seems to be related to open being varargs.

> 

> Anyhow, we don't really need the new trait.  We can just 

> use decltype and a trailing return.  I don't know why I didn't

> think of it in the first place.

> 

> Here's an updated patch.  I believe this one should work for you too.

> 


This version works for me.

> 

> From 37d06f47a379e44a895f68a22cbb0f745da33ec5 Mon Sep 17 00:00:00 2001

> From: Pedro Alves <pedro@palves.net>

> Date: Tue, 8 Sep 2020 17:34:41 +0100

> Subject: [PATCH v2] 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 (handle_eintr): Replace Ret template parameter with

> 	ErrorValType.  Use it as type of the failure value.  Deduce the

> 	function's return type using decltype.  Use lowercase for function

> 	parameter names.

> ---

>  gdb/nat/linux-waitpid.c | 11 ++---------

>  gdbserver/netbsd-low.cc | 10 +++++-----

>  gdbsupport/eintr.h      | 32 +++++++++++++++++++-------------

>  3 files changed, 26 insertions(+), 27 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..02c26e4def1 100644

> --- a/gdbsupport/eintr.h

> +++ b/gdbsupport/eintr.h

> @@ -43,25 +43,31 @@ namespace gdb

>  

>     You could wrap it by writing the wrapped form:

>  

> -   ssize_t ret = gdb::handle_eintr<ssize_t> (-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.  */

> -

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

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

> +   ssize_t ret = gdb::handle_eintr (-1, ::write, pipe[1], "+", 1);

> +

> +   ERRVAL specifies the failure value indicating that the call to the

> +   F function with ARGS... arguments was possibly interrupted with a

> +   signal.  */

> +

> +template <typename ErrorValType,

> +	  typename Fun,

> +	  typename... Args>

> +inline auto

> +handle_eintr (ErrorValType errval, const Fun &f, const Args &... args)

> +  -> decltype (f(args...))

>  {

> -  Ret ret;

> +  decltype (f(args...)) 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 */

> 

> base-commit: 87a37e5e078f506fa9905b74e9238593c537fcd5

>
Tom Tromey Oct. 16, 2020, 8:51 p.m. | #2
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:


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

Thanks for doing this.  This is closer to how I imagined this wrapper
being written.

Pedro> +template <typename ErrorValType,
Pedro> +	  typename Fun,
Pedro> +	  typename... Args>
Pedro> +inline auto
Pedro> +handle_eintr (ErrorValType errval, const Fun &f, const Args &... args)
Pedro> +  -> decltype (f(args...))

It seems to me that errval and ErrorValType are unchanging properties of
the function being wrapped.  And, normally they are -1 / int.

Also is there ever a case where the return type isn't the same as
ErrorValType?

So maybe instead of requiring these to all be redundantly specified, the
template could use a helper template class that specifies these things
(defaulting to the usual), and then one would write:

pid_t pid = gdb::handle_eintr<::waitpid> (...normal waitpid args);

I'm not sure it's really worth implementing this, but it's closer to
what I was picturing initially.

Tom
Pedro Alves Oct. 26, 2020, 2 p.m. | #3
On 10/16/20 9:51 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

> 

> Pedro> This eliminates the need to specify the return type when using

> Pedro> handle_eintr.  We let the compiler deduce it for us.

> 

> Thanks for doing this.  This is closer to how I imagined this wrapper

> being written.

> 

> Pedro> +template <typename ErrorValType,

> Pedro> +	  typename Fun,

> Pedro> +	  typename... Args>

> Pedro> +inline auto

> Pedro> +handle_eintr (ErrorValType errval, const Fun &f, const Args &... args)

> Pedro> +  -> decltype (f(args...))

> 

> It seems to me that errval and ErrorValType are unchanging properties of

> the function being wrapped.  And, normally they are -1 / int.

> 

> Also is there ever a case where the return type isn't the same as

> ErrorValType?


I don't think so.  But I don't think it really matters -- I look at
it as ErrorValType being the type of the value used to compare with
the wrapped function's return type.

Like, with:

   ssize_t read(int fildes, void *buf, size_t nbyte);

we normally write:

   ssize_t res = read (fildes, buf, nbyte);
   if (res == -1)

instead of 

   ssize_t res = read (fildes, buf, nbyte);
   if (res == (ssize_t) -1)

So ErrorValType is the type of the "-1" expression.  It just
has to be convertible to the return type.

If we get rid of the ErrorValType template parameter, then we
could do instead:

 template<typename Fun, typename... Args>
 using return_type = decltype (std::declval<Fun> () (std::declval<Args> ()...));

 template <typename Fun, typename... Args>
 inline auto
 handle_eintr (return_type<Fun, Args...> errval,
 	      const Fun &f, const Args &... args)
   -> decltype (f (args...))
 {
   decltype (f (args...)) ret;

   do
     {
       errno = 0;
       ret = f (args...);
     }
   while (ret == errval && errno == EINTR);
 
   return ret;
 }

That works for me too, though it looks a little more complicated,
I think.  

Let me know which version you prefer.

> So maybe instead of requiring these to all be redundantly specified, the

> template could use a helper template class that specifies these things

> (defaulting to the usual), and then one would write:

> 

> pid_t pid = gdb::handle_eintr<::waitpid> (...normal waitpid args);

> 

> I'm not sure it's really worth implementing this, but it's closer to

> what I was picturing initially.


That thought initially crossed my mind too, but IMHO it's not worth
it.  You have to write the helper template class, so it
doesn't look like a net win over writing a plain wrapper like:

 int
 my_waitpid (int pid, int *status, int flags)
 {
   return gdb::handle_eintr (-1, ::waitpid, pid, status, flags);
 }

and using my_waitpid throughout.  I.e., this way we also only
write the -1 once.
Tom Tromey Oct. 26, 2020, 2:20 p.m. | #4
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:


Pedro> That works for me too, though it looks a little more complicated,
Pedro> I think.  

Pedro> Let me know which version you prefer.

I don't have much preference but I suppose simpler is better.

Tom
Pedro Alves Oct. 26, 2020, 6:59 p.m. | #5
On 10/26/20 2:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

> 

> Pedro> That works for me too, though it looks a little more complicated,

> Pedro> I think.  

> 

> Pedro> Let me know which version you prefer.

> 

> I don't have much preference but I suppose simpler is better.


Alright, I pushed the simpler version.

Thanks,
Pedro Alves

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..02c26e4def1 100644
--- a/gdbsupport/eintr.h
+++ b/gdbsupport/eintr.h
@@ -43,25 +43,31 @@  namespace gdb
 
    You could wrap it by writing the wrapped form:
 
-   ssize_t ret = gdb::handle_eintr<ssize_t> (-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.  */
-
-template <typename Ret, typename Fun, typename... Args>
-inline Ret handle_eintr (const Ret &R, const Fun &F, const Args &... A)
+   ssize_t ret = gdb::handle_eintr (-1, ::write, pipe[1], "+", 1);
+
+   ERRVAL specifies the failure value indicating that the call to the
+   F function with ARGS... arguments was possibly interrupted with a
+   signal.  */
+
+template <typename ErrorValType,
+	  typename Fun,
+	  typename... Args>
+inline auto
+handle_eintr (ErrorValType errval, const Fun &f, const Args &... args)
+  -> decltype (f(args...))
 {
-  Ret ret;
+  decltype (f(args...)) 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 */