* [PATCH] Handle DW_AT_decl_file 0
@ 2024-02-10 2:52 Aaron Merey
2024-02-12 17:31 ` Mark Wielaard
0 siblings, 1 reply; 5+ messages in thread
From: Aaron Merey @ 2024-02-10 2:52 UTC (permalink / raw)
To: elfutils-devel; +Cc: Aaron Merey
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.
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.
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;
- }
-
/* 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);
}
- 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;
}
- 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;
+ }
if (idx >= cu->files->nfiles)
{
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
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
+
+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
+
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
--
2.43.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Handle DW_AT_decl_file 0
2024-02-10 2:52 [PATCH] Handle DW_AT_decl_file 0 Aaron Merey
@ 2024-02-12 17:31 ` Mark Wielaard
2024-02-12 18:16 ` Aaron Merey
0 siblings, 1 reply; 5+ messages in thread
From: Mark Wielaard @ 2024-02-12 17:31 UTC (permalink / raw)
To: Aaron Merey, elfutils-devel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Handle DW_AT_decl_file 0
2024-02-12 17:31 ` Mark Wielaard
@ 2024-02-12 18:16 ` Aaron Merey
2024-02-12 21:13 ` Mark Wielaard
0 siblings, 1 reply; 5+ messages in thread
From: Aaron Merey @ 2024-02-12 18:16 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi Mark,
On Mon, Feb 12, 2024 at 12:31 PM Mark Wielaard <mark@klomp.org> wrote:
> > (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?
My thinking was to rely on dwarf_getsrclines setting the libdw errno
if an error occurred. If we always use DWARF_E_INVALID_DWARF then we
might overwrite an error code that describes the failure more specifically.
If we want to ensure that the libdw errno is set whenever we reach this
condition, we could check if dwarf_getsrclines set the errno. If it did,
then just leave that errno set. If it didn't, then set errno 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.
Ok, fixed.
> > --- 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.
It doesn't need to be -O0. I'll just leave out -O altogether.
Aaron
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Handle DW_AT_decl_file 0
2024-02-12 18:16 ` Aaron Merey
@ 2024-02-12 21:13 ` Mark Wielaard
2024-02-13 1:44 ` Aaron Merey
0 siblings, 1 reply; 5+ messages in thread
From: Mark Wielaard @ 2024-02-12 21:13 UTC (permalink / raw)
To: Aaron Merey; +Cc: elfutils-devel
Hi Aaron,
On Mon, Feb 12, 2024 at 01:16:30PM -0500, Aaron Merey wrote:
> On Mon, Feb 12, 2024 at 12:31 PM Mark Wielaard <mark@klomp.org> wrote:
> > > (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?
>
> My thinking was to rely on dwarf_getsrclines setting the libdw errno
> if an error occurred. If we always use DWARF_E_INVALID_DWARF then we
> might overwrite an error code that describes the failure more specifically.
Ah, yes. That makes sense. But because of caching dwarf_getsrclines
only sets an error on first try.
> If we want to ensure that the libdw errno is set whenever we reach this
> condition, we could check if dwarf_getsrclines set the errno. If it did,
> then just leave that errno set. If it didn't, then set errno to
> DWARF_E_INVALID_DWARF.
Good idea. Or we could (also) cache the error in the cu
(files_libdwerr?), that is what e.g. dwfl_module_getdwarf does
(see mod->dwerr). But I think either solution is more like a
redesign/factoring. And you might consider doing it separate from this
bug fix.
If you have time, you could then also look into this
(performance/caching) issue:
https://sourceware.org/bugzilla/show_bug.cgi?id=27405
"libdw_get_srcfiles should not imply srclines"
Cheers,
Mark
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Handle DW_AT_decl_file 0
2024-02-12 21:13 ` Mark Wielaard
@ 2024-02-13 1:44 ` Aaron Merey
0 siblings, 0 replies; 5+ messages in thread
From: Aaron Merey @ 2024-02-13 1:44 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi Mark,
On Mon, Feb 12, 2024 at 4:14 PM Mark Wielaard <mark@klomp.org> wrote:
>
> On Mon, Feb 12, 2024 at 01:16:30PM -0500, Aaron Merey wrote:
> > On Mon, Feb 12, 2024 at 12:31 PM Mark Wielaard <mark@klomp.org> wrote:
> > > > (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?
> >
> > My thinking was to rely on dwarf_getsrclines setting the libdw errno
> > if an error occurred. If we always use DWARF_E_INVALID_DWARF then we
> > might overwrite an error code that describes the failure more specifically.
>
> Ah, yes. That makes sense. But because of caching dwarf_getsrclines
> only sets an error on first try.
>
> > If we want to ensure that the libdw errno is set whenever we reach this
> > condition, we could check if dwarf_getsrclines set the errno. If it did,
> > then just leave that errno set. If it didn't, then set errno to
> > DWARF_E_INVALID_DWARF.
>
> Good idea. Or we could (also) cache the error in the cu
> (files_libdwerr?), that is what e.g. dwfl_module_getdwarf does
> (see mod->dwerr). But I think either solution is more like a
> redesign/factoring. And you might consider doing it separate from this
> bug fix.
>
> If you have time, you could then also look into this
> (performance/caching) issue:
> https://sourceware.org/bugzilla/show_bug.cgi?id=27405
> "libdw_get_srcfiles should not imply srclines"
Ok I'll look at PR27405. I've removed the error handling changes from
this patch. I also recompiled the testfile without -O0. It didn't end up
improving the size of the testfile but the -O0 was unnecessary either way.
Pushed as commit add63e0317b6e.
Aaron
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-13 1:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-10 2:52 [PATCH] Handle DW_AT_decl_file 0 Aaron Merey
2024-02-12 17:31 ` Mark Wielaard
2024-02-12 18:16 ` Aaron Merey
2024-02-12 21:13 ` Mark Wielaard
2024-02-13 1:44 ` Aaron Merey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).