[PATCHv2 1/7] hurd: fix gnu_debug_flag type

[PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Samuel Thibault]

Fixes

../../gdb/gnu-nat.c:96:6: error: conflicting declaration ‘bool gnu_debug_flag’
   96 | bool gnu_debug_flag = false;
../../gdb/gnu-nat.c: In function ‘void _initialize_gnu_nat()’:
../../gdb/gnu-nat.c:3511:7: error: cannot

gdb/ChangeLog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* gnu-nat.h (gnu_debug_flag): Set type to bool.

diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
index 77c57817b2..7c36778394 100644
--- a/gdb/gnu-nat.h
+++ b/gdb/gnu-nat.h
@@ -111,7 +111,7 @@ extern char *proc_string (struct proc *proc);
 	      __proc_pid (__proc), __proc->tid, \
 	      host_address_to_string (__proc) , ##args); } while (0)
 
-extern int gnu_debug_flag;
+extern bool gnu_debug_flag;
 
 #define debug(msg, args...) \
  do { if (gnu_debug_flag) \

[PATCHv2 2/7] hurd: add missing awk script dependency

[Samuel Thibault]

gdb/ChangeLog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* config/i386/i386gnu.mn (%_reply_S.c): Add dependency on
	$(srcdir)/reply_mig_hack.awk.

diff --git a/gdb/config/i386/i386gnu.mn b/gdb/config/i386/i386gnu.mn
index 1c3453823d..5c81efd007 100644
--- a/gdb/config/i386/i386gnu.mn
+++ b/gdb/config/i386/i386gnu.mn
@@ -7,7 +7,7 @@ MIGCOM = $(MIG) -cc cat - /dev/null
 
 # Reply servers need special massaging of the code mig generates, to make
 # them work correctly for error returns in some cases.
-%_reply_S.h %_reply_S.c: %_reply.defs
+%_reply_S.h %_reply_S.c: %_reply.defs $(srcdir)/reply_mig_hack.awk
 	$(CPP) $(CPPFLAGS) -DSERVERPREFIX=S_ -x c $< \
 	| $(MIGCOM) -sheader $*_reply_S.h -server $*_reply_S.raw -user /dev/null -header /dev/null \
 	&& $(AWK) -f $(srcdir)/reply_mig_hack.awk < $*_reply_S.raw > $*_reply_S.c

Re: [PATCHv2 2/7] hurd: add missing awk script dependency

[Simon Marchi]

On 2020-05-29 6:01 p.m., Samuel Thibault wrote:
> gdb/ChangeLog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* config/i386/i386gnu.mn (%_reply_S.c): Add dependency on
> 	$(srcdir)/reply_mig_hack.awk.
> 
> diff --git a/gdb/config/i386/i386gnu.mn b/gdb/config/i386/i386gnu.mn
> index 1c3453823d..5c81efd007 100644
> --- a/gdb/config/i386/i386gnu.mn
> +++ b/gdb/config/i386/i386gnu.mn
> @@ -7,7 +7,7 @@ MIGCOM = $(MIG) -cc cat - /dev/null
>  
>  # Reply servers need special massaging of the code mig generates, to make
>  # them work correctly for error returns in some cases.
> -%_reply_S.h %_reply_S.c: %_reply.defs
> +%_reply_S.h %_reply_S.c: %_reply.defs $(srcdir)/reply_mig_hack.awk
>  	$(CPP) $(CPPFLAGS) -DSERVERPREFIX=S_ -x c $< \
>  	| $(MIGCOM) -sheader $*_reply_S.h -server $*_reply_S.raw -user /dev/null -header /dev/null \
>  	&& $(AWK) -f $(srcdir)/reply_mig_hack.awk < $*_reply_S.raw > $*_reply_S.c
> 

Thanks, this is OK, although even if it's obvious to you, I'd appreciate at least
a small commit message that explains why this is needed (i.e. make sure those out
files are re-generated whenever the script that generates them changes).

Simon

[PATCHv2 3/7] hurd: make function cast stronger

[Samuel Thibault]

Fixes

process_reply_S.c:104:23: error: function called through a non-compatible type [-Werror]
  104 |      OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) S_proc_setmsgport_reply) (In0P->Head.msgh_request_port, In0P-

gdb/ChangeLog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* reply_mig_hack.awk (Error return): Cast function through
	void *, to bypass compiler function call check.

diff --git a/gdb/reply_mig_hack.awk b/gdb/reply_mig_hack.awk
index 52ab90bba3..6ff683a841 100644
--- a/gdb/reply_mig_hack.awk
+++ b/gdb/reply_mig_hack.awk
@@ -130,7 +130,8 @@ parse_phase == 5 && /^#if[ \t]TypeCheck/ {
   # two arguments.
   # This is possibly bogus, but easier than supplying bogus values for all
   # the other args (we can't just pass 0 for them, as they might not be scalar).
-  print "\t    OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) " user_function_name ") (In0P->Head.msgh_request_port, In0P->" arg_name[0] ");";
+  print "\t    void * __error_call = " user_function_name ";";
+  print "\t    OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) __error_call) (In0P->Head.msgh_request_port, In0P->" arg_name[0] ");";
   print "\t    return;";
   print "\t  }";
   print "";

Re: [PATCHv2 3/7] hurd: make function cast stronger

[Simon Marchi]

On 2020-05-29 6:01 p.m., Samuel Thibault wrote:
> Fixes
> 
> process_reply_S.c:104:23: error: function called through a non-compatible type [-Werror]
>   104 |      OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) S_proc_setmsgport_reply) (In0P->Head.msgh_request_port, In0P-
> 
> gdb/ChangeLog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* reply_mig_hack.awk (Error return): Cast function through
> 	void *, to bypass compiler function call check.

If you are silencing a compiler warning, please explain why it is safe to do so.  Why
are we calling a function in a non-compatible way, is it a warning false positive?
Because just like that, it just sounds like we are passing the wrong arguments and it
should not be fixed by just silencing the warning, but by fixing the call.

Simon

Re: [PATCHv2 3/7] hurd: make function cast stronger

[Samuel Thibault]

Simon Marchi, le sam. 30 mai 2020 10:47:50 -0400, a ecrit:
> On 2020-05-29 6:01 p.m., Samuel Thibault wrote:
> > process_reply_S.c:104:23: error: function called through a non-compatible type [-Werror]
> >   104 |      OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) S_proc_setmsgport_reply) (In0P->Head.msgh_request_port, In0P-
> > 
> > gdb/ChangeLog:
> > 
> > 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> > 
> > 	* reply_mig_hack.awk (Error return): Cast function through
> > 	void *, to bypass compiler function call check.
> 
> If you are silencing a compiler warning, please explain why it is safe to do so.

It is not actually safe, as explained by the comment above the changed
lines, but as explained by the comment above really fixing it is very
far from trivial.

In my repo I have added

“
As the existing comment says, it is in general not safe to drop some parameters
like this, but this is the error handling case, where the called function does
not actually read them, and mig is currently planned to be used on i386 and
x86_64 only, where this is not a problem. As the existing comment says, fixing
it properly would be far from trivial: we can't just pass 0 for them, as they
might not be scalar.
”

Is that enough of an explanation for the changelog?

Samuel

Re: [PATCHv2 3/7] hurd: make function cast stronger

[Simon Marchi]

On 2020-05-30 11:32 a.m., Samuel Thibault wrote:
> Simon Marchi, le sam. 30 mai 2020 10:47:50 -0400, a ecrit:
>> On 2020-05-29 6:01 p.m., Samuel Thibault wrote:
>>> process_reply_S.c:104:23: error: function called through a non-compatible type [-Werror]
>>>   104 |      OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) S_proc_setmsgport_reply) (In0P->Head.msgh_request_port, In0P-
>>>
>>> gdb/ChangeLog:
>>>
>>> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
>>>
>>> 	* reply_mig_hack.awk (Error return): Cast function through
>>> 	void *, to bypass compiler function call check.
>>
>> If you are silencing a compiler warning, please explain why it is safe to do so.
> 
> It is not actually safe, as explained by the comment above the changed
> lines, but as explained by the comment above really fixing it is very
> far from trivial.
> 
> In my repo I have added
> 
> “
> As the existing comment says, it is in general not safe to drop some parameters
> like this, but this is the error handling case, where the called function does
> not actually read them, and mig is currently planned to be used on i386 and
> x86_64 only, where this is not a problem. As the existing comment says, fixing
> it properly would be far from trivial: we can't just pass 0 for them, as they
> might not be scalar.
> ”
> 
> Is that enough of an explanation for the changelog?

Ok, thanks, I missed that comment.  But yeah, I think that text for the commit
message (not ChangeLog entry) is helpful!

Simon

[PATCHv2 4/7] hurd: add missing include

[Samuel Thibault]

Fixes

../../gdb/gnu-nat.c:2522:14: error: ‘target_gdbarch’ was not declared in this scope; did you mean ‘target_detach’?
 2522 |    paddress (target_gdbarch (), memaddr), pulongest (len),

gdb/Changelog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* gnu-nat.c: Include "gdbarch.h".

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 3b438a9a43..78e9ab7f71 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -64,6 +64,7 @@ extern "C"
 #include "language.h"
 #include "target.h"
 #include "gdbsupport/gdb_wait.h"
+#include "gdbarch.h"
 #include "gdbcmd.h"
 #include "gdbcore.h"
 #include "gdbthread.h"

Re: [PATCHv2 4/7] hurd: add missing include

[Simon Marchi]

On 2020-05-29 6:02 p.m., Samuel Thibault wrote:
> Fixes
> 
> ../../gdb/gnu-nat.c:2522:14: error: ‘target_gdbarch’ was not declared in this scope; did you mean ‘target_detach’?
>  2522 |    paddress (target_gdbarch (), memaddr), pulongest (len),
> 
> gdb/Changelog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* gnu-nat.c: Include "gdbarch.h".
> 
> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index 3b438a9a43..78e9ab7f71 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -64,6 +64,7 @@ extern "C"
>  #include "language.h"
>  #include "target.h"
>  #include "gdbsupport/gdb_wait.h"
> +#include "gdbarch.h"
>  #include "gdbcmd.h"
>  #include "gdbcore.h"
>  #include "gdbthread.h"
> 

Thanks, this is ok.

Simon

[PATCHv2 5/7] hurd: remove unused variables

[Samuel Thibault]

Fixes

../../gdb/gnu-nat.c:2554:7: error: unused variable ‘res’ [-Werror=unused-variable]
 2554 |   int res;
../../gdb/gnu-nat.c:2644:20: error: unused variable ‘old_address’ [-Werror=unused-variable]
 2644 |       vm_address_t old_address = region_address;

gdb/ChangeLog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* gnu-nat.c (gnu_xfer_auxv): Remove unused `res' variable.
	(gnu_nat_target::find_memory_regions): Remove unused
	`old_address' variable.

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 3b438a9a43..78e9ab7f71 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2551,7 +2554,6 @@ gnu_xfer_auxv (gdb_byte *readbuf, const gdb_byte *writebuf,
 		    ? gnu_current_inf->task->port : 0)
 		 : 0);
   process_t proc;
