This is the mail archive of the gdb-patches@sources.redhat.com 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]

Re: RFA: libunwind basic support


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?

- who owns it?

- what are its licence terms, and are they GPL compatible?

- is it considered a "system library" (like libc, or libthread_db)?

- have you used, or refered to David Mossberger's [sp] old libunwind patch (it was eventually contributed to the FSF).

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.

2003-10-15 Jeff Johnston <jjohnstn@redhat.com>

    * 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.

+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?


+#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 <name>[] = ..." and local to libunwind_load) makes ones debugging life much easier? If this is the case, can you add some commentary?

+ memset (valuep, 0, DEPRECATED_REGISTER_RAW_SIZE (regnum));

? you probably want register_size().

+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.

+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.

Andrew



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]