Re: [Libunwind-devel] [PATCH 3/5] Rework the lookup of the ARM specific unwind entry for local unwinding

[Ken Werner]

Hi Arun,

thanks for your comments!

On 08/06/2011 09:54 PM, Arun Sharma wrote:
> On Wed, Aug 3, 2011 at 5:12 AM, Ken Werner<address@hidden>  wrote:
> > Use the dh_iterate_phdr callback in Gfind_proc_info-lsb.c to find the ARM
> > specific unwind tables rather than doing it on our own at the unw_init_local.
>
> You probably meant dl_iterate_phdr?

Yes, exactly - good catch.
I'll fix the commit comment.

> > index 6c94f05..584f392 100644
> > --- a/include/libunwind-dynamic.h
> > +++ b/include/libunwind-dynamic.h
> > @@ -75,7 +75,8 @@ typedef enum
> >    {
> >      UNW_INFO_FORMAT_DYNAMIC,           /* unw_dyn_proc_info_t */
> >      UNW_INFO_FORMAT_TABLE,             /* unw_dyn_table_t */
> > -    UNW_INFO_FORMAT_REMOTE_TABLE       /* unw_dyn_remote_table_t */
> > +    UNW_INFO_FORMAT_REMOTE_TABLE,      /* unw_dyn_remote_table_t */
> > +    UNW_INFO_FORMAT_ARM_EXIDX          /* ARM specific unwind info */
>
> Could you add a reference to the doc that describes ARM_EXIDX some place?
> Not sure if this is the right one.
> 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044d/IHI0044D_aaelf.pdf

There is a document dedicated to the ARM exception handling ABI. What 
about adding someting like the comment below to src/arm/Gex_tables.c?

/* This file contains functionality for parsing and interpreting the ARM
specific unwind information.  Documentation about the exception handling
ABI for the ARM architecture can be found at:
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038a/IHI0038A_ehabi.pdf
*/

> > diff --git a/src/dwarf/Gfind_proc_info-lsb.c b/src/dwarf/Gfind_proc_info-lsb.c
> > index d65342c..5c1931d 100644
> > --- a/src/dwarf/Gfind_proc_info-lsb.c
> > +++ b/src/dwarf/Gfind_proc_info-lsb.c
> > @@ -57,6 +57,9 @@ struct callback_data
> >      int single_fde;            /* did we find a single FDE? (vs. a table) */
> >      unw_dyn_info_t di;         /* table info (if single_fde is false) */
> >      unw_dyn_info_t di_debug;   /* additional table info for .debug_frame */
> > +#if UNW_TARGET_ARM
> > +    unw_dyn_info_t di_arm;     /* additional table info for .ARM.exidx */
> > +#endif
> >    };
>
> I'm a bit concerned about the arch-specific stuff getting added to
> callback_data. What are the minimal arch specific hooks you need to
> get this to work without an #ifdef UNW_TARGET_ARM? Something along the
> lines of:
>
> unw_dyn_info_t *di_tdep;

I'm not sure I can follow. I agree that these ifdefs aren't very nice 
but what are the alternatives given the design of the libunwind API? The 
libunwind ARM backend could define its own tdep_find_proc_info but 
almost all of the code would be identical. It would walk the program 
headers using dl_iterate_phdr to collect the unw_dyn_info_t information 
and then call tdep_search_unwind_table to fill unw_proc_info_t.

> > @@ -783,14 +809,21 @@ dwarf_find_proc_info (unw_addr_space_t as, unw_word_t ip,
> >
> >    /* search the table: */
> >    if (cb_data.di.format != -1)
> > -    ret = dwarf_search_unwind_table (as, ip,&cb_data.di,
> > -                                     pi, need_unwind_info, arg);
> > +    ret = tdep_search_unwind_table (as, ip,&cb_data.di,
> > +                                   pi, need_unwind_info, arg);
> >    else
> >      ret = -UNW_ENOINFO;
> >
> > +#if UNW_TARGET_ARM
> > +  if (ret == -UNW_ENOINFO&&  cb_data.di_arm.format != -1)
> > +    ret = tdep_search_unwind_table (as, ip,&cb_data.di_arm, pi,
> > +                                   need_unwind_info, arg);
> > +#endif
> > +
> >    if (ret == -UNW_ENOINFO&&  cb_data.di_debug.format != -1)
> > -    ret = dwarf_search_unwind_table (as, ip,&cb_data.di_debug, pi,
> > -                                    need_unwind_info, arg);
> > +    ret = tdep_search_unwind_table (as, ip,&cb_data.di_debug, pi,
> > +                                   need_unwind_info, arg);
> > +
> >    return ret;
>
> I'd like to minimize the number of calls between dwarf_find_proc_info
> and target dependent code. Two sounds better than three.

My understanding of what tdep_search_unwind_table does is that it gets 
the IP and unw_dyn_info as its input and does whatever is necessary to 
find the corresponding unwind info to be stored in unw_proc_info. Since 
there is no .eh_frame information on ARM the cb_data.di.format will be 
-1, no call will be made to tdep_search_unwind_table which leaves us 
with a max of 2 calls for ARM as well.

Thanks,
Ken