[PATCH] libdw: fix offset for sig8 lookup in dwarf_formref_die

[PATCH] libdw: fix offset for sig8 lookup in dwarf_formref_die

[Jason Leasure]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

The type_offset of a type unit header is relative to the beginning
of the type unit header.

Signed-off-by: Jason P. Leasure <jpleasu@super.org>
---
 libdw/dwarf_formref_die.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
index 63f6697..8b92e22 100644
--- a/libdw/dwarf_formref_die.c
+++ b/libdw/dwarf_formref_die.c
@@ -95,7 +95,7 @@ dwarf_formref_die (attr, result)
 
       datap = cu->dbg->sectiondata[IDX_debug_types]->d_buf;
       size = cu->dbg->sectiondata[IDX_debug_types]->d_size;
-      offset = cu->type_offset;
+      offset = cu->start + cu->type_offset;
     }
   else
     {
-- 
2.2.2

Re: [PATCH] libdw: fix offset for sig8 lookup in dwarf_formref_die

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

Hi Jason, Hi Josh,

On Wed, 2015-01-14 at 09:59 -0800, Josh Stone wrote:
> On 01/14/2015 08:47 AM, Mark Wielaard wrote:
> > On Wed, 2015-01-14 at 09:26 -0500, Jason P. Leasure wrote:
> >> -      offset = cu->type_offset;
> >> +      offset = cu->start + cu->type_offset;
> > 
> > Thanks, I believe this is correct. I am surprised we didn't encounter
> > this earlier. Do you happen to have a testcase for it?
> 
> It's a regression from commit 9202665816763, before which cu->start was
> used with the offset everywhere.
> 
> I can see this in my dwarvish tool with Jason's example source.  You
> just need a ref_sig8 that's not in the first type_unit, cu->start > 0.
> So here, struct A has a ref_sig8 to struct B in the second type_unit.
> 
> In the bad case I see "signature  ref_sig8  [30] 0", where those last
> two bits are supposed to be the offset and tag.
> 
> I see "signature  ref_sig8  [72] structure_type" with 0.160, or with
> master and this patch, and it expands the tree of attributes from there.
> 
> Of course you won't want a GUI for tests, but it should be easy to craft
> this one directly.

Aha. So this is my bug, I broke it. Sorry about that :{
So I added a ChangeLog entry and pushed Jason's patch. Then tweaked the
typeiter2 testcase a little to show the name and offset of the type DIE
found to show the regression and added that to master too as attached.

Thanks,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tests-Add-testfile-debug-types-test-case.patch --]
[-- Type: text/x-patch, Size: 8079 bytes --]

From 47efc3bef9f39a0494f9957b2e5f7da33482865b Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 14 Jan 2015 21:38:16 +0100
Subject: [PATCH] tests: Add testfile-debug-types test case.

Test for regression fixed in commit 7c713822:
"libdw: fix offset for sig8 lookup in dwarf_formref_die"

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 tests/ChangeLog                |   7 +++++++
 tests/Makefile.am              |   5 +++--
 tests/run-typeiter.sh          |  14 +++++++++++---
 tests/testfile-debug-types.bz2 | Bin 0 -> 3024 bytes
 tests/typeiter2.c              |   6 ++++--
 5 files changed, 25 insertions(+), 7 deletions(-)
 create mode 100755 tests/testfile-debug-types.bz2

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 6dd553b..365d98d 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,10 @@
+2015-01-14  Mark Wielaard  <mjw@redhat.com>
+
+	* testfile-debug-types.bz2: New testfile.
+	* Makefile.am (EXTRA_DIST): Add testfile-debug-types.bz2.
+	* typeiter2.c (main): Print both name and offset of found form DIE.
+	* run-typeiter.s: Adjust output and add testfile-debug-types.
+
 2014-12-26  Mark Wielaard  <mjw@redhat.com>
 
 	* run-test-archive64.sh: Add nm test.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 303f783..14adebd 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,6 +1,6 @@
 ## Process this file with automake to create Makefile.in
 ##
-## Copyright (C) 1996-2014 Red Hat, Inc.
+## Copyright (C) 1996-2015 Red Hat, Inc.
 ## This file is part of elfutils.
 ##
 ## This file is free software; you can redistribute it and/or modify
@@ -280,7 +280,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \
 	     linkmap-cut.bz2 linkmap-cut.core.bz2 \
 	     run-aggregate-size.sh testfile-sizes1.o.bz2 testfile-sizes2.o.bz2 \
 	     testfile-sizes3.o.bz2 \
-	     run-readelf-A.sh testfileppc32attrs.o.bz2
+	     run-readelf-A.sh testfileppc32attrs.o.bz2 \
+	     testfile-debug-types.bz2
 
 if USE_VALGRIND
 valgrind_cmd='valgrind -q --error-exitcode=1 --run-libc-freeres=no'
diff --git a/tests/run-typeiter.sh b/tests/run-typeiter.sh
index 605ee2a..2687bab 100755
--- a/tests/run-typeiter.sh
+++ b/tests/run-typeiter.sh
@@ -1,5 +1,5 @@
 #! /bin/sh
-# Copyright (C) 2012 Red Hat, Inc.
+# Copyright (C) 2012, 2015 Red Hat, Inc.
 # This file is part of elfutils.
 #
 # This file is free software; you can redistribute it and/or modify
@@ -41,14 +41,22 @@
 #
 # g++ -gdwarf-4 -g -fdebug-types-section
 
-testfiles testfile59
+# echo 'struct A{ struct B {} x;};A a; A::B b;int main(){return 0;}' \
+#  | g++ -x c++  -g -fdebug-types-section -o testfile-debug-types -
+
+testfiles testfile59 testfile-debug-types
 
 testrun_compare ${abs_builddir}/typeiter testfile59 <<\EOF
 ok
 EOF
 
 testrun_compare ${abs_builddir}/typeiter2 testfile59 <<\EOF
-ok
+ok s1 [25]
+EOF
+
+testrun_compare ${abs_builddir}/typeiter2 testfile-debug-types <<\EOF
+ok A [68]
+ok B [38]
 EOF
 
 exit 0
diff --git a/tests/testfile-debug-types.bz2 b/tests/testfile-debug-types.bz2
new file mode 100755
index 0000000000000000000000000000000000000000..a41f4938cee6ad6994f716b592b9d1d09e64cdbd
GIT binary patch
literal 3024
zcmV;>3orCST4*^jL0KkKS?tm1f&dEmfB*mg|Ns8~|NsB@|NsC0|NZZ8%>P?-(OCb)
zde7H>fB)bL4rph<Gjlg-d1&p=G`#my-g@d3Q{Kk#(IZnTey5>N8m1G`q-4S{gF{Uk
zXlNQV$&fVDKn(*;GyrL)fB<MT2dSr|$)Gd<27^rv28@FcG$I-S2AUXxB=perO_cQ1
z%>?xsHlP{?hKHyCG}BEzK+&d68Z>F4rbdH68UO}>0004?!~;apC#mRCY3c@=Hc<Ug
zG&BGJ00003KmY&@001-q000009-uS;000d#L?B9espzNar9V>unNKM6#GarTJxv~=
z<qVrq>SzrB0QEgUG|&Uo000dQ8UO*HXaE2J(dr;*XaEfj0BB?~0MU@h28{+lV1Q%|
zGyn!hhJa`Q4FDPdXaE7A05k>(6co@Y=+F{-BAA2PlP9T@CYn7#kTf&^8V^X&9;P5P
z0gwOyGH3t*2dDrv0000Kvo=^r+&ij1xUfCr4pB&>cNAqKG=rj14U8&g9wCA<3Lo#{
zg#jdj7##!yTNZW&%3~@D>FArQmamsyS<srmP6UnO3PA#0?`k{W7|C@kVyI$R3Di-<
zEuGaV*h1!4(;B>0;$FmuF}S~moEg`H6-nTGy`;`U+{zLeVVXX<ER7^86|2-%c=#Yq
zthFF5#*tHvY3c;UMa&@hvcb%o&5XO!voV#!`3Dw`RFD>mYwabyZCpgrmjRgzLdb%|
z#s&S1p5wW?x$X9RUYdqR>xo|i{_ghYhP(ZpsnP(-fpk<#BnTm*1uContQcHQhNE;r
z$#Q`@P-)@T#>wF*-cq52Q)&nnJA@dpvQl75EH;)M*S_0dwS8w!wZMH8>jgJo!jJR&
zDi;Cy#TY6rLI5BJibe69f!_hf9*<kr$m(E#1Wlzh8HFMYGe0d;l3;)!8Zsgw2hyg^
zzo3_fz1@{W?CBz+A5Mx6Jl3uV*3T9Has;*PK)8kgd#`A~GA`hQ&2ZCOj|sY6X}a7R
zO38hwuPY;0EK!9{YJv6H5Xs;myX)<IX*%`&n$~ubq){jfXWqe04FK2;je-S?#2m{)
zNY^nWjT106&<QaZ>@$r77>XDL3=mM#Kvd1Q363OL02~`?2{zjp#tOp_QyNHE%tfa*
z894SfIt?8oV$dbqj7qyeRbs5tKsJyUwe%1ogpsdmY)P~oLr`GaLvMG03JonxAc12v
zFCB?RfoT#5a-D-RbTugp0s{nDtP)@WK&pn;5FmnMr$#VOC`q8iP9X*r>C%2fScV%O
zvfw$j<f?5A6|~R?Qk@H}T_~?r`bFY7XcddEP}nZeg?Q1aJPs7%Y0`xsGn&U>YUZ3F
z6J{vJTWi(j(2yu50@4PCLf0piM!p-0ShjaAv8Oo3+|)LwQN`R{$7Nl}-Ps;8w$Q~?
zV}`W!sl`>HOAQNzj+(F6u6<o}(h-(!2zD)w+LgY!jm!j`D^CMe2I+=^W}3C|dV7oW
zgQXQ(Hfa_zk}mCV<z|BfM#zBV3YDig6NqelyvHtTHmQZ|uDlj@wvS=ze>K%~Ha@7@
zr>?*!rcp+NBQi4pNP{H?ro^+x{hpkRT-gAm<k-Nd;QClV6=8#5xGd#>Bw4_F*_&8k
zyX`hO)Td9N2xLaMk>{;>&5&i|ZvB>ygl{conmnc>38NAaKy6ppD+OicDN0h5^1Y}A
zR#X7Yv40teC63XC{EguPvM8hk-KK`odcae`4OVzm5ebHYZ~GxLGYjoi?Q9;Fps|gU
zG~oi-gqns7#705TKx%y;@uW44zu|G5=OdWSQ-v6f6ktxB5efL#m<<tZ66R<&Q@NYm
zdI2tR@{n}(QUVcNBf2@MLOMiiTHblGaJ-rjMnGvSwjuMgUA75q=8|E2C%l0Y1OO@w
z4rz`{#-qmVWKx#(ipd8Gad6SsB2hWme8o_Xvdl!PFe$c|<6m#mn<hyMKdp+GHpGT%
zV1cc`*k}AEA*+uyf<}M^sN<nLvkOhg-aA|6@FCmu^tbdf1uN?EuwXN!n6NWU&Qpge
z-6Iyk^9t9hI10nsMkM3y&7Rtjpw^aaFy0P7CD-EhSGh<iA}|MIpxpfsBQuLU3s0$H
zWF5>1vQqphoJ`HjPbl#%n?pq8PN4|I$!b+kpJ%GTnkS##q6S0#c<2G4V-I@r-(tZG
z<SmvM6%5<a@6Tn-3v{L+o-<Pd3R6}@XaUf;#KAtuT`t71O5=ll^{#2DYIzD_QIHz|
ze9A<DrAuiL)>A`5!3{Gn2v%waRa=Ba8xrJYhk4EIcH3}xl8Tqs_FVW#>U+D1Rmdlq
zms^vcfA(kkn@wH@7^`Y3S^_@!Nw9a8b(3bTqL*1AGKxMtZDrA9v@?HQwkFx6xOv+H
zN)^HiwZO)_-08p$;iV>t7%bAz2;8$a<*O#>T<$dgiuKAGFz`dx8Xlx<KLIF!7|V<P
zNZ9Tu4-cy%HW1u{8exDthw<?}^UY64>|s?>!=bmpF#(n?rv(#lA$n}CTGi+(SZ>9b
z#w0ZEuxCqV(P!#klG2(Ou-j3EQ${!W`TH*tK_Qrr=UJ^5G_rz#3J#D#2_52s5fM;>
zxh3L-va0fsM5ie{%q9pkfTc=b73M~2B_?afIGluR-G=NdB^MFIpMPO(bs)v1q1SiQ
zGYt<9%AS&D`=$Bz=LWnTryRoQda*dl*8>8r`=f)sq`FrlWu4Pyp43AQ60K2XorOEq
zaGFAV>rCMRP_r~4F#$IiR7-3&Qmmwg11l#cnO7lbBwHzPV%`{|-wU&$fxdg*&&`}{
z1i*V=h8jHMcAcEaEEp8K7fFC-X}^nZ8BToKyHyQHy`2|><AB~tHtaAVwE~&9l`()|
z5>Li~j2<xYaLwHqRdYN$X8NaDW#izgAp=C3)K7FKh*Hi_k9JxuV2l`CT{(+8+>w!>
zL9HdxvAwjToH9sgM!N$vRrOmQvy#K1^W+Cb)hZ`VqE-z>b1h1aD7&Og;Ob{bD|&F-
zn%O2W2_gw3l31`wlM+~8>FGq>Sp;f|iG@pG6=2mCLhlL`Ok$*hRSikdB4q-vmdmJO
zVUd8CI~gohvLzv#%{*dr6Dfhm``v28>wP2@Nd-V|(nlEIT-Z@1BAiR+iH36ucs0?@
z)FUJ{ewJjoPP7Z*R~h*%O6msy4wK248^<7M%}qr5aIy_n*8ZGe&GW8d{8uiz=@rjq
z>HrS`04ORP)Pe?IZru*Lb3E(is`K;;6^IK~WRlFKXwAsxAx#09WTNS??EoSov9@wb
zT=ZhN#uzqjl!tuU;gACuX=0#!3t|U`EE0#_GkOT~CR7?wBMAGAg(?z8G~r~Y8gmIU
zGGMR@YJxPIDg-VD8&;8kIt84~aKfe<1z0{U21FaAB_H1P*dt39J<7lez*meFWLR)Q
zkqObIA_B4yApro0#nrrOG7n2wp$*MC2jOTL3baxU2gn54B;W&rhhwEsG`;#%Jq=|)
zOw0q(-mhb{w)ptq6{?j%kwY^U?I6faP+&j{%UAe#I7S&jP#h-<Q2h0L3bMQdp}H;r
zAqf51SZ{rO)W5N71#veG*<{ZB@Jwlv2*nf{5*?9T3gRsk0|x;QYFb+wTDe0PK_bRj
z-qz#-3<*~5T_iDEXG<{Cm4yH>Q>Zz))TW9Km1STvr)krMp#!y@$&WE8<}nV2WzrsG
zP%$)*XlL0f2AgMz6i@z@C9B}yYurV?8UrFM_obBk9ATFJIQ+6Z;__|{pH&`>5Kgk)
zXs)#D`M3rpx|ORd75AAuo68Ms-{>`HFfZVaka+>n6jawjg+;|?ZxLBqALT$p6eu7p
z7_d|=n9FEhXBPc3vqP~sh2ZcvOIQse4a$Ve(g~6=y=J?7Om(E8bx@$7cA`SN7Pz-1
z7?4XXa!m6s?EIM6noD*W*Ij_+=qc2MH&0w98YBUK5QBopZjdC~t5-mQo1?YtKU(xX
SJ4}gx@pmLsg$WML9*7{vGDm;_

literal 0
HcmV?d00001

diff --git a/tests/typeiter2.c b/tests/typeiter2.c
index 6ddfa38..35b6a12 100644
--- a/tests/typeiter2.c
+++ b/tests/typeiter2.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2012, 2013 Red Hat, Inc.
+/* Copyright (C) 2012, 2013, 2015 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -23,6 +23,7 @@
 #include <stdio.h>
 #include <unistd.h>
 #include <dwarf.h>
+#include <inttypes.h>
 
 int
 main (int argc, char *argv[])
@@ -71,7 +72,8 @@ main (int argc, char *argv[])
 		      if (form == NULL)
 			printf ("fail\n");
 		      else
-			printf ("ok\n");
+			printf ("ok %s [%" PRIx64 "]\n",
+				dwarf_diename (form), dwarf_dieoffset (form));
 		    }
 
 		  if (dwarf_siblingof (iter, &iter_mem) != 0)
-- 
1.8.3.1

Re: [PATCH] libdw: fix offset for sig8 lookup in dwarf_formref_die

[Josh Stone]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On 01/14/2015 08:47 AM, Mark Wielaard wrote:
> On Wed, 2015-01-14 at 09:26 -0500, Jason P. Leasure wrote:
>> The type_offset of a type unit header is relative to the beginning
>> of the type unit header.
>>
>> Signed-off-by: Jason P. Leasure <jpleasu@super.org>
>> ---
>>  libdw/dwarf_formref_die.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
>> index 63f6697..8b92e22 100644
>> --- a/libdw/dwarf_formref_die.c
>> +++ b/libdw/dwarf_formref_die.c
>> @@ -95,7 +95,7 @@ dwarf_formref_die (attr, result)
>>  
>>        datap = cu->dbg->sectiondata[IDX_debug_types]->d_buf;
>>        size = cu->dbg->sectiondata[IDX_debug_types]->d_size;
>> -      offset = cu->type_offset;
>> +      offset = cu->start + cu->type_offset;
> 
> Thanks, I believe this is correct. I am surprised we didn't encounter
> this earlier. Do you happen to have a testcase for it?

It's a regression from commit 9202665816763, before which cu->start was
used with the offset everywhere.

I can see this in my dwarvish tool with Jason's example source.  You
just need a ref_sig8 that's not in the first type_unit, cu->start > 0.
So here, struct A has a ref_sig8 to struct B in the second type_unit.

In the bad case I see "signature  ref_sig8  [30] 0", where those last
two bits are supposed to be the offset and tag.

I see "signature  ref_sig8  [72] structure_type" with 0.160, or with
master and this patch, and it expands the tree of attributes from there.

Of course you won't want a GUI for tests, but it should be easy to craft
this one directly.

Re: [PATCH] libdw: fix offset for sig8 lookup in dwarf_formref_die

[Jason Leasure]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On 01/14/2015 11:47 AM, Mark Wielaard wrote:
> On Wed, 2015-01-14 at 09:26 -0500, Jason P. Leasure wrote:
>> The type_offset of a type unit header is relative to the beginning
>> of the type unit header.
>>
>> Signed-off-by: Jason P. Leasure <jpleasu@super.org>
>> ---
>>   libdw/dwarf_formref_die.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
>> index 63f6697..8b92e22 100644
>> --- a/libdw/dwarf_formref_die.c
>> +++ b/libdw/dwarf_formref_die.c
>> @@ -95,7 +95,7 @@ dwarf_formref_die (attr, result)
>>   
>>         datap = cu->dbg->sectiondata[IDX_debug_types]->d_buf;
>>         size = cu->dbg->sectiondata[IDX_debug_types]->d_size;
>> -      offset = cu->type_offset;
>> +      offset = cu->start + cu->type_offset;
> Thanks, I believe this is correct. I am surprised we didn't encounter
> this earlier. Do you happen to have a testcase for it?
>
> Cheers,
>
> Mark

Sorry, I don't have a good testcase, but you can generate a binary with 
two type units in the debug_types section using:

   echo 'struct A{ struct B {} x;};int main(){A a;return 0;}' | g++ -x 
c++  -g -fdebug-types-section  -

to see the global die offsets:

   dwarfdump -Gi ./a.out


Jason

Re: [PATCH] libdw: fix offset for sig8 lookup in dwarf_formref_die

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Wed, 2015-01-14 at 09:26 -0500, Jason P. Leasure wrote:
> The type_offset of a type unit header is relative to the beginning
> of the type unit header.
> 
> Signed-off-by: Jason P. Leasure <jpleasu@super.org>
> ---
>  libdw/dwarf_formref_die.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
> index 63f6697..8b92e22 100644
> --- a/libdw/dwarf_formref_die.c
> +++ b/libdw/dwarf_formref_die.c
> @@ -95,7 +95,7 @@ dwarf_formref_die (attr, result)
>  
>        datap = cu->dbg->sectiondata[IDX_debug_types]->d_buf;
>        size = cu->dbg->sectiondata[IDX_debug_types]->d_size;
> -      offset = cu->type_offset;
> +      offset = cu->start + cu->type_offset;

Thanks, I believe this is correct. I am surprised we didn't encounter
this earlier. Do you happen to have a testcase for it?

Cheers,

Mark