Re: [Patch]: libbacktrace - add support of PE/COFF
On 28 May 2015, at 17:14, Ian Lance Taylor i...@google.com wrote: On Thu, May 28, 2015 at 5:01 AM, Tristan Gingold ging...@adacore.com wrote: On 28 May 2015, at 02:26, Ian Lance Taylor i...@google.com wrote: The #include windows.h will break cross-compilers. It's not OK for trunk until that is fixed. I am confused by this comment, for two reasons: - I don’t see how that would break cross-compilers. Cross compilers hosted on windows are not impacted by this include when the library is used for the tools. When then backtrace library is used for the target, pecoff is not used unless the target is windows. So I don’t see a case where the include breaks cross-compilers. The way you have written the code, I'm fairly sure that it will be compiled for an i386-coff target. And the only coff target supported is djgpp, right ? - If the case exists, I don’t see how to implement backtrace within shared libraries: I need a windows specific function to get the list of DLL. I would be OK with a #include windows.h that is conditional on something that indicates that the host (from the point of view of libbacktrace) really is Windows. Is it ok to test if the _WIN32 macro is defined (like in libiberty) ? The new version of the patch is OK. Thanks, now committed. Tristan.
Re: [Patch]: libbacktrace - add support of PE/COFF
On Fri, May 29, 2015 at 1:43 AM, Tristan Gingold ging...@adacore.com wrote: On 28 May 2015, at 17:14, Ian Lance Taylor i...@google.com wrote: The way you have written the code, I'm fairly sure that it will be compiled for an i386-coff target. And the only coff target supported is djgpp, right ? Really? I didn't know that we had dropped COFF support. That is good to hear. I would be OK with a #include windows.h that is conditional on something that indicates that the host (from the point of view of libbacktrace) really is Windows. Is it ok to test if the _WIN32 macro is defined (like in libiberty) ? Sure. Ian
Re: [Patch]: libbacktrace - add support of PE/COFF
On 27 May 2015, at 15:36, Jeff Law l...@redhat.com wrote:
+static int
+coff_is_symbol (const b_coff_internal_symbol *isym)
+{
+ return isym-type == 0x20 isym-sec 0;
+}
You probably want const or enum so that you can have a symbolic name rather
than 0x20 here. It also seems like the name ought to better indicate it's
testing for function symbols.
Yes, this is now changed.
It's a given that you know COFF specifics better than I ever did, so I'm
comfortable assuming you got the COFF specifics right.
The overall structure of elf.c coff.c is the same with code templates that
are very similar, except they work on different underlying types. Presumably
there wasn't a good way to factor any of the generic looking bits out? And
no, I'm not requesting you rewrite all this in BFD :-)
The dummy callback could indeed be easily shared. For the remaining, that’s
not so simple given the types. Maybe we can create a ‘C class’ for symbol
infos.
Tristan.
Re: [Patch]: libbacktrace - add support of PE/COFF
On 28 May 2015, at 02:26, Ian Lance Taylor i...@google.com wrote: The #include windows.h will break cross-compilers. It's not OK for trunk until that is fixed. I am confused by this comment, for two reasons: - I don’t see how that would break cross-compilers. Cross compilers hosted on windows are not impacted by this include when the library is used for the tools. When then backtrace library is used for the target, pecoff is not used unless the target is windows. So I don’t see a case where the include breaks cross-compilers. - If the case exists, I don’t see how to implement backtrace within shared libraries: I need a windows specific function to get the list of DLL. Tristan.
Re: [Patch]: libbacktrace - add support of PE/COFF
On Thu, May 28, 2015 at 5:01 AM, Tristan Gingold ging...@adacore.com wrote: On 28 May 2015, at 02:26, Ian Lance Taylor i...@google.com wrote: The #include windows.h will break cross-compilers. It's not OK for trunk until that is fixed. I am confused by this comment, for two reasons: - I don’t see how that would break cross-compilers. Cross compilers hosted on windows are not impacted by this include when the library is used for the tools. When then backtrace library is used for the target, pecoff is not used unless the target is windows. So I don’t see a case where the include breaks cross-compilers. The way you have written the code, I'm fairly sure that it will be compiled for an i386-coff target. - If the case exists, I don’t see how to implement backtrace within shared libraries: I need a windows specific function to get the list of DLL. I would be OK with a #include windows.h that is conditional on something that indicates that the host (from the point of view of libbacktrace) really is Windows. The new version of the patch is OK. Thanks. Ian
Re: [Patch]: libbacktrace - add support of PE/COFF
On 05/21/2015 06:41 AM, Tristan Gingold wrote:
Hello,
this patch adds basic support to libbacktrace for PE32 and PE32+ (Windows and
Windows64 object formats).
Support is ‘basic’ because neither DLL nor PIE (if that exists) are handled.
Furthermore, there is no windows versions of mmapio.c and mmap.c
Finally, I have disabled the support of data symbols for PE because I wasn’t
able to pass ‘make check’ with that: symbol ‘_global’ is at the same address as
a symbol defined by the linker and I haven’t found any way to discard the
latter. As I think data symbol support isn’t a required feature, I have
preferred to disable that feature on PE.
The new file, pecoff.c, mostly follows the structure of elf.c
Tested on both windows and windows64.
No regression on Gnu/Linux x86.
Tristan.
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.
+
+/* 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;
+}
You probably want const or enum so that you can have a symbolic name
rather than 0x20 here. It also seems like the name ought to better
indicate it's testing for function symbols.
It's a given that you know COFF specifics better than I ever did, so
I'm comfortable assuming you got the COFF specifics right.
The overall structure of elf.c coff.c is the same with code templates
that are very similar, except they work on different underlying types.
Presumably there wasn't a good way to factor any of the generic
looking bits out? And no, I'm not requesting you rewrite all this in
BFD :-)
OK for the trunk. Any future issues with the coff bits I'll send your way.
jeff
Re: [Patch]: libbacktrace - add support of PE/COFF
On Wed, May 27, 2015 at 6:36 AM, Jeff Law l...@redhat.com wrote:
On 05/21/2015 06:41 AM, Tristan Gingold wrote:
Hello,
this patch adds basic support to libbacktrace for PE32 and PE32+ (Windows
and Windows64 object formats).
Support is ‘basic’ because neither DLL nor PIE (if that exists) are
handled. Furthermore, there is no windows versions of mmapio.c and mmap.c
Finally, I have disabled the support of data symbols for PE because I
wasn’t able to pass ‘make check’ with that: symbol ‘_global’ is at the same
address as a symbol defined by the linker and I haven’t found any way to
discard the latter. As I think data symbol support isn’t a required
feature, I have preferred to disable that feature on PE.
The new file, pecoff.c, mostly follows the structure of elf.c
Tested on both windows and windows64.
No regression on Gnu/Linux x86.
Tristan.
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.
+
+/* 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;
+}
You probably want const or enum so that you can have a symbolic name rather
than 0x20 here. It also seems like the name ought to better indicate it's
testing for function symbols.
It's a given that you know COFF specifics better than I ever did, so I'm
comfortable assuming you got the COFF specifics right.
The overall structure of elf.c coff.c is the same with code templates that
are very similar, except they work on different underlying types.
Presumably there wasn't a good way to factor any of the generic looking bits
out? And no, I'm not requesting you rewrite all this in BFD :-)
OK for the trunk. Any future issues with the coff bits I'll send your way.
The #include windows.h will break cross-compilers. It's not OK for
trunk until that is fixed.
Ian
Re: [Patch]: libbacktrace - add support of PE/COFF
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
[Patch]: libbacktrace - add support of PE/COFF
Hello, this patch adds basic support to libbacktrace for PE32 and PE32+ (Windows and Windows64 object formats). Support is ‘basic’ because neither DLL nor PIE (if that exists) are handled. Furthermore, there is no windows versions of mmapio.c and mmap.c Finally, I have disabled the support of data symbols for PE because I wasn’t able to pass ‘make check’ with that: symbol ‘_global’ is at the same address as a symbol defined by the linker and I haven’t found any way to discard the latter. As I think data symbol support isn’t a required feature, I have preferred to disable that feature on PE. The new file, pecoff.c, mostly follows the structure of elf.c Tested on both windows and windows64. No regression on Gnu/Linux x86. Tristan. 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. commit ac17f650356728fc07121c71213401e1e159df2f Author: Tristan Gingold ging...@adacore.com Date: Thu May 21 14:29:44 2015 +0200 Add support for PE/COFF to libbacktrace. diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog index c6604d9..139521a 100644 --- a/libbacktrace/ChangeLog +++ b/libbacktrace/ChangeLog @@ -1,3 +1,17 @@ +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. + 2015-05-13 Michael Haubenwallner michael.haubenwall...@ssi-schaefer.com * Makefile.in: Regenerated with automake-1.11.6. diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am index a93b82a..c5f0dcb 100644 --- a/libbacktrace/Makefile.am +++ b/libbacktrace/Makefile.am @@ -56,6 +56,7 @@ BACKTRACE_FILES = \ FORMAT_FILES = \ elf.c \ + pecoff.c \ unknown.c VIEW_FILES = \ @@ -124,6 +125,7 @@ fileline.lo: config.h backtrace.h internal.h mmap.lo: config.h backtrace.h internal.h mmapio.lo: config.h backtrace.h internal.h nounwind.lo: config.h internal.h +pecoff.lo: config.h backtrace.h internal.h posix.lo: config.h backtrace.h internal.h print.lo: config.h backtrace.h internal.h read.lo: config.h backtrace.h internal.h diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index a949f29..b434d76e 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -299,6 +299,7 @@ BACKTRACE_FILES = \ FORMAT_FILES = \ elf.c \ + pecoff.c \ unknown.c VIEW_FILES = \ @@ -753,6 +754,7 @@ fileline.lo: config.h backtrace.h internal.h mmap.lo: config.h backtrace.h internal.h mmapio.lo: config.h backtrace.h internal.h nounwind.lo: config.h internal.h +pecoff.lo: config.h backtrace.h internal.h posix.lo: config.h backtrace.h internal.h print.lo: config.h backtrace.h internal.h read.lo: config.h backtrace.h internal.h 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@ diff --git a/libbacktrace/btest.c b/libbacktrace/btest.c index 9424a92..9821e34 100644 --- a/libbacktrace/btest.c +++ b/libbacktrace/btest.c @@ -616,6 +616,8 @@ f33 (int f1line, int f2line) return failures; } +#if BACKTRACE_SUPPORTS_DATA + int global = 1; static int @@ -684,6 +686,8 @@ test5 (void) return failures; } +#endif /* BACKTRACE_SUPPORTS_DATA */ + static void error_callback_create (void *data ATTRIBUTE_UNUSED, const char *msg, int errnum) @@ -708,8 +712,10 @@ main (int argc ATTRIBUTE_UNUSED, char **argv) test2 (); test3 (); test4 (); +#if BACKTRACE_SUPPORTS_DATA test5 (); #endif +#endif exit (failures ? EXIT_FAILURE : EXIT_SUCCESS); } diff --git a/libbacktrace/configure b/libbacktrace/configure index fa81659..19418c9 100755 --- a/libbacktrace/configure +++ b/libbacktrace/configure @@
