[RFA] Darwin/x86 port (v4 - part 2/4: darwin-nat.?)

[Tristan Gingold]

On Nov 18, 2008, at 3:18 AM, Stan Shebs wrote:

> Tristan Gingold wrote:
>>
>> darwin-nat.h:
>>
>>
>> DEF_VEC_I(thread_t);
> As per my comments on machoread.c.

Space added.

>> struct darwin_inferior
>> {
>>  int pid;
>>  task_t task;
> I think there should be at least a one-liner describing each field.   
> Even if it's obvious, the one-liner at least tells us that the  
> meaning is indeed the obvious one, and not something tricky.

Done.

>>  /* Previous port for requesr notification on task.  */
> "request"

Oops!

>> extern darwin_inferior *darwin_inf;
> I guess it would be mean to insist that the Darwin port be multi- 
> process ready. :-)

Yes.  I will try to work on that later.  Shouldn't be that hard.

>> #if (defined __GNUC__)
>> #define __MACH_CHECK_FUNCTION __PRETTY_FUNCTION__
>> #else
>> #define __MACH_CHECK_FUNCTION ((__const char *) 0)
>> #endif
> Follow what gdb_assert.h does for __PRETTY_FUNCTION__ .

I now use ASSERT_FUNCTIOn from gdb_assert.h it it is defined.  That's  
avoid code duplication.
(OTOH only GCC 4.x or later is available on Darwin)

>> static union {
>>  mach_msg_header_t hdr;
>>  char data[1024];
>> } msgin, msgout;
>>
> Definitely want to say a few words about what we have to do to work  
> with Mach. Refer people to a URL or manual for full details, what  
> we're interested in here is a) basic hints, and b) special issues  
> for GDB.

Done.

>> #define X86_EFLAGS_T 0x100UL
>>
>> static void
>> darwin_set_sstep (thread_t thread, int enable)
>> {
>>  x86_thread_state_t regs;
>>  unsigned int count = x86_THREAD_STATE_COUNT;
> Won't the PowerPC port get indigestion from this part? :-)

Yes, moved to i386-darwin-nat.c

>
>>          res = PTRACE (PT_THUPDATE, pid,
>>                (void *)exc_msg.thread_port, nsignal);
> Let's just call darwin_ptrace directly. I don't see much value in a  
> macro, it's just in this one file.

The macro is used to stringify the request name (and thus generating a  
nicer message).

>> #ifdef HAVE_64_BIT_MACH_EXCEPTIONS
> Where does this macro come from? (Yes, I could look it up, but don't  
> make me, add a comment. :-) )

Removed.

>>    case EXC_BREAKPOINT:
>>      /* Many internal GDB routines expect breakpoints to be reported
>>         as TARGET_SIGNAL_TRAP, and will report TARGET_EXC_BREAKPOINT
>>         as a spurious signal. */
>>      status->value.sig = TARGET_SIGNAL_TRAP;
> Hmmm. OK for now, I'm sure we'll have to revisit.

Hmm, maybe.

>>       return ptid_build (pid, 0, exc_msg.thread_port);
> Thread port is the id we want to use for threads? I guess it's  
> plausible, we should say that somewhere.

Yes, that makes the code simpler.  To be revisited at least to display  
a much nicer value to the user.

>> #if 0
>> static void
>> darwin_list_gdb_ports (const char *msg)
> If the code has a use, keep it and make a maintenance command, else  
> discard. Our policy these days is to discourage if-0 blocks  
> everywhere.

Discarded.

>> /* The child must synchronize with gdb: gdb must set the exception  
>> port
>>   before the child call PTRACE_SIGEXC.  We use a pipe to achieve  
>> this.
>>   FIXME: is there a lighter way ?  */
> Huh, interesting. I don't have any better ideas myself.

The question is still open!

>>  add_setshow_zinteger_cmd ("inferior", class_obscure,
>>                &inferior_debug_flag, _("\
>> Set if printing inferior communication debugging statements."), _("\
>> Show if printing inferior communication debugging statements."),  
>> NULL,
>>                NULL, NULL,
>>                &setdebuglist, &showdebuglist);
> Wasn't this in a different file too? Make sure there is only one.

The command is now "darwin" and the flag darwin_debug_flag.

>>  add_setshow_boolean_cmd ("mach-exceptions", class_support,
>>               &enable_mach_exceptions, _("\
>> Set if mach exceptions are caught."), _("\
>> Show if mach exceptions are caught."), _("\
>> When this mode is on, all low level exceptions are reported before  
>> being\n\
>> reported by the kernel."),
>>               &set_enable_mach_exceptions, NULL,
>>               &setlist, &showlist);
>> }
>>
> Likewise.

There is only one.

> I'd like to see the x86-specific bits pulled out, we should get off  
> on the right foot with that sort of thing.

Sure.

> Otherwise this file is fine, once feedback is incorporated.

Thanks.

Tristan.