Re: [RFC] QNX Neutrino/i386 support

["Kris Warkentin" <kewarken at qnx dot com>]

> * Your files should include the blurb about the GPL.  Look at the
>   files that are already part of GDB.

I had just looked at the vxworks files.  I should have looked at a few
others.

> #ifdef USG
> #include <sys/types.h>
> #endif
>
> >>> We shouldn't introduce any new uses of the USG define.

What should I check for?  Is there a HAVE_SYS_TYPES define or some such?

> extern unsigned nto_cpuinfo_flags;
> extern int nto_cpuinfo_valid;
> extern int nto_ostype, nto_cputype;
>
> >>> Are these global variables really needed?  If they are, they
> >>> probably belong in a header file of some sort, and they would
> >>> deserve a comment.

Actually, they're in nto-tdep.h.  I just missed removing the externs.  As
far as whether they're needed, they just help us work around a few ugly
issues like whether the remote supports fxsave or not.  Also used to figure
out remote targets when using qnet, our "transparent" networking protocol
where you could do something like 'target procfs /net/some_machine'.  There
are probably better ways of doing these things.  That's where you guys come
in - to help me remove hacks. ;-)  I'll definitely make some comments.

>   else
>     {
>       regset = -1; /* error */
>     }
>   return regset;
> }
>
> >>> Personally, I would leave out the braces here.

Eew.  Good thing I didn't write that function. ;-)  There have been many
fingers in these pies and clean up is not fun.  You should see our mips and
sh support....

>   supply_register (first_regno, (char *) &minusone);
>
> >>> GDB's convention is to fill registers with zero instead of -1.  At
> >>> least that's what all the other i386 targets do.

I think that the -1 might have been from a long long time ago.  These files
were originally from gdb 4.17 or earlier.

>   /* If remote-nto-i386.c calls nto_supply_register in remote-nto.c,
>    * then remote-nto.c calls nto_cpu_register_fetch.  Since this never
>    * happens, this function is left unimplemented. */

> >>> This comment references remote-i386-nto.c.  There is no such file
> >>> in your patch.

My register refactoring eliminated this function last week.  The register
refactoring is on our head branch which I need to port to my FSF submission
branch....some of it also needs to be back ported to our previous release
branch....  Arghh....too many trees make Kris go crazy....

>   /* setjmp()'s return PC saved in EDX (5) */
>   tdep->jb_pc_offset = 5 * sizeof (int);
>
> >>> You cannot use sizeof(int) here, since that might very well differ
> >>> from 4 on other systems.

Ooh...good catch.  That might spank me on sparc64.

> /* After a watchpoint trap, the PC points to the instruction after
>    the one that caused the trap.  Therefore we don't need to step over it.
>    But we do need to reset the status register to avoid another trap.  */
>   HAVE_CONTINUABLE_WATCHPOINT = 1;
>
> >>> This HAVE_CONTINUABLE_WATCHPOINT stuff has been moved to the
> >>> target vector, so that's where you should provide the appropriate
> >>> definition.

I did the target vector patch but I guess I don't know where this should
have gone...Help?

>   /* read our extra gdbinit file */
>   nto_source_extra_gdbinit (".ntox86-gdbinit");
>
> >>> This extra-gdbinit-stuff doesn't belong here.  We have had a
> >>> discussion about these target-dependent init files recently, and I
> >>> believe the outcome was that if they were wanted, some general
> >>> mechanism should be added rather than these kind of ad-hoc hacks.

This is not a critical feature and can be removed for an FSF submission
pending a better way of doing it.  I could volunteer to try and find one.

Thanks for the commentary Mark.  I'm on semi-vacation next week (AFK Monday,
Wednesday, Friday) but I'll try to keep this flowing.

cheers,

Kris