-  int res;
   kern_return_t err;
   vm_address_t entry;
   ElfW(auxv_t) auxv[2];
@@ -2641,7 +2643,6 @@ gnu_nat_target::find_memory_regions (find_memory_region_ftype func,
       mach_port_t object_name;
       vm_offset_t offset;
       vm_size_t region_length = VM_MAX_ADDRESS - region_address;
-      vm_address_t old_address = region_address;
 
       err = vm_region (task,
 		       &region_address,

Re: [PATCHv2 5/7] hurd: remove unused variables

[Simon Marchi]

On 2020-05-29 6:03 p.m., Samuel Thibault wrote:
> Fixes
> 
> ../../gdb/gnu-nat.c:2554:7: error: unused variable ‘res’ [-Werror=unused-variable]
>  2554 |   int res;
> ../../gdb/gnu-nat.c:2644:20: error: unused variable ‘old_address’ [-Werror=unused-variable]
>  2644 |       vm_address_t old_address = region_address;
> 
> gdb/ChangeLog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* gnu-nat.c (gnu_xfer_auxv): Remove unused `res' variable.
> 	(gnu_nat_target::find_memory_regions): Remove unused
> 	`old_address' variable.
> 
> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index 3b438a9a43..78e9ab7f71 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -2551,7 +2554,6 @@ gnu_xfer_auxv (gdb_byte *readbuf, const gdb_byte *writebuf,
>  		    ? gnu_current_inf->task->port : 0)
>  		 : 0);
>    process_t proc;
> -  int res;
>    kern_return_t err;
>    vm_address_t entry;
>    ElfW(auxv_t) auxv[2];
> @@ -2641,7 +2643,6 @@ gnu_nat_target::find_memory_regions (find_memory_region_ftype func,
>        mach_port_t object_name;
>        vm_offset_t offset;
>        vm_size_t region_length = VM_MAX_ADDRESS - region_address;
> -      vm_address_t old_address = region_address;
>  
>        err = vm_region (task,
>  		       &region_address,
> 

Thanks, this is ok.

Simon

[PATCHv2 6/7] hurd: add gnu_target pointer to fix thread API calls

[Samuel Thibault]

Fixes

../../gdb/gnu-nat.c:1110:28: error: cannot convert ‘ptid_t’ to ‘process_stratum_target*’
 1110 |        thread_change_ptid (inferior_ptid, ptid);

and others related to 5b6d1e4fa ("Multi-target support")

gdb/ChangeLog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* gnu-nat.h (gnu_target): New variable declaration.
	* i386-gnu-nat.c (_initialize_i386gnu_nat): Initialize
	gnu_target.
	* gnu-nat.c (gnu_target): New variable.
	(inf_validate_procs): Pass gnu_target to thread_change_ptid,
	add_thread_silent, and add_thread calls.
	(gnu_nat_target::create_inferior): Pass gnu_target to
	add_thread_silent, thread_change_ptid call.
	(gnu_nat_target::detach): Pass gnu_target to detach_inferior
	call.

diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
index 77c57817b2..7c36778394 100644
--- a/gdb/gnu-nat.h
+++ b/gdb/gnu-nat.h
@@ -150,4 +150,7 @@ struct gnu_nat_target : public inf_child_target
   void stop (ptid_t) override;
 };
 
+/* The final/concrete instance.  */
+extern gnu_nat_target *gnu_target;
+
 #endif /* GNU_NAT_H */
diff --git a/gdb/i386-gnu-nat.c b/gdb/i386-gnu-nat.c
index afbe8eff49..6382ca8acb 100644
--- a/gdb/i386-gnu-nat.c
+++ b/gdb/i386-gnu-nat.c
@@ -439,6 +439,8 @@ _initialize_i386gnu_nat ()
   x86_set_debug_register_length (4);
 #endif /* i386_DEBUG_STATE */
 
+  gnu_target = &the_i386_gnu_nat_target;
+
   /* Register the target.  */
   add_inf_child_target (&the_i386_gnu_nat_target);
 }
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 3b438a9a43..78e9ab7f71 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -84,6 +85,8 @@ extern "C"
 #include "msg_U.h"
 }
 
