Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-10-02 Thread Jakub Jelinek 
On Mon, Oct 02, 2017 at 01:32:01PM +0200, Jakub Jelinek wrote:
> On Mon, Oct 02, 2017 at 01:12:24PM +0200, Martin Liška wrote:
> > Hi.
> > 
> > Currently I see with --with-build-config=bootstrap-ubsan:
> > 
> > /home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/prev-x86_64-pc-linux-gnu/libsanitizer/ubsan/.libs/libubsan.a(elf.o):
> >  In function `backtrace_uncompress_zdebug':
> > /home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/x86_64-pc-linux-gnu/libsanitizer/libbacktrace/../../.././../libsanitizer/libbacktrace/../../libbacktrace/elf.c:2489:
> >  multiple definition of `backtrace_uncompress_zdebug'
> > ../libbacktrace/.libs/libbacktrace.a(elf.o):/home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/libbacktrace/.././../libbacktrace/elf.c:2489:
> >  first defined here
> > collect2: error: ld returned 1 exit status
> > make[3]: *** [Makefile:2904: gcov] Error 1
> > make[3]: *** Waiting for unfinished jobs
> 
> I think this should fix it, I'm going to bootstrap/regtest this now (though
> not --with-build-config=bootstrap-ubsan).
> 
> 2017-10-02  Jakub Jelinek  
> 
>   * libbacktrace/backtrace-rename.h (backtrace_uncompress_zdebug):
>   Define.

Committed as obvious after bootstrap/regtest on x86_64-linux and i686-linux.
> 
> --- libsanitizer/libbacktrace/backtrace-rename.h.jj   2014-09-25 
> 15:01:25.0 +0200
> +++ libsanitizer/libbacktrace/backtrace-rename.h  2017-10-02 
> 13:30:23.096271411 +0200
> @@ -11,6 +11,7 @@
>  #define backtrace_qsort __asan_backtrace_qsort
>  #define backtrace_release_view __asan_backtrace_release_view
>  #define backtrace_syminfo __asan_backtrace_syminfo
> +#define backtrace_uncompress_zdebug __asan_backtrace_uncompress_zdebug
>  #define backtrace_vector_finish __asan_backtrace_vector_finish
>  #define backtrace_vector_grow __asan_backtrace_vector_grow
>  #define backtrace_vector_release __asan_backtrace_vector_release

Jakub

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-10-02 Thread Jakub Jelinek 
On Mon, Oct 02, 2017 at 01:12:24PM +0200, Martin Liška wrote:
> Hi.
> 
> Currently I see with --with-build-config=bootstrap-ubsan:
> 
> /home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/prev-x86_64-pc-linux-gnu/libsanitizer/ubsan/.libs/libubsan.a(elf.o):
>  In function `backtrace_uncompress_zdebug':
> /home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/x86_64-pc-linux-gnu/libsanitizer/libbacktrace/../../.././../libsanitizer/libbacktrace/../../libbacktrace/elf.c:2489:
>  multiple definition of `backtrace_uncompress_zdebug'
> ../libbacktrace/.libs/libbacktrace.a(elf.o):/home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/libbacktrace/.././../libbacktrace/elf.c:2489:
>  first defined here
> collect2: error: ld returned 1 exit status
> make[3]: *** [Makefile:2904: gcov] Error 1
> make[3]: *** Waiting for unfinished jobs

I think this should fix it, I'm going to bootstrap/regtest this now (though
not --with-build-config=bootstrap-ubsan).

2017-10-02  Jakub Jelinek  

* libbacktrace/backtrace-rename.h (backtrace_uncompress_zdebug):
Define.

--- libsanitizer/libbacktrace/backtrace-rename.h.jj 2014-09-25 
15:01:25.0 +0200
+++ libsanitizer/libbacktrace/backtrace-rename.h2017-10-02 
13:30:23.096271411 +0200
@@ -11,6 +11,7 @@
 #define backtrace_qsort __asan_backtrace_qsort
 #define backtrace_release_view __asan_backtrace_release_view
 #define backtrace_syminfo __asan_backtrace_syminfo
+#define backtrace_uncompress_zdebug __asan_backtrace_uncompress_zdebug
 #define backtrace_vector_finish __asan_backtrace_vector_finish
 #define backtrace_vector_grow __asan_backtrace_vector_grow
 #define backtrace_vector_release __asan_backtrace_vector_release


Jakub

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-10-02 Thread Martin Liška 
Hi.

Currently I see with --with-build-config=bootstrap-ubsan:

/home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/prev-x86_64-pc-linux-gnu/libsanitizer/ubsan/.libs/libubsan.a(elf.o):
 In function `backtrace_uncompress_zdebug':
/home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/x86_64-pc-linux-gnu/libsanitizer/libbacktrace/../../.././../libsanitizer/libbacktrace/../../libbacktrace/elf.c:2489:
 multiple definition of `backtrace_uncompress_zdebug'
../libbacktrace/.libs/libbacktrace.a(elf.o):/home/marxin/BIG/buildbot/slave/gcc-master-bootstrap-ubsan/build/builddir/libbacktrace/.././../libbacktrace/elf.c:2489:
 first defined here
collect2: error: ld returned 1 exit status
make[3]: *** [Makefile:2904: gcov] Error 1
make[3]: *** Waiting for unfinished jobs

Thanks,
Martin

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-20 Thread Ian Lance Taylor via gcc-patches 
On Sun, Sep 10, 2017 at 2:11 PM, Ian Lance Taylor  wrote:
> On Sat, Jul 29, 2017 at 1:42 PM, Denis Khalikov
>  wrote:
>>
>> Hello Ian,
>> thanks for review.
>> I've updated the patch, can you please take a look.
>
> Apologies again for the length of time it took to reply.  I've had a
> hard time understanding the patch.  It's quite likely that I don't
> understand how it works, but it seems to pass the same file descriptor
> to process_elf_header twice.  It seems to look for debug files with
> the buildid in places where they will not be found.  It seems to work
> out the file name a second time, even though the file name must
> already be known.  I eventually just wrote my own implementation.
>
> Could you try this patch and see if it works for your cases?  The
> patch is against current mainline.  Thanks.

I have committed this patch as appended.  Bootstrapped and tested on
x86_64-pc-linux-gnu.

Ian

2017-09-20  Ian Lance Taylor  
Denis Khalikov  

PR sanitizer/77631
Support for external debug info.
* elf.c: Include , , .
(S_ISLNK): Define if not defined.
(xstrnlen): Define if strnlen is not available.
(b_elf_note): Define type.
(NT_GNU_BUILD_ID): Define macro.
(elf_crc32, elf_crc32_file): New static functions.
(elf_is_symlink, elf_readlink): New static functions.
(elf_open_debugfile_by_buildid): New static function.
(elf_try_debugfile): New static function.
(elf_find_debugfile_by_debuglink): New static function.
(elf_open_debugfile_by_debuglink): New static function.
(elf_add): Add filename and debuginfo parameters.  Adjust all
callers.  Look for external debug info notes, and try to fetch
debug info from external file.
(struct phdr_data): Add exe_filename field.
(phdr_callback): Pass filename to elf_add.
(backtrace_initialize): Add filename parameter.
* internal.h (backtrace_initialize): Add filename parameter.
* fileline.c (fileline_initialize): Pass filename to
backtrace_initialize.
* pecoff.c (fileline_initialize): Add unused filename parameter.
* unknown.c (fileline_initialize): Likewise.
* xcoff.c (fileline_initialize): Likewise.
* configure.ac: Check for objcopy --add-gnu-debuglink.
* Makefile.am (dtest): New test target.
* configure, Makefile.in: Rebuild.
Index: Makefile.am
===
--- Makefile.am (revision 253025)
+++ Makefile.am (working copy)
@@ -122,6 +122,16 @@ ttest_LDADD = libbacktrace.la
 
 endif HAVE_PTHREAD
 
+if HAVE_OBJCOPY_DEBUGLINK
+
+TESTS += dtest
+
+dtest: btest
+   $(OBJCOPY) --only-keep-debug btest btest.debug
+   $(OBJCOPY) --strip-debug --add-gnu-debuglink=btest.debug btest dtest
+
+endif HAVE_OBJCOPY_DEBUGLINK
+
 endif NATIVE
 
 # We can't use automake's automatic dependency tracking, because it
Index: configure.ac
===
--- configure.ac(revision 253025)
+++ configure.ac(working copy)
@@ -404,6 +404,20 @@ AC_SUBST(PTHREAD_CFLAGS)
 
 AM_CONDITIONAL(HAVE_PTHREAD, test "$libgo_cv_lib_pthread" = yes)
 
