[Bug libdw/31111] Handle Clang DWARF 5 DW_AT_decl_file 0
https://sourceware.org/bugzilla/show_bug.cgi?id=3 Aaron Merey changed: What|Removed |Added CC||amerey at redhat dot com Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #5 from Aaron Merey --- Fixed in the following commit: commit add63e0317b6e27dd4077b19e998e6c56a3becb5 Author: Aaron Merey Date: Fri Feb 9 21:10:19 2024 -0500 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 -- You are receiving this mail because: You are on the CC list for the bug.
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] PR31265 - rework debuginfod archive-extract fdcache
Hi -
I plan to roll out this improvement to all the servers I look after,
via COPR builds or such, it's that impactful.
commit 7bfc10acc7f0e00c5bc45416d1bf8ee183d79ff3 (HEAD -> master)
Author: Frank Ch. Eigler
Date: Thu Feb 8 19:33:55 2024 -0500
PR31265 - rework debuginfod archive-extract fdcache
Completely replace the "fdcache" algorithms in debuginfod, which
manages files extracted from archives. Previous logic was a LRU queue
for files requested by users, and a separate LRU queue for prefetched
files found nearby the ones users requested. The code did not handle
annoying edge cases like infrequently accessed but very costly
extraction of files like fedora kernels' vdso.debug. In addition, the
queue was searched linearly for normal lookups. It was also
unceremoniously dropped at each groom cycle.
New code replaces this with an indexed datastructure for quick
lookups, and extra metadata for use during eviction decisions. Each
entry tracks size and such, but now also tracks how recently and how
many times it was requested, how long it took to originally extract.
The new code combines these quantities in a score, by which eviction
eligibility is ranked. Intuitively, the effect is to prefer to hoard
small / slow-to-access files, and prefer to jettison large / fast /
never accessed ones.
It's a tricky thing to balance. The parameters in this configuration
were tested by timing-accurate replaying a few days' worth of actual
traffic of the main fedora debuginfod server. The peer
debuginfod.stg.fedoraproject.org runs the new code. It shows good
performance, excellent use of the cache storage, and strong preference
to hold onto those vdso.debug files. But who knows, it might need
tweaking later. The new code adds more prometheus metrics to make it
possible to grok the effectiveness of the few remaining
fdcache-related options.
Patch includes doc updates and NEWS. The changes are invisible to the
testsuite (except with respect to the new metrics). Code changes are
focused on all the member functions of class libarchive_fdcache, and
their callers. Unused parameters are removed, with previous command
line options hidden/accepted/ignored. Some other minor error-path
tempfile-gc was fixed in the extraction paths.
Signed-Off-By: Frank Ch. Eigler
diff --git a/NEWS b/NEWS
index 98dc78f5130b..21263456b9b7 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
Version 0.191 (after 0.190)
+debuginfod: Caching eviction logic improvements to improve retention
+of small/frequent/slow files such as Fedora's vdso.debug.
+
srcfiles: Can now fetch the source files of a DWARF/ELF file and
place them into a zip.
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 6b21f46f70ea..63424e59c28c 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1,5 +1,5 @@
/* Debuginfo-over-http server.
- Copyright (C) 2019-2021 Red Hat, Inc.
+ Copyright (C) 2019-2024 Red Hat, Inc.
Copyright (C) 2021, 2022 Mark J. Wielaard
This file is part of elfutils.
@@ -69,6 +69,8 @@ extern "C" {
#include
#include
#include
+#include
+#include
/* If fts.h is included before config.h, its indirect inclusions may not
@@ -417,7 +419,7 @@ static const struct argp_option options[] =
{ "verbose", 'v', NULL, 0, "Increase verbosity.", 0 },
{ "regex-groom", 'r', NULL, 0,"Uses regexes from -I and -X arguments to
groom the database.",0},
#define ARGP_KEY_FDCACHE_FDS 0x1001
- { "fdcache-fds", ARGP_KEY_FDCACHE_FDS, "NUM", 0, "Maximum number of archive
files to keep in fdcache.", 0 },
+ { "fdcache-fds", ARGP_KEY_FDCACHE_FDS, "NUM", OPTION_HIDDEN, NULL, 0 },
#define ARGP_KEY_FDCACHE_MBS 0x1002
{ "fdcache-mbs", ARGP_KEY_FDCACHE_MBS, "MB", 0, "Maximum total size of
archive file fdcache.", 0 },
#define ARGP_KEY_FDCACHE_PREFETCH 0x1003
@@ -425,11 +427,9 @@ static const struct argp_option options[] =
#define ARGP_KEY_FDCACHE_MINTMP 0x1004
{ "fdcache-mintmp", ARGP_KEY_FDCACHE_MINTMP, "NUM", 0, "Minimum free space%
on tmpdir.", 0 },
#define ARGP_KEY_FDCACHE_PREFETCH_MBS 0x1005
- { "fdcache-prefetch-mbs", ARGP_KEY_FDCACHE_PREFETCH_MBS, "MB", 0,"Megabytes
allocated to the \
- prefetch cache.", 0},
+ { "fdcache-prefetch-mbs", ARGP_KEY_FDCACHE_PREFETCH_MBS, "MB",
OPTION_HIDDEN, NULL, 0},
#define ARGP_KEY_FDCACHE_PREFETCH_FDS 0x1006
- { "fdcache-prefetch-fds", ARGP_KEY_FDCACHE_PREFETCH_FDS, "NUM", 0,"Number
of files allocated to the \
- prefetch cache.", 0},
+ { "fdcache-prefetch-fds", ARGP_KEY_FDCACHE_PREFETCH_FDS, "NUM",
OPTION_HIDDEN, NULL, 0},
#define ARGP_KEY_FORWARDED_TTL_LIMIT 0x1007
{"forwarded-ttl-limit", ARGP_KEY_FORWARDED_TTL_LIMIT, "NUM", 0, "Limit of
X-Forwarded-For hops, default 8.", 0},
#define ARGP_KEY_PASSIVE 0x1008
@@ -482,12 +482,9 @@ static
