Re: [patch] Add QNX remote protocol headers.

Daniel Jacobowitz
2005-09-12T13:33:34Z

On Mon, Sep 12, 2005 at 09:11:35AM -0400, Kris Warkentin wrote:
> Hi Daniel. Thanks for looking at this for me. I had split the headers
> to make the patch simpler but perhaps that wasn't the best idea. I've
> attached the remote-nto.c file that uses these headers to this email.
> The headers are used by remote-nto.c to implement the QNX pdebug
> protocol which you get with 'target qnx <device>|<host:port>'

Yes, that's what I thought. So the assumption of remote only is
correct.

> >Assuming remote only, then this:
> >> /* __DEBUG_H_INCLUDED is Neutrino's native debug.h header. We don't
> >want
> >> these duplicate definitions if we're compiling natively and have
> >already
> >> included it. */
> >> #ifndef __DEBUG_H_INCLUDED
> >> #define __DEBUG_H_INCLUDED
> >
> >is pretty suspect - just give them different names from the system
> >version, or avoid including the system header, preferably the latter.
> >
>
> I could re-name the structures reasonably easily. I'm not so sure about
> avoiding the system header since the native support is using all of our
> kernel interfaces to issue devctls to the procfs debug interface.

Avoiding the system headers should be easy. You shouldn't be including
them from any tm-*.h files anyway, so remote-nto.c is only going to
have included (A) what defs.h gives it and (B) what it includes itself.

Renaming's a good choice too though.

> >And basically everything else in the file is broken. You can't do
> >anything even vaguely complicated involving cross-debugging using host
> >system types. You're relying on the host's alignment and padding
> >rules, and system types like clock_t.
> >
>
> Well, I don't know about "can't". We support debugging of i386, sh,
> mips, arm and powerpc from Windows, Linux, Solaris and self-hosted
> Neutrino and have for many years using this code. I definitely agree
> with you that the gdb remote protocol is far more portable and that
> passing structures around isn't the safest way to do things. This is,
> however, the way pdebug has always been defined and provided everyone is
> using gcc, everything works fine.
>
> Basically, I'm acknowledging the limitations of this code and hoping
> that, with certain changes and cleanup, you might consider accepting it
> anyway. Several points: 1) It's a tried and tested solution that has
> been in the field supported by us for more than half a decade. 2) It
> doesn't actually have anything to do with any other gdb mechanisms.
> Completely self contained. If we broke anything, it would only be our
> own stuff. 3) In order to make it completely portable, I'd most likely
> have to avoid passing structures completely and totally re-write the
> entire protocol and server.

That's all well and good, but you've missed my point. I'm not
complaining about you passing structures around. I'm complaining about
you using structures on the host side to unpack them.

Here's how it goes: you are contributing support for a new remote
target to GDB. That means you get to pick the target. So you can pick
32-bit targets with standard packing (well, depending on the content of
the structure, the ARM padding rules may bite you; I'll assume from
your description that this isn't a problem).

But that new code has no inherent _host_ dependency. It's not
acceptable to take it and say "here's some non-portable code that will
only work on x86, sparc, and neutrino". I can guarantee you that, if
it's in FSF GDB, someone is eventually going to try upgrading to x86_64
and notice when the pieces all fall apart.

Passing structures is one thing. Decoding them by dumping binary blobs
into host-side structures is an entirely different thing. But wait!
Fortunately you're working in a debugger with lots of infrastructure
for cross-debugging environments! It'll be a little bulkier, but
there's nothing unsolvable about it.

> >And whatever this does:
> >
> >> #ifdef __QNX__
> >> __BEGIN_DECLS
> >> #include <_pack64.h>
> >> #endif
> >
> >is similarly not OK. If it's intended to work cross, then it should
> >work from everywhere, not just from x86 and QNX.
> >
>
> That's easy enough to fix.
>
> >I think I need a mile-high explanation of what you're trying to do,
> >first. By the way, is pdebug publically documented?
> >
>
> I hope I've more or less explained it above. I want to submit a
> self-contained patch for our remote protocol that, while not the most
> portable of solutions, will not interfere with anything else. As far as

I don't speak for everyone here, but I'm really not interested in
adding non-portable target code to GDB. Non-portable native code,
sure...

> documentation goes, the descriptions within the headers and source files
> are just about it. Gdb blocks on the remote which eventually sends back
> a reply stating the type of event that occurred after which gdb gathers
> whichever information it needs and issues commands back to the target.
> There's a separate channel for I/O. Fairly simple all told.

Trust me, nothing is ever simple about this sort of thing. I was
hoping there was a manual, but if you don't have one either, we can
make to with what we've got.

The reason I'm so interested, both in documentation and in portability,
is that you've got a working remote protocol with channels. There's a
lot of incentive to leverage that for other targets.

[I didn't spend much time looking at the attachment. Overall nothing
jumped out at me except the binary blobs on the wire issue.]