[RFA,5/5,v2] Darwin: fix SIGTRAP when debugging

Message ID a8c673ff-2bd9-8f21-6ac6-29f4814e927d@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Sept. 9, 2018, 1:55 p.m. UTC
  On 2018-09-09 02:27 PM, Xavier Roirand wrote:
> Debugging a program under Darwin does not work:
> 
> (gdb) start
> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
> Starting program: /private/tmp/helloworld
> [New Thread 0x2903 of process 60326]
> During startup program terminated with signal SIGTRAP, Trace/breakpoint
> trap.
> 
> Field signaled from darwin_thread_info is not initialized thus signal
> sent to the debuggee is considered as not sent by GDB whereas it should.
> 
> This patch fixes this problem and also updates (change type, initialize)
> single_step value in the same structure at the same time.
> 
> gdb/ChangeLog:
> 
>         * darwin-nat.h (struct darwin_thread_info) <signaled>: Change
>         type and initialize.
>         (struct darwin_thread_info) <single_step>: Change type and
>         initialize.
> 
> Change-Id: I0fe2a6985df9d0dfcc8a2a258a3ef70cfa19b403
> ---
>  gdb/darwin-nat.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/darwin-nat.h b/gdb/darwin-nat.h
> index 5965df08ab6..4232b5e0365 100644
> --- a/gdb/darwin-nat.h
> +++ b/gdb/darwin-nat.h
> @@ -126,10 +126,10 @@ struct darwin_thread_info : public private_thread_info
>    enum darwin_msg_state msg_state;
>  
>    /* True if this thread is single-stepped.  */
> -  unsigned char single_step;
> +  bool single_step = 0;
>  
>    /* True if a signal was manually sent to the thread.  */
> -  unsigned char signaled;
> +  bool signaled = 0;
>  
>    /* The last exception received.  */
>    struct darwin_exception_msg event;
> 

As mentioned in the last review, I think all files should be initialized to a
sensible value, either to a constant value or by the constructor.  This is
what I would suggest, on top of your patch (not tested).  Would that work
for you?


From 45abaf5843f1c1f55a233d00cc0c07ed38f64d89 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Sun, 9 Sep 2018 14:53:17 +0100
Subject: [PATCH] fixup

---
 gdb/darwin-nat.c | 6 ++----
 gdb/darwin-nat.h | 6 +++++-
 2 files changed, 7 insertions(+), 5 deletions(-)
  

Comments

Xavier Roirand Sept. 9, 2018, 1:57 p.m. UTC | #1
Le 9/9/18 à 3:55 PM, Simon Marchi a écrit :
> On 2018-09-09 02:27 PM, Xavier Roirand wrote:
>> Debugging a program under Darwin does not work:
>>
>> (gdb) start
>> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
>> Starting program: /private/tmp/helloworld
>> [New Thread 0x2903 of process 60326]
>> During startup program terminated with signal SIGTRAP, Trace/breakpoint
>> trap.
>>
>> Field signaled from darwin_thread_info is not initialized thus signal
>> sent to the debuggee is considered as not sent by GDB whereas it should.
>>
>> This patch fixes this problem and also updates (change type, initialize)
>> single_step value in the same structure at the same time.
>>
>> gdb/ChangeLog:
>>
>>          * darwin-nat.h (struct darwin_thread_info) <signaled>: Change
>>          type and initialize.
>>          (struct darwin_thread_info) <single_step>: Change type and
>>          initialize.
>>
>> Change-Id: I0fe2a6985df9d0dfcc8a2a258a3ef70cfa19b403
>> ---
>>   gdb/darwin-nat.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/darwin-nat.h b/gdb/darwin-nat.h
>> index 5965df08ab6..4232b5e0365 100644
>> --- a/gdb/darwin-nat.h
>> +++ b/gdb/darwin-nat.h
>> @@ -126,10 +126,10 @@ struct darwin_thread_info : public private_thread_info
>>     enum darwin_msg_state msg_state;
>>   
>>     /* True if this thread is single-stepped.  */
>> -  unsigned char single_step;
>> +  bool single_step = 0;
>>   
>>     /* True if a signal was manually sent to the thread.  */
>> -  unsigned char signaled;
>> +  bool signaled = 0;
>>   
>>     /* The last exception received.  */
>>     struct darwin_exception_msg event;
>>
> 
> As mentioned in the last review, I think all files should be initialized to a
> sensible value, either to a constant value or by the constructor.  This is
> what I would suggest, on top of your patch (not tested).  Would that work
> for you?
> 
> 

I woud have think of two patches, one for the signaled field and one for 
whole field initialization but that's not a problem to merge in one. 
I'll do this.
  

Patch

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index d23706d..326e97b 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -339,10 +339,8 @@  darwin_check_new_threads (struct inferior *inf)
       if (new_ix < new_nbr && (old_ix == old_nbr || new_id < old_id))
 	{
 	  /* A thread was created.  */
-	  darwin_thread_info *pti = new darwin_thread_info;
-
-	  pti->gdb_port = new_id;
-	  pti->msg_state = DARWIN_RUNNING;
+	  darwin_thread_info *pti
+	    = new darwin_thread_info (new_id, DARWIN_RUNNING);

 	  /* Add the new thread.  */
 	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
diff --git a/gdb/darwin-nat.h b/gdb/darwin-nat.h
index 4232b5e..fee535b 100644
--- a/gdb/darwin-nat.h
+++ b/gdb/darwin-nat.h
@@ -113,12 +113,16 @@  enum darwin_msg_state

 struct darwin_thread_info : public private_thread_info
 {
+  darwin_thread_info (thread_t gdb_port, darwin_msg_state msg_state)
+  : gdb_port (gdb_port), msg_state (msg_state)
+  {}
+
   /* The thread port from a GDB point of view.  */
   thread_t gdb_port;

   /* The thread port from the inferior point of view.  Not to be used inside
      gdb except for get_ada_task_ptid.  */
-  thread_t inf_port;
+  thread_t inf_port = 0;

   /* Current message state.
      If the kernel has sent a message it expects a reply and the inferior