Re: [PATCH] Handle DW_AT_decl_file 0
Hi Mark, On Mon, Feb 12, 2024 at 4:14 PM Mark Wielaard 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 wrote: > > > >(void) INTUSE(dwarf_getsrclines) ( (cu), , ); > > > > - 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
Re: [PATCH] Handle DW_AT_decl_file 0
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 wrote: > > >(void) INTUSE(dwarf_getsrclines) ( (cu), , ); > > > - 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
Re: [PATCH] Handle DW_AT_decl_file 0
Hi Mark, On Mon, Feb 12, 2024 at 12:31 PM Mark Wielaard wrote: > >(void) INTUSE(dwarf_getsrclines) ( (cu), , ); > > - 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
Re: [PATCH] Handle DW_AT_decl_file 0
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=3 > > Signed-off-by: Aaron Merey > --- > 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 > #endif > > -#include > #include > #include "libdwP.h" > > @@ -48,13 +47,6 @@ dwarf_decl_file (Dwarf_Die *die) > ) != 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) ( (cu), , ); > - 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 \ >
[PATCH] Handle DW_AT_decl_file 0
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=3 Signed-off-by: Aaron Merey --- 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 #endif -#include #include #include "libdwP.h" @@ -48,13 +47,6 @@ dwarf_decl_file (Dwarf_Die *die) ) != 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) ( (cu), , ); - 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 ..ab62b707bd7371268d6e685a2f86a1a3a868b0a9 GIT binary patch literal 2764 zcmV;-3N!UWT4*^jL0KkKS?3bM4FC%f|NsC0|Nrmr|NH;%|NsBz|NsC0Y46D+q(klP z_GEV7|6kwvWB?5S00E!|h!aMdntDLhJSItiq3U^0)f#B@ntFzs z000kAWYa*NhhJXR4jD~;!41flWFeaKAGyq761dR-6)G8o+yQEuT0j3ws|*~V~G%!nj`>pELHrd!u%YwKaMQF=^V6yoOE5?X2@ zAafd`~H9AZ>f5^t-XG(SSp>!ekGkT%;963hq(I|)o+t(C9o`~IK4