[patch] gdbserver: Add qnx target
Aleksandar Ristovski
aristovski@qnx.com
Wed Jun 24 18:51:00 GMT 2009
Pedro Alves wrote:
> On Friday 19 June 2009 20:58:14, Aleksandar Ristovski wrote:
>> Pedro Alves wrote:
>> > > +static void
>> > > +do_detach ()
>> ^ (void)
>>
>
> You missed this one.
ok.
>
>> > > +static ptid_t
>> > > +nto_wait (ptid_t ptid,
>> > > + struct target_waitstatus *ourstatus, int
>> target_options)
>> > > +{
>> > > + sigset_t set;
>> > > + siginfo_t info;
>> > > + procfs_status status;
>> > > + static int exit_signo = 0; /* For tracking exit
>> status. */
>>
>> Pedro: Why is this static?
>>
>> We first register that signal has hit the inferior, but it
>> hasn't died just yet. We set exit_signo and return
>> TARGET_WAITKIND_STOPPED. On next resume it really
>> terminates, we set recorded signal from the static.
>
> Ah, I see.
>
>> It should really go into "nto_inferior" structure. I plan to
>> support multiple processes so there will be a chance for
>> cleanups.
>
> I don't see that it would take more than a few lines
> of code to move this to struct nto_inferior.
Moved.
>
> It looks to me that you could set the exit signo here instead:
>
> nto_resume:
> if (status.why & (_DEBUG_WHY_SIGNALLED | _DEBUG_WHY_FAULTED))
> {
> if (signal_to_pass != status.info.si_signo)
> {
> kill (status.pid, signal_to_pass);
> run.flags |= _DEBUG_RUN_CLRFLT | _DEBUG_RUN_CLRSIG;
> }
>>>> else /* Let it kill the program without telling us. */
>>>> sigdelset (&run.trace, signal_to_pass); <
> }
>
> (and always clear it on entry to nto_resume)
>
> .... but I may be wrong. It's OK to leave as is (I don't care *that*
> much about this file ;-) ), but if you want to leave as is,
> then please paste that explanation in the code.
Didn't change this.
>
>> Pedro: AFAICS, nowhere have you set the current_inferior.
>>
>> add_thread will set it (see inferiors.c:184)
>
> How does that help in the nto_wait case I pointed? You have
> to set current_inferior to the thread that is reporting the
> event, otherwise, the following register reads may read from
> the wrong thread (*). Question: did you run the testsuite
> against this? I'd be curious to know how does this
> compares against native gdb.
>
> (*) - unless I'm missing/forgetting something. Sounds
> like something we can now assure / clean up in generic code,
> since target_wait now returns a ptid_t...
I think your feeling was right: I did not have code for
adding new threads. Rectified now - thanks for all your help!
>
>> Pedro: I don't really know a thing about nto/qnx apis, but,
>> do you really need this nto_comctrl enable/disable calls?
>> Can't you just do what nto_comctrl does once unconditionaly,
>> and then rely on signal (SIGIO, SIG_IGN|input_interrupt),
>> as you seem to already ?
>>
>>
>> Unfortunately, I couldn't find a way. Whatever I looked at
>> requires that handle is present. The problem here is that
>> while we are blocked, we will not receive SIGIO signal
>> unless we explicitly setup an event.
>> And we need to do it
>> every time before we go into sigwaitinfo.
>
> Ok, I peeked at ionotify's docs, and from what I've
> understood, this is fine. I was considering if this
> wasn't racy, but it seems like ionotify (_NOTIFY_ACTION_POLLARM)
> behaves a bit like select --- if there's data already there, it
> triggers an action immediately. I'm not all that happy with
> having __QNX__ wrapped code, but it's mostly contained, and
> not really worse than USE_WIN32API... If there's more of
> these to come, than let's think about abstracting out the
> posix|win32|qnx initialize|disable|enable_async_io functions to
> separate files somehow.
>
>
> Comments on the new patch:
>
>> +static int
>> +nto_stopped_by_watchpoint (void)
>> +{
>> + procfs_status status;
>> + int ret = 0;
>> + const int watchmask = _DEBUG_FLAG_TRACE_RD | _DEBUG_FLAG_TRACE_WR
>> + | _DEBUG_FLAG_TRACE_MODIFY;
>> +
>> + TRACE ("%s...", __func__);
>> + if (nto_inferior.ctl_fd != -1)
>> + {
>> + int err = devctl (nto_inferior.ctl_fd, DCMD_PROC_STATUS, &status,
>> + sizeof (status), 0);
>
> ^^^^^^ tab vs space
>
>
>> +}
>> +
>> +
>
> You used 1 line spacing everywhere else.
Ok, I *think* I addressed all spacing/tabbing issues.
>
>> +static int
>> +nto_supports_non_stop (void)
>> +{
>> + TRACE ("%s\n", __func__);
>> + return 0;
>> +}
>> +
>> +
>> +
>
> This function isn't really needed though. The
> default is 0. (Not installing such callbacks may make
> life easier for someone else in the future, if e.g., the
> interface of such functions needs to change, but it's no
> biggie.)
I would leave it as a testimony to my commitment to enable
non-stop on Neutrino. :-)
>
>> + int (*register_offset)(int gdbregno);
> ^ missing space
>
>
>> +/* Activated by env. variable GDBSERVER_DEBUG. */
>> +extern int nto_debug;
>
> This isn't defined anywhere. Please remove.
>
>> * configure.srv (i[34567]86-*-nto*): New target.
>> * nto-low.c, nto-low.h, nto-x86-low.c: New files.
>
>> * remote-utils.c (include sys/iomgr.h): New include for
>> __QNX__ only.
>> (nto_comctrl): New function for __QNX__ only.
>> (enable_async_io, disable_async_io): Call nto_comcntrl for
>> __QNX__ only.
>
> There's a standard way to mention these conditionalized
> changes. It goes something like this:
>
> * remote-utils.c [__QNX__]: Include sys/iomgr.h.
> (nto_comctrl) [__QNX__]: New function.
> (enable_async_io, disable_async_io) [__QNX__]: Call nto_comcntrl.
>
>
> Other than the above remarks, this is good to go, once
> its dependencies are in.
>
Ok, here is my new patch. I addressed all of the above, and
probably introduced some new issues :-). For my bonus
points, I added comments for each function definition in
nto-low.c
Let me know what you think (once this goes in, I will change
gdb's configure.tgt to say "yes" to generating gdbserver for
Neutrino - in a separate patch submission).
Thanks,
--
Aleksandar Ristovski
QNX Software Systems
ChangeLog (NOTE: I merged both configuration/Makefile
changes and new files into one all-inclusive patch).
* configure: Regenerated.
* configure.ac: Add case for srv_qnx and set LIBS
accordingly..
* configure.srv (i[34567]86-*-nto*): New target.
* nto-low.c, nto-low.h, nto-x86-low.c: New files.
* remote-utils.c [__QNX__]: Include sys/iomgr.h
(nto_comctrl) [__QNX__]: New function.
(enable_async_io, disable_async_io) [__QNX__]: Call nto_comctrl.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gdbserver-ntotarget-20090624.diff
Type: text/x-patch
Size: 31273 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20090624/8dc676c7/attachment.bin>
More information about the Gdb-patches
mailing list