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

[Tristan Gingold <gingold at adacore dot com>]

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.