+AC_ARG_VAR(OBJCOPY, [location of objcopy])
+AC_CHECK_PROG(OBJCOPY, objcopy, objcopy,)
+AC_CACHE_CHECK([whether objcopy supports debuglink],
+[libbacktrace_cv_objcopy_debuglink],
+[if test -n "${with_target_subdir}"; then
+  libbacktrace_cv_objcopy_debuglink=no
+elif ${OBJCOPY} --add-gnu-debuglink=x /bin/ls /tmp/ls$$; then
+  rm -f /tmp/ls$$
+  libbacktrace_cv_objcopy_debuglink=yes
+else
+  libbacktrace_cv_objcopy_debuglink=no
+fi])
+AM_CONDITIONAL(HAVE_OBJCOPY_DEBUGLINK, test 
"$libbacktrace_cv_objcopy_debuglink" = yes)
+
 AC_CACHE_CHECK([whether tests can run],
   [libbacktrace_cv_sys_native],
   [AC_RUN_IFELSE([AC_LANG_PROGRAM([], [return 0;])],
Index: elf.c
===
--- elf.c   (revision 253025)
+++ elf.c   (working copy)
@@ -32,9 +32,12 @@ POSSIBILITY OF SUCH DAMAGE.  */
 
 #include "config.h"
 
+#include 
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #ifdef HAVE_DL_ITERATE_PHDR
 #include 
@@ -43,6 +46,35 @@ POSSIBILITY OF SUCH DAMAGE.  */
 #include "backtrace.h"
 #include "internal.h"
 
+#ifndef S_ISLNK
+ #ifndef S_IFLNK
+  #define S_IFLNK 012
+ #endif
+ #ifndef S_IFMT
+  #define S_IFMT 017
+ #endif
+ #define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
+#endif
+
+#if !defined(HAVE_DECL_STRNLEN) || !HAVE_DECL_STRNLEN
+
+/* If strnlen is not declared, provide our own version.  */
+
+static size_t
+xstrnlen (const char *s, size_t maxlen)
+{
+  size_t i;
+
+  for (i = 0; i < maxlen; ++i)
+if (s[i] == '\0')
+  break;
+  return i;
+}
+
+#define strnlen xstrnlen
+
+#endif
+
 #ifndef HAVE_DL_ITERATE_PHDR
 
 /* Dummy version of dl_iterate_phdr for systems that don't have it.  */
@@ -105,6 +137,7 @@ dl_iterate_phdr (int (*callback) (struct
 #undef SHT_DYNSYM
 #undef STT_OBJECT
 #undef STT_FUNC
+#undef NT_GNU_BUILD_ID
 
 /* Basic types.  */
 
@@ -224,6

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-20 Thread Ian Lance Taylor via gcc-patches 
On Wed, Sep 20, 2017 at 1:53 AM, Maxim Ostapenko
 wrote:
> Hi Ian,
>
> On 12/09/17 17:05, Ian Lance Taylor via gcc-patches wrote:
>>
>> On Mon, Sep 11, 2017 at 3:06 AM, Denis Khalikov
>>  wrote:
>>>
>>> Thanks for answer.
>>> I understood all points which you mentioned, but can't
>>> find last one

 It seems to work
 out the file name a second time, even though the file name must
 already be known.
>>>
>>> Can you please show me where I've missed that, if you have a time for
>>> that.
>>
>> It's quite possible that I misunderstand the patch.  I was looking at
>> the call to a copy of get_exec_filename in fileline_initialize.
>>
>>> Anyway, your patch works for me.
>>
>> Thanks for testing.  I will commit it later, probably today.
>
>
> As far as I can see, this patch wasn't committed to trunk so far. Did you
> meet some issues with current implementation? Do we need to perform more
> testing here?

No, I've just been busy.  Will get to it soon.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-20 Thread Maxim Ostapenko 

Hi Ian,

On 12/09/17 17:05, Ian Lance Taylor via gcc-patches wrote:

On Mon, Sep 11, 2017 at 3:06 AM, Denis Khalikov
 wrote:

Thanks for answer.
I understood all points which you mentioned, but can't
find last one

It seems to work
out the file name a second time, even though the file name must
already be known.

Can you please show me where I've missed that, if you have a time for that.

It's quite possible that I misunderstand the patch.  I was looking at
the call to a copy of get_exec_filename in fileline_initialize.


Anyway, your patch works for me.

Thanks for testing.  I will commit it later, probably today.


As far as I can see, this patch wasn't committed to trunk so far. Did 
you meet some issues with current implementation? Do we need to perform 
more testing here?


Thanks,
-Maxim



Ian



On 09/11/2017 12:11 AM, Ian Lance Taylor wrote:

On Sat, Jul 29, 2017 at 1:42 PM, Denis Khalikov
 wrote:


Hello Ian,
thanks for review.
I've updated the patch, can you please take a look.


Apologies again for the length of time it took to reply.  I've had a
hard time understanding the patch.  It's quite likely that I don't
understand how it works, but it seems to pass the same file descriptor
to process_elf_header twice.  It seems to look for debug files with
the buildid in places where they will not be found.  It seems to work
out the file name a second time, even though the file name must
already be known.  I eventually just wrote my own implementation.

Could you try this patch and see if it works for your cases?  The
patch is against current mainline.  Thanks.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-12 Thread Ian Lance Taylor via gcc-patches 
On Mon, Sep 11, 2017 at 3:06 AM, Denis Khalikov
 wrote:
> Thanks for answer.
> I understood all points which you mentioned, but can't
> find last one
>> It seems to work
>> out the file name a second time, even though the file name must
>> already be known.
>
> Can you please show me where I've missed that, if you have a time for that.

It's quite possible that I misunderstand the patch.  I was looking at
the call to a copy of get_exec_filename in fileline_initialize.

> Anyway, your patch works for me.

Thanks for testing.  I will commit it later, probably today.

Ian


> On 09/11/2017 12:11 AM, Ian Lance Taylor wrote:
>>
>> On Sat, Jul 29, 2017 at 1:42 PM, Denis Khalikov
>>  wrote:
>>>
>>>
>>> Hello Ian,
>>> thanks for review.
>>> I've updated the patch, can you please take a look.
>>
>>
>> Apologies again for the length of time it took to reply.  I've had a
>> hard time understanding the patch.  It's quite likely that I don't
>> understand how it works, but it seems to pass the same file descriptor
>> to process_elf_header twice.  It seems to look for debug files with
>> the buildid in places where they will not be found.  It seems to work
>> out the file name a second time, even though the file name must
>> already be known.  I eventually just wrote my own implementation.
>>
>> Could you try this patch and see if it works for your cases?  The
>> patch is against current mainline.  Thanks.
>>
>> Ian
>>
>

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-11 Thread Denis Khalikov 

Thanks for answer.
I understood all points which you mentioned, but can't
find last one
> It seems to work
> out the file name a second time, even though the file name must
> already be known.

Can you please show me where I've missed that, if you have a time for that.

Anyway, your patch works for me.
Thanks.

On 09/11/2017 12:11 AM, Ian Lance Taylor wrote:

On Sat, Jul 29, 2017 at 1:42 PM, Denis Khalikov
 wrote:


Hello Ian,
thanks for review.
I've updated the patch, can you please take a look.


Apologies again for the length of time it took to reply.  I've had a
hard time understanding the patch.  It's quite likely that I don't
understand how it works, but it seems to pass the same file descriptor
to process_elf_header twice.  It seems to look for debug files with
the buildid in places where they will not be found.  It seems to work
out the file name a second time, even though the file name must
already be known.  I eventually just wrote my own implementation.

Could you try this patch and see if it works for your cases?  The
patch is against current mainline.  Thanks.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-10 Thread Ian Lance Taylor via gcc-patches 
On Sat, Jul 29, 2017 at 1:42 PM, Denis Khalikov
 wrote:
>
> Hello Ian,
> thanks for review.
> I've updated the patch, can you please take a look.

Apologies again for the length of time it took to reply.  I've had a
hard time understanding the patch.  It's quite likely that I don't
understand how it works, but it seems to pass the same file descriptor
to process_elf_header twice.  It seems to look for debug files with
the buildid in places where they will not be found.  It seems to work
out the file name a second time, even though the file name must
already be known.  I eventually just wrote my own implementation.

Could you try this patch and see if it works for your cases?  The
patch is against current mainline.  Thanks.

Ian
Index: Makefile.am
===
--- Makefile.am (revision 251948)
+++ Makefile.am (working copy)
@@ -122,6 +122,16 @@ ttest_LDADD = libbacktrace.la
 
 endif HAVE_PTHREAD
 
+if HAVE_OBJCOPY_DEBUGLINK
+
+TESTS += dtest
+
+dtest: btest
+   $(OBJCOPY) --only-keep-debug btest btest.debug
+   $(OBJCOPY) --strip-debug --add-gnu-debuglink=btest.debug btest dtest
+
+endif HAVE_OBJCOPY_DEBUGLINK
+
 endif NATIVE
 
 # We can't use automake's automatic dependency tracking, because it
Index: Makefile.in
===
--- Makefile.in (revision 251948)
+++ Makefile.in (working copy)
@@ -86,6 +86,7 @@ target_triplet = @target@
 check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2)
 @NATIVE_TRUE@am__append_1 = btest stest edtest
 @HAVE_PTHREAD_TRUE@@NATIVE_TRUE@am__append_2 = ttest
+@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@am__append_3 = dtest
 subdir = .
 DIST_COMMON = README ChangeLog $(srcdir)/Makefile.in \
$(srcdir)/Makefile.am $(top_srcdir)/configure \
@@ -217,6 +218,7 @@ MAKEINFO = @MAKEINFO@
 MKDIR_P = @MKDIR_P@
 NM = @NM@
 NMEDIT = @NMEDIT@
+OBJCOPY = @OBJCOPY@
 OBJDUMP = @OBJDUMP@
 OBJEXT = @OBJEXT@
 OTOOL = @OTOOL@
@@ -343,7 +345,7 @@ libbacktrace_la_LIBADD = \
$(ALLOC_FILE)
 
 libbacktrace_la_DEPENDENCIES = $(libbacktrace_la_LIBADD)
-TESTS = $(check_PROGRAMS)
+TESTS = $(check_PROGRAMS) $(am__append_3)
 @NATIVE_TRUE@btest_SOURCES = btest.c testlib.c
 @NATIVE_TRUE@btest_CFLAGS = $(AM_CFLAGS) -g -O
 @NATIVE_TRUE@btest_LDADD = libbacktrace.la
@@ -799,6 +801,10 @@ uninstall-am:
 @NATIVE_TRUE@  cat $(srcdir)/edtest2.c > tmp-edtest2_build.c
 @NATIVE_TRUE@  $(SHELL) $(srcdir)/../move-if-change tmp-edtest2_build.c 
edtest2_build.c
 @NATIVE_TRUE@  echo timestamp > $@
+
+@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@dtest: btest
+@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ $(OBJCOPY) --only-keep-debug 
btest btest.debug
+@HAVE_OBJCOPY_DEBUGLINK_TRUE@@NATIVE_TRUE@ $(OBJCOPY) --strip-debug 
--add-gnu-debuglink=btest.debug btest dtest
 alloc.lo: config.h backtrace.h internal.h
 backtrace.lo: config.h backtrace.h internal.h
 btest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h
Index: configure
===
--- configure   (revision 251948)
+++ configure   (working copy)
@@ -604,6 +604,9 @@ LTLIBOBJS
 LIBOBJS
 NATIVE_FALSE
 NATIVE_TRUE
+HAVE_OBJCOPY_DEBUGLINK_FALSE
+HAVE_OBJCOPY_DEBUGLINK_TRUE
+OBJCOPY
 HAVE_PTHREAD_FALSE
 HAVE_PTHREAD_TRUE
 PTHREAD_CFLAGS
@@ -746,7 +749,8 @@ CFLAGS
 LDFLAGS
 LIBS
 CPPFLAGS
-CPP'
+CPP
+OBJCOPY'
 
 
 # Initialize some variables set by options.
@@ -1396,6 +1400,7 @@ Some influential environment variables:
   CPPFLAGSC/C++/Objective C preprocessor flags, e.g. -I if
   you have headers in a nonstandard directory 
   CPP C preprocessor
+  OBJCOPY location of objcopy
 
 Use these variables to override the choices made by `configure' or to help
 it to find libraries and programs with nonstandard names/locations.
@@ -11136,7 +11141,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11139 "configure"
+#line 11144 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -11242,7 +11247,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11245 "configure"
+#line 11250 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -12761,6 +12766,69 @@ else
 fi
 
 
+
+# Extract the first word of "objcopy", so it can be a program name with args.
+set dummy objcopy; ac_word=$2
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
+$as_echo_n "checking for $ac_word... " >&6; }
+if test "${ac_cv_prog_OBJCOPY+set}" = set; then :
+  $as_echo_n "(cached) " >&6
+else
+  if test -n "$OBJCOPY"; then
+  ac_cv_prog_OBJCOPY="$OBJCOPY" # Let the user override the test.
+else
+as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
+for as_dir in $PATH
+do
+  IFS=$as_save_IFS
+  test -z "$as_dir" && as_dir=.
+for ac_exec_ext in '' $ac_executable_extensions; do
+  if { test -f

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-09-05 Thread Denis Khalikov 

Hello,
this is a ping for that patch:
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01958.html

Thanks.

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-08-21 Thread Denis Khalikov 

Hello,
this is a ping for that patch:
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01958.html

Thanks.

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-08-14 Thread Denis Khalikov 

Hello,
this is a ping for that patch
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00022.html

Thanks.

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-07-29 Thread Denis Khalikov 

On 06/29/2017 02:59 AM, Ian Lance Taylor wrote:

On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
 wrote:

Hello everyone,

This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631

Can some one please review attached patch.


Sorry to take so long about this.  It's a lot to look at.




diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 096ceb6..4bd97f3 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog


It's best if you don't include the ChangeLog as part of the patch.
The patch never applies cleanly anyhow.  Just put the ChangeLog entry
in the e-mail or as a separate attachment.  Actually I guess you did
that part, just leave ChangeLog out of the patch.  Thanks.


+AC_CHECK_HEADERS(limits.h)
+
+AC_CHECK_HEADERS(sys/param.h)


May as well put these in a single AC_CHECK_HEADERS.  But actually it
looks like you only want these to determine PATH_MAX, and I don't
think it's worth it.  Just use 4096.


+  LINK = 1,
+  REGULAR = 2


These names, and also DYN, INVALID, and EXEC, are a little too short
and too likely to conflict with some sloppy system header file.  The
enums in this code all use a prefix for each element; do that here
too.


+/* Cast from void pointer to uint32_t.  */
+
+static uint32_t
+getl32 (void *p)
+{
+  char *addr = (char *) p;
+  uint32_t v = 0;
+  v = *((uint32_t *) addr);
+  return v;
+}


The comment here doesn't look right; this isn't a cast.  And the
argument should really be char*.  But more importantly, the name
suggests that you want a little-endian read, but this won't fetch a
little-endian value on a big-endian system.  Either do a proper
little-endian fetch byte by byte, as the DWARF code already does, or
clarify the function name.  I don't know what you actually need.


+/* Function that produce a crc32 value for input buffer.  */


Let's move the CRC32 stuff into a different file.  Add a comment
explaining how the table was generated.


+  static const unsigned long crc32_table[256]


Seems like this should be uint32_t.  In any case not `unsigned long`,
which is 64 bits.  In general the CRC code seems to use `unsigned
long` where I would expect `uint32_t`.


+  unsigned char buffer[8 * 1024];


This code is called from signal handlers and on threads; this array is
too long to put on the stack.  Instead of looping and calling read
like this, use backtrace_get_view.


+/* Get length of the path.  */
+
+static int
+pathlen (const char *buffer)


This function doesn't seem to get the length of the path, it seems to
get the length of the base name.


+  if (offset >= section_size)
+return 0;
+  crc32_debug_link = getl32 (debug_link + offset);


Seems like you should compare offset + 4 to section_size to avoid
running off the end.


+  error_callback (data, "executable file is not ELF", 0);


This and other error messages no longer seem correct, as this function
is now used for files other than the executable file.

It doesn't seem like elf_get_section_by_name should call
process_elf_header, it seems like that will do unnecessary extra work.
For that matter elf_get_section_by_name shouldn't read the section
headers and names each time, we should only do that once.


+static int
+backtrace_readlink


This function should return type_of_file, and the enum should have an
error value.

+  if (buffer[0] == '/')

Use IS_ABSOLUTE_PATH.


+  debug_descriptor
+= backtrace_open_debugfile (descriptor, filename, error_callback,
+ data, state);


If you're going to call this from fileline.c, then the function needs
to be defined in pecoff.c also.  But calling it in fileline.c doesn't
seem right; why doesn't backtrace_initialize call it?

Ian



Hello Ian,
thanks for review.
I've updated the patch, can you please take a look.
Thanks.
From cbecf0d858c50aa00e5445b93fdd968e0778d225 Mon Sep 17 00:00:00 2001
From: Denis Khalikov 
Date: Sat, 29 Jul 2017 22:14:48 +0300
Subject: [PATCH] PR sanitizer/77631

* elf.c (enum type_of_file): New enum.
(enum type_of_elf): New enum.
(enum debug_path): New enum.
(get_uint32): New function.
(get_crc32): New function.
(base_name_len): New function.
(check_sum): New function. Verify sum.
(process_elf_header): New function. Process elf header.
(elf_get_section_by_name): New function. Get section by name.
(backtrace_readlink): New function. Get type of file from filename.
(resolve_realname): New function. Resolve real name if file is link.
(backtrace_resolve_realname): New function. Resolve real name for any
file type.
(search_for_debugfile): New function. Search for debug file in known
paths.
(open_debugfile_by_gnulink): New function. Open debug file with
gnulink.
(hex): New function. Convert to hex.
(get_build_id_name): New function. Generate build-id name.
(open_debugfile_by_build_id): New function. Open debug file with
build-id.

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-07-23 Thread Denis Khalikov 

Hello,
this is a ping for that patch
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00022.html

Thanks.

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-07-09 Thread Denis Khalikov 

Hello,
this is a ping for that patch
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00022.html

Thanks.

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-07-01 Thread Denis Khalikov 

Hello Ian,
thanks for review, I've fixed issues and updated the patch.
Can you please take a look.

Thanks.

On 06/29/2017 02:59 AM, Ian Lance Taylor wrote:

On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
 wrote:

Hello everyone,

This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631

Can some one please review attached patch.


Sorry to take so long about this.  It's a lot to look at.




diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 096ceb6..4bd97f3 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog


It's best if you don't include the ChangeLog as part of the patch.
The patch never applies cleanly anyhow.  Just put the ChangeLog entry
in the e-mail or as a separate attachment.  Actually I guess you did
that part, just leave ChangeLog out of the patch.  Thanks.


+AC_CHECK_HEADERS(limits.h)
+
+AC_CHECK_HEADERS(sys/param.h)


May as well put these in a single AC_CHECK_HEADERS.  But actually it
looks like you only want these to determine PATH_MAX, and I don't
think it's worth it.  Just use 4096.


+  LINK = 1,
+  REGULAR = 2


These names, and also DYN, INVALID, and EXEC, are a little too short
and too likely to conflict with some sloppy system header file.  The
enums in this code all use a prefix for each element; do that here
too.


+/* Cast from void pointer to uint32_t.  */
+
+static uint32_t
+getl32 (void *p)
+{
+  char *addr = (char *) p;
+  uint32_t v = 0;
+  v = *((uint32_t *) addr);
+  return v;
+}


The comment here doesn't look right; this isn't a cast.  And the
argument should really be char*.  But more importantly, the name
suggests that you want a little-endian read, but this won't fetch a
little-endian value on a big-endian system.  Either do a proper
little-endian fetch byte by byte, as the DWARF code already does, or
clarify the function name.  I don't know what you actually need.


+/* Function that produce a crc32 value for input buffer.  */


Let's move the CRC32 stuff into a different file.  Add a comment
explaining how the table was generated.


+  static const unsigned long crc32_table[256]


Seems like this should be uint32_t.  In any case not `unsigned long`,
which is 64 bits.  In general the CRC code seems to use `unsigned
long` where I would expect `uint32_t`.


+  unsigned char buffer[8 * 1024];


This code is called from signal handlers and on threads; this array is
too long to put on the stack.  Instead of looping and calling read
like this, use backtrace_get_view.


+/* Get length of the path.  */
+
+static int
+pathlen (const char *buffer)


This function doesn't seem to get the length of the path, it seems to
get the length of the base name.


+  if (offset >= section_size)
+return 0;
+  crc32_debug_link = getl32 (debug_link + offset);


Seems like you should compare offset + 4 to section_size to avoid
running off the end.


+  error_callback (data, "executable file is not ELF", 0);


This and other error messages no longer seem correct, as this function
is now used for files other than the executable file.

It doesn't seem like elf_get_section_by_name should call
process_elf_header, it seems like that will do unnecessary extra work.
For that matter elf_get_section_by_name shouldn't read the section
headers and names each time, we should only do that once.


+static int
+backtrace_readlink


This function should return type_of_file, and the enum should have an
error value.

+  if (buffer[0] == '/')

Use IS_ABSOLUTE_PATH.


+  debug_descriptor
+= backtrace_open_debugfile (descriptor, filename, error_callback,
+ data, state);


If you're going to call this from fileline.c, then the function needs
to be defined in pecoff.c also.  But calling it in fileline.c doesn't
seem right; why doesn't backtrace_initialize call it?

Ian



From 053f4fab56ed45dec17c1d4c66868678c817acf9 Mon Sep 17 00:00:00 2001
From: Denis Khalikov 
Date: Sat, 1 Jul 2017 23:37:34 +0300
Subject: [PATCH] PR sanitizer/77631

* elf.c (enum type_of_file): New enum.
(enum type_of_elf): New enum.
(enum debug_path): New enum.
(get_uint32): New function.
(get_crc32): New function.
(base_name_len): New function.
(check_sum): New function. Verify sum.
(process_elf_header): New function. Process elf header.
(elf_get_section_by_name): New function. Get section by name.
(backtrace_readlink): New function. Get type of file from filename.
(resolve_realname): New function. Resolve real name if file is link.
(backtrace_resolve_realname): New function. Resolve real name for any
file type.
(search_for_debugfile): New function. Search for debug file in known
paths.
(open_debugfile_by_gnulink): New function. Open debug file with
gnulink.
(hex): New function. Convert to hex.
(get_build_id_name): New function. Generate build-id name.
(open_debugfile_by_build_id): New function. Open debug file with
build-id.

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-06-28 Thread Ian Lance Taylor via gcc-patches 
On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
 wrote:
> Hello everyone,
>
> This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631
>
> Can some one please review attached patch.

Sorry to take so long about this.  It's a lot to look at.



> diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
> index 096ceb6..4bd97f3 100644
> --- a/libbacktrace/ChangeLog
> +++ b/libbacktrace/ChangeLog

It's best if you don't include the ChangeLog as part of the patch.
The patch never applies cleanly anyhow.  Just put the ChangeLog entry
in the e-mail or as a separate attachment.  Actually I guess you did
that part, just leave ChangeLog out of the patch.  Thanks.

> +AC_CHECK_HEADERS(limits.h)
> +
> +AC_CHECK_HEADERS(sys/param.h)

May as well put these in a single AC_CHECK_HEADERS.  But actually it
looks like you only want these to determine PATH_MAX, and I don't
think it's worth it.  Just use 4096.

> +  LINK = 1,
> +  REGULAR = 2

These names, and also DYN, INVALID, and EXEC, are a little too short
and too likely to conflict with some sloppy system header file.  The
enums in this code all use a prefix for each element; do that here
too.

> +/* Cast from void pointer to uint32_t.  */
> +
> +static uint32_t
> +getl32 (void *p)
> +{
> +  char *addr = (char *) p;
> +  uint32_t v = 0;
> +  v = *((uint32_t *) addr);
> +  return v;
> +}

The comment here doesn't look right; this isn't a cast.  And the
argument should really be char*.  But more importantly, the name
suggests that you want a little-endian read, but this won't fetch a
little-endian value on a big-endian system.  Either do a proper
little-endian fetch byte by byte, as the DWARF code already does, or
clarify the function name.  I don't know what you actually need.

> +/* Function that produce a crc32 value for input buffer.  */

Let's move the CRC32 stuff into a different file.  Add a comment
explaining how the table was generated.

> +  static const unsigned long crc32_table[256]

Seems like this should be uint32_t.  In any case not `unsigned long`,
which is 64 bits.  In general the CRC code seems to use `unsigned
long` where I would expect `uint32_t`.

> +  unsigned char buffer[8 * 1024];

This code is called from signal handlers and on threads; this array is
too long to put on the stack.  Instead of looping and calling read
like this, use backtrace_get_view.

> +/* Get length of the path.  */
> +
> +static int
> +pathlen (const char *buffer)

This function doesn't seem to get the length of the path, it seems to
get the length of the base name.

> +  if (offset >= section_size)
> +return 0;
> +  crc32_debug_link = getl32 (debug_link + offset);

Seems like you should compare offset + 4 to section_size to avoid
running off the end.

> +  error_callback (data, "executable file is not ELF", 0);

This and other error messages no longer seem correct, as this function
is now used for files other than the executable file.

It doesn't seem like elf_get_section_by_name should call
process_elf_header, it seems like that will do unnecessary extra work.
For that matter elf_get_section_by_name shouldn't read the section
headers and names each time, we should only do that once.

> +static int
> +backtrace_readlink

This function should return type_of_file, and the enum should have an
error value.

+  if (buffer[0] == '/')

Use IS_ABSOLUTE_PATH.

> +  debug_descriptor
> += backtrace_open_debugfile (descriptor, filename, error_callback,
> + data, state);

If you're going to call this from fileline.c, then the function needs
to be defined in pecoff.c also.  But calling it in fileline.c doesn't
seem right; why doesn't backtrace_initialize call it?

Ian

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-06-25 Thread Denis Khalikov 

Hello everyone,
this is a ping for patch
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01209.html

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-06-19 Thread Denis Khalikov 

Hello Matthias,
thanks for review.

As far as I understood that build-id should look like this:

https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html

"For the “build ID” method, GDB looks in the .build-id subdirectory of 
each one of the global debug directories for a file named 
nn/.debug, where nn are the first 2 hex characters of the build 
ID bit string, and  are the rest of the bit string. (Real build 
ID strings are 32 or more hex characters, not 10.)"


I also check gdb internals, for PR binutils/20876, which you provided.


1926   build_id = get_build_id (abfd);
1927   if (build_id == NULL)
1928 return NULL;
1929
1930   /* Compute the debug pathname corresponding to the build-id.  */
1931   name = bfd_malloc (strlen (".build-id/") + build_id->size * 2 + 2 
+ strlen (".debug"));

1932   if (name == NULL)
1933 {
1934   bfd_set_error (bfd_error_no_memory);
1935   return NULL;
1936 }
1937   n = name;
1938   d = build_id->data;
1939   s = build_id->size;
1940
1941   n += sprintf (n, ".build-id/");
1942   n += sprintf (n, "%02x", (unsigned) *d++); s--;
1943   n += sprintf (n, "/");
1944   while (s--)
1945 n += sprintf (n, "%02x", (unsigned) *d++);
1946   n += sprintf (n, ".debug");
1947


In my patch I do the same, in case we can't use printf functions family, 
because we can't use malloc.


 972   debug_postfix = ".debug";
 973   debug_prefix = ".build-id/";
 ...
 990   memset (build_id_name, 0, *len);
 991   memcpy (build_id_name, debug_prefix, debug_prefix_len);
 992   temp = build_id_name + debug_prefix_len;
 993
 994   *temp++ = hex ((*hash_start & 0xF0) >> 4);
 995   *temp++ = hex (*hash_start & 0x0F);
 996   ++hash_start;
 997   --hash_size;
 998
 999   memcpy (temp, "/", 1);
1000   ++temp;
1001
1002   while (hash_size--)
1003 {
1004   *temp++ = hex ((*hash_start & 0xF0) >> 4);
1005   *temp++ = hex (*hash_start & 0x0F);
1006   ++hash_start;
1007 }

In this case if we have binary with build id:

0x0123456789abcdef0123456789abcdef01234567

For example we can use linker option to specify build-id manually:

-Wl,--build-id=0x0123456789abcdef0123456789abcdef01234567

I expect to find .build-id link to debug file at least at path

/usr/lib/debug/.build-id/01/23456789abcdef0123456789abcdef01234567.debug

May be I'am missing something ?

I also can provide 3 more tests for this patch, but it will
increase patch size in about 1,5 times.

Thanks.

On 06/18/2017 05:08 PM, Matthias Klose wrote:

On 16.06.2017 17:39, Denis Khalikov wrote:

Hello everyone,

This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631

Can some one please review attached patch.


not a full review, but it looks like the system debug files based on build-id's
are not found.  In newer distro releases you find these at

$ ls /usr/lib/debug/.build-id/
00/ 0d/ 1a/ 25/ 2e/ 3a/ 45/ 4d/ 57/ 64/ 6d/ 77/ 82/ 8b/ 94/ a0/ a9/ b7/ c1/ cd/
d6/ e0/ eb/ f6/
01/ 0e/ 1b/ 26/ 2f/ 3b/ 46/ 4e/ 58/ 65/ 6e/ 78/ 83/ 8c/ 95/ a1/ ac/ b9/ c4/ ce/
d7/ e1/ ec/ f7/
02/ 11/ 1c/ 27/ 30/ 3d/ 47/ 50/ 59/ 66/ 6f/ 79/ 84/ 8e/ 96/ a2/ ae/ ba/ c5/ d0/
d8/ e3/ ee/ f9/
03/ 15/ 1d/ 28/ 32/ 3e/ 48/ 51/ 5b/ 67/ 70/ 7a/ 85/ 8f/ 99/ a3/ b0/ bb/ c6/ d1/
d9/ e4/ ef/ fb/
05/ 16/ 1e/ 29/ 35/ 41/ 49/ 52/ 5c/ 68/ 72/ 7b/ 87/ 90/ 9a/ a4/ b1/ bc/ c8/ d2/
db/ e5/ f1/ fc/
08/ 17/ 1f/ 2a/ 36/ 42/ 4a/ 53/ 5e/ 69/ 73/ 7c/ 88/ 91/ 9b/ a5/ b2/ be/ c9/ d3/
dc/ e6/ f2/ fd/
09/ 18/ 20/ 2b/ 37/ 43/ 4b/ 54/ 60/ 6a/ 75/ 7f/ 89/ 92/ 9c/ a6/ b4/ bf/ cb/ d4/
dd/ e7/ f3/ fe/
0a/ 19/ 24/ 2d/ 39/ 44/ 4c/ 55/ 61/ 6b/ 76/ 80/ 8a/ 93/ 9f/ a7/ b5/ c0/ cc/ d5/
de/ e8/ f5/ ff/

the first two bytes of the crc make the sub dir name, the debug file name has
these first two bytes omitted.

$ ls /usr/lib/debug/.build-id/0d
c55467dc9eb81a00c7715a790844e7cf035345.debug

objdump/objcopy in binutils 2.28 has these lookups properly working.

Matthias

[PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-06-16 Thread Denis Khalikov 

Hello everyone,

This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631

Can some one please review attached patch.

Thanks.
From ae74cf3d632b06a91a986e32e3a6c16223767b24 Mon Sep 17 00:00:00 2001
From: Denis Khalikov 
Date: Fri, 16 Jun 2017 12:13:13 +0300
Subject: [PATCH] PR sanitizer/77631

	* Makefile.in: Regenerated.
	* configure.ac: Add searching for limits.h, sys/param.h
	* config.h.in: Regenerated.
	* configure: Regenerated.
	* elf.c (enum type_of_file): New enum.
	(enum type_of_elf): New enum.
	(enum debug_path): New enum.
	(getl32): New function.
	(gnu_debuglink_crc32): New function. Generate crc32 sum.
	(get_crc32): New function.
	(pathlen): New function.
	(check_sum): New function. Verify sum.
	(process_elf_header): New function. Verify elf header.
	(elf_get_section_by_name): New function. Get section by name.
	(backtrace_readlink): New function. Get type of file from filename.
	(resolve_realname): New function. Resolve real name if file is link.
	(backtrace_resolve_realname): New function. Resolve real name for any
	file type.
	(search_for_debugfile): New function. Search for debug file in known
	paths.
	(open_debugfile_by_gnulink): New function. Open debug file with
	gnulink.
	(hex): New function. Convert to hex.
	(get_build_id_name): New function. Generate build-id name.
	(open_debugfile_by_build_id): New function. Open debug file with
	build-id.
	(backtrace_open_debugfile): New function. Open debug file.
	(elf_add): Move code which reads elf header to elf_header_is_valid.
	(phdr_callback): Call backtrace_open_debugfile function for shared
	library.
	* fileline.c (fileline_initialize): Call backtrace_open_debugfile for
	executable.
	* internal.h: Updated.
---
 libbacktrace/ChangeLog|  37 ++
 libbacktrace/Makefile.in  |   2 +-
 libbacktrace/config.h.in  |   6 +
 libbacktrace/configure|  26 ++
 libbacktrace/configure.ac |   4 +
 libbacktrace/elf.c| 949 +-
 libbacktrace/fileline.c   |  13 +-
 libbacktrace/internal.h   |   8 +
 8 files changed, 958 insertions(+), 87 deletions(-)

diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 096ceb6..4bd97f3 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
@@ -1,3 +1,40 @@
+2017-06-16  Denis Khalikov  
+
+	PR sanitizer/77631
+	* Makefile.in: Regenerated.
+	* configure.ac: Add searching for limits.h, sys/param.h
+	* config.h.in: Regenerated.
+	* configure: Regenerated.
+	* elf.c (enum type_of_file): New enum.
+	(enum type_of_elf): New enum.
+	(enum debug_path): New enum.
+	(getl32): New function.
+	(gnu_debuglink_crc32): New function. Generate crc32 sum.
+	(get_crc32): New function.
+	(pathlen): New function.
+	(check_sum): New function. Verify sum.
+	(process_elf_header): New function. Verify elf header.
+	(elf_get_section_by_name): New function. Get section by name.
+	(backtrace_readlink): New function. Get type of file from filename.
+	(resolve_realname): New function. Resolve real name if file is link.
+	(backtrace_resolve_realname): New function. Resolve real name for any
+	file type.
+	(search_for_debugfile): New function. Search for debug file in known
+	paths.
+	(open_debugfile_by_gnulink): New function. Open debug file with
+	gnulink.
+	(hex): New function. Convert to hex.
+	(get_build_id_name): New function. Generate build-id name.
+	(open_debugfile_by_build_id): New function. Open debug file with
+	build-id.
+	(backtrace_open_debugfile): New function. Open debug file.
+	(elf_add): Move code which reads elf header to elf_header_is_valid.
+	(phdr_callback): Call backtrace_open_debugfile function for shared
+	library.
+	* fileline.c (fileline_initialize): Call backtrace_open_debugfile for
+	executable.
+	* internal.h: Updated.
+
 2017-05-02  Release Manager
 
 	* GCC 7.1.0 released.
diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in
index de74b5d..e604b6c 100644
--- a/libbacktrace/Makefile.in
+++ b/libbacktrace/Makefile.in
@@ -16,7 +16,7 @@
 @SET_MAKE@
 
 # Makefile.am -- Backtrace Makefile.
-# Copyright (C) 2012-2016 Free Software Foundation, Inc.
+# Copyright (C) 2012-2017 Free Software Foundation, Inc.
 
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in
index 87cb805..edbd9af 100644
--- a/libbacktrace/config.h.in
+++ b/libbacktrace/config.h.in
@@ -28,6 +28,9 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_INTTYPES_H
 
+/* Define 1 if  is available. */
+#undef HAVE_LIMITS_H
+
 /* Define to 1 if you have the  header file. */
 #undef HAVE_LINK_H
 
@@ -52,6 +55,9 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_SYS_MMAN_H
 
+/* Define 1 if  is available. */
+#undef HAVE_SYS_PARAM_H
+
 /* Define to 1 if you have the  header file. */
 #undef HAVE_SYS_STAT_H
 
diff --git

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-05-25 Thread Denis Khalikov 

Hello everyone,

this is a ping for that patch
https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01462.html

Can someone please review that patch.
Thanks.

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-05-18 Thread Denis Khalikov 

Hello everyone,
i've updated the patch recently for this issue:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631

Can someone please review it.

Thanks.


On 04/13/2017 01:07 AM, Jeff Law wrote:

Given this doesn't look like a regression, I'm going to punt to gcc-8.

jeff
From: Denis Khalikov 
Date: Thu, 18 May 2017 16:04:17 +0300
Subject: [PATCH] 	PR sanitizer/77631

	* Makefile.am: Regenerated.
	* Makefile.in: Regenerated.
	* configure.ac: Add searching for limits.h and sys/param.h
	* config.h.in: Regenerated.
	* configure: Regenerated.
	* elf.c (enum type_of_file): New enum.
	* elf.c (enum_type_of_elf): New enum.
	(enum debug_path): New enum.
	(getl32): New function.
	(gnu_debuglink_crc32): New function. Generate crc32 sum.
	(get_crc32): New function.
	(pathlen): New function.
	(check_sum): New function. Verify sum.
	(process_elf_header): New function. Verify elf header.
	(elf_get_section_by_name): New function. Get section by name.
	(backtrace_readlink): New function. Get type of file from filename.
	(resolve_realname): New function. Resolve real name if file is link.
	(backtrace_resolve_realname): New function. Resolve real name for any
	file type.
	(search_for_debugfile): New function. Search for debug file in known
	paths.
	(open_debugfile_by_gnulink): New function. Open debug file with
	gnulink.
	(hex): New function. Convert to hex.
	(get_build_id_name): New function. Generate build-id name.
	(open_debugfile_by_build_id): New function. Open debug file with
	build-id.
	(backtrace_open_debugfile): New function. Open debug file.
	(elf_add): Move code which reads elf header to elf_header_is_valid.
	(phdr_callback): Call backtrace_open_debugfile function for shared
	library.
	* fileline.c (fileline_initialize): Call backtrace_open_debugfile for
	executable.
	* internal.h: Updated.
---
 libbacktrace/ChangeLog|  38 ++
 libbacktrace/Makefile.in  |   2 +-
 libbacktrace/config.h.in  |   6 +
 libbacktrace/configure|  18 +
 libbacktrace/configure.ac |   6 +
 libbacktrace/elf.c| 977 +-
 libbacktrace/fileline.c   |  11 +-
 libbacktrace/internal.h   |   5 +
 8 files changed, 971 insertions(+), 92 deletions(-)

diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 25cd921..33bbf63 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
@@ -1,3 +1,41 @@
+2017-05-18  Denis Khalikov  
+
+	PR sanitizer/77631
+	* Makefile.am: Regenerated.
+	* Makefile.in: Regenerated.
+	* configure.ac: Add searching for limits.h and sys/param.h
+	* config.h.in: Regenerated.
+	* configure: Regenerated.
+	* elf.c (enum type_of_file): New enum.
+	* elf.c (enum_type_of_elf): New enum.
+	(enum debug_path): New enum.
+	(getl32): New function.
+	(gnu_debuglink_crc32): New function. Generate crc32 sum.
+	(get_crc32): New function.
+	(pathlen): New function.
+	(check_sum): New function. Verify sum.
+	(process_elf_header): New function. Verify elf header.
+	(elf_get_section_by_name): New function. Get section by name.
+	(backtrace_readlink): New function. Get type of file from filename.
+	(resolve_realname): New function. Resolve real name if file is link.
+	(backtrace_resolve_realname): New function. Resolve real name for any
+	file type.
+	(search_for_debugfile): New function. Search for debug file in known
+	paths.
+	(open_debugfile_by_gnulink): New function. Open debug file with
+	gnulink.
+	(hex): New function. Convert to hex.
+	(get_build_id_name): New function. Generate build-id name.
+	(open_debugfile_by_build_id): New function. Open debug file with
+	build-id.
+	(backtrace_open_debugfile): New function. Open debug file.
+	(elf_add): Move code which reads elf header to elf_header_is_valid.
+	(phdr_callback): Call backtrace_open_debugfile function for shared
+	library.
+	* fileline.c (fileline_initialize): Call backtrace_open_debugfile for
+	executable.
+	* internal.h: Updated.
+
 2017-03-08  Sam Thursfield  
 
 	* btest.c (test5): Replace #ifdef guard with 'unused' attribute
diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in
index de74b5d..e604b6c 100644
--- a/libbacktrace/Makefile.in
+++ b/libbacktrace/Makefile.in
@@ -16,7 +16,7 @@
 @SET_MAKE@
 
 # Makefile.am -- Backtrace Makefile.
-# Copyright (C) 2012-2016 Free Software Foundation, Inc.
+# Copyright (C) 2012-2017 Free Software Foundation, Inc.
 
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in
index 87cb805..edbd9af 100644
--- a/libbacktrace/config.h.in
+++ b/libbacktrace/config.h.in
@@ -28,6 +28,9 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_INTTYPES_H
 
+/* Define 1 if  is available. */
+#undef HAVE_LIMITS_H
+
 /* Define to 1 if you have the  header file. */
 #undef HAVE_LINK_H
 
@@ -52,6 +55,9 @@
 /* Define to 1

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-04-12 Thread Jeff Law 

On 03/22/2017 09:28 AM, Denis Khalikov wrote:

Hello everyone,
I've fixed some issues and implemented functionality
to search debug file by build-id.
Can someone please review my patch.

Given this doesn't look like a regression, I'm going to punt to gcc-8.

jeff

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-04-10 Thread Denis Khalikov 

Hello everyone,

this is a ping for

https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01171.html

[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-29 Thread Denis Khalikov 

Hello everyone,

can someone please review that patch
https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01171.html

Thanks.

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-22 Thread Denis Khalikov 

Hello everyone,
I've fixed some issues and implemented functionality
to search debug file by build-id.
Can someone please review my patch.

> As far as I know all the debuglink code is ELF-specific.  I would do
> it all in elf.c.  While reading the sections of the executable, look
> for a debuglink section, and use it if present.  Keep the readlink
> code in posix.c, I suppose.  Apologies if this doesn't make sense.

I've moved backtrace_open_debugfile() function into elf.c.
This function checks debug sections (build-id/debuglink),
and if one of them exist - the function starts to search and open debug 
files.


Also I'll be very appreciated if someone give me advise about issues:
1.
| Skimming over the patch I noticed you duplicate libiberties xcrc32
| functionality.

To verify that debug file is valid, we should
verify crc32 sum, I took that algorithm from gdb, and
put it in the libbacktrace sources. However
this functionality is implemented by libiberty.
In case if libbactrace is being built as a static library,
I can't just link libbacktrace against static libbiberty.
Libtool can do it with objects which has "la" suffix,
but in time when libbacktrace build process happens
libiberty.la already removed.
Should i extract *.o files from the libiberty archive
and create libbacktrace archive with crc32.o from
libiberty or just keep this code in libbacktrace ?


2.
> > +clean_separate:glinktest.debug
> > + rm -f glinktest.debug
> > +
> > +separate: glinktest
> > + if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
> > + then \
> > + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
> > + $(STRIP) glinktest;\
> > + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
> > + fi;
>
> |As far as I know "separate" is not a special thing in automake.  We
> | should always run (and clean) this test if the necessary objcopy
> | option is available.
>
In the attached patch I overrode default target check-TESTS
to use objcopy and strip, before test suite will run.
But the solution to move all test suite code from Makefile.in to 
Makefile.am seems not really good.
I searched for default target, which can be run before test suite logic, 
but can't find it for Makefile.am.

Should I keep current solution, or we have better alternative?

Thanks.


On 03/14/2017 04:49 PM, Ian Lance Taylor wrote:

On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov
 wrote:

Hello everyone, i have a patch for this issue.

List of implemented functionality:

1.Reading .gnu_debuglink section from ELF file:
 a. Reading name of debug info file.
 b. Verifying crc32 sum.

2. Searching for separate debug info file from paths:
 a. /usr/lib/debug/path/to/executable
 b. /path/to/executable
 c. /path/to/executable/.debug

Assumed that debug info file generated by objcopy from binutils.

objcopy --only-keep-debug foo foo.debug
strip -g foo
objcopy --add-gnu-debuglink=foo.debug foo



+clean_separate:glinktest.debug
+ rm -f glinktest.debug
+
+separate: glinktest
+ if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
+ then \
+ $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
+ $(STRIP) glinktest;\
+ $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
+ fi;


As far as I know "separate" is not a special thing in automake.  We
should always run (and clean) this test if the necessary objcopy
option is available.


+glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h


Missing '$'  in "$(INCDIR)".


+/* Return 1 if header is valid and -1 on fail */


This comment does not explain what the function actually does.


+/* Return the pointer to char array with data from .gnudebuglink section 
inside.  */


Line too long, we use 80 column lines.


+unsigned char *
+elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor,
+   backtrace_error_callback error_callback, void *data,
+   int exe, int *gnulink_data_len_out)


This should be static.  I see that you are calling it elsewhere, but
it doesn't make sense to call an "elf" function outside of elf.c.
This library is used on non-ELF systems.


+  /* Look for for the .gnu_debuglink section  */


Period at end of sentence.


   /* To translate PC to file/line when using DWARF, we need to find
- the .debug_info and .debug_line sections.  */
+   the .debug_info and .debug_line sections.  */


Why change the indentation like this?  I think the original was correct.


-  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
-   pd->data, _not_exist);
+  descriptor
+ = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data,
+_does_not_exist, pd->state, 0);
+  if (descriptor < 0)
+  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
+   pd->data, _not_exist);


This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?


+/*Just a simple test copied from btest.c, but in this case we don't have debug
+ * info in

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Denis Khalikov 

Thanks for answer, i got it now.
I will also delete all readlink code. Looks like is no reason
to use it instead just one call of realpath(char*,char*). Binutils
using realpath in the same cases.

On 03/14/2017 07:26 PM, Ian Lance Taylor wrote:

On Tue, Mar 14, 2017 at 7:30 AM, Denis Khalikov
 wrote:

Thanks for review, got all of my mistakes, except one.



-  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
-   pd->data, _not_exist);
+  descriptor
+ = backtrace_open_debugfile (info->dlpi_name, pd->error_callback,
pd->data,
+_does_not_exist, pd->state, 0);
+  if (descriptor < 0)
+  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
+   pd->data, _not_exist);


This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?

Should i move code below

  /* check if debug section does exist */
 debug_link_data
= elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe,
_link_data_len);

from backtrace_open_debugfile .


As far as I know all the debuglink code is ELF-specific.  I would do
it all in elf.c.  While reading the sections of the executable, look
for a debuglink section, and use it if present.  Keep the readlink
code in posix.c, I suppose.  Apologies if this doesn't make sense.

Ian




On 03/14/2017 04:49 PM, Ian Lance Taylor wrote:


On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov
 wrote:


Hello everyone, i have a patch for this issue.

List of implemented functionality:

1.Reading .gnu_debuglink section from ELF file:
 a. Reading name of debug info file.
 b. Verifying crc32 sum.

2. Searching for separate debug info file from paths:
 a. /usr/lib/debug/path/to/executable
 b. /path/to/executable
 c. /path/to/executable/.debug

Assumed that debug info file generated by objcopy from binutils.

objcopy --only-keep-debug foo foo.debug
strip -g foo
objcopy --add-gnu-debuglink=foo.debug foo




+clean_separate:glinktest.debug
+ rm -f glinktest.debug
+
+separate: glinktest
+ if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
+ then \
+ $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
+ $(STRIP) glinktest;\
+ $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
+ fi;



As far as I know "separate" is not a special thing in automake.  We
should always run (and clean) this test if the necessary objcopy
option is available.


+glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h



Missing '$'  in "$(INCDIR)".


+/* Return 1 if header is valid and -1 on fail */



This comment does not explain what the function actually does.


+/* Return the pointer to char array with data from .gnudebuglink section
inside.  */



Line too long, we use 80 column lines.


+unsigned char *
+elf_gnu_debuglink_section (struct backtrace_state *state, int
descriptor,
+   backtrace_error_callback error_callback, void *data,
+   int exe, int *gnulink_data_len_out)



This should be static.  I see that you are calling it elsewhere, but
it doesn't make sense to call an "elf" function outside of elf.c.
This library is used on non-ELF systems.


+  /* Look for for the .gnu_debuglink section  */



Period at end of sentence.


   /* To translate PC to file/line when using DWARF, we need to find
- the .debug_info and .debug_line sections.  */
+   the .debug_info and .debug_line sections.  */



Why change the indentation like this?  I think the original was correct.


-  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
-   pd->data, _not_exist);
+  descriptor
+ = backtrace_open_debugfile (info->dlpi_name, pd->error_callback,
pd->data,
+_does_not_exist, pd->state, 0);
+  if (descriptor < 0)
+  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
+   pd->data, _not_exist);



This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?


+/*Just a simple test copied from btest.c, but in this case we don't have
debug
+ * info in the executable and test should verify that we can read debug
info
+ * from separate file. See Makefile check-TESTS target. */



No leading '*' on subsequent lines of multi-line comments, see existing
code.

I'm not sure I see the point of glinktest.c.  Why don't we just use
btest.c?


+#define MAX_PATH_LEN 4096 /*  from linux/limits.h   */



This library works on systems other than GNU/Linux.  We should
dynamically allocate the buffer.


+  while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1



while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1]))

or just call basename.  I'm not sure why you bother with the count
variable; doesn't full_filename_len - count exactly == len?


+  sign_byte = 0x00;
+  count = 0;
+  while (*(buffer + count) != sign_byte && size > count)
+++count;
+  return count;



This looks like `return strnlen(buffer, size)`.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Ian Lance Taylor via gcc-patches 
On Tue, Mar 14, 2017 at 7:30 AM, Denis Khalikov
 wrote:
> Thanks for review, got all of my mistakes, except one.
>
>
>> -  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
>> -   pd->data, _not_exist);
>> +  descriptor
>> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback,
>> pd->data,
>> +_does_not_exist, pd->state, 0);
>> +  if (descriptor < 0)
>> +  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
>> +   pd->data, _not_exist);
>
> This seems like unnecessary work.  Shouldn't we only try to open the
> debug file if we find a .gnu_debuglink section?
>
> Should i move code below
>
>   /* check if debug section does exist */
>  debug_link_data
> = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe,
> _link_data_len);
>
> from backtrace_open_debugfile .

As far as I know all the debuglink code is ELF-specific.  I would do
it all in elf.c.  While reading the sections of the executable, look
for a debuglink section, and use it if present.  Keep the readlink
code in posix.c, I suppose.  Apologies if this doesn't make sense.

Ian



> On 03/14/2017 04:49 PM, Ian Lance Taylor wrote:
>>
>> On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov
>>  wrote:
>>>
>>> Hello everyone, i have a patch for this issue.
>>>
>>> List of implemented functionality:
>>>
>>> 1.Reading .gnu_debuglink section from ELF file:
>>>  a. Reading name of debug info file.
>>>  b. Verifying crc32 sum.
>>>
>>> 2. Searching for separate debug info file from paths:
>>>  a. /usr/lib/debug/path/to/executable
>>>  b. /path/to/executable
>>>  c. /path/to/executable/.debug
>>>
>>> Assumed that debug info file generated by objcopy from binutils.
>>>
>>> objcopy --only-keep-debug foo foo.debug
>>> strip -g foo
>>> objcopy --add-gnu-debuglink=foo.debug foo
>>
>>
>>> +clean_separate:glinktest.debug
>>> + rm -f glinktest.debug
>>> +
>>> +separate: glinktest
>>> + if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
>>> + then \
>>> + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
>>> + $(STRIP) glinktest;\
>>> + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
>>> + fi;
>>
>>
>> As far as I know "separate" is not a special thing in automake.  We
>> should always run (and clean) this test if the necessary objcopy
>> option is available.
>>
>>> +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h
>>
>>
>> Missing '$'  in "$(INCDIR)".
>>
>>> +/* Return 1 if header is valid and -1 on fail */
>>
>>
>> This comment does not explain what the function actually does.
>>
>>> +/* Return the pointer to char array with data from .gnudebuglink section
>>> inside.  */
>>
>>
>> Line too long, we use 80 column lines.
>>
>>> +unsigned char *
>>> +elf_gnu_debuglink_section (struct backtrace_state *state, int
>>> descriptor,
>>> +   backtrace_error_callback error_callback, void *data,
>>> +   int exe, int *gnulink_data_len_out)
>>
>>
>> This should be static.  I see that you are calling it elsewhere, but
>> it doesn't make sense to call an "elf" function outside of elf.c.
>> This library is used on non-ELF systems.
>>
>>> +  /* Look for for the .gnu_debuglink section  */
>>
>>
>> Period at end of sentence.
>>
>>>/* To translate PC to file/line when using DWARF, we need to find
>>> - the .debug_info and .debug_line sections.  */
>>> +   the .debug_info and .debug_line sections.  */
>>
>>
>> Why change the indentation like this?  I think the original was correct.
>>
>>> -  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
>>> -   pd->data, _not_exist);
>>> +  descriptor
>>> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback,
>>> pd->data,
>>> +_does_not_exist, pd->state, 0);
>>> +  if (descriptor < 0)
>>> +  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
>>> +   pd->data, _not_exist);
>>
>>
>> This seems like unnecessary work.  Shouldn't we only try to open the
>> debug file if we find a .gnu_debuglink section?
>>
>>> +/*Just a simple test copied from btest.c, but in this case we don't have
>>> debug
>>> + * info in the executable and test should verify that we can read debug
>>> info
>>> + * from separate file. See Makefile check-TESTS target. */
>>
>>
>> No leading '*' on subsequent lines of multi-line comments, see existing
>> code.
>>
>> I'm not sure I see the point of glinktest.c.  Why don't we just use
>> btest.c?
>>
>>> +#define MAX_PATH_LEN 4096 /*  from linux/limits.h   */
>>
>>
>> This library works on systems other than GNU/Linux.  We should
>> dynamically allocate the buffer.
>>
>>> +  while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1
>>
>>
>> while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1]))
>>
>> or just call basename.  I'm not sure why you bother with the count
>> variable; doesn't full_filename_len - count exactly == len?
>>
>>> +  sign_byte = 0x00;
>>> +  count = 0;
>>> +  while

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Denis Khalikov 

Thanks for review, got all of my mistakes, except one.


> -  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
> -   pd->data, _not_exist);
> +  descriptor
> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, 
pd->data,

> +_does_not_exist, pd->state, 0);
> +  if (descriptor < 0)
> +  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
> +   pd->data, _not_exist);

This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?

Should i move code below

  /* check if debug section does exist */
 debug_link_data
= elf_gnu_debuglink_section (state, descriptor, error_callbackdata, 
exe, _link_data_len);


from backtrace_open_debugfile .

On 03/14/2017 04:49 PM, Ian Lance Taylor wrote:

On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov
 wrote:

Hello everyone, i have a patch for this issue.

List of implemented functionality:

1.Reading .gnu_debuglink section from ELF file:
 a. Reading name of debug info file.
 b. Verifying crc32 sum.

2. Searching for separate debug info file from paths:
 a. /usr/lib/debug/path/to/executable
 b. /path/to/executable
 c. /path/to/executable/.debug

Assumed that debug info file generated by objcopy from binutils.

objcopy --only-keep-debug foo foo.debug
strip -g foo
objcopy --add-gnu-debuglink=foo.debug foo



+clean_separate:glinktest.debug
+ rm -f glinktest.debug
+
+separate: glinktest
+ if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
+ then \
+ $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
+ $(STRIP) glinktest;\
+ $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
+ fi;


As far as I know "separate" is not a special thing in automake.  We
should always run (and clean) this test if the necessary objcopy
option is available.


+glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h


Missing '$'  in "$(INCDIR)".


+/* Return 1 if header is valid and -1 on fail */


This comment does not explain what the function actually does.


+/* Return the pointer to char array with data from .gnudebuglink section 
inside.  */


Line too long, we use 80 column lines.


+unsigned char *
+elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor,
+   backtrace_error_callback error_callback, void *data,
+   int exe, int *gnulink_data_len_out)


This should be static.  I see that you are calling it elsewhere, but
it doesn't make sense to call an "elf" function outside of elf.c.
This library is used on non-ELF systems.


+  /* Look for for the .gnu_debuglink section  */


Period at end of sentence.


   /* To translate PC to file/line when using DWARF, we need to find
- the .debug_info and .debug_line sections.  */
+   the .debug_info and .debug_line sections.  */


Why change the indentation like this?  I think the original was correct.


-  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
-   pd->data, _not_exist);
+  descriptor
+ = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data,
+_does_not_exist, pd->state, 0);
+  if (descriptor < 0)
+  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
+   pd->data, _not_exist);


This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?


+/*Just a simple test copied from btest.c, but in this case we don't have debug
+ * info in the executable and test should verify that we can read debug info
+ * from separate file. See Makefile check-TESTS target. */


No leading '*' on subsequent lines of multi-line comments, see existing code.

I'm not sure I see the point of glinktest.c.  Why don't we just use btest.c?


+#define MAX_PATH_LEN 4096 /*  from linux/limits.h   */


This library works on systems other than GNU/Linux.  We should
dynamically allocate the buffer.


+  while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1


while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1]))

or just call basename.  I'm not sure why you bother with the count
variable; doesn't full_filename_len - count exactly == len?


+  sign_byte = 0x00;
+  count = 0;
+  while (*(buffer + count) != sign_byte && size > count)
+++count;
+  return count;


This looks like `return strnlen(buffer, size)`.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Ian Lance Taylor via gcc-patches 
On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov
 wrote:
> Hello everyone, i have a patch for this issue.
>
> List of implemented functionality:
>
> 1.Reading .gnu_debuglink section from ELF file:
>  a. Reading name of debug info file.
>  b. Verifying crc32 sum.
>
> 2. Searching for separate debug info file from paths:
>  a. /usr/lib/debug/path/to/executable
>  b. /path/to/executable
>  c. /path/to/executable/.debug
>
> Assumed that debug info file generated by objcopy from binutils.
>
> objcopy --only-keep-debug foo foo.debug
> strip -g foo
> objcopy --add-gnu-debuglink=foo.debug foo

> +clean_separate:glinktest.debug
> + rm -f glinktest.debug
> +
> +separate: glinktest
> + if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
> + then \
> + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
> + $(STRIP) glinktest;\
> + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
> + fi;

As far as I know "separate" is not a special thing in automake.  We
should always run (and clean) this test if the necessary objcopy
option is available.

> +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h

Missing '$'  in "$(INCDIR)".

> +/* Return 1 if header is valid and -1 on fail */

This comment does not explain what the function actually does.

> +/* Return the pointer to char array with data from .gnudebuglink section 
> inside.  */

Line too long, we use 80 column lines.

> +unsigned char *
> +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor,
> +   backtrace_error_callback error_callback, void *data,
> +   int exe, int *gnulink_data_len_out)

This should be static.  I see that you are calling it elsewhere, but
it doesn't make sense to call an "elf" function outside of elf.c.
This library is used on non-ELF systems.

> +  /* Look for for the .gnu_debuglink section  */

Period at end of sentence.

>/* To translate PC to file/line when using DWARF, we need to find
> - the .debug_info and .debug_line sections.  */
> +   the .debug_info and .debug_line sections.  */

Why change the indentation like this?  I think the original was correct.

> -  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
> -   pd->data, _not_exist);
> +  descriptor
> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data,
> +_does_not_exist, pd->state, 0);
> +  if (descriptor < 0)
> +  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
> +   pd->data, _not_exist);

This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?

> +/*Just a simple test copied from btest.c, but in this case we don't have 
> debug
> + * info in the executable and test should verify that we can read debug info
> + * from separate file. See Makefile check-TESTS target. */

No leading '*' on subsequent lines of multi-line comments, see existing code.

I'm not sure I see the point of glinktest.c.  Why don't we just use btest.c?

> +#define MAX_PATH_LEN 4096 /*  from linux/limits.h   */

This library works on systems other than GNU/Linux.  We should
dynamically allocate the buffer.

> +  while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1

while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1]))

or just call basename.  I'm not sure why you bother with the count
variable; doesn't full_filename_len - count exactly == len?

> +  sign_byte = 0x00;
> +  count = 0;
> +  while (*(buffer + count) != sign_byte && size > count)
> +++count;
> +  return count;

This looks like `return strnlen(buffer, size)`.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Denis Khalikov 

Ok, thanks,
i will change patch to use crc32 from libiberty and
also implement searching for debuginfo with build id.
As it was implemented to binutils PR binutils/20876.

On 03/14/2017 04:21 PM, Ian Lance Taylor wrote:

On Tue, Mar 14, 2017 at 3:20 AM, Denis Khalikov
 wrote:

Thanks for review,


Skimming over the patch I noticed you duplicate libiberties xcrc32
functionality.


should i take care about standalone libbacktrace ?
https://github.com/ianlancetaylor/libbacktrace


No, don't worry about it.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Ian Lance Taylor via gcc-patches 
On Tue, Mar 14, 2017 at 3:20 AM, Denis Khalikov
 wrote:
> Thanks for review,
>
>> Skimming over the patch I noticed you duplicate libiberties xcrc32
>> functionality.
>
> should i take care about standalone libbacktrace ?
> https://github.com/ianlancetaylor/libbacktrace

No, don't worry about it.

Ian

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Denis Khalikov 

Thanks for review,

> Skimming over the patch I noticed you duplicate libiberties xcrc32
> functionality.

should i take care about standalone libbacktrace ?
https://github.com/ianlancetaylor/libbacktrace

> Also the additions to posix.c probably belong to dwarf.c and elf.c 
(the feature

> is dwarf + elf specific but proper abstraction / #ifdefing should
> ensure compiling
> also succeeds for non-dwarf / non-elf platforms).

thanks, i will move code to elf.c since it's about reading section from 
elf format.



On 03/14/2017 11:27 AM, Richard Biener wrote:

On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov
 wrote:

Hello everyone, i have a patch for this issue.


Great!


List of implemented functionality:

1.Reading .gnu_debuglink section from ELF file:
 a. Reading name of debug info file.
 b. Verifying crc32 sum.

2. Searching for separate debug info file from paths:
 a. /usr/lib/debug/path/to/executable
 b. /path/to/executable
 c. /path/to/executable/.debug

Assumed that debug info file generated by objcopy from binutils.

objcopy --only-keep-debug foo foo.debug
strip -g foo
objcopy --add-gnu-debuglink=foo.debug foo


Skimming over the patch I noticed you duplicate libiberties xcrc32
functionality.

Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature
is dwarf + elf specific but proper abstraction / #ifdefing should
ensure compiling
also succeeds for non-dwarf / non-elf platforms).

Leaving actual review to the maintainer (CCed).

Thanks,
Richard.

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Matthias Klose 
On 14.03.2017 09:27, Richard Biener wrote:
> On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov
>  wrote:
>> Hello everyone, i have a patch for this issue.
> 
> Great!
> 
>> List of implemented functionality:
>>
>> 1.Reading .gnu_debuglink section from ELF file:
>>  a. Reading name of debug info file.
>>  b. Verifying crc32 sum.
>>
>> 2. Searching for separate debug info file from paths:
>>  a. /usr/lib/debug/path/to/executable
>>  b. /path/to/executable
>>  c. /path/to/executable/.debug
>>
>> Assumed that debug info file generated by objcopy from binutils.
>>
>> objcopy --only-keep-debug foo foo.debug
>> strip -g foo
>> objcopy --add-gnu-debuglink=foo.debug foo

These days using the build-id for separate debug info seems to be the new way of
doing that, so adding support for the build-id method would be useful. See PR
binutils/20876 for how binutils is doing that.

Matthias

Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-14 Thread Richard Biener 
On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov
 wrote:
> Hello everyone, i have a patch for this issue.

Great!

> List of implemented functionality:
>
> 1.Reading .gnu_debuglink section from ELF file:
>  a. Reading name of debug info file.
>  b. Verifying crc32 sum.
>
> 2. Searching for separate debug info file from paths:
>  a. /usr/lib/debug/path/to/executable
>  b. /path/to/executable
>  c. /path/to/executable/.debug
>
> Assumed that debug info file generated by objcopy from binutils.
>
> objcopy --only-keep-debug foo foo.debug
> strip -g foo
> objcopy --add-gnu-debuglink=foo.debug foo

Skimming over the patch I noticed you duplicate libiberties xcrc32
functionality.

Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature
is dwarf + elf specific but proper abstraction / #ifdefing should
ensure compiling
also succeeds for non-dwarf / non-elf platforms).

Leaving actual review to the maintainer (CCed).

Thanks,
Richard.

[PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

2017-03-13 Thread Denis Khalikov 

Hello everyone, i have a patch for this issue.

List of implemented functionality:

1.Reading .gnu_debuglink section from ELF file:
 a. Reading name of debug info file.
 b. Verifying crc32 sum.

2. Searching for separate debug info file from paths:
 a. /usr/lib/debug/path/to/executable
 b. /path/to/executable
 c. /path/to/executable/.debug

Assumed that debug info file generated by objcopy from binutils.

objcopy --only-keep-debug foo foo.debug
strip -g foo
objcopy --add-gnu-debuglink=foo.debug foo

commit 6147ee1a9aeeb748563a8998033f2ce195460162
Author: Denis Khalikov 
Date:   Mon Mar 13 18:55:36 2017 +0300

   PR sanitizer/77631
   * Makefile.am: Update to support glinktest.
   * Makefile.in: Regenerated.
   * configure.ac: Add AC_CHECK_PROG for objcopy.
   * configure: Regenerated.
   * elf.c (elf_header_is_valid): New function. Verify elf header.
   (elf_add): Move code which reads elf header to elf_header_is_valid.
   (elf_gnu_debuglink_section): New function. Read debuglink section from
elf.
   (phdr_callback): Call backtrace_open_debugfile function for shared
library.
   * fileline.c (fileline_initialize): Call backtrace_open_debugfile
function for executable.
   * glinktest.c: New test.
   * internal.h (MAX_PATH_LEN): Defined new variable.
   * posix.c (enum type_of_file): New enum.
   (enum debug_path): New enum.
   (getl32): New function.
   (gnu_debuglink_crc32): New function. Generate crc32 sum.
   (get_crc32): New function.
   (pathlen): New function.
   (check_sum): New function. Verify sum.
   (get_debug_filename_len): New function.
   (backtrace_readlink): New function.
   (backtrace_open_debugfile): New function.

diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 25cd921..cec5d3f 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
@@ -1,3 +1,30 @@
+2017-03-13  Denis Khalikov  
+
+   PR sanitizer/77631
+   * Makefile.am: Update to support glinktest.
+   * Makefile.in: Regenerated.
+   * configure: Add script to find objcopy in the system.
+   * elf.c (elf_header_is_valid): New function. Verify elf header.
+   (elf_add): Move code which reads elf header to elf_header_is_valid.
+   (elf_gnu_debuglink_section): New function. Read debuglink section from
+elf.
+   (phdr_callback): Call backtrace_open_debugfile function for shared
+library.
+   * fileline.c (fileline_initialize): Call backtrace_open_debugfile
+function for executable.
+   * glinktest.c: New test.
+   * internal.h (MAX_PATH_LEN): Defined new variable.
+   * posix.c (enum type_of_file): New enum.
+   (enum debug_path): New enum.
+   (getl32): New function.
+   (gnu_debuglink_crc32): New function. Generate crc32 sum.
+   (get_crc32): New function.
+   (pathlen): New function.
+   (check_sum): New function. Verify sum.
+   (get_debug_filename_len): New function.
+   (backtrace_readlink): New function.
+   (backtrace_open_debugfile): New function.
+
 2017-03-08  Sam Thursfield  
 
* btest.c (test5): Replace #ifdef guard with 'unused' attribute
diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am
index 344dad5..17460a5 100644
--- a/libbacktrace/Makefile.am
+++ b/libbacktrace/Makefile.am
@@ -81,6 +81,17 @@ libbacktrace_la_LIBADD = \
 
 libbacktrace_la_DEPENDENCIES = $(libbacktrace_la_LIBADD)
 
+clean_separate:glinktest.debug
+   rm -f glinktest.debug
+
+separate: glinktest
+   if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
+   then \
+   $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
+   $(STRIP) glinktest;\
+   $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
+   fi;
+
 # Testsuite.
 
 check_PROGRAMS =
@@ -100,6 +111,12 @@ stest_LDADD = libbacktrace.la
 
 check_PROGRAMS += stest
 
+glinktest_SOURCES = glinktest.c
+glinktest_CFLAGS = $(AM_CFLAGS) -g -O
+glinktest_LDADD = libbacktrace.la
+
+check_PROGRAMS += glinktest
+
 endif NATIVE
 
 # We can't use automake's automatic dependency tracking, because it
@@ -134,3 +151,4 @@ sort.lo: config.h backtrace.h internal.h
 stest.lo: config.h backtrace.h internal.h
 state.lo: config.h backtrace.h backtrace-supported.h internal.h
 unknown.lo: config.h backtrace.h internal.h
+glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h
diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in
index de74b5d..354ecc7 100644
--- a/libbacktrace/Makefile.in
+++ b/libbacktrace/Makefile.in
@@ -84,7 +84,7 @@ build_triplet = @build@
 host_triplet = @host@
 target_triplet = @target@
 check_PROGRAMS = $(am__EXEEXT_1)
-@NATIVE_TRUE@am__append_1 = btest stest
+@NATIVE_TRUE@am__append_1 = btest stest glinktest
 subdir = .
 DIST_COMMON = README ChangeLog