Please see latest patch attached which addresses questions below. I have added a configuration option: --enable-libunwind per your suggestion. Ok so far? I have to wait for Kevin to get through my intermediate patch before I can send you the ia64-tdep.c side of the libunwind patch. 2003-10-15 Jeff Johnston * libunwind-frame.c: New file. * libunwind-frame.h: New file. * configure.in: Add --enable-libunwind option support. * configure: Regenerated. * Makefile.in: Add support for libunwind-frame.o. * config.in: Regenerated. J. Johnston wrote: > Andrew Cagney wrote: > >> Jeff, >> >> Is it possible to post (or commit to a branch) the other >> (work-in-progress?) parts to this change? >> Ignoring a few nits, this code appears to be going in the right >> direction, however its hard to tell without seeing things in toto. >> >>> The attached patch adds basic libunwind frame support. I will be >>> shortly submitting an ia64-tdep.c patch which works in conjunction >>> with this patch. The libunwind code is protected by looking for the >>> libunwind header files to compile. At runtime, the libunwind frame >>> sniffer will fail if either the libunwind library cannot be >>> dynamically loaded or the frame in question does not have proper >>> libunwind info. >> >> >> >> First some straight legal questions: >> >> - from where can libunwind be obtained? >> > > http://www.hpl.hp.com/research/linux/libunwind/ > >> - who owns it? >> > > Hewlitt-Packard > >> - what are its licence terms, and are they GPL compatible? >> > > It has a BSD-like license. Yes, it is GPL compatible. > > > Copyright (c) 2002 Hewlett-Packard Co. > > Permission is hereby granted, free of charge, to any person obtaining > a copy of this software and associated documentation files (the > "Software"), to deal in the Software without restriction, including > without limitation the rights to use, copy, modify, merge, publish, > distribute, sublicense, and/or sell copies of the Software, and to > permit persons to whom the Software is furnished to do so, subject to > the following conditions: > > The above copyright notice and this permission notice shall be > included in all copies or substantial portions of the Software. > > THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE > LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION > OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION > WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. > >> - is it considered a "system library" (like libc, or libthread_db)? >> > > I would have to say no. It is currently an optional library. This is > one of the reasons I chose to use a dynamic load mechanism. > >> - have you used, or refered to David Mossberger's [sp] old libunwind >> patch (it was eventually contributed to the FSF). >> > > No. Not in this code. > >>> For configuration I have added checks for libunwind.h and >>> libunwind-ia64.h as this is being added for ia64 support only at >>> present. Regarding testing, I have this code working with my ia64 >>> changes. I have tested on systems with the libunwind-0.92 >>> library/headers installed and not installed. >> >> >> >> I see, to make this optional, you've wrapped the code in #ifdef >> HAVE_LIBUNWIND_H, and then modified the Makefile so that it is >> unconditionally built :-( >> >> These files should only be linked in when when needed, or at least >> only when there's a libunwind around. Have a look at --with-mmalloc >> and --with-sysroot for how to implement the option --with-libunwind >> which lets the user force their inclusion; and --disable-gdbcli, >> --enable-tui, and --enable-sim for idea's on how to make linking the >> object files optional. >> > > I'll take a look. The only thing this allows a user to do is to build > gdb on a system that it can legimately build libunwind support and > purposely disable it at configuration. Can you disable dwarf2 cfi > support? If not, why would you want to do this for libunwind which is a > similar level of functionality? > >>> 2003-10-15 Jeff Johnston >>> >>> * libunwind-frame.c: New file. >>> * libunwind-frame.h: New file. >>> * configure.in: Add checks for libunwind.h and libunwind-ia64.h. >>> * configure: Regenerated. >>> * Makefile.in: Add support for libunwind-frame.o. >>> * config.in: Regenerated. >> >> >> >> Without seeing the rest of the code I'm really not in a position to >> comment on the technical design. >> >> However, a quick glance did through up a few nits. I'm noting them >> now, so that, hopefully an additional round trip can be avoided later. >> >> + Contributed by Jeff Johnston >> >> Legal nit. "Written by Jeff Johnston, contributed by Red Hat Inc.", >> you don't have an assignment on file. >> > > Will change. > > >>> +void >>> +libunwind_set_descr_handle (void *handle) >>> +{ >>> + libunwind_descr_handle = handle; >>> +} >> >> >> >> I don't understand this. A guess is that this is trying to implement >> a mechanism that lets an external module set this modules >> per-architecture data? If that is the case, then can you have a look >> at the set_gdbarch_data doco and regrroups.c's reggroup_add method for >> how to do this? >> > > I did look at said functionality originally. I ran into a problem which > this solved. I will have to look again to see what the problem was > because I don't remember off-hand. > >> +#define STRINGIFY2(name) #name >> +#define STRINGIFY(name) STRINGIFY2(name) >> + >> +static char *get_reg_name = STRINGIFY(UNW_OBJ(get_reg)); >> +static char *get_fpreg_name = STRINGIFY(UNW_OBJ(get_fpreg)); >> +static char *get_saveloc_name = STRINGIFY(UNW_OBJ(get_saveloc)); >> +static char *step_name = STRINGIFY(UNW_OBJ(step)); >> +static char *init_remote_name = STRINGIFY(UNW_OBJ(init_remote)); >> +static char *create_addr_space_name = >> STRINGIFY(UNW_OBJ(create_addr_space)); >> +static char *search_unwind_table_name = >> STRINGIFY(UNW_OBJ(search_unwind_table)); >> +static char *find_dyn_list_name = STRINGIFY(UNW_OBJ(find_dyn_list)); >> >> I don't understand this. A guess is that UNW_OBJ() is doing something >> evil (use "include/sym-cat.h") to those names and having the array >> (use "static const char [] = ..." and local to libunwind_load) >> makes ones debugging life much easier? If this is the case, can you >> add some commentary? >> > > Yes. The libunwind code is slightly ugly with respect to the fact that > the function names are not aliased with generic names. They all have > platform prefixes so I must spell them out. Function names are > generated automatically using the UNW_OBJ macro. > >> + memset (valuep, 0, DEPRECATED_REGISTER_RAW_SIZE (regnum)); >> >> ? you probably want register_size(). >> > > Yes. > >> +static int >> +libunwind_load (void) >> +{ >> + void *handle; >> + >> + handle = dlopen (LIBUNWIND_SO, RTLD_NOW); >> + if (handle == NULL) >> + return 0; >> >> For thread-db.c I added code that printed out the library that was >> loaded. Should the same be done here? Note that, due to querks with >> the way GDB starts up, the message needs to be delayed - see >> thread-db.c for more details. >> > > IMO, there is little value-add. There are architecture messages that > will print out whether libunwind is being used or not. The problems > with libthread-db don't exist in this case. > >> +const struct frame_unwind * >> +libunwind_frame_sniffer (struct frame_info *next_frame); >> >> can you please write the declaration thus: >> >> const struct frame_unwind *libunwind_frame_sniffer (.... >> >> so that it is consistent with the rest of GDB. >> > > Ok. I copied the declaration from below and forget to rejoin the lines. > > -- Jeff J. > >