This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] libdw: Make dwarf_getfuncs find all (defining) DW_TAG_subprogram DIEs.


On Fri, 2013-10-18 at 16:21 -0700, Josh Stone wrote:
> On 10/10/2013 02:48 AM, Mark Wielaard wrote:
> > Unfortunately you seem to be right, even if that is somewhat unlikely.
> > [...]
> > Please check the attached.
> 
> AFAICS, the tests you added don't even have any subprograms in their alt
> files, let alone colliding DIE offsets.  I grant you, it's hard to craft
> such a collision, but I managed it. :P

Impressive and evil! :) Yeah, I admit I considered it only a theoretical
problem, because I couldn't create such a situation, but you proved me
wrong again...

> See attached.  I compiled these like so:
> 
> $ gcc -g -O2 test-offset-loop.c -o test-offset-loop
> $ cp test-offset-loop test-offset-loop2
> $ dwz test-offset-loop test-offset-loop2 -m test-offset-loop.alt
> 
> I didn't include loop2 in the tar, as it's just a copy to kick dwz into
> action.  The main file has subprograms [2e] "padding" and [63] "main",
> where the alt has [5a] "get_errno" and [63] "is_error".
> 
> Without your patch, allfcts lists get_errno, is_error, padding, main,
> but then main's [63] makes it revert as if it were back at is_error, so
> it repeats padding, main, padding, main, ...
> 
> With your addr patch, it terminates correctly.
> 
> I'm not sure how to add this test though, because while testrun_compare
> could see success just fine, it can't handle the unpatched failing case
> with infinite output.  Feel free to take and commit my test files if you
> can figure this out.

We just have to truncate the output before running the compare to make
if fail fast (without the patch). Added your test as attached and pushed
to master.

Thanks,

Mark
>From e6a9bb8c63adaeaceafc50301b085121c962b631 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Thu, 10 Oct 2013 11:40:12 +0200
Subject: [PATCH] libdw: Handle dwz multi files correctly in dwarf_getfuncs.

Don't use DIE offsets, but use their addresses to make sure they are unique.
Include test cases where main and alt file have subprograms at same offsets.

Reported-by: Josh Stone <jistone@redhat.com>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog                |    8 +++++
 libdw/dwarf_getfuncs.c         |   26 +++++++++---------
 tests/ChangeLog                |   10 +++++++
 tests/Makefile.am              |    4 ++-
 tests/run-allfcts-multi.sh     |   56 ++++++++++++++++++++++++++++++++++++++++
 tests/test-offset-loop.alt.bz2 |  Bin 0 -> 685 bytes
 tests/test-offset-loop.bz2     |  Bin 0 -> 3062 bytes
 7 files changed, 90 insertions(+), 14 deletions(-)
 create mode 100755 tests/run-allfcts-multi.sh
 create mode 100644 tests/test-offset-loop.alt.bz2
 create mode 100755 tests/test-offset-loop.bz2

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 960f5aa..1bf1de9 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,11 @@
+2013-10-10  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf_getfuncs.c (struct visitor_info): Rename start_offset to
+	start_addr and rename last_offset to last_addr. Now both void *.
+	(tree_visitor): Use start_add and die_addr instead of start_offset
+	and die_offset.
+	(dwarf_getfuncs): Use last_addr instead of last_offset.
+
 2013-10-06  Mark Wielaard  <mjw@redhat.com>
 
 	* cfi.c (execute_cfi): Make sure DW_CFA_expression and
diff --git a/libdw/dwarf_getfuncs.c b/libdw/dwarf_getfuncs.c
index 87e0341..82894c9 100644
--- a/libdw/dwarf_getfuncs.c
+++ b/libdw/dwarf_getfuncs.c
@@ -43,11 +43,11 @@ struct visitor_info
   /* The user arg value to dwarf_getfuncs.  */
   void *arg;
 
-  /* The DIE offset where to (re)start the search.  Zero for all.  */
-  Dwarf_Off start_offset;
+  /* Addr of the DIE offset where to (re)start the search.  Zero for all.  */
+  void *start_addr;
 
-  /* Last subprogram DIE offset seen.  */
-  Dwarf_Off last_offset;
+  /* Last subprogram DIE addr seen.  */
+  void *last_addr;
 
   /* The CU only contains C functions.  Allows pruning of most subtrees.  */
   bool c_cu;
