On Thu, May 21, 2015 at 5:41 AM, Tristan Gingold <ging...@adacore.com> wrote:
>
> 2015-05-21 Tristan Gingold <ging...@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
