Re: [Patch]: libbacktrace - add support of PE/COFF

[Ian Lance Taylor <iant at google dot com>]

On Thu, May 21, 2015 at 5:41 AM, Tristan Gingold <gingold@adacore.com> wrote:
>
> 2015-05-21  Tristan Gingold  <gingold@adacore.com>
>
>         * pecoff.c: New file.
>         * Makefile.am (FORMAT_FILES): Add pecoff.c and dependencies.
>         * Makefile.in: Regenerate.
>         * filetype.awk: Detect pecoff.
>         * configure.ac: Define BACKTRACE_SUPPORTS_DATA on elf platforms.
>         Add pecoff.
>         * btest.c (test5): Test enabled only if BACKTRACE_SUPPORTS_DATA is
>         true.
>         * backtrace-supported.h.in (BACKTRACE_SUPPORTS_DATA): Define.
>         * configure: Regenerate.
>         * pecoff.c: New file.


Thanks for working on this.


> diff --git a/libbacktrace/backtrace-supported.h.in b/libbacktrace/backtrace-supported.h.in
> index 5115ce1..4574635 100644
> --- a/libbacktrace/backtrace-supported.h.in
> +++ b/libbacktrace/backtrace-supported.h.in
> @@ -59,3 +59,8 @@ POSSIBILITY OF SUCH DAMAGE.  */
>     as 0.  */
>
>  #define BACKTRACE_SUPPORTS_THREADS @BACKTRACE_SUPPORTS_THREADS@
> +
> +/* BACKTRACE_SUPPORTS_DATA will be #defined'd as 1 if the backtrace library
> +   also handles data symbols, 0 if not.  */
> +
> +#define BACKTRACE_SUPPORTS_DATA @BACKTRACE_SUPPORTS_DATA@

End users are expected to read and understand this file, so I think
this comment is too obscure.  I suggest:

BACKTRACE_SUPPORTS_DATA will be #define'd as 1 if backtrace_syminfo
will work for variables.  It will always work for functions.


I would have thought you could distinguish relevant symbols using the
storage class and type.  But perhaps not.



> diff --git a/libbacktrace/filetype.awk b/libbacktrace/filetype.awk
> index 0a656f7..37099ad 100644
> --- a/libbacktrace/filetype.awk
> +++ b/libbacktrace/filetype.awk
> @@ -1,3 +1,4 @@
>  # An awk script to determine the type of a file.
>  /\177ELF\001/ { if (NR == 1) { print "elf32"; exit } }
>  /\177ELF\002/ { if (NR == 1) { print "elf64"; exit } }
> +/\114\001/    { if (NR == 1) { print "pecoff"; exit } }

That works for 32-bit, but I think not for 64-bit.  For 64-bit I would
expect \144\206.


> +#include <windows.h>

Where is <windows.h> going to come from when building a
cross-compiler?  I think this needs to be removed.  I see that you
define the structs yourself, as you should, so why do you need
<windows.h>?



> +/* Read a potentially unaligned 2 byte word at P, using native endianness.  */

Is there really ever a case where a 2 byte word might be misaligned?


> +/* Return true iff SYM is a defined symbol for a function.  Data symbols
> +   are discarded because they aren't easily identified.  */
> +
> +static int
> +coff_is_symbol (const b_coff_internal_symbol *isym)
> +{
> +  return isym->type == 0x20 && isym->sec > 0;
> +}

Is this really right?  This seems to test for DT_FCN set, but won't a
function returning, say, int, have type 0x24 (DT_FCN << N_TBSHFT) | T_INT?

Also, the name isn't right--this is coff_is_function_symbol.


> +      if (coff_expand_symbol (&isym, asym, sects_num, strtab, strtab_size) < 0)
> +       {
> +         error_callback (data, "invalid coff symbol", 0);
> +         return 0;
> +       }

That's not a very useful error message--can you be more specific?


> +  /* Allocate memory for symbols are strings.  */

Comment looks wrong--omit "are"?


Ian