@@ -59,8 +59,8 @@ tree_visitor (unsigned int depth __attribute__ ((unused)),
 {
   struct visitor_info *const v = arg;
   Dwarf_Die *die = &chain->die;
-  Dwarf_Off start_offset = v->start_offset;
-  Dwarf_Off die_offset = INTUSE(dwarf_dieoffset) (die);
+  void *start_addr = v->start_addr;
+  void *die_addr = die->addr;
 
   /* Pure C CUs can only contain defining subprogram DIEs as direct
      children of the CU DIE or as nested function inside a normal C
@@ -75,11 +75,11 @@ tree_visitor (unsigned int depth __attribute__ ((unused)),
       return DWARF_CB_OK;
     }
 
-  /* Skip all DIEs till we found the (re)start offset.  */
-  if (start_offset != 0)
+  /* Skip all DIEs till we found the (re)start addr.  */
+  if (start_addr != NULL)
     {
-      if (die_offset == start_offset)
-	v->start_offset = 0;
+      if (die_addr == start_addr)
+	v->start_addr = NULL;
       return DWARF_CB_OK;
     }
 
@@ -88,7 +88,7 @@ tree_visitor (unsigned int depth __attribute__ ((unused)),
       || INTUSE(dwarf_hasattr) (die, DW_AT_declaration))
     return DWARF_CB_OK;
 
-  v->last_offset = die_offset;
+  v->last_addr = die_addr;
   return (*v->callback) (die, v->arg);
 }
 
@@ -105,13 +105,13 @@ dwarf_getfuncs (Dwarf_Die *cudie, int (*callback) (Dwarf_Die *, void *),
 	       || lang == DW_LANG_C
 	       || lang == DW_LANG_C99);
 
-  struct visitor_info v = { callback, arg, offset, 0, c_cu };
+  struct visitor_info v = { callback, arg, (void *) offset, NULL, c_cu };
   struct Dwarf_Die_Chain chain = { .die = CUDIE (cudie->cu),
 				   .parent = NULL };
   int res = __libdw_visit_scopes (0, &chain, &tree_visitor, NULL, &v);
 
   if (res == DWARF_CB_ABORT)
-    return v.last_offset;
+    return (ptrdiff_t) v.last_addr;
   else
     return res;
 }
diff --git a/tests/ChangeLog b/tests/ChangeLog
index f6b794b..11974e1 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,13 @@
+2013-10-10  Mark Wielaard  <mjw@redhat.com>
+	    Josh Stone  <jistone@redhat.com>
+
+	* run-allfcts-multi.sh: New test.
+	* test-offset-loop.bz2: New testfile.
+	* test-offset-loop.alt.bz2: New testfile.
+	* Makefile.am (TESTS): Add run-allcft-multi.sh if ENABLE_DWZ.
+	(EXTRA_DIST): Add run-allfcts-multi.sh, test-offset-loop.bz2 and
+	test-offset-loop.alt.bz2.
+
 2013-10-15  Mark Wielaard  <mjw@redhat.com>
 
 	* run-unstrip-M.sh: New test.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f3c56bf..d07cb0b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -106,7 +106,7 @@ TESTS += $(asm_TESTS)
 endif
 
 if ENABLE_DWZ
-TESTS += run-readelf-dwz-multi.sh
+TESTS += run-readelf-dwz-multi.sh run-allfcts-multi.sh
 endif
 
 
@@ -164,6 +164,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \
 	     run-readelf-dwz-multi.sh libtestfile_multi_shared.so.bz2 \
 	     testfile_multi.dwz.bz2 testfile_multi_main.bz2 \
 	     testfile-dwzstr.bz2 testfile-dwzstr.multi.bz2 \
+	     run-allfcts-multi.sh \
+	     test-offset-loop.bz2 test-offset-loop.alt.bz2 \
 	     run-prelink-addr-test.sh \
 	     testfile52-32.so.bz2 testfile52-32.so.debug.bz2 \
 	     testfile52-32.prelink.so.bz2 testfile52-32.noshdrs.so.bz2 \
