This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Attachment:
signature.asc
Description: This is a digitally signed message part.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |