Re: [PATCH] Handle DW_AT_decl_file 0

[Mark Wielaard <mark@klomp.org>]

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 <amerey@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
>