diff --git a/tests/run-allfcts-multi.sh b/tests/run-allfcts-multi.sh
new file mode 100755
index 0000000..727b76e
--- /dev/null
+++ b/tests/run-allfcts-multi.sh
@@ -0,0 +1,56 @@
+#! /bin/sh
+# Copyright (C) 2013 Red Hat, Inc.
+# This file is part of elfutils.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# elfutils is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. $srcdir/test-subr.sh
+
+# See run-readelf-dwz-multi.sh
+testfiles libtestfile_multi_shared.so testfile_multi_main testfile_multi.dwz
+testfiles testfile-dwzstr testfile-dwzstr.multi
+
+testrun_compare ${abs_builddir}/allfcts testfile_multi_main libtestfile_multi_shared.so testfile-dwzstr <<\EOF
+/home/mark/src/tests/dwz/main.c:3:main
+/home/mark/src/tests/dwz/shared.c:3:call_foo
+/home/mark/src/tests/main.c:8:main
+EOF
+
+# - test-offset-loop.c
+#
+# #include <stdbool.h>
+# #include <string.h>
+# #include <errno.h>
+# void padding (int x, int y, int z) { }
+# static inline bool is_error (int err) { return err != 0; }
+# static inline int get_errno (void) { return errno; }
+# int main () { return is_error (get_errno ()); }
+#
+# gcc -g -O2 test-offset-loop.c -o test-offset-loop
+# cp test-offset-loop test-offset-loop2
+# dwz test-offset-loop test-offset-loop2 -m test-offset-loop.alt
+
+testfiles test-offset-loop test-offset-loop.alt
+tempfiles allfcts.out
+
+# Use head to capture output because the output could be infinite...
+testrun ${abs_builddir}/allfcts test-offset-loop | head -n 20 > allfcts.out
+testrun_compare cat allfcts.out <<\EOF
+/tmp/test-offset-loop.c:6:get_errno
+/tmp/test-offset-loop.c:5:is_error
+/tmp/test-offset-loop.c:4:padding
+/tmp/test-offset-loop.c:7:main
+EOF
+
+exit 0
diff --git a/tests/test-offset-loop.alt.bz2 b/tests/test-offset-loop.alt.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..c1906b6da8a9e3e1408bf2411cce4cbc11d5c4c7
GIT binary patch
literal 685
zcmV;e0#f}#T4*^jL0KkKSpoj~-2ef)|NsBZ=D0-T|6kwb(h<M!-*}4X0u@L>AOMOW
z0SJI25CA{|4pO$-fvJ+4P{>Bq8Uc{?G-${fHld&Z(U5w9wLMKR)X)Zi)FGgmFan;L
zCV*(r00Tjw05kvq13~2uG&E??XwWnO0000o0000q20#IW5g;WRV<w}~JqDT?Lo}XI
zrXvx77(i$Y5YyDtCPv*-pb;W%-Son{UZoCMEJ8%GW;uZ4B93J3%}bUb$aKSn5u-ri
z=)23g%hmk5H$t=0z=2n^*1Yv$0z?r4;sCO=k{OD02s~))A@oymv8;(1x@<+rHc|vw
zN8qv11HLnugf(v5XQ;fWE4Fvcm=o-oFv(epkx5kTMo*W-maj|&fJq0de&Z&K772T0
zc_rq5l!1VW1B)k#*+R=2sC?pJ1$meY0ZN^o_65*E7fIoH6i!@C4gJW-Rzm^4P}>41
zG?}6uQUJms_>lm0>S0#vNAKf}iqoB$DcZAJpbVu<)azp4oHbg^U4iwCP@otgp^=HC
zDGR<4j19|e@qFi*c<*J%>n)x)oI(W=5xkPCtSg$qBv1+EygT*`a9vW0o`|;fuQe14
zC{jhOpqvR-9}CN1XlF;}rvNTNWuBN)3HkRliTIf44I|p~GrPMrIOPL&lvd~wE#jkq
z?ufd^Rb#hN2?aqP1sUlH|6?BY3xbCT9NcOWAu(kQk(G=^VU_!0yCq_hCaKWCiPeF_
zA-woP6R6}y`v~GVfTVCEFd{mbSTZOobOiFa=0U`j#FfzvT03$j38Hx9gr*=kOful0
zAEHA(pn<fAD6SF=f|-RvQXDYiin`z<Yep7MlxC)#CO(#)eG0R$Svt3Pbq!u*|BEqT
TvR9d!fAM!DQ-uitAMdTu7P2#4

literal 0
HcmV?d00001

diff --git a/tests/test-offset-loop.bz2 b/tests/test-offset-loop.bz2
new file mode 100755
index 0000000000000000000000000000000000000000..62185c0acdc328054e1cf3cacd1e249aa6a18674
GIT binary patch
literal 3062
zcmV<S3kmc>T4*^jL0KkKSqE}H0sspUfB*mg|NsC0|NsC0|NsC0-}~Qx*3|N6$<+Vs
zdTw`re_P-RF7C5;*INU7-Mbm-yRFpnt#Q@eW=4VCW3bSXphiky)X34MYHE5%p*N|K
zGebi}V@ZNxGH0o&fuWNrhJr8wqA(f++fqFV=s~niJqe&TrqM7Srhqg=$TSLiWHL-9
zjT1@fJQ4<g83Cq%G|}oA0QCpzk5kk#44MEm2dFdv000dd4FCWKsrsNa6G0G6CYn=v
zrquN}qMK<$^rxx%riRi6ngG!C0MkZ*02*k?pwa3BKmnsnfB-ZAdYS+L02%^Bi84)0
zQTm#mOs2{`4K&pC8fefO8&hgzG7nHR4Lw2WG-zS~4KgwSGzNx%0B8Vc00000AZTa+
z4GjQjWHJEJkjMs&20&ndWDPU`21bT}XaEfW8USbj0iXah1`L#upqLW~$)=uwObMo#
zCV`;SCK09tU`&|?5vG`#Fh-gN38svh84QM)MiWMcCL;(MGyz<gPo<?}Xmt0kfatqc
zdviM1-m9aANOBZ_lx9-N5XR`k1?;#`q}bS)i%KCY#t_g=wnK+I8DjEsY`e>4iqwq*
zFs2N|P1utRM>5pHh;7{A99SG?p6QQYAqwEWp-+(2pkTSmE}eReHf6jWObi;-NVh8G
z#NeT0cQVsIBf51&kqL$s@Pb%7r&7?YwG0HAMMpU0c_MMiSJTN?&vl0vQj^fOjLWyK
z<<wsAb*iQ!n5T0U+|&)W#y47qOBX>Go{xvH#>`l>;Ba$!e8yt@%IPoRMVkRbltkKb
zLx#bb(;=9(AP9&G9m<aV2sDxmAt|iVFSOKS@O&cQfLw)|L)j#WMg`+C5G4}C9XU3i
z#H*hTl$%a$xV_L_v0c8O5_kN%-?mz_AS_G)#4rem6bKNIUfr8UxIG9(K~gKHS+wO@
zt$+!L->$YJ`UN!+TRNO`pb-MLvdm#7rlGbQ1dz~z0skG|IABfHY7`2wtf~5`*#wrk
zf)^CP4YT=Jf}1;glra|7;%?55S4_tCwdiSxM%lA9V4|Y5Z-1q7#h~x<gZ9Z)D}ytC
z-LTENLOYrWYFUEH@lY)ifP#nt1rQJbc|s<Tf(QtN5IQITSb(#d7(onLmJ~1$6&WW5
zLm-p_f(D305J1L|8NV683s%r6vX>17`zd;vz>MyTB1jNyh9bjBROw(>X&@UySS=t%
zy{Rxwq;$+7qV<r{*)&5oF$GKo3|!Pg5SvAzDK!I-B;^InL6Tw!VZGUcvS^(|B!>;c
z#FUuE>AQ!#bX7fFG*)d_8fa!H2{vk~%!Deic2m)84J65=)~2?F9Bwx)i#MHEcbMFq
z<BnOZTgs@z(3Qo=3V17>bGQun>>mJkt_56U5AqN=^*HEA6j7$*H5+EIlvF$UDD2;>
zSr_%|ts2F5b{utz@D2u+p@9mekCi>@McAFK!it$<<}T+sWZ8z)n%cYG!VM)`2^b8M
zZydkpWV8cNBm<nf^dfz0VQU^I@$~JFy){}@@CybqGr_B>z>xsNNMih8D8P#90O5kc
z#?Z^}vQ~dRL!GV<qrLvr+j{l>Df-&&*TZiU0~s$b*kC~9)G!q2h-nzN`Yt|X(?NyC
zkP=|~ViBqswfsaN(_2DI2x@zDECvM(_%C|A{<2c=Y*kZZ9+pl9!uj${zHwO2ZaY5j
zDT$4^S_+m|dVPGf(@9rg(i_ax2#U>n1z@bX%o&-PvrJ)__Hz&}S`iLc8-mk!mfl6V
zm@M(^^ke~FkthHG-y9=6>gp63?^Am#GdbnlBm_Eu29yM{J_QK_gpZDF;km>vlB_Kv
z;u6rdpeoE(3d!dK$H&H9qz*J(qZ?nBDc_}3&8L0(+P^0`4-)+e&QG-J>kLO^lWqx~
zR;Us9Na(&>UncIOPqD27F<8dmTq2kO&*d4)ds8*DrW}Sh(~c0`XYaWc)@Z6zA+VS@
zo;y*H4Q#RA^6!HSlo_VOgbEBZ2<ucp3e7}y08f}0^9>%3);gjJB{~oUb4}LQ_xb!N
zxGa$26bb|uK?eZ<EQle35idN12n-%W=s}wd7h4n1MSI99I_P}UJqK|6IH+X-oCyIL
zB8f^RCfw~-Y5T7nxX6|6CZB4_u8?WiP`pO4*l@cN--qKewD7hcx+pBcU6l;XBLh&S
zp-fVtqqUs1QJCe-IZYKMBW%>`zw_bA+LKz{MmwDJtVW%iKe7@iW1*|8NtHXNS+wpZ
zR~7~aUC5}RydsEBqPCDl&nTD%j{wk=Dj-6e0JNc6@&I|kZ*3`_)_*BE$W|c*2E!=s
zp9gQkTOju4$by4`qAY5pgFrAC`$ZK#3v;#aVxyBcbIt*FG@MLM!9J=*p>+Y&+;yhq
zN)kC4V#@})q}10wXe)7Vq1c?w)QQCaz70h4YifMuy$x>65zmx6{(imGk*NXQ850;N
zy#Z8Y0z9%42KI`#ZidIKX=w!<_?E@N#e9dUdD9!7HChB^rEjpg4Td3*Ciu!hgvFmd
z$}*tpwK|IwD9H^KNrpw?A@*Zg82MYt_?;b+&A#SNt(!Z`v9#7`$=kqvz}?NEL8!jZ
zaEY?oEz~%W!Xi>;fgp;`450~C48kfR8LAs23^kMk%orL3gh-4M1_NONno>2%3kyXs
zvG5L9sS~j8v@`X&J0H$h4lEtsq1yb+=A<=ABfc&wOH>a9aj6p}s>hhVPrfM<iCDYA
zPB7QGa<Y?{gz7yvGSz(>SKz`4$RL)G-UBor{4Z}^R#GaOzl(*5M6w6*(b2O@c4_4D
zs_4gf-1<xcw_ye+y#I@2Z#hkqc#PR&l8-5N9ltVY7~&u7UMEWVns~JDq*$nmNmv;l
zV%b_-45WNGj{@cyawS8`HN;R;b@ORV91(9^z~6(i8W=hQXSEd(w1a~qt7(^9dYIa8
zt7{<N@K3Z62yz&$8VI|4lCb5ZoWYJWIs7^~>w&pa2-%b9Sl^^m>l&gpS>!I%Vu|*K
zVZrOoY+QLE0m7Uh(J~GX6o#2-KT<EG@l3|6cPEwK3?{T97|4vfklRhR;e@21ZKDJ*
z`1RwqsS65#f)wkF<ku|G%S$K{*g$xR!T~JUXONsMHVUy*$fOa2Xek6gBnc#Y1OS0q
z#}Yg?$TS-gsr`wxBGVoCq-$Iprbx<`BgmW`8uPeVG7kdn7Qv)a5^!-K0^U1JmOz{s
zQ3DJ@0O+W=EeWR&Bq|dwtBuwKtxThU8L;6n**BL<ve#92F;D;!C?LQfA?cWe<v<QB
zEk>m_S5Sd4Df6F%sKHwkF$fY2$)p?=LC``1EDcxq<5+mp>+gj@sKb_6Mly0{AuU-#
zN#TqdC>;izo#D}4(6B3>1@US{R1p?P8%`cfrX~!5p<t`Rv07}Y5v2mrm62Ujgrr&^
z%4INk2*QY?DMJIH*=^ZDXt+pvP=}z66)j0I34o}m!5dE-1%(n4utrdW2p||BGGjz2
za3+I*oOwXROMEHPgb=7e=EimygGs$0oHf2mmR}K=Ip=el8steKR3<zg0K$kEqPoZq
zWR0-m2uHoMxRvoGQCK<yqBzs*($?*vrs)R5^{bE&2ytU@{|UM**@XNJ%$t}1?3>dn
z0b0?ZkaYy0KDG?3Bqz4azrCC_i7&@HRnM6aSQ8I4!3||B-Mxb59Bh1`$Ys#-nRsxQ
zp{{98RpVxa;{pspALyzT-((=HcdNmMb!xs*%$V#06l}3<A+Vl{3Byp2UHV|ZWi%r#
z>E(Qnz8z@uETxvaVbOol-7@QIqnu**;|b1YK=H=-U<rvzZP?EYJ3Y<WY>M<6<$dc_
zHw|867mK#fR4^R~sF<*CVy-Gm4T{rk*Vbn4l1_O094%YuJ7!Vq_{W~nz5=zcheFjr
z;D8{|e%b;6hjr{|S#f6Dd4j@j3PHFWnKY8vPhW(9O>N$C|HL2e<L<MyyH+@OoJf9f
zB)S$eq-E2}3=jFGg6JBM&sD@k4ZV~@+Bg}3h>>ec>z;a_^4WVnw>_fnNT&)C0PaV?
EK&hU0A^-pY

literal 0
HcmV?d00001

-- 
1.7.1


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]