[PATCH 1/2] gdb: bfin: new port
Mike Frysinger
vapier@gentoo.org
Thu Dec 9 03:35:00 GMT 2010
On Wednesday, December 08, 2010 16:25:38 Joel Brobecker wrote:
> First of all, I apologize of the delay in reviewing this. Overall,
> I think that this is very good work. I can't really help with the
> correctness of the code, since I don't know the bfin architecture.
np. if there's any feedback i didnt respond to here, it's because i just did
as asked since it made perfect sense.
> > + bfin --target=bfin-elf ,-Werror
> > + Mike Frysinger vapier@gentoo.org
> > +
>
> We usually wait to see a few contributions before inviting a contributor
> to become the official maintainer. So, would you mind taking that hunk
> out of your patch?
i thought people would want to know how to find out who knows the Blackfin
code in case they had a question. ive made contributions in the past to gdb,
but more so to the binutils project. but if you want me to leave it out for
now, that's obviously your call.
> > +static int
> > +bfin_linux_pc_in_sigtramp (struct frame_info *this_frame, CORE_ADDR pc)
> > +{
> [...]
> > + gdb_byte buf[10];
>
> The use of that buffer is most peculiar. You first start by reading
> a 2-byte instruction and storing it at buf+4. Then, depending on
> the instruction, you you extract the other instruction either at
> buf or buf+6. If it works, then no need to change...
i think the idea is for buf[] to reflect the actual code to make debugging
this code easier. i'm sure the buffer could be compacted, but i dont think
it'd have any sort of profound savings, so i'd prefer to leave it be.
> I'm also wondering whether you know about tramp-frame.h, and whether
> that might be sufficient for your needs...
this port was originally written against an older version of gdb which didnt
have tramp-frame stuff. i'll log this into our tracker to get this updated.
> > +#ifdef _DEBUG
> > + fprintf (stderr, "frameless pc case base %x\n", cache->base);
> > +#endif
>
> Can you avoid the #ifdef? I personally that debugging logs are a good
> idea. But I don't think they should only be activated (or not) at
> compile time. The typical way of doing this is to conditionalize this
> on a global variable controlled by a setting. For instance, how about
>
> (gdb) set debug bfin
>
> (see `set debug infrun' for instance).
i'll drop it for now, but i like the idea of runtime debug in this code, and i
have more stuff i could sprinkle throughout.
> > +static int
> > +is_minus_minus_sp (int op)
>
> Just out of curiosity: Why two 'minus' in the name?
the actual insn is "[--SP] = ...;"
> > +enum gcc_regnum {
> > + BFIN_GCC_R0_REGNUM = 0,
>
> What is this used for?
that's a good question. looking through the history, it doesnt seem to be
referenced when it was first added over 5 years ago. i cant really ask the
guy who committed it as it was a contractor who hasnt worked for us in a very
long time. considering the current dwarf/gdb map seems to work, i'll just
punt it.
> > +/* The ABIs for Blackfin. */
> > +enum bfin_abi
> > +{
> > + BFIN_ABI_FLAT
> > +};
> > +
> > +/* Target-dependent structure in gdbarch. */
> > +struct gdbarch_tdep
> > +{
> > + /* Which ABI is in use? */
> > + enum bfin_abi bfin_abi;
> > +};
>
> Why is this used, since there is only one ABI?
i have a follow up patch to add the FDPIC ABI, but it requires rework in the
FRV FDPIC, and the frame parsing doesnt work quite right when going through
the PLT after we updated from our older gdb version. since these issues are
taking time, i'd like to get the core Blackfin code merged as people are using
that today for various targets.
> > +/* Return the Blackfin ABI associated with GDBARCH. */
> > +static inline enum bfin_abi
> > +bfin_abi (struct gdbarch *gdbarch)
> > +{
> > + return gdbarch_tdep (gdbarch)->bfin_abi;
> > +}
>
> Please - no code in .h files.
it's used by both the core Blackfin code and the FDPIC solib code (which isnt
part of this patch per above). i could duplicate the function in two files,
but it seems to make more sense to have one shared instance.
> I doubt that making that function inline is bringing any measurable benefit
> - is it?
it avoids warnings when people include this header but dont use the func
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20101209/d5daf1b5/attachment.sig>
More information about the Gdb-patches
mailing list