+struct gnu_nat_target *gnu_target;
+
 static process_t proc_server = MACH_PORT_NULL;
 
 /* If we've sent a proc_wait_request to the proc server, the pid of the
@@ -1106,12 +1109,12 @@ inf_validate_procs (struct inf *inf)
 	    if (inferior_ptid == ptid_t (inf->pid))
 	      /* This is the first time we're hearing about thread
 		 ids, after a fork-child.  */
-	      thread_change_ptid (inferior_ptid, ptid);
+	      thread_change_ptid (gnu_target, inferior_ptid, ptid);
 	    else if (inf->pending_execs != 0)
 	      /* This is a shell thread.  */
-	      add_thread_silent (ptid);
+	      add_thread_silent (gnu_target, ptid);
 	    else
-	      add_thread (ptid);
+	      add_thread (gnu_target, ptid);
 	  }
       }
 
@@ -2149,7 +2152,7 @@ gnu_nat_target::create_inferior (const char *exec_file,
   /* We have something that executes now.  We'll be running through
      the shell at this point (if startup-with-shell is true), but the
      pid shouldn't change.  */
-  add_thread_silent (ptid_t (pid));
+  add_thread_silent (gnu_target, ptid_t (pid));
 
   /* Attach to the now stopped child, which is actually a shell...  */
   inf_debug (inf, "attaching to child: %d", pid);
@@ -2167,7 +2170,7 @@ gnu_nat_target::create_inferior (const char *exec_file,
   inf_resume (inf);
 
   /* We now have thread info.  */
-  thread_change_ptid (inferior_ptid,
+  thread_change_ptid (gnu_target, inferior_ptid,
 		      ptid_t (inf->pid, inf_pick_first_thread (), 0));
 
   gdb_startup_inferior (pid, START_INFERIOR_TRAPS_EXPECTED);
@@ -2273,7 +2276,7 @@ gnu_nat_target::detach (inferior *inf, int from_tty)
   inf_detach (gnu_current_inf);
 
   inferior_ptid = null_ptid;
-  detach_inferior (find_inferior_pid (pid));
+  detach_inferior (find_inferior_pid (gnu_target, pid));
 
   maybe_unpush_target ();
 }

Re: [PATCHv2 6/7] hurd: add gnu_target pointer to fix thread API calls

[Simon Marchi]

On 2020-05-29 6:03 p.m., Samuel Thibault wrote:
> Fixes
> 
> ../../gdb/gnu-nat.c:1110:28: error: cannot convert ‘ptid_t’ to ‘process_stratum_target*’
>  1110 |        thread_change_ptid (inferior_ptid, ptid);
> 
> and others related to 5b6d1e4fa ("Multi-target support")
> 
> gdb/ChangeLog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* gnu-nat.h (gnu_target): New variable declaration.
> 	* i386-gnu-nat.c (_initialize_i386gnu_nat): Initialize
> 	gnu_target.
> 	* gnu-nat.c (gnu_target): New variable.
> 	(inf_validate_procs): Pass gnu_target to thread_change_ptid,
> 	add_thread_silent, and add_thread calls.
> 	(gnu_nat_target::create_inferior): Pass gnu_target to
> 	add_thread_silent, thread_change_ptid call.
> 	(gnu_nat_target::detach): Pass gnu_target to detach_inferior
> 	call.
> 
> diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
> index 77c57817b2..7c36778394 100644
> --- a/gdb/gnu-nat.h
> +++ b/gdb/gnu-nat.h
> @@ -150,4 +150,7 @@ struct gnu_nat_target : public inf_child_target
>    void stop (ptid_t) override;
>  };
>  
> +/* The final/concrete instance.  */
> +extern gnu_nat_target *gnu_target;
> +
>  #endif /* GNU_NAT_H */
> diff --git a/gdb/i386-gnu-nat.c b/gdb/i386-gnu-nat.c
> index afbe8eff49..6382ca8acb 100644
> --- a/gdb/i386-gnu-nat.c
> +++ b/gdb/i386-gnu-nat.c
> @@ -439,6 +439,8 @@ _initialize_i386gnu_nat ()
>    x86_set_debug_register_length (4);
>  #endif /* i386_DEBUG_STATE */
>  
> +  gnu_target = &the_i386_gnu_nat_target;
> +
>    /* Register the target.  */
>    add_inf_child_target (&the_i386_gnu_nat_target);
>  }
> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index 3b438a9a43..78e9ab7f71 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -84,6 +85,8 @@ extern "C"
>  #include "msg_U.h"
>  }
>  
> +struct gnu_nat_target *gnu_target;
> +
>  static process_t proc_server = MACH_PORT_NULL;
>  
>  /* If we've sent a proc_wait_request to the proc server, the pid of the
> @@ -1106,12 +1109,12 @@ inf_validate_procs (struct inf *inf)
>  	    if (inferior_ptid == ptid_t (inf->pid))
>  	      /* This is the first time we're hearing about thread
>  		 ids, after a fork-child.  */
> -	      thread_change_ptid (inferior_ptid, ptid);
> +	      thread_change_ptid (gnu_target, inferior_ptid, ptid);
>  	    else if (inf->pending_execs != 0)
>  	      /* This is a shell thread.  */
> -	      add_thread_silent (ptid);
> +	      add_thread_silent (gnu_target, ptid);
>  	    else
> -	      add_thread (ptid);
> +	      add_thread (gnu_target, ptid);
>  	  }
>        }
>  
> @@ -2149,7 +2152,7 @@ gnu_nat_target::create_inferior (const char *exec_file,
>    /* We have something that executes now.  We'll be running through
>       the shell at this point (if startup-with-shell is true), but the
>       pid shouldn't change.  */
> -  add_thread_silent (ptid_t (pid));
> +  add_thread_silent (gnu_target, ptid_t (pid));
>  
>    /* Attach to the now stopped child, which is actually a shell...  */
>    inf_debug (inf, "attaching to child: %d", pid);
> @@ -2167,7 +2170,7 @@ gnu_nat_target::create_inferior (const char *exec_file,
>    inf_resume (inf);
>  
>    /* We now have thread info.  */
> -  thread_change_ptid (inferior_ptid,
> +  thread_change_ptid (gnu_target, inferior_ptid,
>  		      ptid_t (inf->pid, inf_pick_first_thread (), 0));
>  
>    gdb_startup_inferior (pid, START_INFERIOR_TRAPS_EXPECTED);
> @@ -2273,7 +2276,7 @@ gnu_nat_target::detach (inferior *inf, int from_tty)
>    inf_detach (gnu_current_inf);
>  
>    inferior_ptid = null_ptid;
> -  detach_inferior (find_inferior_pid (pid));
> +  detach_inferior (find_inferior_pid (gnu_target, pid));
>  
>    maybe_unpush_target ();
>  }
> 

I think it would have been more straightforward and clean to use `inf->process_target ()`
(assuming it's the correct thing to do), given that you have access to the appropriate
inferior everywhere you made changes.

But I also see that you've pretty much replicated what the linux-nat target does, so it
is ok as well if you want to keep it that way.

Simon

Re: [PATCHv2 6/7] hurd: add gnu_target pointer to fix thread API calls

[Samuel Thibault]

Simon Marchi, le sam. 30 mai 2020 10:57:42 -0400, a ecrit:
> > @@ -1106,12 +1109,12 @@ inf_validate_procs (struct inf *inf)
> >  	    if (inferior_ptid == ptid_t (inf->pid))
> >  	      /* This is the first time we're hearing about thread
> >  		 ids, after a fork-child.  */
> > -	      thread_change_ptid (inferior_ptid, ptid);
> > +	      thread_change_ptid (gnu_target, inferior_ptid, ptid);
> 
> 
> I think it would have been more straightforward and clean to use `inf->process_target ()`
> (assuming it's the correct thing to do), given that you have access to the appropriate
> inferior everywhere you made changes.

I didn't see a way to access the inferior here (inf is a
gnu-nat.c-specific structure with no link to struct inferior)

> But I also see that you've pretty much replicated what the linux-nat target does

Yes, that seemed simpler to stick along what Linux is currently doing.

Samuel

Re: [PATCHv2 6/7] hurd: add gnu_target pointer to fix thread API calls

[Simon Marchi]

On 2020-05-30 11:53 a.m., Samuel Thibault wrote:
> Simon Marchi, le sam. 30 mai 2020 10:57:42 -0400, a ecrit:
>>> @@ -1106,12 +1109,12 @@ inf_validate_procs (struct inf *inf)
>>>  	    if (inferior_ptid == ptid_t (inf->pid))
>>>  	      /* This is the first time we're hearing about thread
>>>  		 ids, after a fork-child.  */
>>> -	      thread_change_ptid (inferior_ptid, ptid);
>>> +	      thread_change_ptid (gnu_target, inferior_ptid, ptid);
>>
>>
>> I think it would have been more straightforward and clean to use `inf->process_target ()`
>> (assuming it's the correct thing to do), given that you have access to the appropriate
>> inferior everywhere you made changes.
> 
> I didn't see a way to access the inferior here (inf is a
> gnu-nat.c-specific structure with no link to struct inferior)

Ah I see.  Well, I didn't check all code paths, but for example, for the one that goes:

- gnu_nat_target::attach
- inf_attach
- inf_startup
- inf_set_pid

A solution would be to make all these inf_* functions to be private methods of the
gnu_nat_target class.  They would then pass `this` as the process_stratum_target.  A
bit like what was done in commit

  e7eee665a1524cc4569d0c2f5c9d4aa2be64c9e8
  gdb: fix darwin-nat.c build / adapt to multi-target

Even if you are not doing this, a good cleanup would be to make all those functions
(like inf_attach) static.  They are only used in gnu-nat.c, but are exported.

Simon

Re: [PATCHv2 6/7] hurd: add gnu_target pointer to fix thread API calls

[Samuel Thibault]

Simon Marchi, le sam. 30 mai 2020 12:08:53 -0400, a ecrit:
> On 2020-05-30 11:53 a.m., Samuel Thibault wrote:
> > Simon Marchi, le sam. 30 mai 2020 10:57:42 -0400, a ecrit:
> >>> @@ -1106,12 +1109,12 @@ inf_validate_procs (struct inf *inf)
> >>>  	    if (inferior_ptid == ptid_t (inf->pid))
> >>>  	      /* This is the first time we're hearing about thread
> >>>  		 ids, after a fork-child.  */
> >>> -	      thread_change_ptid (inferior_ptid, ptid);
> >>> +	      thread_change_ptid (gnu_target, inferior_ptid, ptid);
> >>
> >>
> >> I think it would have been more straightforward and clean to use `inf->process_target ()`
> >> (assuming it's the correct thing to do), given that you have access to the appropriate
> >> inferior everywhere you made changes.
> > 
> > I didn't see a way to access the inferior here (inf is a
> > gnu-nat.c-specific structure with no link to struct inferior)
> 
> Ah I see.  Well, I didn't check all code paths, but for example, for the one that goes:
> 
> - gnu_nat_target::attach
> - inf_attach
> - inf_startup
> - inf_set_pid
> 
> A solution would be to make all these inf_* functions to be private methods of the
> gnu_nat_target class.

Ok, I see. thread_change_ptid is however also called from
inf_validate_procs, called from inf_update_procs, called from
i386_gnu_dr_set_control, referenced from the x86 backend
x86_dr_low.set_control = i386_gnu_dr_set_control;

For simplicity I'll keep gnu_target for now, but also work on cleaning
these should-be-static/private functions.

Samuel

[PATCHv2 7/7] hurd: fix pushing target on inferior creation

[Samuel Thibault]

This fixes creating inferiors, which was broken since 5b6d1e4fa ('Multi-target support')

gdb/ChangeLog:

2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	* gnu-nat.c (gnu_nat_target::create_inferior): Move push_target call
	before fork_inferior call. Avoid calling it if target_is_pushed returns false.

---
v2: Added to patch series.

Index: binutils-gdb/gdb/gnu-nat.c
===================================================================
--- binutils-gdb.orig/gdb/gnu-nat.c
+++ binutils-gdb/gdb/gnu-nat.c
@@ -2146,6 +2146,9 @@ gnu_nat_target::create_inferior (const c
 
   inf_debug (inf, "creating inferior");
 
+  if (!target_is_pushed (this))
+    push_target (this);
+
   pid = fork_inferior (exec_file, allargs, env, gnu_ptrace_me,
                        NULL, NULL, NULL, NULL);
 
@@ -2159,8 +2162,6 @@ gnu_nat_target::create_inferior (const c
 
   inf_attach (inf, pid);
 
-  push_target (this);
-
   inf->pending_execs = 1;
   inf->nomsg = 1;
   inf->traced = 1;

Re: [PATCHv2 7/7] hurd: fix pushing target on inferior creation

[Simon Marchi]

On 2020-05-29 6:04 p.m., Samuel Thibault wrote:
> This fixes creating inferiors, which was broken since 5b6d1e4fa ('Multi-target support')
> 
> gdb/ChangeLog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* gnu-nat.c (gnu_nat_target::create_inferior): Move push_target call
> 	before fork_inferior call. Avoid calling it if target_is_pushed returns false.

`returns false` -> `returns true`?

Otherwise, LGTM.

Simon

Re: [PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Simon Marchi]

On 2020-05-29 6:00 p.m., Samuel Thibault wrote:
> Fixes
> 
> ../../gdb/gnu-nat.c:96:6: error: conflicting declaration ‘bool gnu_debug_flag’
>    96 | bool gnu_debug_flag = false;
> ../../gdb/gnu-nat.c: In function ‘void _initialize_gnu_nat()’:
> ../../gdb/gnu-nat.c:3511:7: error: cannot
> 
> gdb/ChangeLog:
> 
> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* gnu-nat.h (gnu_debug_flag): Set type to bool.
> 
> diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
> index 77c57817b2..7c36778394 100644
> --- a/gdb/gnu-nat.h
> +++ b/gdb/gnu-nat.h
> @@ -111,7 +111,7 @@ extern char *proc_string (struct proc *proc);
>  	      __proc_pid (__proc), __proc->tid, \
>  	      host_address_to_string (__proc) , ##args); } while (0)
>  
> -extern int gnu_debug_flag;
> +extern bool gnu_debug_flag;
>  
>  #define debug(msg, args...) \
>   do { if (gnu_debug_flag) \
> 

Thanks, this is OK.

Simon

Re: [PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Samuel Thibault]

Simon Marchi, le sam. 30 mai 2020 10:39:19 -0400, a ecrit:
> On 2020-05-29 6:00 p.m., Samuel Thibault wrote:
> > Fixes
> > 
> > ../../gdb/gnu-nat.c:96:6: error: conflicting declaration ‘bool gnu_debug_flag’
> >    96 | bool gnu_debug_flag = false;
> > ../../gdb/gnu-nat.c: In function ‘void _initialize_gnu_nat()’:
> > ../../gdb/gnu-nat.c:3511:7: error: cannot
> > 
> > gdb/ChangeLog:
> > 
> > 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> > 
> > 	* gnu-nat.h (gnu_debug_flag): Set type to bool.
> > 
> > diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
> > index 77c57817b2..7c36778394 100644
> > --- a/gdb/gnu-nat.h
> > +++ b/gdb/gnu-nat.h
> > @@ -111,7 +111,7 @@ extern char *proc_string (struct proc *proc);
> >  	      __proc_pid (__proc), __proc->tid, \
> >  	      host_address_to_string (__proc) , ##args); } while (0)
> >  
> > -extern int gnu_debug_flag;
> > +extern bool gnu_debug_flag;
> >  
> >  #define debug(msg, args...) \
> >   do { if (gnu_debug_flag) \
> > 
> 
> Thanks, this is OK.

Just to be sure, I don't have commit access so can't commit myself.
I didn't see that case documented on
https://sourceware.org/gdb/wiki/ContributionChecklist
linked from
https://www.gnu.org/software/gdb/contribute/

Samuel

Re: [PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Simon Marchi]

On 2020-05-30 12:00 p.m., Samuel Thibault wrote:
> Simon Marchi, le sam. 30 mai 2020 10:39:19 -0400, a ecrit:
>> On 2020-05-29 6:00 p.m., Samuel Thibault wrote:
>>> Fixes
>>>
>>> ../../gdb/gnu-nat.c:96:6: error: conflicting declaration ‘bool gnu_debug_flag’
>>>    96 | bool gnu_debug_flag = false;
>>> ../../gdb/gnu-nat.c: In function ‘void _initialize_gnu_nat()’:
>>> ../../gdb/gnu-nat.c:3511:7: error: cannot
>>>
>>> gdb/ChangeLog:
>>>
>>> 2020-05-29  Samuel Thibault  <samuel.thibault@ens-lyon.org>
>>>
>>> 	* gnu-nat.h (gnu_debug_flag): Set type to bool.
>>>
>>> diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
>>> index 77c57817b2..7c36778394 100644
>>> --- a/gdb/gnu-nat.h
>>> +++ b/gdb/gnu-nat.h
>>> @@ -111,7 +111,7 @@ extern char *proc_string (struct proc *proc);
>>>  	      __proc_pid (__proc), __proc->tid, \
>>>  	      host_address_to_string (__proc) , ##args); } while (0)
>>>  
>>> -extern int gnu_debug_flag;
>>> +extern bool gnu_debug_flag;
>>>  
>>>  #define debug(msg, args...) \
>>>   do { if (gnu_debug_flag) \
>>>
>>
>> Thanks, this is OK.
> 
> Just to be sure, I don't have commit access so can't commit myself.
> I didn't see that case documented on
> https://sourceware.org/gdb/wiki/ContributionChecklist
> linked from
> https://www.gnu.org/software/gdb/contribute/

We can either push on your behalf.  But if you'd like to have push access, because
you plan to make regular contributions, you can ask for an account here, and specify
me as "email address of person who approved request".

  https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Simon

Re: [PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Samuel Thibault]

Simon Marchi, le sam. 30 mai 2020 12:12:24 -0400, a ecrit:
> We can either push on your behalf.  But if you'd like to have push access, because
> you plan to make regular contributions, you can ask for an account here, and specify
> me as "email address of person who approved request".
> 
>   https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Ah, I already have a sourceware account (for glibc), the page says that
in such case I should

“send an email to the overseers mail account at this site”

but I don't understand what that means.

Samuel

Re: [PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Simon Marchi]

On 2020-05-30 12:53, Samuel Thibault wrote:
> Simon Marchi, le sam. 30 mai 2020 12:12:24 -0400, a ecrit:
>> We can either push on your behalf.  But if you'd like to have push 
>> access, because
>> you plan to make regular contributions, you can ask for an account 
>> here, and specify
>> me as "email address of person who approved request".
>> 
>>   https://sourceware.org/cgi-bin/pdw/ps_form.cgi
> 
> Ah, I already have a sourceware account (for glibc), the page says that
> in such case I should
> 
> “send an email to the overseers mail account at this site”
> 
> but I don't understand what that means.
> 
> Samuel

"this site" is sourceware.org, so overseers at sourceware dot org.  They 
are also very responsive on the #overseers IRC channel on Freenode.

Simon

Re: [PATCHv2 1/7] hurd: fix gnu_debug_flag type

[Samuel Thibault]

Ok, I pushed this series of 7 patches for now.

Thanks for the reviews,
Samuel