[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