Hi Aaron, On Fri, 2024-02-09 at 21:52 -0500, Aaron Merey wrote: > Modify dwarf_decl_file to support DW_AT_decl_file with value 0. > > Because of inconsistencies in the DWARF 5 spec, it is ambiguous whether > DW_AT_decl_file value 0 is a valid .debug_line file table index for the > main source file or if it means that there is no source file specified. > > dwarf_decl_file interprets DW_AT_decl_file 0 as meaning no source file > is specified. This works with DWARF 5 produced by gcc, which duplicates > the main source file name at index 0 and 1 of the file table and avoids > using DW_AT_decl_file 0. > > However clang uses DW_AT_decl_file 0 for the main source index with no > duplication at another index. In this case dwarf_decl_file will be > unable to find the file name of the main file. > > This patch changes dwarf_decl_file to treat DW_AT_decl_file 0 as a normal > index into the file table, allowing it to work with DWARF 5 debuginfo > produced by clang.
This analysis makes sense. > As for earlier DWARF versions which exclusively use DW_AT_decl_file 0 > to indicate that no source file is specified, dwarf_decl_file will now > return the name "???" if called on a DIE with DW_AT_decl_file 0. And before it would have returned NULL. I think this change is OK too. It is unlikely a producer would actually use DW_AT_decl_file zero before DWARF5 to indicate no file is associated with the DIE. It is far easier/efficient to just not add the attribute in that case. > https://sourceware.org/bugzilla/show_bug.cgi?id=31111 > > Signed-off-by: Aaron Merey <ame...@redhat.com> > --- > libdw/dwarf_decl_file.c | 25 ++++++++++--------------- > tests/Makefile.am | 3 ++- > tests/run-allfcts.sh | 17 +++++++++++++++++ > tests/testfile-dwarf5-line-clang.bz2 | Bin 0 -> 2764 bytes > 4 files changed, 29 insertions(+), 16 deletions(-) > create mode 100755 tests/testfile-dwarf5-line-clang.bz2 > > diff --git a/libdw/dwarf_decl_file.c b/libdw/dwarf_decl_file.c > index 75662a33..07b69f8d 100644 > --- a/libdw/dwarf_decl_file.c > +++ b/libdw/dwarf_decl_file.c > @@ -31,7 +31,6 @@ > # include <config.h> > #endif > > -#include <assert.h> > #include <dwarf.h> > #include "libdwP.h" > > @@ -48,13 +47,6 @@ dwarf_decl_file (Dwarf_Die *die) > &idx) != 0) > return NULL; > > - /* Zero means no source file information available. */ > - if (idx == 0) > - { > - __libdw_seterrno (DWARF_E_NO_ENTRY); > - return NULL; > - } > - OK, this seems to be main change. > /* Get the array of source files for the CU. */ > struct Dwarf_CU *cu = attr_mem.cu; > if (cu->lines == NULL) > @@ -63,20 +55,23 @@ dwarf_decl_file (Dwarf_Die *die) > size_t nlines; > > /* Let the more generic function do the work. It'll create more > - data but that will be needed in an real program anyway. */ > + data but that will be needed in a real program anyway. */ > (void) INTUSE(dwarf_getsrclines) (&CUDIE (cu), &lines, &nlines); > - assert (cu->lines != NULL); > } I see why would like to get rid of asserts in the code base. But I believe the assert is valid. dwarf_getsrclines will check whether cu->lines is NULL, in which case it tries to load the line table. It then sets cu->lines to the newly parsed line table, or to -1 to indicate there was an error parsing (or no) line table. > > - if (cu->lines == (void *) -1l) > + if (cu->lines == NULL || cu->lines == (void *) -1l) > { > - /* If the file index is not zero, there must be file information > - available. */ > - __libdw_seterrno (DWARF_E_INVALID_DWARF); > + /* Line table could not be found. */ > return NULL; > } Which means this is a change in behavior. Now if there was no line table, or a problem parsing it, then no error is set, but NULL is returned anyway. Which means using dwarf_errno or dwarf_errmsg after dwarf_decl_file returns NULL doesn't work reliably anymore. Are you sure libdw errno shouldn't be set to DWARF_E_INVALID_DWARF? > - assert (cu->files != NULL && cu->files != (void *) -1l); > + if (cu->files == NULL || cu->files == (void *) -1l) > + { > + /* If the line table was found then then the file table should > + have also been found. */ > + __libdw_seterrno (DWARF_E_UNKNOWN_ERROR); > + return NULL; > + } > I think if you are going to replace the assert here, then it should (also) be DWARF_E_INVALID_DWARF. It means a decl_file was given, but there is no file table. Which IMHO is invalid. Just like in the case below: > > if (idx >= cu->files->nfiles) > { Here we also set DWARF_E_INVALID_DWARF because the decl_file number is larger than the number of files in the file table. > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 13bd9d56..b075e3c3 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -634,7 +634,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ > testfile-largealign.o.bz2 run-strip-largealign.sh \ > run-funcretval++11.sh \ > test-ar-duplicates.a.bz2 \ > - run-dwfl-core-noncontig.sh testcore-noncontig.bz2 > + run-dwfl-core-noncontig.sh testcore-noncontig.bz2 \ > + testfile-dwarf5-line-clang.bz2 > > > if USE_VALGRIND OK. > diff --git a/tests/run-allfcts.sh b/tests/run-allfcts.sh > index 9c0a55d8..1d4766fe 100755 > --- a/tests/run-allfcts.sh > +++ b/tests/run-allfcts.sh > @@ -170,4 +170,21 @@ testrun_compare ${abs_builddir}/allfcts > testfile-lto-gcc9 <<\EOF > /home/mark/src/tests/testfile-lto-main.c:6:main > EOF > > +# = dwarf5-line.c = > +# int > +# main (int argc, char ** argv) > +# { > +# return 0; > +# } > + > +# Using clang version 17.0.4 (Fedora 17.0.4-1.fc39) > +# clang -gdwarf-5 -O0 -o testfile-dwarf5-line-clang dwarf5-line.c Does it need to be -O0? Not that I really object. Just hoping to get a slightly smaller binary/testfile with -O1 or -O2. > +testfiles testfile-dwarf5-line-clang > + > +# Check that dwarf_decl_file can handle .debug_line file table index 0 > +testrun_compare ${abs_builddir}/allfcts testfile-dwarf5-line-clang <<\EOF > +/home/amerey/test/dwarf5-line.c:2:main > +EOF OK. > exit 0 > diff --git a/tests/testfile-dwarf5-line-clang.bz2 > b/tests/testfile-dwarf5-line-clang.bz2 > new file mode 100755 > index > 0000000000000000000000000000000000000000..ab62b707bd7371268d6e685a2f86a1a3a868b0a9 > GIT binary patch > literal 2764 > zcmV;-3N!UWT4*^jL0KkKS?3bM4FC%f|NsC0|Nrmr|NH;%|NsBz|NsC0Y46D+q(klP > z_GEV7|6kw<e|yKby<ip7M^tIL3eS7g?=6#KRqWVkiK8kpN9s@28X9|4^oNQA)WS5- > z0MHEp27qax01SX=28Nj$Gyu>vWB?5S00E!|h!aMdntDLhJSItiq3U^0)f#B@ntFzs > z000kAWYa*<dW{cAXaS%A0000001Y$@k3s?}dqFmpGa`@G0ia<3HiTe+G}9)400SYA > z!3>NhhJXR4jD~;!41flWFeaKAGyq761dR<O(={+mKxyd%Mw(!n28JL2XaEC72++s? > z0B8n-KmY&(O#lD@5CF(%WC4&g(Vzx_qb7hfVj2d30}uhDKmY?HL8BmIG|`~M4FEI@ > zfB*!c5CSRaV46)dF-@e}N2K)B(?)}8dV|yrsAPIgG<r`^#Lxg5X{LsN0005%02%-Q > z&}pE0nl_aXCD=jAODtA&P3f7fXPc!mh+>-6)G8o+yQE<R6+~?#)`+gmXlIN#oO8~Z > zdvrPd?3kRD#!~u{ODmbB$50Su1ahMIL1c8UOr*{ZMvK6Kj)+V^NautafTr1^ei)R; > z9G)o1#6B#9q-$X5iKthoo+UvrMJ!a~WDXkn7<2-fiy)7`Ljlbod<q4pT{99RfIF~6 > z5f+NnE&#Gg=$Rr$1>uT0j3ws|*~V~G%!nj`>pELHrd!u%YwKaMQF=^V6yoOE5?X2@ > zAafd&nVd`~H9AZ>f5^t-XG&i(S&zxmz5;Sp>!ekGkT%;963hq(I|)o+t(C9o`~IK4 > zjcsIcEeH^UDcGyTczze<lye|J38g^`$%++huBFdgn0x#_YaZSJ09|H8awi#&xWeP* > zL^gteh!caH>(XY`-oLcELm;RU`y3t#x;G2jb8TmIZzQa(i+B5Sq5*a+LSHA-2^nM$ > zklU1GZ~yDnRNg)5tAUm}RI+-C<>;Rh?Bs4utkmoJ>+9V1-)A_^O_(g=UgpGuW#2-^ > zA&E4^hQuJXp#|oI9V-$FG$WT{LRc&f{eeb>d7&XfA3sPXZqT6&O}Cq7#i4Fv3Lt2} > zXh#(gkStaUT+#}dxkgcl%4i@#Q%NIwV3aTdj6!)`wo=KogpAV!m2D#)@S+f?z%4N8 > zRT4-xI2*{+qn>7Yi7WheW^@e{U^bZBu;j5!l~7cwm<wG`HLJ|fq1w>=-c=fRlT?lg > z)$sJGV-&iTaUE?ERWvNBUMWG=NkvHqq1qA#QJEPd5#?BHExuAT&^{Iv@6?jq(}ed3 > z%GnlcZ8fG8vjbn1YYVu*LTui9a35(mQg|tvc0&%2BMDGq-YHHLD^O-};5@@A<V70P > zR){JZ1T^^^&ePO8M;|u<)w4fw+4NkTSc0g2uf6PPI2-co6Luxdv_n_-{4`2z?ix29 > z>wnVt7boS`{=Qp*xU7-3oTBS+yOWg@vrd8$Gw2{A7<~vMGcqD7YJ?GV+vF;;P>vmo > zl0eWZGL0=;K`-km2+xf~K(4mYFO*^7ycUB@H+MN9gvnwTti%@vjSp?`JijX%s6JS* > zMmFRxDT_AJ18iWG!NW+!yM0o-IuoNs3@j$)K?9Dt$=7cRg|t?VD%}a+3DiZRbH)_( > z8Z_^S#DQ6TgM{$#<;8be$h&*EhEt7&)^gZ0wdLl0+i@8rt5t>%k18HdPYgOcD-DOt > zC{f0;<KQ|{GAO}bDD@QzMIgIum5qsxqph5?Hd{~IP2ZoXbovYO;sWzn3?+@>UVut9 > zka)T%TLF^9Y}!_^wW!!{(qgrCzNN{fWO$xr*S%Fg(5SS$5U7w~nRl+XoO`6Kw5}nL > znTDi~Uda%cq$XmAC)cCd+b7;*PY4DaR2EP`V1|3R$PSo2rp(<nIRe@a9Q40D+^aKv > zCFYhq*v$81!H;g*s<&%0Xv#t|5gPWp939%!SyhpwbaHxCW-8OwUV4(W=Ib?3acRK2 > zxq&5!VF;-l<48b}xfL1AnMS8<MS0=&MqRC`w<AfZq_ps6)UP{|?4@*&fu$%)#d$Jm > zKDq{~5IoWV_l$^|1}X_+Q**AMh=9|F2kK{*EKBI4GwBg$b#;V`+Xd<zFBf@&ro63R > z-x9cL!hakuLucHB91uIWvl8kHcczt@nV4aRplMYLB92j1AyG7uz|p}1T9J{*O~1m! > z-u=1oreI6GqWZ{g%}_(8ykA|SdKedSH1A+=SxdCiR7ewDc8Lh)FJ0HjiYk|MCV?`q > zfcJl{pZ=E;qA+d9(v-=&UG=-duts1OP(LoWZTrS6h(;(eI9!)b`9QkMZPO6GS%ogs > z5?o3E;7f{4GX^1qYK~E!LW9T$kNql*ifn`Dzw?u0-KINO#oY8LrZdZDqr&i=?46A| > zln~(l_8c$j^k%aub2RX$L(35lPMUE^tf3M{s$7Io(GAL)Dq+_(Qe=wgQPI}ODP4xq > zrGmhqwGMQcgiN7g1OyJmiOWt~H_(g#;=FnwbSXzUC1hrF2r?{!MSvkQU17@}M*Oiv > zaFk;vBuFSNl$fNHvoy1dtC&H_{LCXQ!>dE4Dw2~%R=^|>^UmVXN<s=4l^kjaG&Gd- > zo4mzqTNCe%4HhRUL=xUcMRxtseQ^1g3<KgNpY-D2CCbMuoEs!$mx92;6ra4udHBHV > z;G-=W`(33_%vNj?0*oh4b6Pr?OfiszBQ*%3*82;B$uLH5qdKY<Gb*KVB0h?Cn4H~w > zD;;TGnP^mHa`fhK+o%?W>neE4$Ig`W9c_JQMoG|*j_@mbk_TPd;(&2E*zB>V>1num > zHm*iml0!knXJ|+YHK+~>d4Qu8LtaD>6IGDO$9LkC$-;3Tp4f$Fi5w*XVKg;u4rH<s > zCNkS`5`xAyg^2ITMToPVCJC&(ZkN|5v}QPt!}BSCiUFFoB_$=xI*6|T`4<_8g?bV} > zgwPpn^GfkSRC)^1B2B_RTtc<X;-WqYSdbj98!$G^PNga&a+fK-_P^JqA<12-s?4_v > zNwBa$oq{B~!sLu40bCp}VF-j+O-3+@l8Zn=X$i0c?9dCNvrttgj6__Jg*j*e6W9RJ > z1Tvr!Gyx!zwPAX(stJ|lIT^S{3OKTAw6wdC8pkyl;{1zhA~2RLkPSJ&EtFA^FvDO- > z;l;*q6(b{vQehg<SA`iWl|`&fD3)!sC30Z6A%;S+NR$WWb+xFMI9mmK73id@vGh$8 > zrC_8En!*w1UQC3L+IzxLfr`oyj5`I%Wyy^gP(J7`Q*+n>iRKOz!CfF?*^4J}sXL%8 > zI2Y_G>zxOvFN)mp1aTt@I260O&PoXX@ESBot>AqX;iy|iB(;db(TKyC>S_gGfM`@{ > zV$W^3j}7)gxS@xPjPkEk1jzAL#X;l8RVM`rs`Pqo8{XFne;62fJU&{WqAF^+X~#{F > z!J1;4gndYWf0<y#NS!Y|(@^YMRR51&l{Vp)g{2Gq3JlSSQ`(xOM9BPCMi-&7JrbZ; > z8+)|1W}OklKR+@`6*Wn+t1gVTrLyyx601o<8qkrab(aN5+M3OGBjzwt*yzmXP>YWX > zqepVh-`8$r-oK#~z_)ZZ)mcZX&uce$Bmy8qmt)w!+3+WifQEsT4228#8~&|K3P`9; > S{udf|{x0N-aG@d2C4?HBH1tyd > > literal 0 > HcmV?d00001 >
