RFA: libunwind basic support

[J. Johnston]

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

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 <name>[] = ..." 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.