Re: [PATCH] readelf: add pretty printing for FDO Dlopen Metadata note
Hi Luca,
On Fri, May 10, 2024 at 10:58:02PM +0100, luca.bocca...@gmail.com wrote:
> Note that the webpage in the comment is not published yet,
> it will be next week when the next systemd RC is tagged.
> The document can be viewed right now on github at:
> https://github.com/systemd/systemd/blob/main/docs/ELF_DLOPEN_METADATA.md
>
> But the node ID and the string format are now fixed, even
> if the content of the string might change, it will still be
> a string.
Not a fan of json, feels very un-ELF. But it is what it is. The patch
looks OK. Could you let us know when the elf.h change is accepted in
glibc, then we'll sync and integrate this.
Thanks,
Mark
>
> Signed-off-by: Luca Boccassi
> ---
> libebl/eblobjnote.c | 9 +++--
> libebl/eblobjnotetypename.c | 3 +++
> libelf/elf.h| 4
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/libebl/eblobjnote.c b/libebl/eblobjnote.c
> index 1ba5d8b3..ad3f49de 100644
> --- a/libebl/eblobjnote.c
> +++ b/libebl/eblobjnote.c
> @@ -288,9 +288,14 @@ ebl_object_note (Ebl *ebl, uint32_t namesz, const char
> *name, uint32_t type,
>if (descsz == 0 && type == NT_VERSION)
> return;
>
> - if (strcmp ("FDO", name) == 0 && type == NT_FDO_PACKAGING_METADATA
> + if (strcmp ("FDO", name) == 0
> && descsz > 0 && desc[descsz - 1] == '\0')
> - printf("Packaging Metadata: %.*s\n", (int) descsz, desc);
> + {
> + if (type == NT_FDO_PACKAGING_METADATA)
> + printf("Packaging Metadata: %.*s\n", (int) descsz, desc);
> + else if (type == NT_FDO_DLOPEN_METADATA)
> + printf("Dlopen Metadata: %.*s\n", (int) descsz, desc);
> + }
>
>/* Everything else should have the "GNU" owner name. */
>if (strcmp ("GNU", name) != 0)
> diff --git a/libebl/eblobjnotetypename.c b/libebl/eblobjnotetypename.c
> index 473a1f2f..79ff010a 100644
> --- a/libebl/eblobjnotetypename.c
> +++ b/libebl/eblobjnotetypename.c
> @@ -104,6 +104,9 @@ ebl_object_note_type_name (Ebl *ebl, const char *name,
> uint32_t type,
>if (strcmp (name, "FDO") == 0 && type == NT_FDO_PACKAGING_METADATA)
> return "FDO_PACKAGING_METADATA";
>
> + if (strcmp (name, "FDO") == 0 && type == NT_FDO_DLOPEN_METADATA)
> + return "FDO_DLOPEN_METADATA";
> +
>if (strcmp (name, "GNU") != 0)
> {
> /* NT_VERSION is special, all data is in the name. */
> diff --git a/libelf/elf.h b/libelf/elf.h
> index f2206e5c..bbf4565c 100644
> --- a/libelf/elf.h
> +++ b/libelf/elf.h
> @@ -1336,6 +1336,10 @@ typedef struct
> https://systemd.io/COREDUMP_PACKAGE_METADATA/ */
> #define NT_FDO_PACKAGING_METADATA 0xcafe1a7e
>
> +/* dlopen metadata as defined on
> + https://systemd.io/ELF_DLOPEN_METADATA/ */
> +#define NT_FDO_DLOPEN_METADATA 0x407c0c0a
> +
> /* Note section name of program property. */
> #define NOTE_GNU_PROPERTY_SECTION_NAME ".note.gnu.property"
>
> --
> 2.39.2
>
Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
Hi, I think things worked out in the end, so that is good. Personally I didn't think Dmitry's request to take his review into account was exaggerating. But that might be because I know him and am happy with his suggestions in general. If the tone of some request was interpreted as "not collegial and exaggerated", then maybe next time also provide an example of how to formulate the request that sounds better. Tone is a difficult thing, especially on the mailinglist. Cheers, Mark
Re: [rfc] [patch] PR28204: debuginfod ima signature verification
Hi Aaron, On Thu, 2024-05-09 at 13:56 -0400, Aaron Merey wrote: > I know there's already been a lot of discussion re. ima:permissive and > I'm weighing in rather late, but FWIW I do support including it. > Currently individual ELF sections cannot be downloaded when > ima:enforcing is active. With ima:permissive we could support proper > section queries while also being able to perform some amount of > ima verification. But what would "some amount of ima verification" mean? I think we (me included, for suggesting some of it in the first place) made things way to complicated by supporting multiple different ima certificates and then also splitting ima verification policy per server URL. If we also add different policies for the "amount" of ima we do then it because really hard to reason about imho. We should probably take a step back and formulate the security attack we are trying to defend against with ima verification first. Cheers, Mark
Re: [PATCH] readelf: Fix printing of DW_FORM_strx and DW_MACRO parsing
Hi, On Sun, 2024-05-05 at 00:15 +0200, Mark Wielaard wrote: > print_form_data didn't take the offset_len (4 or 8 bytes) into account > causing the wrong entry to be read from .debug_str_offsets. > print_debug_macro_section did sanity checking before calling > print_form_data, which does sanity checking itself. The sanity check > for DW_FORM_strx was wrong in print_debug_macro_section (but correct > in print_form_data). > > Add a new testfile for run-readelf-macro.sh, this one compiled with > clang -gdwarf-5 -fdebug-macro. > > * src/readelf.c (print_form_data): Multiply by offset_len for > strx_val. > (print_debug_macro_section): Remove sanity checks before calling > print_form_data. > * tests/testfileclangmacro.bz2: New testfile. > * tests/Makefile.am (EXTRA_DIST): Add testfileclangmacro.bz2. > * tests/run-readelf-macro.sh: Add testfileclangmacro output. Pushed to main. Cheers, Mark
Re: [PATCH] ar: Replace one alloca use by xmalloc
Hi Aaron, On Fri, May 10, 2024 at 05:16:06PM -0400, Aaron Merey wrote: > On Tue, Apr 30, 2024 at 10:39 AM Mark Wielaard wrote: > > > > This alloca use is inside a lexical block and is used to replace one > > element of argv. Use a function local variable, xmalloc and free to > > make memory usage pattern more clear. > > > > * src/ar.c (main): Move newp char pointer declaration up. > > Use xmalloc to allocate space. free at end of main. > > > > Signed-off-by: Mark Wielaard > > LGTM. This patch shouldn't change anything except possibly help > some static analyzers detect correct memory usage. Thanks. Pushed as commit 3c71cab7c5bfba0549d0a1716e7061d07eafd794 Cheers, Mark
[PATCH] readelf: Fix printing of DW_FORM_strx and DW_MACRO parsing
print_form_data didn't take the offset_len (4 or 8 bytes) into account
causing the wrong entry to be read from .debug_str_offsets.
print_debug_macro_section did sanity checking before calling
print_form_data, which does sanity checking itself. The sanity check
for DW_FORM_strx was wrong in print_debug_macro_section (but correct
in print_form_data).
Add a new testfile for run-readelf-macro.sh, this one compiled with
clang -gdwarf-5 -fdebug-macro.
* src/readelf.c (print_form_data): Multiply by offset_len for
strx_val.
(print_debug_macro_section): Remove sanity checks before calling
print_form_data.
* tests/testfileclangmacro.bz2: New testfile.
* tests/Makefile.am (EXTRA_DIST): Add testfileclangmacro.bz2.
* tests/run-readelf-macro.sh: Add testfileclangmacro output.
Signed-off-by: Mark Wielaard
---
src/readelf.c| 13 +-
tests/Makefile.am| 2 +-
tests/run-readelf-macro.sh | 817 ++-
tests/testfileclangmacro.bz2 | Bin 0 -> 7985 bytes
4 files changed, 820 insertions(+), 12 deletions(-)
create mode 100755 tests/testfileclangmacro.bz2
diff --git a/src/readelf.c b/src/readelf.c
index 0e931184ed38..c945b371966f 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -8869,11 +8869,12 @@ print_form_data (Dwarf *dbg, int form, const unsigned
char *readp,
strx_val:
data = dbg->sectiondata[IDX_debug_str_offsets];
if (data == NULL
- || data->d_size - str_offsets_base < val)
+ || data->d_size - str_offsets_base < val * offset_len)
str = "???";
else
{
- const unsigned char *strreadp = data->d_buf + str_offsets_base + val;
+ const unsigned char *strreadp = (data->d_buf + str_offsets_base
+ + val * offset_len);
const unsigned char *strreadendp = data->d_buf + data->d_size;
if ((size_t) (strreadendp - strreadp) < offset_len)
str = "???";
@@ -10853,8 +10854,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod
__attribute__ ((unused)),
case DW_MACRO_define_sup:
get_uleb128 (u128, readp, readendp);
- if (readp + offset_len > readendp)
- goto invalid_data;
printf ("%*s#define ", level, "");
readp = print_form_data (dbg, DW_FORM_strp_sup,
readp, readendp, offset_len,
@@ -10864,8 +10863,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod
__attribute__ ((unused)),
case DW_MACRO_undef_sup:
get_uleb128 (u128, readp, readendp);
- if (readp + offset_len > readendp)
- goto invalid_data;
printf ("%*s#undef ", level, "");
readp = print_form_data (dbg, DW_FORM_strp_sup,
readp, readendp, offset_len,
@@ -10887,8 +10884,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod
__attribute__ ((unused)),
case DW_MACRO_define_strx:
get_uleb128 (u128, readp, readendp);
- if (readp + offset_len > readendp)
- goto invalid_data;
printf ("%*s#define ", level, "");
readp = print_form_data (dbg, DW_FORM_strx,
readp, readendp, offset_len,
@@ -10898,8 +10893,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod
__attribute__ ((unused)),
case DW_MACRO_undef_strx:
get_uleb128 (u128, readp, readendp);
- if (readp + offset_len > readendp)
- goto invalid_data;
printf ("%*s#undef ", level, "");
readp = print_form_data (dbg, DW_FORM_strx,
readp, readendp, offset_len,
diff --git a/tests/Makefile.am b/tests/Makefile.am
index b7fb72384f32..46f553a9b991 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -378,7 +378,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
testfile46.bz2 testfile47.bz2 testfile48.bz2 testfile48.debug.bz2 \
testfile49.bz2 testfile50.bz2 testfile51.bz2 \
testfile-macros-0xff.bz2 \
-run-readelf-macro.sh testfilemacro.bz2 \
+run-readelf-macro.sh testfilemacro.bz2 testfileclangmacro.bz2 \
run-readelf-loc.sh testfileloc.bz2 \
splitdwarf4-not-split4.dwo.bz2 \
testfile-splitdwarf4-not-split4.debug.bz2 \
diff --git a/tests/run-readelf-macro.sh b/tests/run-readelf-macro.sh
index 8b17f7da6e05..92401b59ff35 100755
--- a/tests/run-readelf-macro.sh
+++ b/tests/run-readelf-macro.sh
@@ -342,4 +342,819 @@ DWARF section [32] '.debug_macro' at offset 0x2480:
EOF
-exit 0
+# Same sources as above, but now using clang
+# gcc -g3 -c hello.c
+# gcc -g3 -c world
[PATCH] ar: Replace one alloca use by xmalloc
This alloca use is inside a lexical block and is used to replace one
element of argv. Use a function local variable, xmalloc and free to
make memory usage pattern more clear.
* src/ar.c (main): Move newp char pointer declaration up.
Use xmalloc to allocate space. free at end of main.
Signed-off-by: Mark Wielaard
---
src/ar.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/ar.c b/src/ar.c
index e6d6d58f2b3b..fcb8bfb90a9f 100644
--- a/src/ar.c
+++ b/src/ar.c
@@ -41,6 +41,7 @@
#include
#include
+#include "libeu.h"
#include "arlib.h"
@@ -154,10 +155,11 @@ main (int argc, char *argv[])
/* For historical reasons the options in the first parameter need
not be preceded by a dash. Add it now if necessary. */
+ char *newp = NULL;
if (argc > 1 && argv[1][0] != '-')
{
size_t len = strlen (argv[1]) + 1;
- char *newp = alloca (len + 1);
+ newp = (char *) xmalloc (len + 1);
newp[0] = '-';
memcpy ([1], argv[1], len);
argv[1] = newp;
@@ -271,6 +273,8 @@ MEMBER parameter required for 'a', 'b', and 'i'
modifiers"));
break;
}
+ free (newp);
+
return status;
}
--
2.44.0
Re: [rfc] [patch] PR28204: debuginfod ima signature verification
Hi Frank, On Wed, Apr 10, 2024 at 05:01:36PM -0400, Frank Ch. Eigler wrote: > > > - to drop "permissive" mode > > > > We discussed a bit on irc about "wording". But I think it isn't really > > how it is worded, but that there is just different features. What is > > called "enforcing" is an authenticity scheme. While "permissive" is > > more like an (optional) error-detecting mode. IMHO it makes sense to > > simply separate those. > > For the record, on IRC, I presented these two additional arguments: > > > in the case of federated debuginfod servers, such as > debuginfod.elfutils.org, a user is stuck with "ignore" mode > operation, because not all upstream servers will have ima > signatures to pass. this sacrifices all possible assurance that > could come from the federated server relaying ima signatures from > those servers that have them > > even in non-federated cases such as fedoraproject.org, the ideal > case for "enforcing" mode as a default, it will fail the moment > the user happens to try to debug some pre-fedora-38 binary that > may be sitting on the system --- because those rpms just don't > have signatures available at all > > for both these scenarios, the original "permissive" mode would be > suitable > > > IOW, without a "permissive" mode being available at all, we could not > ask users to enable this new code at all for our own federated > servers, nor even for fedora. That's because no server can guarantee > the availability of signatures for all content they can serve. I don't understand why you say we cannot ask users to enable the ima checking code. Isn't the goal to make sure that the user only gets ima-signed/verified files for the distro they are debugging/analyzing on? Especially if a server can also provide non-verifiable files, then you would want to have strict checking to make sure. > > That way you don't have a authentication scheme that is easily > > defeated (when put in "permissive" mode). > > This "easily defeated" theory needs a threat model. It sounds like > you are thinking of > - an evil debuginfod instance that > - you trust enough to list in DEBUGINFOD_URLS > - but not enough to label it with "ima:enforcing" > - which may strip the X-DEBUGINFOD-FILE-SIGNATURE header" en route > then yeah, but sounds far-fetched rather than easy. I think I simply don't understand what the thread model is that ima:permissive guards against. It seems not to protect against the main thing you want to do ima checking for. You only want debuginfod-client to provide files that are signed and can be verified. > > And you can implement a simpler error-detection mode that can work > > in more cases (by using the executable .gnu_debuglink CRC) > > No, we already know that this is incapable of checking e.g. source > files. And there is no separate CRC for executables vs. debuginfo > files. And note that this provides zero protection in the same threat > model above (as the CRC field could be MITM'd). Sure. I guess if you don't enforce ima checking you can just rely on https. And it is kind of the point that this is similar to the threat model that "permissive" guards against (random, bit flipping, accidential replacement of files). Relying on https and CRC checking seems simpler to understand for that. But you are right that there isn't really anyhing for the main executables. For the source files you could check the lenght and time stamps (or the MD5 sums if added by the DWARF producer). > > [...] > > One "big picture" question is whether this should be a per server URL > > policy or something that is enabled/disabled for all server URLs? > > That makes it less "flexible" but should simplify things a bit for the > > user (and the server urls parsing). > > Blame some guy named Mark for getting Ryan to build that out last year. :-) > > https://sourceware.org/pipermail/elfutils-devel/2023q3/006299.html Sorry. I see back then I also didn't get use case for the "permissive" policy. But yes, it might have been a mistake to suggest different policies per URL/server. Cheers, Mark
Re: [PATCH] libdw: dwarf_getsrcfiles should not imply dwarf_getsrclines
Hi Aaron, On Wed, Apr 10, 2024 at 04:43:53PM -0400, Aaron Merey wrote: > > > - /* Pass the file data structure to the caller. */ > > > - if (filesp != NULL) > > > -*filesp = files; > > > + const char **newdirs = (void *) >info[nnewfiles]; > > > + const char **prevdirs = (void *) >info[nprevfiles]; > > > + > > > + /* Copy prevdirs to newdirs. */ > > > + for (size_t n = 0; n < ndirs; n++) > > > + newdirs[n] = prevdirs[n]; > > > > Again slightly off indentation. > > But I also don't fully follow the prevdirs/newdirs copying. > > Why is this? No newdirs are defined here, are there? > > Maybe I don't understand the data-structure used here. > > The directories are the same but we still need to copy them so that > dwarf_getsrcdirs can find newfiles' dir names. > > Dwarf_Files is an unusual structure since it doesn't contain a member > specifically for the array of dirnames. Instead they're stored at > the end of the Dwarf_Fileinfo array member. Aha. Thanks for explaining. Clearly I should not try to review code if I don't understand the underlying data structures. > > So testfile-define-file is actually testfile36.debug but with a new > > line program? How did you edit/insert that one? > > I used xxd to create a hexdump of testfile36.debug and modified the line > program by hand with a text editor. I converted the modified hexdump > back to a binary with xxd -r. > > I chose testfile36.debug because its .debug_line includes multiple > directory and file names. It also contains a single short line program > that was easy to replace with two DW_LNE_define_file opcodes without > corrupting things. Fun. Could you add a small description to tests/run-get-files.sh where testfile-define-file is use so future hackers know how the file was created? All looks good BTW. Please do push (if possible with the above change). Thanks, Mark
Re: [PATCH] libdw: dwarf_getsrcfiles should not imply dwarf_getsrclines
Hi Aaron,
On Tue, 2024-04-09 at 23:45 -0400, Aaron Merey wrote:
> dwarf_getsrcfiles causes line data to be read in addition to file data.
> This is wasteful for programs which only need file or directory names.
> Debuginfod server is one such example.
>
> Fix this by moving the srcfile reading in read_srclines into a separate
> function read_srcfiles. This change improves debuginfod server's max
> resident set size by up to 75% during rpm indexing.
>
> * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Replace
> dwarf_getsrclines and __libdw_getsrclines with
> __libdw_getsrcfiles.
> * libdw/dwarf_getsrclines.c (read_line_header): New function.
> (read_srcfiles): New function.
> (read_srclines): Move srcfile reading into read_srcfiles.
> Add parameter to use cached srcfiles if available.
> Also merge srcfiles with any files from DW_LNE_define_file.
> (__libdw_getsrclines): Changed to call get_lines_or_files.
> (__libdw_getsrcfiles): New function. Calls get_lines_or_files.
> (get_lines_or_files): New function based on the old
> __libdw_getsrclines. Call read_srcfiles if linesp is NULL,
> otherwise call read_srclines. Pass previously read srcfiles
> to read_srclines if available.
> * libdw/dwarf_macro_getsrcfiles.c (dwarf_macro_getsrcfiles):
> Replace __libdw_getsrclines with __libdw_getsrcfiles.
> * libdw/libdwP.h (__libdw_getsrcfiles): New declaration.
> * tests/.gitignore: Add new test binary.
> * tests/get-files.c: Verify that dwarf_getsrcfiles does
> not cause srclines to be read.
> * tests/get-lines.c: Verify that srclines can be read
> after srcfiles have been read.
> * tests/Makefile.am: Add new testfiles.
> * tests/get-files-define-file.c: Print file names before
> and after reading DW_LNE_define_file.
> * tests/run-get-files.sh: Add get-files-define-file test.
> * tests/testfile-define-file.bz2: New testfile. Copy of
> testfile36.debug but with a line program consisting of two
> DW_LNE_define_file opcodes.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=27405
>
> Signed-off-by: Aaron Merey
> ---
>
> v2 changes:
> Restored support for DW_LNE_define_file.
Great. And sorry I first suggested to just drop it and then said I
would like it back. This was more work than I though.
> Added DW_LNE_define_file testcase and testfile.
Nice. How did you edit this file?
> Added new function get_lines_or_files which is based on the old
> __libdw_getsrclines. __libdw_getsrclines and __libdw_getsrcfiles now
> call get_lines_or_files.
It is just a simple rename, but so much nicer to my eyes. Thanks.
> Mentioned max resident set size improvement in the commit message. I
> did not mention a 5% speed up to rpm indexing that I have previously
> talked about since I could not reliably reproduce it with v2 of the patch.
> I suspect that speed up may have been due to the smaller max RSS reducing
> swapping during that round of testing.
Makes sense. And a reduction of 75% of the max resident size it pretty
huge in itself.
> +case DW_LNE_define_file:
> + {
> +char *fname = (char *) linep;
> +uint8_t *endp = memchr (linep, '\0', lineendp - linep);
> +if (endp == NULL)
> + goto invalid_data;
> +size_t fnamelen = endp - linep;
> +linep = endp + 1;
> +
> +unsigned int diridx;
> +if (unlikely (linep >= lineendp))
> + goto invalid_data;
> +get_uleb128 (diridx, linep, lineendp);
> +
> + size_t ndirs = (*filesp)->ndirs;
This one line uses tabs for indentation, all others spaces.
> +if (unlikely (diridx >= ndirs))
> + {
> +__libdw_seterrno (DWARF_E_INVALID_DIR_IDX);
> +goto invalid_data;
> + }
> +Dwarf_Word mtime;
> +if (unlikely (linep >= lineendp))
> + goto invalid_data;
> +get_uleb128 (mtime, linep, lineendp);
> +Dwarf_Word filelength;
> +if (unlikely (linep >= lineendp))
> + goto invalid_data;
> +get_uleb128 (filelength, linep, lineendp);
> +
> + /* Add new_file to filelist that will be merged with filesp. */
> +struct filelist *new_file = malloc (sizeof (struct
> filelist));
> + if (unlikely (new_file == NULL))
> {
> - __libdw_seterrno (DWARF_E_INVALID_DIR_IDX);
> - goto invalid_data;
> + __libdw_seterrno (DWARF_E_NOMEM);
> + goto out;
> }
And this last block uses tabs again.
> - Dwarf_Word mtime;
> - if (unlikely (linep >= lineendp))
> - goto
Re: [rfc] [patch] PR28204: debuginfod ima signature verification
Hi Frank,
On Wed, 2024-04-03 at 17:04 -0400, Frank Ch. Eigler wrote:
> The following raw diff reworks this long-blocked patch to overcome
> these three objections last fall:
>
> - to drop "permissive" mode
We discussed a bit on irc about "wording". But I think it isn't really
how it is worded, but that there is just different features. What is
called "enforcing" is an authenticity scheme. While "permissive" is
more like an (optional) error-detecting mode. IMHO it makes sense to
simply separate those. That way you don't have a authentication scheme
that is easily defeated (when put in "permissive" mode). And you can
implement a simpler error-detection mode that can work in more cases
(by using the executable .gnu_debuglink CRC)
> - to stop redistributing published distro ima certificates
> - to not use libimaevm.so (due to concurrency / licensing concerns)
Nice, you managed to open code it with just openssl primitives.
> This is a raw diff only. I'll be proposing some changes shortly
> downthread.
Understood. Just a few quick comments below.
One "big picture" question is whether this should be a per server URL
policy or something that is enabled/disabled for all server URLs?
That makes it less "flexible" but should simplify things a bit for the
user (and the server urls parsing).
Also is this all rpm/koji specific? Or do other distros also use ima
signatures, but encode/store them differently?
> diff --git a/config/Makefile.am b/config/Makefile.am
> index ae14e625b726..5a28e66d4408 100644
> --- a/config/Makefile.am
> +++ b/config/Makefile.am
> @@ -46,12 +46,16 @@ pkgconfig_DATA += libdebuginfod.pc
> if [ -n "@DEBUGINFOD_URLS@" ]; then \
> echo "@DEBUGINFOD_URLS@" >
> $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.urls; \
> fi
> + if [ -n "@DEBUGINFOD_IMA_CERT_PATH@" ]; then \
> + echo "@DEBUGINFOD_IMA_CERT_PATH@" >
> $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.certpath; \
> + fi
>
> uninstall-local:
> rm -f $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.sh
> rm -f $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.csh
> rm -f $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish
> rm -f $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.urls
> + rm -f $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.certpath
> -rmdir $(DESTDIR)$(sysconfdir)/debuginfod
> endif
>
> diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in
> index 4d802a25ad5f..460729972420 100644
> --- a/config/elfutils.spec.in
> +++ b/config/elfutils.spec.in
> @@ -43,6 +43,12 @@ BuildRequires: curl
> # For run-debuginfod-response-headers.sh test case
> BuildRequires: socat
>
> +# For debuginfod rpm IMA verification
> +BuildRequires: rpm-devel
> +BuildRequires: ima-evm-utils-devel
> +BuildRequires: openssl-devel
> +BuildRequires: rpm-sign
So ima-evm-utils-devel isn't needed here anymore?
> %define _gnu %{nil}
> %define _programprefix eu-
>
> diff --git a/config/profile.csh.in b/config/profile.csh.in
> index d962d969c05b..1da9626c711b 100644
> --- a/config/profile.csh.in
> +++ b/config/profile.csh.in
> @@ -4,13 +4,19 @@
> # See also [man debuginfod-client-config] for other environment variables
> # such as $DEBUGINFOD_MAXSIZE, $DEBUGINFOD_MAXTIME, $DEBUGINFOD_PROGRESS.
>
> +set prefix="@prefix@"
> if (! $?DEBUGINFOD_URLS) then
> -set prefix="@prefix@"
> set DEBUGINFOD_URLS=`sh -c 'cat /dev/null "$0"/*.urls 2>/dev/null; :'
> "@sysconfdir@/debuginfod" | tr '\n' ' '`
> if ( "$DEBUGINFOD_URLS" != "" ) then
> setenv DEBUGINFOD_URLS "$DEBUGINFOD_URLS"
> else
> unset DEBUGINFOD_URLS
> endif
> -unset prefix
> +set DEBUGINFOD_IMA_CERT_PATH=`sh -c 'cat /dev/null "$0"/*.certpath
> 2>/dev/null; :' "@sysconfdir@/debuginfod" | tr '\n' ':'`
> +if ( "$DEBUGINFOD_IMA_CERT_PATH" != "" ) then
> +setenv DEBUGINFOD_IMA_CERT_PATH "$DEBUGINFOD_IMA_CERT_PATH"
> +else
> +unset DEBUGINFOD_IMA_CERT_PATH
> +endif
> endif
> +unset prefix
> diff --git a/config/profile.sh.in b/config/profile.sh.in
> index 84d3260ddcfc..7db399960915 100644
> --- a/config/profile.sh.in
> +++ b/config/profile.sh.in
> @@ -4,9 +4,15 @@
> # See also [man debuginfod-client-config] for other environment variables
> # such as $DEBUGINFOD_MAXSIZE, $DEBUGINFOD_MAXTIME, $DEBUGINFOD_PROGRESS.
>
> +prefix="@prefix@"
> if [ -z "$DEBUGINFOD_URLS" ]; then
> prefix="@prefix@"
> DEBUGINFOD_URLS=$(cat /dev/null "@sysconfdir@/debuginfod"/*.urls
> 2>/dev/null | tr '\n' ' ' || :)
> [ -n "$DEBUGINFOD_URLS" ] && export DEBUGINFOD_URLS || unset
> DEBUGINFOD_URLS
> -unset prefix
> fi
> +
> +if [ -z "$DEBUGINFOD_IMA_CERT_PATH" ]; then
> +DEBUGINFOD_IMA_CERT_PATH=$(cat "@sysconfdir@/debuginfod"/*.certpath
> 2>/dev/null | tr '\n' ':' || :)
> +[ -n "$DEBUGINFOD_IMA_CERT_PATH" ] && export DEBUGINFOD_IMA_CERT_PATH ||
> unset DEBUGINFOD_IMA_CERT_PATH
> +fi
> +unset prefix
We now also have a fish profile.
>
Re: Elfutils Code of Conduct
Hi, On Thu, Mar 28, 2024 at 12:25:32PM -0400, Aaron Merey wrote: > This code of conduct includes a committee to handle any complaints or > concerns that fall under the code. Mark and I have offered to be on this > committee. If anyone else would like to be a member of the elfutils code > of conduct committee, please let us know! > > I've included the text of Contributor Covenant v2.1 below with changes > to the following sections: > > Enforcement Responsibilities: Clarified who is a "community leader". > Scope: Used examples specific to elfutils. > Enforcement: Mention our code of conduct committee and names of members. > > Any other comments or questions regarding an elfutils code of conduct are > also welcome. Aaron and I already discussed this off-list before he proposed it, so it isn't surprising I am happy to adopt this. The only small issue I see is that both Aaron and I get paid by the same company. So it would be nice to get someone who isn't affiliated with Red Hat join the committee. But if we don't get any volunteers for that in say a week, I would suggest we adopt it as is. Unless there are any objections or questions we cannot answer of course. Cheers, Mark > --- > > # Contributor Covenant Code of Conduct > > ## Our Pledge > > We as members, contributors, and leaders pledge to make participation in > our community a harassment-free experience for everyone, regardless of > age, body size, visible or invisible disability, ethnicity, sex > characteristics, gender identity and expression, level of experience, > education, socio-economic status, nationality, personal appearance, > race, caste, color, religion, or sexual identity and orientation. > > We pledge to act and interact in ways that contribute to an open, welcoming, > diverse, inclusive, and healthy community. > > ## Our Standards > > Examples of behavior that contributes to a positive environment for our > community include: > > * Demonstrating empathy and kindness toward other people > * Being respectful of differing opinions, viewpoints, and experiences > * Giving and gracefully accepting constructive feedback > * Accepting responsibility and apologizing to those affected by our mistakes, > and learning from the experience > * Focusing on what is best not just for us as individuals, but for the > overall community > > Examples of unacceptable behavior include: > > * The use of sexualized language or imagery, and sexual attention or > advances of any kind > * Trolling, insulting or derogatory comments, and personal or political > attacks > * Public or private harassment > * Publishing others' private information, such as a physical or email > address, without their explicit permission > * Other conduct which could reasonably be considered inappropriate in a > professional setting > > ## Enforcement Responsibilities > > Community leaders (i.e. regular contributors and/or those with elfutils > git commit access) are responsible for clarifying and enforcing our standards > of acceptable behavior and will take appropriate and fair corrective action > in response to any behavior that they deem inappropriate, threatening, > offensive, or harmful. > > Community leaders have the right and responsibility to remove, edit, or > reject comments, commits, code, wiki edits, issues, and other contributions > that are not aligned to this Code of Conduct, and will communicate reasons > for moderation decisions when appropriate. > > ## Scope > > This Code of Conduct applies within all community spaces, such as the > elfutils IRC channel, website, mailing list and bug reports. It also > applies when an individual is officially representing the community > in public spaces. Examples of representing our community include acting > as an appointed representative at an online or offline event. > > ## Enforcement > > Instances of abusive, harassing, or otherwise unacceptable behavior may > be reported to the elfutils code of conduct committee at > elfutils-cond...@sourceware.org. > > The current members of the elfutils code of conduct committee are: > Mark Wielaard > Aaron Merey > > All complaints will be reviewed and investigated promptly and fairly. > > All community leaders are obligated to respect the privacy and security > of the reporter of any incident. > > ## Enforcement Guidelines > > Community leaders will follow these Community Impact Guidelines in > determining the consequences for any action they deem in violation of > this Code of Conduct: > > ### 1. Correction > > Community Impact: Use of inappropriate language or other behavior deemed > unprofessional or unwelcome in the community. > >
Re: [PATCH] strip: Add check for elf_begin() result
Hi Maks,
On Fri, Apr 05, 2024 at 09:53:03PM +0300, Maks Mishin wrote:
> Return value of a function 'elf_begin' is dereferenced at strip.c:1166
> without checking for NULL, but it is usually checked for this function.
Please stop sending these "patches" till you actually understand the
code.
In this case if debugelf == NULL then gelf_newehdr will also return
NULL to indicate the earlier error.
This is a common pattern in the libelf code.
Thanks,
Mark
> Found by RASU JSC.
>
> Signed-off-by: Maks Mishin
> ---
> src/strip.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/strip.c b/src/strip.c
> index 6436443d..ebab4866 100644
> --- a/src/strip.c
> +++ b/src/strip.c
> @@ -1153,7 +1153,9 @@ handle_elf (int fd, Elf *elf, const char *prefix, const
> char *fname,
> {
>/* Also create an ELF descriptor for the debug file */
>debugelf = elf_begin (debug_fd, ELF_C_WRITE, NULL);
> - if (unlikely (gelf_newehdr (debugelf, gelf_getclass (elf)) == 0))
> + ELF_CHECK (debugelf != NULL, _("cannot create ELF descriptor: %s"));
> +
> + if (unlikely (gelf_newehdr (debugelf, gelf_getclass (elf)) == 0))
> {
> error (0, 0, _("cannot create new ehdr for file '%s': %s"),
>debug_fname, elf_errmsg (-1));
> --
> 2.30.2
>
Re: [PATCH v3] Hexagon: implement machine flag check
Hi Matheus, On Thu, 2024-04-04 at 16:56 -0300, Matheus Tavares Bernardino wrote: > BTW, just out of curiosity, since the last incident with xz's backdoor > (which apparently involved malicious code disguised as a test binary), > has the elfutils community already considered using something like > Dockerfiles to generate the tests/*.ko.bz2 binaries instead of checking > than in the git repo? Just something that crossed my mind while I was > developing these patches. Good question. Especially since I have been poking backend developers to add more of them... I think the honest question is, no we haven't considered, or even discussed, getting rid of them. So lets :) In the xz-backdoor case it was actually hidden in a test binary which wasn't actually used in the testsuite. So that is certainly something to watch out for. Does someone add a binary file for no good reason? Also this seems to be a somewhat sophisticated hack and the would probably found some other way to hide something. But you are right of course that hiding something is much easier in a binary file. So it is reasonable to ask what the alternatives are. We do actually have native testcases. So we could just rely on those. But it is really helpful to see if the code works cross-32/64 bit and cross-endian. Because one of the advantages of elfutils is that you can inspect and manipulate binaries from other arches. Another might be what bzip2 does, have all these binaries in a separate test git repo https://sourceware.org/cgit/bzip2-tests/tree/README But that kind of just moves the issue. We would encourage people to always check out that repo and run all tests in it. Another would be what you suggest. Create containers for all arches supported and (re)generate all test binaries in that container. But that would be a lot of containers and for some arches you like to have different versions of the tools to generate them. And can that be done for all arches? e.g. Does hexagon have qemu support? One advantage of this might be that you could just rely on the native tests. Assuming we add them all to builder.sourceware.org and run them on each commit. Finally we might want to only create binary files from some easily understood textual representation. Maybe using GNU Poke ELF pickles? https://jemarch.net/poke-elf-1.0-manual/poke-elf.html That sounds really attractive, but it would be a lot of work recreating the existing binary test files. And nobody has tried yet, so we don't know if this (or some other tool) is expressive enough. Cheers, Mark
Re: [PATCH v3 6/6] backends: Add register_info, return_value_location, core_note function on mips
Hi, On Tue, Mar 05, 2024 at 05:51:22PM +0800, Ying Huang wrote: > From: Ying Huang > > Signed-off-by: Ying Huang > --- > backends/Makefile.am | 3 +- > backends/mips_corenote.c | 85 + > backends/mips_init.c | 3 + > backends/mips_regs.c | 135 +++ > backends/mips_retval.c | 196 +++ > 5 files changed, 421 insertions(+), 1 deletion(-) > create mode 100644 backends/mips_corenote.c > create mode 100644 backends/mips_regs.c > create mode 100644 backends/mips_retval.c Similar to the previous one. Added a ChangeLog entry to the commit message: * backends/Makefile.am (mips_SRCS): Add mips_regs.c, mips_retval.c and mips_corenote.c. * backends/mips_init.c (mips_init): HOOK register_info, return_value_location and core_note. * backends/mips_corenote.c: New file. * backends/mips_regs.c: Likewise. * backends/mips_retval.c: Likewise. The implementations of core_note, register_info and return_value_location do look plausible. Pushed. Although there is one small issue/question: > diff --git a/backends/mips_corenote.c b/backends/mips_corenote.c > new file mode 100644 > index ..aeadeb17 > --- /dev/null > +++ b/backends/mips_corenote.c > @@ -0,0 +1,85 @@ > +/* MIPS specific core note handling. > + Copyright (C) 2024 CIP United Inc. > + This file is part of elfutils. > [...] > +#ifdef HAVE_CONFIG_H > +# include > +#endif > + > +#include > +#include > +#include > +#include > +#include > + > +#define BACKEND mips_ > +#include "libebl_CPU.h" > + > +#define BITS 64 > +#ifndef BITS > +# define BITS32 > +#else > +# define BITS64 > +#endif So this only handles 64 BITS. I left it this way. But it is a question about the whole series. MIPS has multiple abis, some 32, some 64 bits, some big and some little endian. If I understand correctly this backend only handles the 64 bit little endian one? Again it would be nice if you could come up with non-native variants of the tests so the implementation can also be tested on non-MIPS arches. Specifically for tests/run-allregs.sh, tests/run-funcretval.sh and/or tests/run-readelf-mixed-corenote.sh. Thanks, Mark
Re: [PATCH v3 5/6] stack: Fix stack unwind failure on mips
Hi Ying,
On Tue, Mar 05, 2024 at 05:51:21PM +0800, Ying Huang wrote:
> From: Ying Huang
>
> Add abi_cfi, set_initial_registers_tid, unwind on mips.
Sorry for reviewing out of order. But this one looked easy enough.
The new abi_cfi, unwind and set_initial_registers_tid implementations
looks correct (even though I don't really known MIPS).
Pushed with the following ChangeLog entry added to the commit message.
* backends/Makefile.am (mips_SRCS): Add mips_initreg.c,
mips_cfi.c and mips_unwind.c.
* backends/mips_init.c (mips_init): HOOK abi_cfi, unwind and
set_initial_registers_tid. Set frame_nregs to 71.
* backends/mips_cfi.c: New file.
* backends/mips_initreg.c: Likewise.
* backends/mips_unwind.c: Likewise.
It would be helpful to see if you could come up with a testcase
similar to the ones in tests/run-backtrace-core-.sh with
tests/tests/backtrace..{exec,core}.bz2 so it can be tested from
a non-MIPS setup (as opposed to the tests/run-backtrace-native.sh and
tests/run-backtrace-native-core.sh tests).
Thanks,
Mark
Re: [PATCH v3] Hexagon: implement machine flag check
Hi Matheus, On Thu, Apr 04, 2024 at 02:19:40PM -0300, Matheus Tavares Bernardino wrote: > This fixes the "invalid machine flag" error from eu-elflint when passing > hexagon binaries. > > * backends/hexagon_init.c (hexagon_init): Hook > machine_flag_check > * backends/hexagon_symbol.c (hexagon_machine_flag_check): > new function > * libelf/elf-knowledge.h: add EF_HEXAGON_TINY constant > > Signed-off-by: Matheus Tavares Bernardino > --- > > v2: https://sourceware.org/pipermail/elfutils-devel/2024q2/006987.html Note that we also have a public-inbox instance, which is useful in some cases since you can address patches by message-id: https://inbox.sourceware.org/elfutils-devel/d198f1806a486bd5129c996f10680b947ecdc581.1712087613.git.quic_mathb...@quicinc.com/ > Changes in v3: > - Added ChangeLog to commit message. > - Implemented better machine_flag_check operation, as suggested by > bcain. > - Extracted only patch 2/2 as 1/2 was already merged. This looks good. Pushed, Mark
Re: [PATCH v2 2/2] Hexagon: implement machine flag check
Hi Brian, On Thu, 2024-04-04 at 16:31 +, Brian Cain wrote: > > > ... implies a new EF_HEXAGON_TINY 0x8000 definition BTW. > > > > You obviously know this architecture better than me. But is the TINY > > flag (bit 15) appropriate for all machines? It looks like it can only > > be set on V67 and V71. If so maybe just check those two explicitly? > > So far, V67T and V71T are the only ones with this variation. > It could also be specified on yet-to-be-created future devices > (some theoretical V99T might exist). > What should the behavior be in those cases? OK, if there could be other TINY variants then either the check suggested by Matheus or you would be more correct. I do like the idea to have a self documenting EF_HEXAGON_TINY constant for this. Thanks, Mark
Re: [PATCH v2 1/2] Add support for Hexagon
Hi Matheus, On Tue, 2024-04-02 at 16:55 -0300, Matheus Tavares Bernardino wrote: > This implements initial support for the Hexagon architecture. The > Hexagon ABI spec can be seen at > https://lists.llvm.org/pipermail/llvm-dev/attachments/20190916/21516a52/attachment-0001.pdf > > A hello_hexagon.ko test is also added. > > $ head tests/test-suite.log > [...] > # TOTAL: 275 > # PASS: 269 > # SKIP: 6 > # XFAIL: 0 > # FAIL: 0 > # XPASS: 0 > # ERROR: 0 > > $ cat tests/run-strip-reloc-ko.sh.log > [...] > runtest hello_hexagon.ko > PASS run-strip-reloc-ko.sh (exit status: 0) Very nice. Thanks for including the new testcase. Now we have at least a little test coverage that can be run on any arch to check hexagon is supported. The only thing I did before pushing this was to add a ChangeLog entry to the commit message: * backends/Makefile.am (modules): Add hexagon. (hexagon_SRCS): New var for hexagon_init.c and hexagon_symbol.c. (libebl_backends_a_SOURCES): Add hexagon_SRCS. * backends/hexagon_init.c: New file. * backends/hexagon_reloc.def: Likewise. * backends/hexagon_symbol.c: Likewise. * libebl/eblopenbackend.c (hexagon_init): Declare. (machines): Add hexagon. * libelf/elf-knowledge.h: Add hexagon e_flags values, section indices and and relocs. * src/elflint.c (valid_e_machine): Add EM_QDSP6. * tests/Makefile.am (EXTRA_DIST): Add hello_hexagon.ko.bz2. * tests/hello_hexagon.ko.bz2: New test file. * tests/run-strip-reloc-ko.sh: Add hello_hexagon.ko. This is mainly for my own review, so I know all changes were actually intended. Pushed, Mark
Re: [PATCH v2 2/2] Hexagon: implement machine flag check
Hi,
On Tue, 2024-04-02 at 21:38 +, Brian Cain wrote:
> > diff --git a/backends/hexagon_symbol.c b/backends/hexagon_symbol.c
> > index b341243e..1e681e9f 100644
> > --- a/backends/hexagon_symbol.c
> > +++ b/backends/hexagon_symbol.c
> > @@ -56,3 +56,11 @@ hexagon_reloc_simple_type (Ebl *ebl __attribute__
> > ((unused)), int type,
> >return ELF_T_NUM;
> > }
> > }
> > +
> > +bool
> > +hexagon_machine_flag_check (GElf_Word flags)
> > +{
> > + GElf_Word arch_variant = flags &~ EF_HEXAGON_MACH;
> > + /* 0x8000 covers the "tiny core" arch variants. */
> > + return arch_variant == 0 || arch_variant == 0x8000;
> > +}
> > --
> > 2.37.2
>
> What about this instead?
>
> bool hexagon_machine_flag_check(GElf_Word flags) {
> GElf_Word reserved_flags = ~(EF_HEXAGON_TINY | EF_HEXAGON_MACH);
>
> return (flags & reserved_flags) == 0;
> }
>
> ... implies a new EF_HEXAGON_TINY 0x8000 definition BTW.
You obviously know this architecture better than me. But is the TINY
flag (bit 15) appropriate for all machines? It looks like it can only
be set on V67 and V71. If so maybe just check those two explicitly?
Cheers,
Mark
Re: [PATCH] segment: Fix memory leak in insert()
On Tue, Apr 02, 2024 at 11:32:50PM +0300, Maks Mishin wrote: > Dynamic memory, referenced by 'naddr', is allocated at segment.c:66 > by calling function 'realloc' and lost at segment.c:92. It isn't lost, it is assigned to dwfl->lookup_addr at segment.c:77 > > Found by RASU JSC. > > Signed-off-by: Maks Mishin > --- > libdwfl/segment.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libdwfl/segment.c b/libdwfl/segment.c > index f6a3e84e..618c14e6 100644 > --- a/libdwfl/segment.c > +++ b/libdwfl/segment.c > @@ -89,6 +89,7 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr > end, int segndx) > return true; > } > } > + free (naddr); > } > >if (unlikely (i < dwfl->lookup_elts)) > -- > 2.30.2 >
Re: [PATCH] segment: Fix dangling pointer
Hi Maks,
Adding the elfutils-devel list back to the CC because that is where
patches are discussed.
On Tue, Apr 02, 2024 at 10:45:59PM +0300, Максим Мишин wrote:
> RASU JSC is a part of Rosatom, a company where one of the areas of work is
> software development based on the Linux kernel.
> That's why I'm doing a static analysis of the Linux-core components.
>
> We use the Svace static analyzer:
> https://www.ispras.ru/en/technologies/svace/
>
> This patch is the processing of the analyzer's triggers with the
> DANGLING_POINTER type for pointer `old`.
OK, but that doesn't really make sense. old isn't a dangling
pointer. It is a local pointer that is freed before returning from the
function. What do you try to accomplish by assigning it the value
NULL?
What real issue are you trying to fix?
Thanks,
Mark
> пт, 29 мар. 2024 г. в 00:04, Mark Wielaard :
>
> > Hi Maks,
> >
> > On Thu, Mar 28, 2024 at 11:29:22PM +0300, Maks Mishin wrote:
> > > Pointer 'lookup_module' which is a field of the structure 'Dwfl'
> > > freed at segment.c:88 is not overwritten, but it is usually overwritten
> > > after free.
> >
> > But the very next statement is a return true; so old isn't in scope
> > anymore. Why would we assign NULL to it?
> >
> > > Found by RASU JSC.
> >
> > What or who is that?
> >
> > > Signed-off-by: Maks Mishin
> > > ---
> > > libdwfl/segment.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/libdwfl/segment.c b/libdwfl/segment.c
> > > index f6a3e84e..af76f2f8 100644
> > > --- a/libdwfl/segment.c
> > > +++ b/libdwfl/segment.c
> > > @@ -86,6 +86,7 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start,
> > GElf_Addr end, int segndx)
> > > if (unlikely (dwfl->lookup_module == NULL))
> > > {
> > > free (old);
> > > + old = NULL;
> > > return true;
> > > }
> > > }
> > > --
> > > 2.30.2
> > >
> >
>
>
> --
> С уважением,
> Максим Мишин
> +7 (915) 958-41-07
> maks.mishi...@gmail.com
Re: [PATCH] libdw: dwarf_getsrcfiles should not imply dwarf_getsrclines
Hi Aaron,
On Thu, 2024-03-28 at 21:12 -0400, Aaron Merey wrote:
> dwarf_getsrcfiles causes line data to be read in addition to file data.
> This is wasteful for programs which only need file or directory names.
> Debuginfod server is one such example.
>
> Fix this by moving the srcfile handling in read_srclines into a separate
> function read_srcfiles.
Very nice. On irc you said that this patch speeds up debuginfod server
rpm indexing by 5% and reduces max resident set size during indexing by
up to 75%. Which are impressive numbers. Please feel free to add those
to the commit message and/or add a NEWS entry.
> read_srclines also no longer handles DW_LNE_define_file due to lack of
> use and to simplify the separation of srcfile and srcline reading.
I did say so, and DWARF5 says "The DW_LNE_define_file operation defined
in earlier versions of DWARF is deprecated in DWARF Version 5." It also
isn't officially defined anymore: "Code 0x03 is reserved to allow
backward compatible support of the DW_LNE_define_file operation which
was defined in DWARF Version 4 and earlier." Also I haven't seen it used
ever. But I still wonder if completely rejecting it is the right thing
to do.
DWARF4 says about DW_LNE_define_file:
The primary source file is described by an entry whose path name
exactly matches that given in the DW_AT_name attribute in the
compilation unit, and whose directory index is 0. The files are
numbered, starting at 1, in the order in which they appear; the
names in the header come before names defined by the
DW_LNE_define_file instruction. These numbers are used in the file
register of the state machine.
Which seems to imply that files defined with DW_LNE_define_file are
only used in the line program, not in the DIE attributes. So maybe we
can accept them and just add/expand the srcfiles when we see them (this
does mean we might have to dynamically grow the array after the fact,
but given this almost never happens it shouldn't really impact
performance normally).
You can of course say this is way too much work for a nice feature. If
so then do add to the NEWS entry that DWARF .debug_line containing
DW_LNE_define_file aren't accepted anymore.
> * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Replace
> dwarf_getsrclines and __libdw_getsrclines with
> __libdw_getsrcfiles.
> * libdw/dwarf_getsrclines.c (read_line_header): New function.
> (read_srcfiles): New function.
> (read_srclines): Move file reading into read_srcfiles.
> Remove DW_LNE_define_file handling. Add parameter so
> that previously read srcfiles can be used if available.
> (__libdw_getsrclines): Call read_srcfiles if linesp is
> NULL. Pass previously read srcfiles to read_srclines
> if available.
> (__libdw_getsrcfiles): New function.
I did found it slightly confusing that __libdw_getsrclines still just
calls __libdw_getsrclines. But that is just a naming/indirection issue
which hopefully doesn't confuse anybody else looking at the
implementation.
> * libdw/dwarf_macro_getsrcfiles.c (dwarf_macro_getsrcfiles):
> Replace __libdw_getsrclines with __libdw_getsrcfiles.
> * libdw/libdwP.h (__libdw_getsrcfiles): New declaration.
> * tests/get-files.c: Verify that dwarf_getsrcfiles does
> not cause srclines to be read.
> * tests/get-lines.c: Verify that srclines can be read
> after srcfiles have been read.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=27405
>
> Signed-off-by: Aaron Merey
> ---
> libdw/dwarf_getsrcfiles.c | 24 +-
> libdw/dwarf_getsrclines.c | 506 ++--
> libdw/dwarf_macro_getsrcfiles.c | 4 +-
> libdw/libdwP.h | 10 +
> tests/get-files.c | 8 +
> tests/get-lines.c | 20 +-
> 6 files changed, 346 insertions(+), 226 deletions(-)
>
> diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c
> index cd2e5b5a..24e4b7d2 100644
> --- a/libdw/dwarf_getsrcfiles.c
> +++ b/libdw/dwarf_getsrcfiles.c
> @@ -70,10 +70,9 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
> size_t *nfiles)
> {
> /* We are only interested in the files, the lines will
>always come from the skeleton. */
> - res = __libdw_getsrclines (cu->dbg, dwp_off,
> + res = __libdw_getsrcfiles (cu->dbg, dwp_off,
>__libdw_getcompdir (cudie),
> - cu->address_size, NULL,
> - >files);
> + cu->address_size, >files);
> }
> }
> else
OK.
> @@ -89,12 +88,19 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
> size_t *nfiles)
> }
>else
> {
> - Dwarf_Lines *lines;
> - size_t nlines;
> -
> - /* Let the more
Re: [PATCH] readelf: Fix division by zero in handle a relocation sections
Hi Maks, On Sat, 2024-03-30 at 00:23 +0300, Maks Mishin wrote: > Variable 'sh_entsize', whose possible value set allows a zero value > by calling function 'gelf_fsize', is used as a denominator > in calculation of 'nentries' variable. > > Found by RASU JSC. Sorry, but I am going to stop reviewing your patches till you have responded to some of the previous reviews. All your suggested changes are (subtly) wrong. In this case gelf_fsize cannot return zero for example. Please better explain your proposed changes and provide testcases/examples of how the current code is wrong. Thanks, Mark
Re: [PATCH] tests, config: Add more .gitignore files
On Fri, Mar 29, 2024 at 12:43:08AM +0100, Mark Wielaard wrote: > Some new tests and one configure file weren't in .gitignore. Also > we made a copy of libelf.h in tests/ which should be an symlink. > > * config/.gitignore: Add profile.fish. > * tests/.gitignore: Add funcretval_test_struct, libelf.h > and system-elf-gelf-test. > * tests/Makefile.am (libelf.h): Make symlink instead of copy. Pushed to main.
Re: [PATCH v3 1/6] Support Mips architecture
Hi Ying,
On Tue, Mar 05, 2024 at 05:51:17PM +0800, Ying Huang wrote:
> From: Ying Huang
>
> Signed-off-by: Ying Huang
> ---
> backends/Makefile.am| 6 +-
> backends/mips_init.c| 52
> backends/mips_reloc.def | 93 +++
> backends/mips_symbol.c | 63 +
> libebl/eblopenbackend.c | 2 +
> libelf/libelfP.h| 3 +
> tests/libelf.h | 541
Note that this adds tests/libelf.h by accident. I see how that could
happen, because it should have been in .gitignore. I posted a patch
to do that:
https://inbox.sourceware.org/elfutils-devel/20240328234308.1032110-1-m...@klomp.org/
Besides that the patch looks fine. I did add a ChangeLog entry to the
commit message. Pushed as attached.
Thanks,
Mark
>From e259f126f5077923e415e306915de50ed0f0db56 Mon Sep 17 00:00:00 2001
From: Ying Huang
Date: Tue, 5 Mar 2024 17:51:17 +0800
Subject: [PATCH] Support Mips architecture
* backends/Makefile.am (modules): Add mips.
(mips_SRCS): New var for mips_init.c mips_symbol.c.
(libebl_backends_a_SOURCES): Add mips_SRCS.
* backends/mips_init.c: New file.
* backends/mips_reloc.def: Likewise.
* backends/mips_symbol.c: Likewise.
* libebl/eblopenbackend.c (mips_init): Declare.
(machines): Add mips.
* libelf/libelfP.h: Add ELF64_MIPS_R_TYPE{1,2,3}
Signed-off-by: Ying Huang
---
backends/Makefile.am| 6 ++-
backends/mips_init.c| 52 +++
backends/mips_reloc.def | 93 +
backends/mips_symbol.c | 63
libebl/eblopenbackend.c | 2 +
libelf/libelfP.h| 3 ++
6 files changed, 217 insertions(+), 2 deletions(-)
create mode 100644 backends/mips_init.c
create mode 100644 backends/mips_reloc.def
create mode 100644 backends/mips_symbol.c
diff --git a/backends/Makefile.am b/backends/Makefile.am
index bbb2aac79ab8..b946fd30495f 100644
--- a/backends/Makefile.am
+++ b/backends/Makefile.am
@@ -37,7 +37,7 @@ AM_CPPFLAGS += -I$(top_srcdir)/libebl -I$(top_srcdir)/libasm \
noinst_LIBRARIES = libebl_backends.a libebl_backends_pic.a
modules = i386 sh x86_64 ia64 alpha arm aarch64 sparc ppc ppc64 s390 \
- m68k bpf riscv csky loongarch arc
+ m68k bpf riscv csky loongarch arc mips
i386_SRCS = i386_init.c i386_symbol.c i386_corenote.c i386_cfi.c \
i386_retval.c i386_regs.c i386_auxv.c \
@@ -102,12 +102,14 @@ loongarch_SRCS = loongarch_init.c loongarch_symbol.c
loongarch_cfi.c \
arc_SRCS = arc_init.c arc_symbol.c
+mips_SRCS = mips_init.c mips_symbol.c
+
libebl_backends_a_SOURCES = $(i386_SRCS) $(sh_SRCS) $(x86_64_SRCS) \
$(ia64_SRCS) $(alpha_SRCS) $(arm_SRCS) \
$(aarch64_SRCS) $(sparc_SRCS) $(ppc_SRCS) \
$(ppc64_SRCS) $(s390_SRCS) \
$(m68k_SRCS) $(bpf_SRCS) $(riscv_SRCS) $(csky_SRCS)
\
- $(loongarch_SRCS) $(arc_SRCS)
+ $(loongarch_SRCS) $(arc_SRCS) $(mips_SRCS)
libebl_backends_pic_a_SOURCES =
am_libebl_backends_pic_a_OBJECTS = $(libebl_backends_a_SOURCES:.c=.os)
diff --git a/backends/mips_init.c b/backends/mips_init.c
new file mode 100644
index ..cedd08ca1339
--- /dev/null
+++ b/backends/mips_init.c
@@ -0,0 +1,52 @@
+/* Initialization of MIPS specific backend library.
+ Copyright (C) 2024 CIP United Inc.
+ This file is part of elfutils.
+
+ This file is free software; you can redistribute it and/or modify
+ it under the terms of either
+
+ * the GNU Lesser General Public License as published by the Free
+ Software Foundation; either version 3 of the License, or (at
+ your option) any later version
+
+ or
+
+ * the GNU General Public License as published by the Free
+ Software Foundation; either version 2 of the License, or (at
+ your option) any later version
+
+ or both in parallel, as here.
+
+ elfutils is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received copies of the GNU General Public License and
+ the GNU Lesser General Public License along with this program. If
+ not, see
[PATCH] tests, config: Add more .gitignore files
Some new tests and one configure file weren't in .gitignore. Also we made a copy of libelf.h in tests/ which should be an symlink. * config/.gitignore: Add profile.fish. * tests/.gitignore: Add funcretval_test_struct, libelf.h and system-elf-gelf-test. * tests/Makefile.am (libelf.h): Make symlink instead of copy. Signed-off-by: Mark Wielaard --- config/.gitignore | 1 + tests/.gitignore | 3 +++ tests/Makefile.am | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/config/.gitignore b/config/.gitignore index 8cd8ccdbf3c1..b7897159babf 100644 --- a/config/.gitignore +++ b/config/.gitignore @@ -11,5 +11,6 @@ /missing /profile.csh /profile.sh +/profile.fish /test-driver /ylwrap diff --git a/tests/.gitignore b/tests/.gitignore index 0289959d73d5..772c7881ae3f 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -71,6 +71,7 @@ /find-prologues /funcretval /funcretval_test++11 +/funcretval_test_struct /funcscopes /get-aranges /get-files @@ -81,6 +82,7 @@ /getphdrnum /getsrc_die /hash +/libelf.h /leb128 /line2addr /low_high_pc @@ -104,6 +106,7 @@ /showptable /strptr /system-elf-libelf-test +/system-elf-gelf-test /test-elf_cntl_gelf_getshdr /test-flag-nobits /test-nlist diff --git a/tests/Makefile.am b/tests/Makefile.am index 344d6706e16e..40e0eaa5a368 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -841,7 +841,7 @@ declfiles_LDADD = $(libdw) BUILT_SOURCES = libelf.h CLEANFILES += libelf.h libelf.h: $(top_srcdir)/libelf/libelf.h - cp $< $@ + ln -s $< $@ if !INSTALL_ELFH system_elf_libelf_test_CPPFLAGS = system_elf_gelf_test_CPPFLAGS = -I. -- 2.39.3
Re: [PATCH] nm: Fix descriptor leak
Hi,
On Thu, Mar 28, 2024 at 11:49:58PM +0300, Maks Mishin wrote:
> The descriptor 'dwfl_fd' is created at nm.c:1278 by calling
> function 'dup' and lost at nm.c:1593.
Sorry, I don't follow, the code at nm.c:1278 says:
/* Duplicate an fd for dwfl_report_offline to swallow. */
int dwfl_fd = dup (fd);
if (likely (dwfl_fd >= 0))
And then the code tracks whether the dwfl_report_offline call is
executed successfully and otherwise closed dwfl_fd. Specifically the
code does:
if (dwfl_report_offline (dwfl, fname, fname, dwfl_fd)
== NULL)
{
/* Consumed on success, not on failure. */
close (dwfl_fd);
}
The dwfl_report functions are documented as "On success, FD is
consumed by the library". Which means fd is freed/closed when dwfl_end
is called.
> Found by RASU JSC.
>
> Signed-off-by: Maks Mishin
> ---
> src/nm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/nm.c b/src/nm.c
> index 3675f59b..fee397dd 100644
> --- a/src/nm.c
> +++ b/src/nm.c
> @@ -1521,6 +1521,8 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
> }
>if (dwfl != NULL)
> dwfl_end (dwfl);
> + if (dwfl_fd != NULL)
> +close(dwfl_fd);
> }
So this would double close dwfl_fd. Also dwfl_fd is an int (file
descriptor) and so shouldn't be compared to NULL, but checked as above
(dwfl_fd >= 0).
Re: [PATCH] segment: Fix dangling pointer
Hi Maks,
On Thu, Mar 28, 2024 at 11:29:22PM +0300, Maks Mishin wrote:
> Pointer 'lookup_module' which is a field of the structure 'Dwfl'
> freed at segment.c:88 is not overwritten, but it is usually overwritten
> after free.
But the very next statement is a return true; so old isn't in scope
anymore. Why would we assign NULL to it?
> Found by RASU JSC.
What or who is that?
> Signed-off-by: Maks Mishin
> ---
> libdwfl/segment.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libdwfl/segment.c b/libdwfl/segment.c
> index f6a3e84e..af76f2f8 100644
> --- a/libdwfl/segment.c
> +++ b/libdwfl/segment.c
> @@ -86,6 +86,7 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr
> end, int segndx)
> if (unlikely (dwfl->lookup_module == NULL))
> {
> free (old);
> + old = NULL;
> return true;
> }
> }
> --
> 2.30.2
>
Re: [PATCH 0/2] Add initial support for Hexagon
Hi Matheus, On Thu, Mar 21, 2024 at 06:09:08PM -0300, Matheus Tavares Bernardino wrote: > The patches were inspired by > https://sourceware.org/cgit/elfutils/commit?id=13a4d1279c5b7847049ca3045d04f2705c45ce31 > > Related to: > https://lore.kernel.org/all/6498586d7d0ed112e6c44be98d439abc549653c7.ca...@klomp.org/t/#u > > Matheus Tavares Bernardino (2): > Add support for Hexagon > Hexagon: implement machine flag check In general this looks good. It is the minimal backend support to get eu-strip --reloc-debug-sections and opening ET_REL (kernel module) debug files with dwfl that are automatically relocated. The only issue is that we like to keep libelf/elf.h synced with glibc elf/elf.h. Would it be possible/make sense to submit the elf.h changes to libc-alpha? Otherwise we should keep the new constants in some other file (maybe libelf/elf-knowledge.h)? Is there a public psabi for Hexagon? Then including an URL to it would be helpful. If possible you might want to include some simple test file. See tests/run-strip-reloc-ko.sh Thanks, Mark
[COMMITTED] tests: Use bash for run-debuginfod-client-profile.sh
The test uses set -o pipefail which is a bashism and so the test fails on systems where /bin/sh isn't bash. * tests/run-debuginfod-client-profile.sh: Use bash. Signed-off-by: Mark Wielaard --- tests/run-debuginfod-client-profile.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/run-debuginfod-client-profile.sh b/tests/run-debuginfod-client-profile.sh index 7435ced83f15..141588d391ea 100755 --- a/tests/run-debuginfod-client-profile.sh +++ b/tests/run-debuginfod-client-profile.sh @@ -1,4 +1,4 @@ -#! /bin/sh +#! /usr/bin/env bash # Copyright (C) 2024 Mark J. Wielaard # This file is part of elfutils. # -- 2.44.0
Re: [PATCH] config/profile.fish.in: Prevent bracketed variables and unmatched wildcard errors
Hi Aaron,
Adding Freso to the CC since he is the original author of the fish
support.
On Tue, Mar 26, 2024 at 07:52:49PM -0400, Aaron Merey wrote:
> Fish does not support bracketed variables in scripts. Remove brackets
> from the variable ${prefix} in profile.fish before installation to
> prevent this error.
>
> Fish also raises an error for unmatched wildcards, except for special
> cases like the set command. Use a wildcard to match .urls files using
> the set command instead of cat to prevent an unmatched wildcard error
> when no .urls files are found.
Not knowing fish this looks OK to me. We might want a testcase like
the one that was just added for config/profile.sh to make sure we ship
something that runs correctly. Could be something as simple as:
type fish 2>/dev/null || (echo "no fish installed"; exit 77)
fish ${abs_top_builddir}/config/profile.fish
> Signed-off-by: Aaron Merey
> ---
> config/Makefile.am | 1 +
> config/profile.fish.in | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/config/Makefile.am b/config/Makefile.am
> index ae14e625..fd41997f 100644
> --- a/config/Makefile.am
> +++ b/config/Makefile.am
> @@ -41,6 +41,7 @@ pkgconfig_DATA += libdebuginfod.pc
> install-data-local:
> $(INSTALL_DATA) profile.sh -D
> $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.sh
> $(INSTALL_DATA) profile.csh -D
> $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.csh
> + sed -i 's/{prefix}/prefix/g' profile.fish
> $(INSTALL_DATA) profile.fish -D
> $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish
> mkdir -p $(DESTDIR)$(sysconfdir)/debuginfod
> if [ -n "@DEBUGINFOD_URLS@" ]; then \
> diff --git a/config/profile.fish.in b/config/profile.fish.in
> index 00e9ca59..c0a234db 100644
> --- a/config/profile.fish.in
> +++ b/config/profile.fish.in
> @@ -7,7 +7,8 @@
> if not set --query DEBUGINFOD_URLS
> # Use local variables so we don't need to manually unset them
> set --local prefix "@prefix@"
> -set --local DEBUGINFOD_URLS (cat /dev/null
> "@sysconfdir@/debuginfod"/*.urls 2>/dev/null | string replace '\n' ' ')
> +set --local files "@sysconfdir@/debuginfod/"*.urls
> +set --local DEBUGINFOD_URLS (cat /dev/null $files 2>/dev/null | string
> replace '\n' ' ')
> if test -n "$DEBUGINFOD_URLS"
> set --global --export DEBUGINFOD_URLS "$DEBUGINFOD_URLS"
> end
> --
> 2.43.0
>
Re: [PATCH] config: Make sure profile.sh succeeds with set -e and set -o pipefail
Hi Dmitry,
On Wed, Mar 27, 2024 at 12:59:13AM +0200, Dmitry V. Levin wrote:
> On Tue, Mar 26, 2024 at 09:49:48PM +0100, Mark Wielaard wrote:
> > profile.sh might fail with set -o pipefail because:
> >
> > cat /dev/null "${prefix}/etc/debuginfod"/*.urls 2>/dev/null | tr '\n' ' '
> >
> > might fail when there isn't an *.urls file the first command in the
> > pipe fails (the 2>/dev/null is there to hide that failure).
> [...]
> > -DEBUGINFOD_URLS=$(cat /dev/null "@sysconfdir@/debuginfod"/*.urls
> > 2>/dev/null | tr '\n' ' ')
> > +DEBUGINFOD_URLS=$(cat /dev/null "@sysconfdir@/debuginfod"/*.urls
> > 2>/dev/null | tr '\n' ' ' || echo -n "")
>
> The idiomatic expression in this case is ||:
Ah, yes. That is nicer indeed.
Pushed with that change and updated commit message:
commit 0ba2e4aa9945019a8c6db95d27d142b660a63a79 (HEAD -> main, origin/main)
Author: Mark Wielaard
Date: Tue Mar 26 21:42:39 2024 +0100
config: Make sure profile.sh succeeds with set -e and set -o pipefail
profile.sh might fail with set -o pipefail because:
cat /dev/null "${prefix}/etc/debuginfod"/*.urls 2>/dev/null | tr '\n' ' '
might fail when there isn't an *.urls file the first command in the
pipe fails (the 2>/dev/null is there to hide that failure).
This can be fixed by adding || : at the end.
This works because : always succeeds and produces no outpur which is
what the script expects when the command would fail.
Also add a new testcase that runs profile.sh with bout set -e
and set -o pipefail.
* config/profile.sh.in: Add || : at end of pipe.
* tests/run-debuginfod-client-profile.sh: New test.
* tests/Makefile.am (TESTS): Add run-debuginfod-client-profile.sh.
(EXTRA_DIST): Likewise.
https://sourceware.org/bugzilla/show_bug.cgi?id=31562
Signed-off-by: Mark Wielaard
Thanks,
Mark
[PATCH] config: Make sure profile.sh succeeds with set -e and set -o pipefail
profile.sh might fail with set -o pipefail because:
cat /dev/null "${prefix}/etc/debuginfod"/*.urls 2>/dev/null | tr '\n' ' '
might fail when there isn't an *.urls file the first command in the
pipe fails (the 2>/dev/null is there to hide that failure).
This can be fixed by adding || echo -n "" at the end.
This works because echo -n "" produces the empty string which is what
the script expects when the command would fail.
Also add a new testcase that runs profile.sh with bout set -e
and set -o pipefail.
* config/profile.sh.in: Add || echo -n "" at end of pipe.
* tests/run-debuginfod-client-profile.sh: New test.
* tests/Makefile.am (TESTS): Add run-debuginfod-client-profile.sh.
(EXTRA_DIST): Likewise.
https://sourceware.org/bugzilla/show_bug.cgi?id=31562
Signed-off-by: Mark Wielaard
---
config/profile.sh.in | 2 +-
tests/Makefile.am | 2 ++
tests/run-debuginfod-client-profile.sh | 27 ++
3 files changed, 30 insertions(+), 1 deletion(-)
create mode 100755 tests/run-debuginfod-client-profile.sh
diff --git a/config/profile.sh.in b/config/profile.sh.in
index 3f4397dcb44d..4488c66511a4 100644
--- a/config/profile.sh.in
+++ b/config/profile.sh.in
@@ -6,7 +6,7 @@
if [ -z "$DEBUGINFOD_URLS" ]; then
prefix="@prefix@"
-DEBUGINFOD_URLS=$(cat /dev/null "@sysconfdir@/debuginfod"/*.urls
2>/dev/null | tr '\n' ' ')
+DEBUGINFOD_URLS=$(cat /dev/null "@sysconfdir@/debuginfod"/*.urls
2>/dev/null | tr '\n' ' ' || echo -n "")
[ -n "$DEBUGINFOD_URLS" ] && export DEBUGINFOD_URLS || unset
DEBUGINFOD_URLS
unset prefix
fi
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9315ec3bbe4c..344d6706e16e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -209,6 +209,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile
test-nlist \
run-disasm-riscv64.sh \
run-pt_gnu_prop-tests.sh \
run-getphdrnum.sh run-test-includes.sh \
+ run-debuginfod-client-profile.sh \
leb128 read_unaligned \
msg_tst system-elf-libelf-test system-elf-gelf-test \
$(asm_TESTS) run-disasm-bpf.sh run-low_high_pc-dw-form-indirect.sh \
@@ -636,6 +637,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
testfile_pt_gnu_prop.bz2 testfile_pt_gnu_prop32.bz2 \
run-getphdrnum.sh testfile-phdrs.elf.bz2 \
run-test-includes.sh run-low_high_pc-dw-form-indirect.sh \
+run-debuginfod-client-profile.sh \
run-readelf-dw-form-indirect.sh testfile-dw-form-indirect.bz2 \
run-nvidia-extended-linemap-libdw.sh
run-nvidia-extended-linemap-readelf.sh \
testfile_nvidia_linemap.bz2 \
diff --git a/tests/run-debuginfod-client-profile.sh
b/tests/run-debuginfod-client-profile.sh
new file mode 100755
index ..7435ced83f15
--- /dev/null
+++ b/tests/run-debuginfod-client-profile.sh
@@ -0,0 +1,27 @@
+#! /bin/sh
+# Copyright (C) 2024 Mark J. Wielaard
+# This file is part of elfutils.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# elfutils is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+. $srcdir/test-subr.sh
+
+# Make sure the profile.sh or profile.d/debuginfod.sh works even with
+# set -e (any command error is an error) and set -o pipefail (any error
+# in a pipe fails the whole pipe command).
+
+set -e
+set -o pipefail
+
+source ${abs_top_builddir}/config/profile.sh
--
2.39.3
Re: ☠ Buildbot (Sourceware): elfutils - failed test (failure) (main)
On Sun, Mar 24, 2024 at 05:45:57PM +, buil...@sourceware.org wrote:
> A new failure has been detected on builder elfutils-fedora-x86_64 while
> building elfutils.
>
> Full details are available at:
> https://builder.sourceware.org/buildbot/#/builders/59/builds/307
>
> Build state: failed test (failure)
> Revision: 6228e2fedf419a7f1d70dc14a3b53a8a97394b88
> Worker: bb2-1
> Build Reason: (unknown)
> Blamelist: Frederik “Freso” S. Olesen
>
> Steps:
>
> - 9: make rpmbuild ( failure )
> Logs:
> - stdio:
> https://builder.sourceware.org/buildbot/#/builders/59/builds/307/steps/9/logs/stdio
> - warnings (31):
> https://builder.sourceware.org/buildbot/#/builders/59/builds/307/steps/9/logs/warnings__31_
Checking for unpackaged file(s): /usr/lib/rpm/check-files
/home/builder/shared/bb2-1/worker/elfutils-fedora-x86_64/build/rpmbuild/BUILDROOT/elfutils-0.191-1.x86_64
error: Installed (but unpackaged) file(s) found:
/usr/share/fish/vendor_conf.d/debuginfod.fish
Installed (but unpackaged) file(s) found:
/usr/share/fish/vendor_conf.d/debuginfod.fish
RPM build errors:
make: *** [Makefile:981: rpmbuild] Error 1
Aha, we forgot to add it to the debuginfod-client package.
That is what the attached patch does. Which I just pushed.
Cheers,
Mark>From 8f3818574a6abe1fbab1682698b2cef146522148 Mon Sep 17 00:00:00 2001
From: Mark Wielaard
Date: Sun, 24 Mar 2024 18:46:02 +0100
Subject: [PATCH] config: Add debuginfod.fish to elfutils.spec as config file.
* config/elfutils.spec.in (debuginfod-client): %files add
%{_datadir}/fish/vendor_conf.d/debuginfod.fish as config file.
Signed-off-by: Mark Wielaard
---
config/elfutils.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in
index d6621c9de702..4d802a25ad5f 100644
--- a/config/elfutils.spec.in
+++ b/config/elfutils.spec.in
@@ -307,6 +307,7 @@ fi
%{_mandir}/man1/debuginfod-find.1*
%{_mandir}/man7/debuginfod*.7*
%config(noreplace) %{_sysconfdir}/profile.d/*
+%config(noreplace) %{_datadir}/fish/vendor_conf.d/debuginfod.fish
%config(noreplace) %{_sysconfdir}/debuginfod/*
%files debuginfod-client-devel
--
2.39.3
Re: [PATCH] Getter and setter for Dwfl's offline_next_address
Hi Martin, On Sun, Mar 24, 2024 at 11:11:21AM -0300, Martin Rodriguez Reboredo wrote: > On 3/20/24 19:52, Mark Wielaard wrote: > >Could you show an example of when/where you need it and what address > >you set it to? > > For example, this test program reports the name and location pointed by > the passed address. > > $ ./report /home/yakoyoku/.debug/.build-id/.../elf 0x0003281f > 0x0003281f > ?? > ??:0 > > But due to the mentioned bias both of them are unknown or out of range. > > >The offline_next_address is only relevant for ET_REL files (like > >object files or kernel modules). In general you will need to check the > >bias, which various dwfl functions return to know the difference > >between the addresses in the ELF, Dwarf or the module load address. > > > >That said, readelf, nm and dwfl_argp do "cheat" by setting the > >offline_next_address to zero if they know they are just inspecting a > >single object file. So maybe this is a functionality we need to > >expose. But I don't fully understand why you need both a getter and a > >setter for any arbitrary address. > > I've erroneously thought that setting `offline_next_address` was a > requirement to obtain what I've needed. But if I use > `dwfl_module_getelf` or `dwfl_module_getdwarf` I can get the correct > name and location. > > 0x0003281f > _ZNSt11__copy_moveILb0ELb0ESt26random_access_iterator_tagE8__copy_mIPcPhEET0_T_S6_S5_ > /usr/include/c++/13.2.1/bits/stl_algobase.h:388,18 > > So this patch is kinda pointless, but at least I've managed to learn > what I was missing. Anyways, thanks for the heads-up. :) I don't think it was pointless. Clearly our documentation is not very good (and given eu-readelf and eu-nm do cheat, maybe our interface/api isn't really good either). Maybe you could post your code for that ./report program and what you had to do to get it to print the correct address/symbols. Then we at least have some documentation for others which hit the same issue. Thanks, Mark
Re: [PATCH v2] config: Add profile script for fish shell
Hi Freso, On Fri, Mar 22, 2024 at 06:21:04PM +0100, Frederik “Freso” S. Olesen wrote: > Add support for setting $DEBUGINFOD_URLS automatically in the fish shell > similar to the profile scripts for POSIX and csh shells. > > Makefile is set to install this into fish’s $XDG_DATA_DIRS vendor > directory instead of under /etc: > https://fishshell.com/docs/current/language.html#configuration-files > > * config/profile.fish.in: Set $DEBUGINFOD_URLS in fish shells. > * configure.ac, config/Makefile.am: Include profile.fish in > install and uninstall targets. Looks good. Pushed. Thanks, Mark
Re: [PATCH] config: Add profile script for fish shell
Hi, On Thu, Mar 21, 2024 at 04:04:11PM +0100, Frederik “Freso” S. Olesen wrote: > Add support for setting $DEBUGINFOD_URLS automatically in the fish shell > similar to the profile scripts for POSIX and csh shells. > > Makefile is set to install this into fish’s $XDG_DATA_DIRS vendor > directory instead of under /etc: > https://fishshell.com/docs/current/language.html#configuration-files > > * config/profile.fish.in: Set $DEBUGINFOD_URLS in fish shells. > * config/Makefile.am: Include profile.fish in install and > uninstall targets. Very nice. I don't know fish, but this looks reasonable. Two small comments below. > Signed-off-by: Frederik “Freso” S. Olesen > --- > config/ChangeLog | 5 + > config/Makefile.am | 5 - > config/profile.fish.in | 14 ++ > 3 files changed, 23 insertions(+), 1 deletion(-) > create mode 100644 config/profile.fish.in > > diff --git a/config/ChangeLog b/config/ChangeLog > index ce1f74f6..7d88c071 100644 > --- a/config/ChangeLog > +++ b/config/ChangeLog > @@ -1,3 +1,8 @@ > +2024-03-21 Frederik “Freso” S. Olesen > + > + * profile.fish.in: Set $DEBUGINFOD_URLS in fish shells. > + * Makefile.am: Include profile.fish in install and uninstall targets. Since we have been putting the ChangeLog entry into the commit message it doesn't need to also go into the actual Changelog file. > 2023-02-21 Mark Wielaard > > * eu.am (USE_AFTER_FREE3_WARNING): Define. > diff --git a/config/Makefile.am b/config/Makefile.am > index 0d3ba164..ae14e625 100644 > --- a/config/Makefile.am > +++ b/config/Makefile.am > @@ -30,7 +30,8 @@ > ## > EXTRA_DIST = elfutils.spec.in known-dwarf.awk 10-default-yama-scope.conf \ >libelf.pc.in libdw.pc.in libdebuginfod.pc.in \ > - debuginfod.service debuginfod.sysconfig profile.sh.in > profile.csh.in > + debuginfod.service debuginfod.sysconfig \ > + profile.sh.in profile.csh.in profile.fish.in > > pkgconfigdir = $(libdir)/pkgconfig > pkgconfig_DATA = libelf.pc libdw.pc > @@ -40,6 +41,7 @@ pkgconfig_DATA += libdebuginfod.pc > install-data-local: > $(INSTALL_DATA) profile.sh -D > $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.sh > $(INSTALL_DATA) profile.csh -D > $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.csh > + $(INSTALL_DATA) profile.fish -D > $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish > mkdir -p $(DESTDIR)$(sysconfdir)/debuginfod > if [ -n "@DEBUGINFOD_URLS@" ]; then \ > echo "@DEBUGINFOD_URLS@" > > $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.urls; \ > @@ -48,6 +50,7 @@ install-data-local: > uninstall-local: > rm -f $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.sh > rm -f $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.csh > + rm -f $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish > rm -f $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.urls > -rmdir $(DESTDIR)$(sysconfdir)/debuginfod > endif Right, with --prefix=/usr that expands to /usr/share/fish/vendor_conf.d which seems to match the default install location you pointed out above. Good. > diff --git a/config/profile.fish.in b/config/profile.fish.in > new file mode 100644 > index ..34b1ab85 > --- /dev/null > +++ b/config/profile.fish.in > @@ -0,0 +1,14 @@ > +# $HOME/.profile* or similar files may first set $DEBUGINFOD_URLS. > +# If $DEBUGINFOD_URLS is not set there, we set it from system *.url files. > +# $HOME/.*rc or similar files may then amend $DEBUGINFOD_URLS. > +# See also [man debuginfod-client-config] for other environment variables > +# such as $DEBUGINFOD_MAXSIZE, $DEBUGINFOD_MAXTIME, $DEBUGINFOD_PROGRESS. > + > +if not set --query DEBUGINFOD_URLS > +# Use local variables so we don't need to manually unset them > +set --local prefix="@prefix@" > +set --local DEBUGINFOD_URLS (cat /dev/null > "@sysconfdir@/debuginfod"/*.urls 2>/dev/null | string replace '\n' ' ') > +if test -n "$DEBUGINFOD_URLS" > +set --global --export DEBUGINFOD_URLS "$DEBUGINFOD_URLS" > +end > +end I don't know fish, but this looks OK. Note that to turn this profile.fish.in into profile.fish you need to mark it as a config file in configure.ac: diff --git a/configure.ac b/configure.ac index 098d13067ee6..a279bb5282c9 100644 --- a/configure.ac +++ b/configure.ac @@ -881,7 +881,7 @@ AC_ARG_ENABLE(debuginfod-urls, fi], [default_debuginfod_urls=""]) AC_SUBST(DEBUGINFOD_URLS, $default_debuginfod_urls) -AC_CONFIG_FILES([config/profile.sh config/profile.csh]) +AC_CONFIG_FILES([config/profile.sh config/profile.csh config/profile.fish]) AC_OUTPUT Could you sent a new patch with those two changes? Thanks, Mark
Re: [PATCH] riscv: Partial implementation of flatten_aggregate
Hi Palmer,
On Wed, Mar 20, 2024 at 01:17:14PM -0700, Palmer Dabbelt wrote:
> >+flatten_aggregate_arg (Dwarf_Die *typedie,
> >+ Dwarf_Word size,
> >+ Dwarf_Die *arg0,
> >+ Dwarf_Die *arg1)
> > {
> >- /* ??? */
> >+ int tag0, tag1;
> >+ Dwarf_Die member;
> >+ Dwarf_Word encoding0, encoding1;
> >+ Dwarf_Attribute attr;
> >+ Dwarf_Word size0, size1;
> >+
> >+ if (size < 8 || size > 16)
> >+return 0;
>
> IIUC elfutils only supports riscv64? Assuming that's the case this
> looks fine.
Yes, at the moment we only support riscv64, we do accept 32bit riscv
ELF files, but don't know anything about how it handles calling
conventions, core file layout, etc.
> >+ tag1 = dwarf_peeled_die_type (arg1, arg1);
> >+ if (tag1 != DW_TAG_base_type)
> >+return 0; /* We can only handle two equal base types for now. */
> >+
> >+ if (dwarf_attr_integrate (arg1, DW_AT_encoding, ) == NULL
> >+ || dwarf_formudata (, ) != 0
> >+ || encoding0 != encoding1)
> >+return 0; /* We can only handle two of the same for now. */
>
> We have that special case in the psABI where "struct { int; float;
> }" gets split into int/FP registers. I don't really understand
> elfutils, but I think it'll be possible to trip this up with
> something along those lines.
aha, I see...
"A struct containing one floating-point real and one integer (or
bitfield), in either order, is passed in a floating-point register and
an integer register, provided the floating-point real is no more than
ABI_FLEN bits wide and the integer is no more than XLEN bits wide."
Yes, that isn't currently recognized. I'll try to add an update to
handle this specific case.
Thanks,
Mark
Re: [PATCH] Getter and setter for Dwfl's offline_next_address
Hi Martin, On Wed, Mar 06, 2024 at 04:22:49PM -0300, Martin Rodriguez Reboredo wrote: > Added new functions dwfl_get_offline_next_address and > dwfl_set_offline_next_address which will get plus set said field from > the Dwfl struct. This is a requirement for listing functions from their > addresses when using libdwfl offline, otherwise wrong symbols are going > to be returned. Could you show an example of when/where you need it and what address you set it to? The offline_next_address is only relevant for ET_REL files (like object files or kernel modules). In general you will need to check the bias, which various dwfl functions return to know the difference between the addresses in the ELF, Dwarf or the module load address. That said, readelf, nm and dwfl_argp do "cheat" by setting the offline_next_address to zero if they know they are just inspecting a single object file. So maybe this is a functionality we need to expose. But I don't fully understand why you need both a getter and a setter for any arbitrary address. Thanks, Mark
Re: [PATCH] riscv: Partial implementation of flatten_aggregate
Hi Aaron, On Wed, Mar 20, 2024 at 02:14:18PM -0400, Aaron Merey wrote: > On Wed, Mar 20, 2024 at 11:03 AM Mark Wielaard wrote: > > > > dwfl_module_return_value_location would fail on riscv for functions > > which return a (small) struct. This patch implements the simplest > > cases of flatten_aggregate in backends/riscv_retval.c. It just handles > > structs containing one or two members of the same base type which fit > > completely or in pieces in one or two general or floating point > > registers. > > > > It also adds a specific test case run-funcretval-struct.sh containing > > small structs of ints, longs, floats and doubles. All these testscases > > now work for riscv. There is already a slightly more extensive > > testcase for this in tests/run-funcretval.sh but that only has a > > testcase for aarch64. > > This patch LGTM. I'm not able to test it myself on riscv This is why there is the run-funcretval-struct.sh with riscv binary testcase. So at least you can see it generates a DWARF expression for the return values in a riscv ELF binary. > but I see that > for the first time a build has successfully passed on the elfutils riscv > trybot [1]. Yes, nice. David also tested it on the fedora riscv koji builder: http://fedora.riscv.rocks/koji/taskinfo?taskID=1654120 Testsuite summary for elfutils 0.191 # TOTAL: 276 # PASS: 270 # SKIP: 6 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 So I think it generally works. Thanks, Mark > > [1] https://builder.sourceware.org/buildbot/#/builders/273 >
[PATCH] riscv: Partial implementation of flatten_aggregate
dwfl_module_return_value_location would fail on riscv for functions
which return a (small) struct. This patch implements the simplest
cases of flatten_aggregate in backends/riscv_retval.c. It just handles
structs containing one or two members of the same base type which fit
completely or in pieces in one or two general or floating point
registers.
It also adds a specific test case run-funcretval-struct.sh containing
small structs of ints, longs, floats and doubles. All these testscases
now work for riscv. There is already a slightly more extensive
testcase for this in tests/run-funcretval.sh but that only has a
testcase for aarch64.
* backends/riscv_retval.c (flatten_aggregate_arg): Implement
for the simple cases where we have a struct with one or two
members of the same base type.
(pass_by_flattened_arg): Likewise. Call either
pass_in_gpr_lp64 or pass_in_fpr_lp64d.
(riscv_return_value_location_lp64ifd): Call
flatten_aggregate_arg including size.
* tests/Makefile.am (TESTS): Add run-funcretval-struct.sh
and run-funcretval-struct-native.sh.
(check_PROGRAMS): Add funcretval_test_struct.
(funcretval_test_struct_SOURCES): New.
(EXTRA_DIST): Add run-funcretval-struct.sh,
funcretval_test_struct_riscv.bz2 and
run-funcretval-struct-native.sh.
* tests/funcretval_test_struct_riscv.bz2: New test binary.
* tests/run-funcretval-struct-native.sh: New test.
* tests/run-funcretval-struct.sh: Likewise.
https://sourceware.org/bugzilla/show_bug.cgi?id=31142
Signed-off-by: Mark Wielaard
---
backends/riscv_retval.c| 123 ++---
tests/Makefile.am | 7 ++
tests/funcretval_test_struct.c | 86 +
tests/funcretval_test_struct_riscv.bz2 | Bin 0 -> 3821 bytes
tests/run-funcretval-struct-native.sh | 22 +
tests/run-funcretval-struct.sh | 35 +++
6 files changed, 262 insertions(+), 11 deletions(-)
create mode 100644 tests/funcretval_test_struct.c
create mode 100755 tests/funcretval_test_struct_riscv.bz2
create mode 100755 tests/run-funcretval-struct-native.sh
create mode 100755 tests/run-funcretval-struct.sh
diff --git a/backends/riscv_retval.c b/backends/riscv_retval.c
index 0a1e02f81cd2..50c451a4ba32 100644
--- a/backends/riscv_retval.c
+++ b/backends/riscv_retval.c
@@ -1,6 +1,7 @@
/* Function return value location for Linux/RISC-V ABI.
Copyright (C) 2018 Sifive, Inc.
Copyright (C) 2013 Red Hat, Inc.
+ Copyright (C) 2024 Mark J. Wielaard
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -105,23 +106,123 @@ pass_in_fpr_lp64d (const Dwarf_Op **locp, Dwarf_Word
size)
return size <= 8 ? 1 : 4;
}
+/* Checks if we can "flatten" the given type, Only handles the simple
+ cases where we have a struct with one or two the same base type
+ elements. */
static int
-flatten_aggregate_arg (Dwarf_Die *typedie __attribute__ ((unused)),
- Dwarf_Die *arg0 __attribute__ ((unused)),
- Dwarf_Die *arg1 __attribute__ ((unused)))
+flatten_aggregate_arg (Dwarf_Die *typedie,
+ Dwarf_Word size,
+ Dwarf_Die *arg0,
+ Dwarf_Die *arg1)
{
- /* ??? */
+ int tag0, tag1;
+ Dwarf_Die member;
+ Dwarf_Word encoding0, encoding1;
+ Dwarf_Attribute attr;
+ Dwarf_Word size0, size1;
+
+ if (size < 8 || size > 16)
+return 0;
+
+ if (dwarf_child (typedie, arg0) != 0)
+return 0;
+
+ tag0 = dwarf_tag (arg0);
+ while (tag0 != -1 && tag0 != DW_TAG_member)
+{
+ if (dwarf_siblingof (arg0, arg0) != 0)
+ return 0;
+ tag0 = dwarf_tag (arg0);
+}
+
+ if (tag0 != DW_TAG_member)
+return 0;
+
+ /* Remember where we are. */
+ member = *arg0;
+
+ tag0 = dwarf_peeled_die_type (arg0, arg0);
+ if (tag0 != DW_TAG_base_type)
+return 0;
+
+ if (dwarf_attr_integrate (arg0, DW_AT_encoding, ) == NULL
+ || dwarf_formudata (, ) != 0)
+return 0;
+
+ if (dwarf_bytesize_aux (arg0, ) != 0)
+return 0;
+
+ if (size == size0)
+return 1; /* This one member is the whole size. */
+
+ if (size != 2 * size0)
+return 0; /* We only handle two of the same. */
+
+ /* Look for another member with the same encoding. */
+ if (dwarf_siblingof (, arg1) != 0)
+return 0;
+
+ tag1 = dwarf_tag (arg1);
+ while (tag1 != -1 && tag1 != DW_TAG_member)
+{
+ if (dwarf_siblingof (arg1, arg1) != 0)
+ return 0;
+ tag1 = dwarf_tag (arg1);
+}
+
+ if (tag1 != DW_TAG_member)
+return 0;
+
+ tag1 = dwarf_peeled_die_type (arg1, arg1);
+ if (tag1 != DW_TAG_base_type)
+return 0; /* We can only handle two equal base types for now. */
+
+ if (dwarf_attr_integrate (arg1, DW_AT_encoding, ) == NULL
+ || dwarf_formudata (, ) != 0
+
Re: ☠ Buildbot (Sourceware): elfutils-snapshots-coverage - failed test (failure) (main)
On Mon, Mar 11, 2024 at 07:05:53AM -0700, Khem Raj wrote: > On Mon, Mar 11, 2024 at 4:23 AM Mark Wielaard wrote: > > > - 5: make check ( failure ) > > > Logs: > > > - stdio: > > > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/5/logs/stdio > > > - test-suite.log: > > > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/5/logs/test-suite_log > > > > That was clearly not your fault. There seems to be a space missing in > > the make -kcheck command (after the -k). I'll go look in the builder > > sources where that came from. > > Thanks Mark ! Found it, it was a silly missing ',' between strings, causing "-k" "check" to become one string, instead of two... doh. https://sourceware.org/cgit/builder/commit/?id=753c1b3e9b56949af4600b9a43424ce5004361c2 Cheers, Mark
Re: ☠ Buildbot (Sourceware): elfutils-snapshots-coverage - failed test (failure) (main)
Hi Khem, On Mon, 2024-03-11 at 02:33 +, buil...@sourceware.org wrote: > A new failure has been detected on builder elfutils-snapshots-coverage while > building elfutils. > > Full details are available at: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105 > > Build state: failed test (failure) > Revision: d74f4c1d572fbeb7454a2ffa02cbc955ea24780d > Worker: snapshots > Build Reason: (unknown) > Blamelist: Khem Raj > > Steps: > > - 0: worker_preparation ( success ) > > - 1: git checkout ( success ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/1/logs/stdio > > - 2: autoreconf ( success ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/2/logs/stdio > > - 3: configure ( success ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/3/logs/stdio > - config.log: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/3/logs/config_log > > - 4: make ( warnings ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/4/logs/stdio > - warnings (3): > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/4/logs/warnings__3_ > > - 5: make check ( failure ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/5/logs/stdio > - test-suite.log: > https://builder.sourceware.org/buildbot/#/builders/250/builds/105/steps/5/logs/test-suite_log That was clearly not your fault. There seems to be a space missing in the make -kcheck command (after the -k). I'll go look in the builder sources where that came from. Cheers, Mark
Re: [PATCH] Setter for Dwfl's offline_next_address
Hi Martin, On Sat, Mar 02, 2024 at 07:43:38PM -0300, Martin Rodriguez Reboredo wrote: > On 3/2/24 17:47, Mark Wielaard wrote: > >On Fri, Mar 01, 2024 at 05:04:05PM -0300, Martin Rodriguez Reboredo wrote: > >>Added a new function dwfl_set_offline_next_addres which will set said > >>field from the Dwfl struct. This is a requirement for listing functions > >>from their addresses when using libdwfl offline, otherwise wrong symbols > >>are going to be returned. > > > >Could you give an example or testcase for this? > > This is intended for the Linux kernel perf tool so you might see it in > action when I publish the changes. In regards to testing I thought that > it was not needed due to the patch being a simple setter, but as > requested I can think something in the lines of. It would be interesting to see the perf tool patch. I don't understand the use case. So I assume perf currently does something which is wrong and with your patch calling this new dwfl_set_offline_next_addres it will do the right thing. That is what I was thinking of when asking for an example or testcase. I agree that on itself such a simple setter doesn't need a dedicated testcase. But maybe we can come up with a testcase given the right context. Cheers, Mark
Re: [PATCH] Setter for Dwfl's offline_next_address
Hi Martin, On Fri, Mar 01, 2024 at 05:04:05PM -0300, Martin Rodriguez Reboredo wrote: > Added a new function dwfl_set_offline_next_addres which will set said > field from the Dwfl struct. This is a requirement for listing functions > from their addresses when using libdwfl offline, otherwise wrong symbols > are going to be returned. Could you give an example or testcase for this? Thanks, Mark
[COMMITTED] GPG-KEY: Add key for Aaron Merey
Signed-off-by: Mark Wielaard --- GPG-KEY | 34 ++ 1 file changed, 34 insertions(+) diff --git a/GPG-KEY b/GPG-KEY index 671373e6..dca558b7 100644 --- a/GPG-KEY +++ b/GPG-KEY @@ -112,3 +112,37 @@ AwhaG1W+Y3LDe7S19M0cUzftEUeq3Jd89hoijC72tdba+BRfW0ncfvEcsk9QifSU 1tvZxQ== =l2J7 -END PGP PUBLIC KEY BLOCK- +-BEGIN PGP PUBLIC KEY BLOCK- + +mQENBGXb21kBCADCyOpVSfJ6XHVp8+dspYCgkSwr26IF+SXHzt9dVyMChAKJIXib +ty8NA/Huh3ZRNzFXdFM09yoYiglnYu3+r1rniliP1L1w3y0tDNxlvlLmVhM5WAfA +IyZ9tI8X1XIfRLYsX70UM3jDvyoXkEQkjhQKDHrgBdBTNARHlHVykj44xD2TCwao +vV0gFu2EmCN8TsMWLdQ1VymtYd/UFB6znlLzSglzzx4OYKyla6anbWqKxvB3siIH +Pf/ULBh0JNTluH39kk486yE9Lh1z9H8FoVlUWcXvlr0rmPPQDl9se02bkd4rA/gD +kRQmlUXxJIJCNYZiQ7k0nZSYQY6JzhY1szBLABEBAAG0H0Fhcm9uIE1lcmV5IDxh +bWVyZXlAcmVkaGF0LmNvbT6JAVEEEwEIADsWIQRsK2MVY7jTMFeNPLR0/T+id55w +cwUCZdvbWQIbAwULCQgHAgIiAgYVCgkICwIEFgIDAQIeBwIXgAAKCRB0/T+id55w +cxHiCAC9evJD8CqSPOfuKFa2iITLoSM2ORdpsp5Wla9OpAV5GnUNKbR7KCLoG7OQ +3Tth9qdHqrCIUigvj2xzcaovdyVRpDLBFG5cbrBB7Jqly0ptJhFUtx8wbDBammZq +7ZAsLFP582ILqlLjcTIOUNk4ABuTDN9FIdewgWXMW7GPp5euJ4ucos2glySwLbC4 ++6kZ1guYYbO3S9YWWyImP2Guf4KVi5kTF0USTklvszJLvwJNkYknQhxnDcWLeOPJ +P4hOdHlwJSsaXcKyOn//FdMXRGDcknKcn43e03esXjflvz8HyXz4VbjEqDpOHS9i +l/NOw6+C7+wcAvHDu583cMbuI7hziEYEEBEIAAYFAmXiB7IACgkQVZbdDOm/ZT2Q +fwCfQSmNDkHu3ZxwvWs+mOFaYR3PGRAAnieQMVnPZd8kgGkVO/GNIJIBeuA7iF0E +EBECAB0WIQRBoMESdLHof+KOTFPj134flRYZkQUCZeIIBAAKCRDj134flRYZkX4N +AJ0Q8p1f7lmWgiHaHmD3DtuJ7uhWHACgk84evo0UsqVNi/4xHZ0wpVvxx+S5AQ0E +ZdvbWQEIAKgzEcJMUfFgsUGwMO/I4mwjvB7+Jx90lSYjS3uM7E3jnYhu/en42skl +nYEMxMR9EWUF6RZ78QiUvD/Ik6fP0YrMuTRBnZ7ZOd5zLPbgDIAOVeiVxFRwGOzj +z01V/plDlz+zCQkS1tOPgpGKCzTol7/M9ks9AHqKE6DWrrk1LbER2qDyE6XMAe5b +LNqdmtfJ2uf2XPThGGz0ujJ0MdistoRMechX9qBIBaLwxuA5edx+3iu6MdjMCeua +yV+Yb4ePG+9IY3OmdC/3wNpJYOQvSsJz9PzX4Nf1PuIaT/JoQSmZa8aZYgtCVMCV +t15UDdpPTyuFeXSBYLkuSQGM6DqARnEAEQEAAYkBNgQYAQgAIBYhBGwrYxVjuNMw +V408tHT9P6J3nnBzBQJl29tZAhsMAAoJEHT9P6J3nnBzAJYH/1eX+r8VEDPU/TKA +yqYW2Mg87jyo8+t2x0zFZfucni/0o3ejaSxSVtnWiFh79OWnACnbq0go+pT59X7V +03VcYQgRpzLuD0OzcOmkHI4kgbB2Q3zNszlqb67Lkt+P9xy16DWS8N9qujfiTw6q ++xT/BsSxmOuWxexrRjui012g28TcihlQIRBOtJIA2vEwUWjuMkzno2Xbrxq99WH8 +iSr3bTGc/jpnMetW4iHE8VxUJ7ixvbcE4vWsfUqPyr/5U25a2D8XNmvqw9J1msNE +5dlWzgmpLMHLVzKigvJffjsYxxLvekbSv/mJgonhDXUQvUnwy0bx41te/neXHrEv +S0Zj1TU= +=iz3/ +-END PGP PUBLIC KEY BLOCK- -- 2.43.2
Re: [PATCH v3 4/4] libdw: Handle overflowed DW_SECT_INFO offsets in DWARF package file indexes
Hi,
On Fri, 2024-03-01 at 15:59 +0100, Mark Wielaard wrote:
> This looks correct, but gcc noticed a path to use tu_offset (and
> tu_index) if they weren't initialized or NULL:
>
> In file included from /home/mark/src/elfutils/libdw/libdwP.h:684,
> from
> /home/mark/src/elfutils/libdw/dwarf_cu_dwp_section_info.c:35:
> In function ‘read_4ubyte_unaligned_1’,
> inlined from ‘__libdw_package_index’ at
> /home/mark/src/elfutils/libdw/dwarf_cu_dwp_section_info.c:302:34:
> /home/mark/src/elfutils/libdw/memory-access.h:291:12: error: ‘tu_offset’ may
> be used uninitialized [-Werror=maybe-uninitialized]
> 291 | return up->u4;
> | ~~^~~~
> /home/mark/src/elfutils/libdw/dwarf_cu_dwp_section_info.c: In function
> ‘__libdw_package_index’:
> /home/mark/src/elfutils/libdw/dwarf_cu_dwp_section_info.c:268:28: note:
> ‘tu_offset’ was declared here
> 268 | const unsigned char *tu_offset;
> |^
> cc1: all warnings being treated as errors
>
> I couldn't immediately disprove gcc here, so I think it is a good idea
> to add an explicit check for tu_index != NULL.
>
> diff --git a/libdw/dwarf_cu_dwp_section_info.c
> b/libdw/dwarf_cu_dwp_section_info.c
> index 3d11c87a..9fdc15bf 100644
> --- a/libdw/dwarf_cu_dwp_section_info.c
> +++ b/libdw/dwarf_cu_dwp_section_info.c
> @@ -297,7 +297,8 @@ __libdw_package_index (Dwarf *dbg, bool tu)
> cu_index->debug_info_offsets[cui++] = off;
> cu_offset += cu_index->section_count * 4;
> }
> - else if (unit_type == DW_UT_split_type && tui < tu_count)
> + else if (unit_type == DW_UT_split_type && tu_index != NULL
> + && tui < tu_count)
> {
> if ((off & UINT32_MAX) != read_4ubyte_unaligned (dbg,
> tu_offset))
> goto not_sorted;
>
> Which makes gcc happy again.
But not all gcc versions apparently. So I added the following on top.
From cc6e53b9f305148bda275ade40c0e625d98da2f2 Mon Sep 17 00:00:00 2001
From: Mark Wielaard
Date: Fri, 1 Mar 2024 17:05:16 +0100
Subject: [PATCH] libdw: Initialize tu_offset in __libdw_package_index
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
dwarf_cu_dwp_section_info.c: In function ‘__libdw_package_index’:
dwarf_cu_dwp_section_info.c:306:25: error: ‘tu_offset’ may be used uninitialized [-Werror=maybe-uninitialized]
306 | tu_offset += tu_index->section_count * 4;
| ~~^~
dwarf_cu_dwp_section_info.c:268:28: note: ‘tu_offset’ was declared here
268 | const unsigned char *tu_offset;
|^
Which is the same issue we thought to have fixed by checking for
tu_index != NULL but not all gcc versions seem able to see that.
So just explicitly initialize tu_offset to NULL. We keep the older
check, so the NULL pointer should never be used.
* libdw/dwarf_cu_dwp_section_info.c (__libdw_package_index):
Initialize tu_offset.
Signed-off-by: Mark Wielaard
---
libdw/dwarf_cu_dwp_section_info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libdw/dwarf_cu_dwp_section_info.c b/libdw/dwarf_cu_dwp_section_info.c
index 9fdc15bf..5a081f5a 100644
--- a/libdw/dwarf_cu_dwp_section_info.c
+++ b/libdw/dwarf_cu_dwp_section_info.c
@@ -265,7 +265,7 @@ __libdw_package_index (Dwarf *dbg, bool tu)
const unsigned char *cu_offset
= cu_index->section_offsets + cu_index->sections[DW_SECT_INFO - 1] * 4;
uint32_t tu_count = 0;
- const unsigned char *tu_offset;
+ const unsigned char *tu_offset = NULL;
if (tu_index != NULL)
{
tu_count = tu_index->unit_count;
--
2.43.2
Re: [PATCH v3 4/4] libdw: Handle overflowed DW_SECT_INFO offsets in DWARF package file indexes
Hi Omar,
On Mon, 2024-02-26 at 11:32 -0800, Omar Sandoval wrote:
> Meta uses DWARF package files for our large, statically-linked C++
> applications. Some of our largest applications have more than 4GB in
> .debug_info.dwo, but the section offsets in .debug_cu_index and
> .debug_tu_index are 32 bits; see the discussion here [1]. We
> implemented a workaround/extension for this in LLVM. Implement the
> equivalent in libdw.
>
> To test this, we need files with more than 4GB in .debug_info.dwo. I
> created these artificially by editing GCC's assembly output. They
> compress down to 6KB. I test them from run-large-elf-file.sh to take
> advantage of the existing checks for large file support.
>
> 1: https://discourse.llvm.org/t/dwarf-dwp-4gb-limit/63902.
>
> * libdw/dwarf_end.c (dwarf_package_index_free): New function.
> * tests/testfile-dwp-4-cu-index-overflow.bz2: New test file.
> * tests/testfile-dwp-4-cu-index-overflow.dwp.bz2: New test file.
> * tests/testfile-dwp-5-cu-index-overflow.bz2: New test file.
> * tests/testfile-dwp-5-cu-index-overflow.dwp.bz2: New test file.
> * tests/testfile-dwp-cu-index-overflow.source: New file.
> * tests/run-large-elf-file.sh: Check
> testfile-dwp-5-cu-index-overflow and
> testfile-dwp-4-cu-index-overflow.
The hack is kind of horrible, but given that this doesn't really
impacts "normal" dwp files and it does work with clang/lldb, lets just
support it too.
> Signed-off-by: Omar Sandoval
> ---
> libdw/dwarf_cu_dwp_section_info.c | 147 ++-
> libdw/dwarf_end.c | 15 +-
> libdw/libdwP.h| 3 +
> tests/Makefile.am | 7 +-
> tests/run-large-elf-file.sh | 174 ++
> tests/testfile-dwp-4-cu-index-overflow.bz2| Bin 0 -> 4490 bytes
> .../testfile-dwp-4-cu-index-overflow.dwp.bz2 | Bin 0 -> 5584 bytes
> tests/testfile-dwp-5-cu-index-overflow.bz2| Bin 0 -> 4544 bytes
> .../testfile-dwp-5-cu-index-overflow.dwp.bz2 | Bin 0 -> 5790 bytes
> tests/testfile-dwp-cu-index-overflow.source | 86 +
> 10 files changed, 426 insertions(+), 6 deletions(-)
> create mode 100755 tests/testfile-dwp-4-cu-index-overflow.bz2
> create mode 100644 tests/testfile-dwp-4-cu-index-overflow.dwp.bz2
> create mode 100755 tests/testfile-dwp-5-cu-index-overflow.bz2
> create mode 100644 tests/testfile-dwp-5-cu-index-overflow.dwp.bz2
> create mode 100644 tests/testfile-dwp-cu-index-overflow.source
>
> diff --git a/libdw/dwarf_cu_dwp_section_info.c
> b/libdw/dwarf_cu_dwp_section_info.c
> index 298f36f9..3d11c87a 100644
> --- a/libdw/dwarf_cu_dwp_section_info.c
> +++ b/libdw/dwarf_cu_dwp_section_info.c
> @@ -30,6 +30,8 @@
> # include
> #endif
>
> +#include
> +
> #include "libdwP.h"
>
> static Dwarf_Package_Index *
> @@ -110,7 +112,9 @@ __libdw_read_package_index (Dwarf *dbg, bool tu)
>
>index->dbg = dbg;
>/* Set absent sections to UINT32_MAX. */
> - memset (index->sections, 0xff, sizeof (index->sections));
> + for (size_t i = 0;
> + i < sizeof (index->sections) / sizeof (index->sections[0]); i++)
> +index->sections[i] = UINT32_MAX;
>for (size_t i = 0; i < section_count; i++)
> {
>uint32_t section = read_4ubyte_unaligned (dbg, sections + i * 4);
> @@ -161,6 +165,7 @@ __libdw_read_package_index (Dwarf *dbg, bool tu)
>index->indices = indices;
>index->section_offsets = section_offsets;
>index->section_sizes = section_sizes;
> + index->debug_info_offsets = NULL;
>
>return index;
> }
> @@ -177,6 +182,137 @@ __libdw_package_index (Dwarf *dbg, bool tu)
>if (index == NULL)
> return NULL;
>
> + /* Offsets in the section offset table are 32-bit unsigned integers. In
> + practice, the .debug_info.dwo section for very large executables can be
> + larger than 4GB. GNU dwp as of binutils 2.41 and llvm-dwp before LLVM
> 15
> + both accidentally truncate offsets larger than 4GB.
> +
> + LLVM 15 detects the overflow and errors out instead; see LLVM commit
> + f8df8114715b ("[DWP][DWARF] Detect and error on debug info offset
> + overflow"). However, lldb in LLVM 16 supports using dwp files with
> + truncated offsets by recovering them directly from the unit headers in
> the
> + .debug_info.dwo section; see LLVM commit c0db06227721 ("[DWARFLibrary]
> Add
> + support to re-construct cu-index"). Since LLVM 17, the overflow error
> can
> + be turned into a warning instead; see LLVM commit 53a483cee801 ("[DWP]
> add
> + overflow check for llvm-dwp tools if offset overflow").
> +
> + LLVM's support for > 4GB offsets is effectively an extension to the
> DWARF
> + package file format, which we implement here. The strategy is to walk
> the
> + unit headers in .debug_info.dwo in lockstep with the DW_SECT_INFO
> columns
> + in the section offset tables.
Re: [PATCH v3 3/4] libdw: Apply DWARF package file section offsets where appropriate
Hi Omar,
On Mon, Feb 26, 2024 at 11:32:50AM -0800, Omar Sandoval wrote:
> The final piece of DWARF package file support is that offsets have to be
> interpreted relative to the section offset from the package index.
> .debug_abbrev.dwo is already covered, so sprinkle around calls to
> dwarf_cu_dwp_section_info for the remaining sections: .debug_line.dwo,
> .debug_loclists.dwo/.debug_loc.dwo, .debug_str_offsets.dwo,
> .debug_macro.dwo/.debug_macinfo.dwo, and .debug_rnglists.dwo. With all
> of that in place, we can finally test various libdw functions on dwp
> files.
>
> * libdw/dwarf_getlocation.c (initial_offset): Call
> dwarf_cu_dwp_section_info and add offset to start_offset.
> * libdw/dwarf_getmacros.c (get_macinfo_table): Call
> dwarf_cu_dwp_section_info and add offset to line_offset.
> (get_offset_from): Call dwarf_cu_dwp_section_info and add offset
> to *retp.
> * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Call
> dwarf_cu_dwp_section_info and pass offset to
> __libdw_getsrclines.
> * libdw/dwarf_next_lines.c (dwarf_next_lines): Call
> dwarf_cu_dwp_section_info and add offset to stmt_off.
> * libdw/libdwP.h (str_offsets_base_off): Call
> dwarf_cu_dwp_section_info and add offset.
> (__libdw_cu_ranges_base): Ditto.
> (__libdw_cu_locs_base): Ditto.
> * tests/run-all-dwarf-ranges.sh: Check testfile-dwp-5 and
> testfile-dwp-4.
> * tests/run-declfiles.sh: Ditto.
> * tests/run-get-lines.sh: Ditto.
> * tests/run-next-lines.sh: Ditto.
> * tests/run-varlocs.sh: Ditto.
> * tests/run-get-files.sh: Check testfile-dwp-5,
> testfile-dwp-5.dwp, testfile-dwp-4, and testfile-dwp-4.dwp
> * tests/run-next-files.sh: Ditto.
> * tests/run-dwarf-getmacros.sh: Check testfile-dwp-5 and
> testfile-dwp-4-strict.
> * tests/run-get-units-split.sh: Ditto.
Very nice. So the only changes from V2 are
> diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c
> index 12fdabf2..cd2e5b5a 100644
> --- a/libdw/dwarf_getsrcfiles.c
> +++ b/libdw/dwarf_getsrcfiles.c
> @@ -64,12 +64,17 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
> size_t *nfiles)
>the table is at offset zero. */
> if (cu->dbg->sectiondata[IDX_debug_line] != NULL)
> {
> - /* We are only interested in the files, the lines will
> - always come from the skeleton. */
> - res = __libdw_getsrclines (cu->dbg, 0,
> - __libdw_getcompdir (cudie),
> - cu->address_size, NULL,
> - >files);
> + Dwarf_Off dwp_off;
> + if (INTUSE(dwarf_cu_dwp_section_info) (cu, DW_SECT_LINE,
> + _off, NULL) == 0)
> + {
> + /* We are only interested in the files, the lines will
> + always come from the skeleton. */
> + res = __libdw_getsrclines (cu->dbg, dwp_off,
> + __libdw_getcompdir (cudie),
> + cu->address_size, NULL,
> + >files);
> + }
> }
> else
> {
Looks correct.
> diff --git a/libdw/dwarf_next_lines.c b/libdw/dwarf_next_lines.c
> index 6a9fe361..a96bd73e 100644
> --- a/libdw/dwarf_next_lines.c
> +++ b/libdw/dwarf_next_lines.c
> @@ -132,6 +132,11 @@ dwarf_next_lines (Dwarf *dbg, Dwarf_Off off,
> && next_cu->unit_type != DW_UT_split_type)
> continue;
>
> + Dwarf_Off dwp_off;
> + if (INTUSE(dwarf_cu_dwp_section_info) (next_cu, DW_SECT_LINE,
> + _off, NULL) == 0)
> + stmt_off += dwp_off;
> +
> if (stmt_off == off)
> {
> *cu = next_cu;
Also correct.
And the new tests in run-declfiles.sh, run-get-files.sh,
run-get-lines.sh, run-next-files.sh and run-next-lines.sh.
You are certainly very thorough. Thanks.
Pushed,
Mark
Re: [PATCH v3 2/4] libdw: Refactor dwarf_next_lines and fix skipped CU
Hi Omar, On Mon, Feb 26, 2024 at 11:32:49AM -0800, Omar Sandoval wrote: > dwarf_next_lines has two loops over CUs: one from the CU after the given > CU to the end, and one from the first CU up to _but not including_ the > given CU. This means that the given CU is never checked. > > This is unlikely to matter in practice since CUs usually correspond 1:1 > with line number tables in the same order, but let's fix it anyways. > Refactoring it to one loop fixes the problem and simplifies the next > change to support DWARF package files. > > * libdw/dwarf_next_lines.c (dwarf_next_lines): Refactor loops > over CUs into one loop. Thanks. The patch itself was hard to understand, but when applied the code is pretty clear. I only had to double check the reversal of the split unit check was correct. Pushed, Mark
Re: [PATCH v3 1/4] libdw: Handle split DWARF in dwarf_decl_file
Hi Omar, On Mon, Feb 26, 2024 at 11:32:48AM -0800, Omar Sandoval wrote: > Calling dwarf_decl_file on a split DWARF DIE fails this assertion: > > dwarf_decl_file.c:72: dwarf_decl_file: Assertion `cu->files != NULL && > cu->files != (void *) -1l' failed. > > This is because dwarf_decl_file calls dwarf_getsrclines to populate > cu->files. For normal units, cu->files is cached by dwarf_getsrclines > when it parses the line number information. However, for split units, > the line number information is parsed for the skeleton unit, then copied > to the split unit's cu->lines. Split units have their own file name > table, so cu->files is not copied. > > The obvious solution is to use dwarf_getsrcfiles instead of relying on > implicit caching. And it cleans up the code nicely. > Also add a test case for dwarf_decl_file. Thanks. > > * libdw/dwarf_decl_file.c (dwarf_decl_file): Use > dwarf_getsrcfiles instead of dwarf_getsrclines. > * tests/Makefile.am (check_PROGRAMS): Add declfiles. > (TESTS): Add run-declfiles.sh. > (EXTRA_DIST): Add run-declfiles.sh. > (declfiles_LDADD): New variable. > * tests/declfiles.c: New test. > * tests/run-declfiles.sh: New test. All looks good. Pushed. Cheers, Mark
Re: [PATCH v2] Add __libdw_getdieranges
Hi Aaron, On Tue, Feb 27, 2024 at 08:11:39PM -0500, Aaron Merey wrote: > __libdw_getdieranges builds an aranges list by iterating over each > CU and recording each address range. > > This function is an alternative to dwarf_getaranges. dwarf_getaranges > attempts to read address ranges from .debug_aranges, which might be > absent or incomplete. > > This patch replaces dwarf_getaranges with __libdw_getdieranges in > dwarf_addrdie and dwfl_module_addrdie. The existing tests in > run-getsrc-die.sh are also rerun with .debug_aranges removed from > the testfiles. > > https://sourceware.org/bugzilla/show_bug.cgi?id=22288 > https://sourceware.org/bugzilla/show_bug.cgi?id=30948 > > Signed-off-by: Aaron Merey > --- > > v2 addresses feedback from Mark's review: > https://sourceware.org/pipermail/elfutils-devel/2024q1/006853.html > > Avoid calling free on arangelist when it's possibly corrupt. > Run tests in run-getsrc-die.sh twice, once with .debug_aranges > present in the testfile and once with the section removed. This looks good to me. Please also add a NEWS entry about this. Thanks, Mark
Re: [PATCH] readelf: Use unsigned loop variables in handle_verneed and handle_verdef
On Wed, 2024-02-21 at 22:19 +0100, Mark Wielaard wrote: > Prevent signed underflow by changing loop variables to unsigned and > doing count checks before decrementing. This isn't really a bug, but > prevents UB detected by ubsan on fuzzed input. The bad (fuzzed) input > data does get detected anyway. > > * src/readelf.c (handle_verneed): Use unsigned cnt, cnt2. > (handle_verdef): Likewise. Pushed after a quick chat with Aaron on irc.
Re: [PATCH] libebl: ebl_object_note print 32bit annobin address ranges correctly
Hi, On Wed, 2024-02-21 at 21:59 +0100, Mark Wielaard wrote: > Annobin address ranges were always printed as if they were 64bit wide > because addr_size was set to twice the size. This was done because the > note description size should contain two addresses. Fix this by setting > the address size to just one address and then check that descsz is > twice that. > > * libebl/eblobjnote.c (ebl_object_note): Set addr_size to one > ELF_T_ADDR. Check descsz equals two times addr_size. After a quick chat with Aaron on irc I pushed this. Cheers, Mark
Re: [PATCH] Add __libdw_getdieranges
Hi Aaron,
On Mon, 2024-02-26 at 10:40 -0500, Aaron Merey wrote:
> __libdw_getdieranges builds an aranges list by iterating over CUs and
> recording each address range.
>
> __libdw_getdieranges provides an alternative to relying on .debug_aranges
> for address ranges, since this section might be absent or incomplete.
Please mention in the commit message that this is now used by
dwarf_addrdie and dwfl_module_addrdie.
You might also want to mention the testcase change in the commit
message as hint for future developers working on this code (see below)
> https://sourceware.org/bugzilla/show_bug.cgi?id=22288
> https://sourceware.org/bugzilla/show_bug.cgi?id=30948
>
> Signed-off-by: Aaron Merey
Looks good, but I have some questions about the assert replacements.
> ---
>
> Once this function's interface is refined it can be added to the public
> libdw API. We might add some form of lazy loading, for example.
>
> I mentioned [1] that there are some clang-compiled shared libraries
> where some subprograms starting at address 0 don't have a corresponding
> entry in .debug_ranges (for a CU that contains DW_AT_ranges). These
> address ranges with no corresponding entry in .debug_ranges won't be
> included in the aranges generated by this patch's CU iteration approach.
Right. But those are "data" addresses not code scopes/ranges.
> [1] https://sourceware.org/pipermail/elfutils-devel/2024q1/006832.html
>
> libdw/dwarf_addrdie.c| 2 +-
> libdw/dwarf_getaranges.c | 190 ++-
> libdw/libdwP.h | 13 ++-
> libdwfl/cu.c | 8 +-
> tests/run-getsrc-die.sh | 5 ++
> 5 files changed, 169 insertions(+), 49 deletions(-)
>
> diff --git a/libdw/dwarf_addrdie.c b/libdw/dwarf_addrdie.c
> index 3a08ab75..48a1aaea 100644
> --- a/libdw/dwarf_addrdie.c
> +++ b/libdw/dwarf_addrdie.c
> @@ -41,7 +41,7 @@ dwarf_addrdie (Dwarf *dbg, Dwarf_Addr addr, Dwarf_Die
> *result)
>size_t naranges;
>Dwarf_Off off;
>
> - if (INTUSE(dwarf_getaranges) (dbg, , ) != 0
> + if (__libdw_getdieranges (dbg, , ) != 0
>|| INTUSE(dwarf_getarangeinfo) (INTUSE(dwarf_getarange_addr) (aranges,
> addr),
> NULL, NULL, ) != 0)
> diff --git a/libdw/dwarf_getaranges.c b/libdw/dwarf_getaranges.c
OK.
> index 27439d37..41fe96d0 100644
> --- a/libdw/dwarf_getaranges.c
> +++ b/libdw/dwarf_getaranges.c
> @@ -33,7 +33,6 @@
> #endif
>
> #include
> -#include
> #include "libdwP.h"
> #include
>
> @@ -54,6 +53,149 @@ compare_aranges (const void *a, const void *b)
>return 0;
> }
>
> +/* Convert ARANGELIST into Dwarf_Aranges and store at ARANGES. */
> +static bool
> +finalize_aranges (Dwarf *dbg, Dwarf_Aranges **aranges, size_t *naranges,
> + struct arangelist *arangelist, unsigned int narangelist)
> +{
> + /* Allocate the array for the result. */
> + void *buf = libdw_alloc (dbg, Dwarf_Aranges,
> +sizeof (Dwarf_Aranges)
> ++ narangelist * sizeof (Dwarf_Arange), 1);
> +
> + /* First use the buffer for the pointers, and sort the entries.
> + We'll write the pointers in the end of the buffer, and then
> + copy into the buffer from the beginning so the overlap works. */
> + eu_static_assert (sizeof (Dwarf_Arange) >= sizeof (Dwarf_Arange *));
> + struct arangelist **sortaranges
> += (buf + sizeof (Dwarf_Aranges)
> + + ((sizeof (Dwarf_Arange) - sizeof sortaranges[0]) * narangelist));
> +
> + /* The list is in LIFO order and usually they come in clumps with
> + ascending addresses. So fill from the back to probably start with
> + runs already in order before we sort. */
> + unsigned int i = narangelist;
> + while (i-- > 0)
> +{
> + sortaranges[i] = arangelist;
> + arangelist = arangelist->next;
> +}
> +
> + /* Something went wrong if narangelist is less then the actual length
> + of arangelist. */
> + if (arangelist != NULL)
> +{
> + __libdw_seterrno (DWARF_E_UNKNOWN_ERROR);
> + return false;
> +}
This replaces assert (arangelist == NULL);
> + /* Sort by ascending address. */
> + qsort (sortaranges, narangelist, sizeof sortaranges[0], _aranges);
> +
> + /* Now that they are sorted, put them in the final array.
> + The buffers overlap, so we've clobbered the early elements
> + of SORTARANGES by the time we're reading the later ones. */
> + *aranges = buf;
> + (*aranges)->dbg = dbg;
> + (*aranges)->naranges = narangelist;
> + if (naranges != NULL)
> +*naranges = narangelist;
> + for (i = 0; i < narangelist; ++i)
> +{
> + struct arangelist *elt = sortaranges[i];
> + (*aranges)->info[i] = elt->arange;
> + free (elt);
> +}
> +
> + return true;
> +}
OK, this comes from dwarf_aranges, to be reused by
__libdw_getdieranges.
> +int
> +__libdw_getdieranges (Dwarf *dbg, Dwarf_Aranges
Sourceware infrastructure updates for Q1 2024
Sourceware infrastructure community updates for Q1 2024 A summary of news about Sourceware, the Free Software hosting project for core toolchain and developer tools, from the last 3 months. - Sourceware now has an official donation page - StarFive VisionFive-2 RISC-V boards for builder.sourceware.org - server2 and server3 disk drive updates - Upgrading project websites from CVS to GIT - Sourceware @ Fosdem - Security policy updates for a CVE system out of control - Summer of Code = Sourceware now has an official donation page Sourceware is a Free Software hosting project for core toolchain and developer tools. Sourceware is maintained by volunteers. Hardware and bandwidth is provided by sponsors. Sourceware is a Software Freedom Conservancy member project. Conservancy handles all non-profit administrivia for Sourceware. Thanks to the Conservancy we already have collected enough money for an emergency hardware replacement fund. In case one of our hardware partners would suddenly and unexpectedly drop support we can now simply buy new hardware. And we now also have an official donation page to help fund accelerating tasks the community feels most useful. https://sourceware.org/donate.html = StarFive VisionFive-2 RISC-V boards for builder.sourceware.org StarFive has donated 4 VisionFive-2 risc-v boards with 8GB, 4-core JH7110 supporting the RV64GC ISA for the CI running on builder.sourceware.org. Which has allowed us to setup CI (and try) builders for various projects: annobin, binutils(+try), bzip2, debugedit, dwz, elfutils(+try), glibc, poke and libabigail(+try). Please contact the builder project if you want to help out with the CI services. https://sourceware.org/mailman/listinfo/buildbot = server2 and server3 disk drive updates One of the drives in server2 broke down. It was part of a 10 drive raid6 setup, which can take 2 bad disks before full failure. We also have a full mirror on server3, which has a similar raid6 setup. We ordered 3 new disks, one as replacement for the bad disk and a spare for server2 and server3 in case of future drive failures. The drive has been replaced and everything is running smoothly. We have a fund for replacing hardware when needed. But if you want to help out keeping everything running smoothly you can donate on our new donation page https://sourceware.org/donate.html = Upgrading project websites from CVS to GIT. Various projects were still creating their project homepages from CVS. We upgraded both glibc and binutils to have a public git htdocs repository now to which the whole community can contribute. https://sourceware.org/cgit/binutils-htdocs/ https://sourceware.org/cgit/glibc-htdocs/ Please contact us if you want to upgrade how you publish your projects homepage. https://sourceware.org/mission.html#organization = Sourceware @ Fosdem 2024 started strong with various Sourceware core toolchain and developer tool projects presenting at Fosdem. If you missed the in person meetings, most talks have video recordings: https://fosdem.org/2024/schedule/track/gcc/ https://fosdem.org/2024/schedule/track/debuggers-and-analysis/ https://fosdem.org/2024/schedule/event/fosdem-2024-2207-the-plan-for-gccrs/ Various Sourceware volunteers, overseers and project leadership committee members also met informally with FSF/GNU and SFC admins to coordinate cross free software infrastructure administration matters. And if you like to organize an online virtual mini-BoF around some topic or project then the Conservancy BBB server is available for all Sourceware projects. You can create your own account at https://bbb.sfconservancy.org/b/signup which we can then activate for you. Note: Anyone is able to join a meeting, accounts are only required to create new meetings. = Security policy updates for a CVE system out of control The Common Vulnerabilities and Exposures (CVE) system seems broken and has been issuing more and more questionable advisories. Various Sourceware hosted projects have been writing security policies to help users know which bugs might have security implications. https://sourceware.org/cgit/elfutils/tree/SECURITY https://sourceware.org/cgit/binutils-gdb/tree/binutils/SECURITY.txt https://gcc.gnu.org/cgit/gcc/tree/SECURITY.txt The glibc project even setup their own security mailinglist and CNA (CVE Numbering Authority) publishing their own advisories: https://sourceware.org/glibc/security.html https://sourceware.org/cgit/glibc/tree/advisories If you need any help adding infrastructure services for your security projects, please reach out: https://sourceware.org/mission.html#organization = Summer of Code Some Sourceware hosted projects will take part in Summer of Code 2024. If you are interested in participating please see https://gcc.gnu.org/wiki/SummerOfCode
Re: [PATCH v2 3/4] libdw: Apply DWARF package file section offsets where appropriate
Hi Omar, On Thu, Feb 22, 2024 at 05:03:44PM -0800, Omar Sandoval wrote: > On Thu, Feb 22, 2024 at 04:53:19PM -0800, Omar Sandoval wrote: > > On Fri, Feb 16, 2024 at 04:00:47PM +0100, Mark Wielaard wrote: > > > Don't we also need to handle DW_SECT_LINE in dwarf_getsrclines and > > > dwarf_next_lines when looking for DW_AT_stmt_list? > > > > .debug_line is the odd one out in split DWARF: the skeleton file > > contains the full .debug_line, and the DWO or DWP files have a skeleton > > .debug_line.dwo that only contains the directory and file name tables > > (for DW_AT_file and macro info to reference). dwarf_getsrclines and co. > > read from the skeleton file, not the DWP file, meaning they shouldn't > > use DW_SECT_LINE. > > Ah, I guess you can call dwarf_getsrclines/dwarf_next_lines on the dwp > file itself, where DW_SECT_LINE may be applicable. I need to think about > that some more... So reading the DWARF5 spec, it says a split DWARF CU/TU DIE may have a DW_AT_stmt_list, which are interpreted as relative to the base offset for .debug_line.dwo. And these tables contain only the directory and filename lists needed to interpret DW_AT_decl_file attributes in the debugging information entries. Actual line number tables remain in the .debug_line section, and remain in the relocatable object (.o) files. So I think the intention is that the main .debug_line (skeleton) section does contain the actual line number table, but only those file/dir table entries that are referenced from that. Not any that are only referenced from the DW_AT_decl_file attributes (which should only appear in the split DWARF DIEs). Maybe in practice these overlap completely, so there is no savings and they are in practice identical. But I don't see anything in the spec that implies you should interpret a lineptr in the .debug_info.dwo as relative to the .debug_line section in the skeleton. Cheers, Mark
Re: [PATCH v2 3/4] libdw: Apply DWARF package file section offsets where appropriate
Hi Omar, On Thu, Feb 22, 2024 at 04:53:19PM -0800, Omar Sandoval wrote: > On Fri, Feb 16, 2024 at 04:00:47PM +0100, Mark Wielaard wrote: > > The code and tests look good. run-varlocs.sh seems good, which seems to > > confirm DW_SECT_LOCLISTS is handled correctly (but why doesn't it need > > a hack similar to ranges in __libdw_formptr?). > > I think it's because ranges have the uniquely weird behavior in DWARF 4 > GNU Debug Fission that DW_AT_GNU_ranges_base is in the skeleton file but > used to interpret the split file. This was cleaned up for DWARF 5 (as I > documented in commit c9c9ffae725009b192b40e2d89035f353ae7055f), and > there was no base attribute for location lists in DWARF 4, so it's not > applicable. Thanks. I forgot about commit c9c9ffae7 "libdw: Handle DW_AT_ranges in split DWARF 5 skeleton in dwarf_ranges". That documents it nicely. Cheers, Mark
Re: [PATCH 0/2] Update LoongArch relocations for psABI v2.30
Hi, On Fri, Feb 23, 2024 at 12:47:26PM +0800, Xi Ruoyao wrote: > LoongArch psABI v2.30 has introduced 17 new reloc types for TLS > descriptor, TLS LE relaxation, and medium code model function call. Add > them to elfutils. > > Tested on loongarch64-linux-gnu with Binutils-2.42 and GCC 14 (trunk). > > Xi Ruoyao (2): > libelf: Sync elf.h from glibc > backends: Update list of LoongArch relocations Thanks, looks good. Both patches pushed. Cheers, Mark
Re: [PATCH] libdw: Update dwarf_cu_dwp_section_info documentation
Hi Omar, On Thu, Feb 22, 2024 at 09:54:29AM -0800, Omar Sandoval wrote: > On Fri, Feb 16, 2024 at 02:34:18PM +0100, Mark Wielaard wrote: > > Update the documentation of dwarf_cu_dwp_section_info to make clear > > that the function only returns an error if the DWARF package file data > > couldn't be read or an unknown section constant is provided. Missing > > DWP information for a given CU isn't an error and will set both OFFSET > > and SIZE to zero. It also makes sure the documentation is < 76 chars > > wide. > > > > * libdw/libdw.h (dwarf_cu_dwp_section_info): Update docs. > > > > Signed-off-by: Mark Wielaard > > This looks good to me, thanks Mark. Thanks for the review. Pushed. Cheers, Mark
[PATCH] readelf: Use unsigned loop variables in handle_verneed and handle_verdef
Prevent signed underflow by changing loop variables to unsigned and
doing count checks before decrementing. This isn't really a bug, but
prevents UB detected by ubsan on fuzzed input. The bad (fuzzed) input
data does get detected anyway.
* src/readelf.c (handle_verneed): Use unsigned cnt, cnt2.
(handle_verdef): Likewise.
Signed-off-by: Mark Wielaard
---
src/readelf.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/readelf.c b/src/readelf.c
index 802f8ede..0e931184 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -3159,7 +3159,7 @@ handle_verneed (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr)
elf_strptr (ebl->elf, shstrndx, glink->sh_name));
unsigned int offset = 0;
- for (int cnt = shdr->sh_info; --cnt >= 0; )
+ for (unsigned int cnt = shdr->sh_info; cnt > 0; cnt--)
{
/* Get the data at the next offset. */
GElf_Verneed needmem;
@@ -3173,7 +3173,7 @@ handle_verneed (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr)
(unsigned short int) need->vn_cnt);
unsigned int auxoffset = offset + need->vn_aux;
- for (int cnt2 = need->vn_cnt; --cnt2 >= 0; )
+ for (unsigned int cnt2 = need->vn_cnt; cnt2 > 0; cnt2--)
{
GElf_Vernaux auxmem;
GElf_Vernaux *aux = gelf_getvernaux (data, auxoffset, );
@@ -3236,7 +3236,7 @@ handle_verdef (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr)
elf_strptr (ebl->elf, shstrndx, glink->sh_name));
unsigned int offset = 0;
- for (int cnt = shdr->sh_info; --cnt >= 0; )
+ for (unsigned int cnt = shdr->sh_info; cnt > 0; cnt--)
{
/* Get the data at the next offset. */
GElf_Verdef defmem;
@@ -3259,7 +3259,7 @@ handle_verdef (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr)
elf_strptr (ebl->elf, shdr->sh_link, aux->vda_name));
auxoffset += aux->vda_next;
- for (int cnt2 = 1; cnt2 < def->vd_cnt; ++cnt2)
+ for (unsigned int cnt2 = 1; cnt2 < def->vd_cnt; ++cnt2)
{
aux = gelf_getverdaux (data, auxoffset, );
if (unlikely (aux == NULL))
--
2.39.3
[PATCH] libebl: ebl_object_note print 32bit annobin address ranges correctly
Annobin address ranges were always printed as if they were 64bit wide
because addr_size was set to twice the size. This was done because the
note description size should contain two addresses. Fix this by setting
the address size to just one address and then check that descsz is
twice that.
* libebl/eblobjnote.c (ebl_object_note): Set addr_size to one
ELF_T_ADDR. Check descsz equals two times addr_size.
Signed-off-by: Mark Wielaard
---
libebl/eblobjnote.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libebl/eblobjnote.c b/libebl/eblobjnote.c
index 5a7c5c62..1ba5d8b3 100644
--- a/libebl/eblobjnote.c
+++ b/libebl/eblobjnote.c
@@ -155,8 +155,8 @@ ebl_object_note (Ebl *ebl, uint32_t namesz, const char
*name, uint32_t type,
} addrs;
size_t addr_size = gelf_fsize (ebl->elf, ELF_T_ADDR,
-2, EV_CURRENT);
- if (descsz != addr_size)
+1, EV_CURRENT);
+ if (descsz != addr_size * 2)
printf ("\n");
else
{
--
2.39.3
Re: [PATCH v2] dwarf_getaranges: Build aranges list from CUs instead of .debug_aranges
Hi Aaron, We already discussed on irc, but just for the record. On Mon, Feb 19, 2024 at 11:20:13PM -0500, Aaron Merey wrote: > On Tue, Feb 13, 2024 at 8:28 AM Mark Wielaard wrote: > > > > > This patch's method of building the aranges list is slower than simply > > > reading .debug_aranges. On my machine, running eu-stack on a 2.9G > > > firefox core file takes about 8.7 seconds with this patch applied, > > > compared to about 3.3 seconds without this patch. > > > > That is significant. 2.5 times slower. > > Did you check with perf or some other profiler where exactly the extra > > time goes. Does the new method find more aranges (and so produces > > "better" backtraces)? > > I took another look at the performance and realized I made a silly > mistake when I originally tested this. My build that was 2.5x slower > was compiled with -O0 but I tested it against an -O2 build. Oops! > > With the optimization level set to -O2 in all cases, the runtime of > 'eu-stack -s' on the original 2.9G firefox core file is only about > 9% slower: 3.6 seconds with the patch applied compared to 3.3 > seconds without the patch. OK, still a slowdown, but 9% is much more reasonable given we are doing more work now. Good. > As for the number of aranges found, there is a difference for libxul.so: > 250435 with the patch compared to 254832 without. So 4397 fewer aranges > are found when using the new CU iteration method. I'll dig into this and > see if there is a problem or if it's just due to some redundancy in > libxul's .debug_aranges. FWIW there was no change to the aranges counts > for the other modules searched during this eu-stack firefox corefile test. A quick way to see where the differences are is using eu-readelf --debug-dump=decodedaranges before/after your patch. This is opposite to what I expected. I had expected there to be more, instead of less ranges. The difference is less than 2%. But still interesting to know what/why. Were there any differences in the backtraces? If not, then those ranges might not actually have been mapping to code. > > Might it be an idea to leave dwarf_getaranges as it is and introduce a > > new (internal) function to get "dynamic" ranges? It looks like what > > programs (like eu-stack and eu-addr2line) really use is dwarf_addrdie > > and dwfl_module_addrdie. These are currently build on dwarf_getaranges, > > but could maybe use a new interface? > > IMO this depends on what users expect from dwarf_getaranges. Do they > want the exact contents of .debug_aranges (whether or not it's complete) > or should dwarf_getaranges go beyond .debug_aranges to ensure the most > complete results? > > The comment for dwarf_getaranges in libdw.h simply reads "Return list > address ranges". Since there's no mention of .debug_aranges specifically, > I think it's fair if dwarf_getaranges does whatever it can to ensure > comprehensive results. In which case dwarf_getaranges should probably > dynamically generate aranges. You might be right that no user really cares. But as seen in the eu-readelf code, it might also be that people expected it to map to the ranges from .debug_aranges. So I would be happier if we just kept the dwarf_getaranges code as is. And just change the code in dwarf_addrdie and dwfl_module_addrdie. We could then also introduce a new public function, dwarf_getdieranges (?) that does the new thing. But it doesn't have to be public on the first try as long as dwarf_addrdie and dwfl_module_addrdie work. (We might want to change the interface of dwarf_getdieranges so it can be "lazy" for example.) Cheers, Mark
Re: [PATCH v2 3/4] libdw: Apply DWARF package file section offsets where appropriate
Hi Omar, On Wed, 2023-12-06 at 01:22 -0800, Omar Sandoval wrote: > The final piece of DWARF package file support is that offsets have to be > interpreted relative to the section offset from the package index. > .debug_abbrev.dwo is already covered, so sprinkle around calls to > dwarf_cu_dwp_section_info for the remaining sections: .debug_line.dwo, > .debug_loclists.dwo/.debug_loc.dwo, .debug_str_offsets.dwo, > .debug_macro.dwo/.debug_macinfo.dwo, and .debug_rnglists.dwo. With all > of that in place, we can finally test various libdw functions on dwp > files. So the offsets for DW_SECT_INFO, DW_SECT_TYPES and DW_SECT_ABBREV were already taken into account when setting up a cu from a dwp. With this patch __libdw_cu_str_off_base/str_offsets_base_off handles DW_SECT_STR_OFFSETS which is used in dwarf_formstring and dwarf_getmacros. __libdw_cu_ranges_base handles DW_SECT_RNGLISTS, which is used by dwarf_ranges. And __libdw_formptr has a special case for DW_FORM_sec_offset for IDX_debug_ranges && version < 5 && unit_type == DW_UT_split_compile to also uses __libdw_cu_ranges_base. __libdw_cu_locs_base handles DW_SECT_LOCLISTS which is used in dwarf_getlocation through initial_offset. I do wonder why the special case in __libdw_formptr isn't needed here too. dwarf_getmacros handles DW_SECT_MACRO through get_offset_from. And when the macros need to refer to the line table, it also handles DW_SECT_LINE. Don't we also need to handle DW_SECT_LINE in dwarf_getsrclines and dwarf_next_lines when looking for DW_AT_stmt_list? > * libdw/dwarf_getmacros.c (get_macinfo_table): Call > dwarf_cu_dwp_section_info and add offset to line_offset. > (get_offset_from): Call dwarf_cu_dwp_section_info and add offset > to *retp. > * libdw/libdwP.h (str_offsets_base_off): Call > dwarf_cu_dwp_section_info and add offset. > (__libdw_cu_ranges_base): Ditto. > (__libdw_cu_locs_base): Ditto. > * libdw/dwarf_getlocation.c (initial_offset): Call > dwarf_cu_dwp_section_info and add offset to start_offset. > * tests/run-varlocs.sh: Check testfile-dwp-5 and testfile-dwp-4. > * tests/run-all-dwarf-ranges.sh: Check testfile-dwp-5 and > testfile-dwp-4. > * tests/run-dwarf-getmacros.sh: Check testfile-dwp-5 and > testfile-dwp-4-strict. > * tests/run-get-units-split.sh: Check testfile-dwp-5, > testfile-dwp-4, and testfile-dwp-4-strict. The code and tests look good. run-varlocs.sh seems good, which seems to confirm DW_SECT_LOCLISTS is handled correctly (but why doesn't it need a hack similar to ranges in __libdw_formptr?). We might want to add a test for run-next-lines.sh and run-next- files.sh? Thanks, Mark
[PATCH] libdw: Update dwarf_cu_dwp_section_info documentation
Update the documentation of dwarf_cu_dwp_section_info to make clear that the function only returns an error if the DWARF package file data couldn't be read or an unknown section constant is provided. Missing DWP information for a given CU isn't an error and will set both OFFSET and SIZE to zero. It also makes sure the documentation is < 76 chars wide. * libdw/libdw.h (dwarf_cu_dwp_section_info): Update docs. Signed-off-by: Mark Wielaard --- libdw/libdw.h | 42 +++--- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/libdw/libdw.h b/libdw/libdw.h index 545ad043..d53dc787 100644 --- a/libdw/libdw.h +++ b/libdw/libdw.h @@ -1081,25 +1081,29 @@ extern int dwarf_frame_register (Dwarf_Frame *frame, int regno, __nonnull_attribute__ (3, 4, 5); -/* Return offset and/or size of CU's contribution to SECTION in a DWARF package - file. - - If CU is not from a DWARF package file, the file does not have SECTION, or CU - does not contribute to SECTION, then *SIZEP is set to 0. - - SECTION is a DW_SECT section identifier. Note that the original GNU DWARF - package file extension for DWARF 4 used slightly different section - identifiers. This function uses the standardized section identifiers and - maps the GNU DWARF 4 identifiers to their standard DWARF 5 analogues: - DW_SECT_LOCLISTS (5) refers to .debug_locs.dwo for DWARF 4. - DW_SECT_MACRO (7) refers to .debug_macinfo.dwo for DWARF 4 or - .debug_macro.dwo for the GNU .debug_macro extension for DWARF 4 (section - identifier 8 is DW_SECT_RNGLISTS in DWARF 5, NOT DW_SECT_MACRO like in the - GNU extension.) - .debug_types.dwo does not have a DWARF 5 equivalent, so this function accepts - the original DW_SECT_TYPES (2). - - Returns 0 for success or -1 for errors. OFFSETP and SIZEP may be NULL. */ +/* Return offset and/or size of CU's contribution to SECTION in a + DWARF package file. + + If CU is not from a DWARF package file, the file does not have + SECTION, or CU does not contribute to SECTION, then *OFFSETP and + *SIZEP are set to 0 (this is not an error and the function will + return 0 in that case). + + SECTION is a DW_SECT section identifier. Note that the original + GNU DWARF package file extension for DWARF 4 used slightly + different section identifiers. This function uses the standardized + section identifiers and maps the GNU DWARF 4 identifiers to their + standard DWARF 5 analogues: DW_SECT_LOCLISTS (5) refers to + .debug_locs.dwo for DWARF 4. DW_SECT_MACRO (7) refers to + .debug_macinfo.dwo for DWARF 4 or .debug_macro.dwo for the GNU + .debug_macro extension for DWARF 4 (section identifier 8 is + DW_SECT_RNGLISTS in DWARF 5, NOT DW_SECT_MACRO like in the GNU + extension.) .debug_types.dwo does not have a DWARF 5 equivalent, + so this function accepts the original DW_SECT_TYPES (2). + + Returns 0 for success or -1 for errors reading the DWARF package + file data or if an unknown SECTION constant is given. OFFSETP and + SIZEP may be NULL. */ extern int dwarf_cu_dwp_section_info (Dwarf_CU *cu, unsigned int section, Dwarf_Off *offsetp, Dwarf_Off *sizep); -- 2.43.2
Re: [PATCH v2 2/4] libdw: Try .dwp file in __libdw_find_split_unit()
Hi Omar,
On Wed, Dec 06, 2023 at 01:22:17AM -0800, Omar Sandoval wrote:
> Try opening the file in the location suggested by the standard (the
> skeleton file name + ".dwp") and looking up the unit in the package
> index. The rest is similar to .dwo files, with slightly different
> cleanup since a single Dwarf handle is shared.
This is indeed an heuristic for finding the dwp file that should work.
I do wonder if this is how distros will do it for separate .debug
files. I can imagine combining separate debuginfo files with dwz
and/or placing the .dwo and index sections into the actual .debug file
itself. But we can play with that later.
The code seems to integrate really nicely with single file split .dwo.
>
> * libdw/libdw_find_split_unit.c (try_dwp_file): New function.
> (__libdw_find_split_unit): Call try_dwp_file.
> * libdw/libdwP.h (Dwarf): Add dwp_dwarf and dwp_fd.
> (__libdw_dwp_findcu_id): New declaration.
> (__libdw_link_skel_split): Handle .debug_addr for dwp.
> * libdw/libdw_begin_elf.c (dwarf_begin_elf): Initialize
> result->dwp_fd.
> * libdw/dwarf_end.c (dwarf_end): Free dwarf->dwp_dwarf and close
> dwarf->dwp_fd.
> (cu_free): Don't free split dbg if it is dwp_dwarf.
>
> Signed-off-by: Omar Sandoval
> ---
> libdw/dwarf_begin_elf.c | 1 +
> libdw/dwarf_cu_dwp_section_info.c | 19
> libdw/dwarf_end.c | 10 -
> libdw/libdwP.h| 23 --
> libdw/libdw_find_split_unit.c | 75 ---
> 5 files changed, 119 insertions(+), 9 deletions(-)
>
> diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
> index 323a91d0..ca2b7e2a 100644
> --- a/libdw/dwarf_begin_elf.c
> +++ b/libdw/dwarf_begin_elf.c
> @@ -567,6 +567,7 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd, Elf_Scn *scngrp)
>
>result->elf = elf;
>result->alt_fd = -1;
> + result->dwp_fd = -1;
>
>/* Initialize the memory handling. Initial blocks are allocated on first
> actual allocation. */
OK.
> diff --git a/libdw/dwarf_cu_dwp_section_info.c
> b/libdw/dwarf_cu_dwp_section_info.c
> index 4a4eac8c..298f36f9 100644
> --- a/libdw/dwarf_cu_dwp_section_info.c
> +++ b/libdw/dwarf_cu_dwp_section_info.c
> @@ -340,6 +340,25 @@ __libdw_dwp_find_unit (Dwarf *dbg, bool debug_types,
> Dwarf_Off off,
> abbrev_offsetp, NULL);
> }
>
> +Dwarf_CU *
> +internal_function
> +__libdw_dwp_findcu_id (Dwarf *dbg, uint64_t unit_id8)
> +{
> + Dwarf_Package_Index *index = __libdw_package_index (dbg, false);
> + uint32_t unit_row;
> + Dwarf_Off offset;
> + Dwarf_CU *cu;
> + if (__libdw_dwp_unit_row (index, unit_id8, _row) == 0
> + && __libdw_dwp_section_info (index, unit_row, DW_SECT_INFO, ,
> +NULL) == 0
> + && (cu = __libdw_findcu (dbg, offset, false)) != NULL
> + && cu->unit_type == DW_UT_split_compile
> + && cu->unit_id8 == unit_id8)
> +return cu;
> + else
> +return NULL;
> +}
> +
Called from try_dwp_file to find the to split compile unit through the
index. When found try_dwp_file will put this cu into the split_tree.
OK.
> int
> dwarf_cu_dwp_section_info (Dwarf_CU *cu, unsigned int section,
> Dwarf_Off *offsetp, Dwarf_Off *sizep)
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index b7f817d9..78224ddb 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -66,7 +66,9 @@ cu_free (void *arg)
> /* The fake_addr_cu might be shared, only release one. */
> if (p->dbg->fake_addr_cu == p->split->dbg->fake_addr_cu)
> p->split->dbg->fake_addr_cu = NULL;
> - INTUSE(dwarf_end) (p->split->dbg);
> + /* There is only one DWP file. We free it later. */
> + if (p->split->dbg != p->dbg->dwp_dwarf)
> + INTUSE(dwarf_end) (p->split->dbg);
> }
> }
> }
> @@ -147,6 +149,12 @@ dwarf_end (Dwarf *dwarf)
> close (dwarf->alt_fd);
> }
>
> + if (dwarf->dwp_fd != -1)
> + {
> + INTUSE(dwarf_end) (dwarf->dwp_dwarf);
> + close (dwarf->dwp_fd);
> + }
> +
>/* The cached path and dir we found the Dwarf ELF file in. */
>free (dwarf->elfpath);
>free (dwarf->debugdir);
OK.
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index 7f8d69b5..54445886 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -180,6 +180,9 @@ struct Dwarf
>/* dwz alternate DWARF file. */
>Dwarf *alt_dwarf;
>
> + /* DWARF package file. */
> + Dwarf *dwp_dwarf;
> +
>/* The section data. */
>Elf_Data *sectiondata[IDX_last];
>
> @@ -197,6 +200,9 @@ struct Dwarf
> close this file descriptor. */
>int alt_fd;
>
> + /* File descriptor of DWARF package file. */
> + int dwp_fd;
> +
>/* Information for traversing the .debug_pubnames section. This is
> an array and separately allocated with malloc. */
>struct pubnames_s
OK.
Re: [PATCH v2 1/4] libdw: Parse DWARF package file index sections
Hi Omar, On Wed, 2023-12-06 at 01:22 -0800, Omar Sandoval wrote: > The .debug_cu_index and .debug_tu_index sections in DWARF package files > are basically hash tables mapping a unit's 8 byte signature to an offset > and size in each section used by that unit [1]. Add support for parsing > and doing lookups in the index sections. > > We look up a unit in the index when we intern it and cache its hash > table row in Dwarf_CU. Then, a new function, dwarf_cu_dwp_section_info, > can be used to look up the section offsets and sizes for a unit. This > will mostly be used internally in libdw, but it will also be needed in > static inline functions shared with eu-readelf. Additionally, making it > public it makes dwp support much easier for external tools that do their > own low-level parsing of DWARF information, like drgn [2]. You convinced me that dwarf_cu_dwp_section_info should be a public function. And speaking of eu-readelf, we should also make it parse the .debug_tu_index and .debug_cu_index sections. But that is for another time. Pushed with one merge conflict resolved. Please look over the review comments below to see if I interpreted anything wrongly. > 1: > https://gcc.gnu.org/wiki/DebugFissionDWP#Format_of_the_CU_and_TU_Index_Sections > 2: https://github.com/osandov/drgn > > * libdw/dwarf.h: Add DW_SECT_TYPES. > * libdw/libdwP.h (Dwarf): Add cu_index and tu_index. > (Dwarf_CU): Add dwp_row. > (Dwarf_Package_Index): New type. > (__libdw_dwp_find_unit): New declaration. > (dwarf_cu_dwp_section_info): New INTDECL. > Add DWARF_E_UNKNOWN_SECTION. > * libdw/Makefile.am (libdw_a_SOURCES): Add > dwarf_cu_dwp_section_info.c. > * libdw/dwarf_end.c (dwarf_end): Free dwarf->cu_index and > dwarf->tu_index. > * libdw/dwarf_error.c (errmsgs): Add DWARF_E_UNKNOWN_SECTION. > * libdw/libdw.h (dwarf_cu_dwp_section_info): New declaration. > * libdw/libdw.map (ELFUTILS_0.190): Add > dwarf_cu_dwp_section_info. > * libdw/libdw_findcu.c (__libdw_intern_next_unit): Call > __libdw_dwp_find_unit, and use it to adjust abbrev_offset and > assign newp->dwp_row. > * libdw/dwarf_cu_dwp_section_info.c: New file. > * tests/Makefile.am (check_PROGRAMS): Add cu-dwp-section-info. > (TESTS): Add run-cu-dwp-section-info.sh > (EXTRA_DIST): Add run-cu-dwp-section-info.sh and new test files. > (cu_dwp_section_info_LDADD): New variable. > * tests/cu-dwp-section-info.c: New test. > * tests/run-cu-dwp-section-info.sh: New test. > * tests/testfile-dwp-4-strict.bz2: New test file. > * tests/testfile-dwp-4-strict.dwp.bz2: New test file. > * tests/testfile-dwp-4.bz2: New test file. > * tests/testfile-dwp-4.dwp.bz2: New test file. > * tests/testfile-dwp-5.bz2: New test file. > * tests/testfile-dwp-5.dwp.bz2: New test file. > * tests/testfile-dwp.source: New file. > > Signed-off-by: Omar Sandoval > --- > libdw/Makefile.am | 2 +- > libdw/dwarf.h | 2 +- > libdw/dwarf_cu_dwp_section_info.c | 371 > libdw/dwarf_end.c | 3 + > libdw/dwarf_error.c | 1 + > libdw/libdw.h | 23 ++ > libdw/libdw.map | 5 + > libdw/libdwP.h | 33 +++ > libdw/libdw_findcu.c| 8 + > tests/.gitignore| 1 + > tests/Makefile.am | 11 +- > tests/cu-dwp-section-info.c | 73 ++ > tests/run-cu-dwp-section-info.sh| 168 + > tests/testfile-dwp-4-strict.bz2 | Bin 0 -> 4169 bytes > tests/testfile-dwp-4-strict.dwp.bz2 | Bin 0 -> 6871 bytes > tests/testfile-dwp-4.bz2| Bin 0 -> 4194 bytes > tests/testfile-dwp-4.dwp.bz2| Bin 0 -> 10098 bytes > tests/testfile-dwp-5.bz2| Bin 0 -> 4223 bytes > tests/testfile-dwp-5.dwp.bz2| Bin 0 -> 10313 bytes > tests/testfile-dwp.source | 102 > 20 files changed, 798 insertions(+), 5 deletions(-) > create mode 100644 libdw/dwarf_cu_dwp_section_info.c > create mode 100644 tests/cu-dwp-section-info.c > create mode 100755 tests/run-cu-dwp-section-info.sh > create mode 100755 tests/testfile-dwp-4-strict.bz2 > create mode 100644 tests/testfile-dwp-4-strict.dwp.bz2 > create mode 100755 tests/testfile-dwp-4.bz2 > create mode 100644 tests/testfile-dwp-4.dwp.bz2 > create mode 100755 tests/testfile-dwp-5.bz2 > create mode 100644 tests/testfile-dwp-5.dwp.bz2 > create mode 100644 tests/testfile-dwp.source > > diff --git a/libdw/Makefile.am b/libdw/Makefile.am > index e548f38c..5363c02a 100644 > --- a/libdw/Makefile.am > +++ b/libdw/Makefile.am > @@ -93,7 +93,7 @@ libdw_a_SOURCES = dwarf_begin.c dwarf_begin_elf.c > dwarf_end.c dwarf_getelf.c \ > dwarf_cu_die.c dwarf_peel_type.c dwarf_default_lower_bound.c \ >
Re: [PATCH v2] dwarf_getaranges: Build aranges list from CUs instead of .debug_aranges
Hi Aaron,
On Mon, 2023-12-11 at 18:18 -0500, Aaron Merey wrote:
> No longer use .debug_aranges to build the aranges list since it could be
> absent or incomplete.
>
> Instead build the aranges list by iterating over each CU and recording
> each address range.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=22288
> https://sourceware.org/bugzilla/show_bug.cgi?id=30948
>
> Signed-off-by: Aaron Merey
> ---
>
> v2 adds a test for generating aranges from a binary with no
> .debug_aranges.
>
> This patch's method of building the aranges list is slower than simply
> reading .debug_aranges. On my machine, running eu-stack on a 2.9G
> firefox core file takes about 8.7 seconds with this patch applied,
> compared to about 3.3 seconds without this patch.
That is significant. 2.5 times slower.
Did you check with perf or some other profiler where exactly the extra
time goes. Does the new method find more aranges (and so produces
"better" backtraces)?
> Ideally we could assume that .debug_aranges is complete if it is present
> and build the aranges list via CU iteration only when .debug_aranges
> is absent. This would let us save time on gcc-compiled binaries, which
> include complete .debug_aranges by default.
Right. This why the question is if the firefox case sees more/less
aranges. If I remember correctly it is build with gcc and rustc, and
rustc might not produce .debug_aranges.
> However the DWARF spec appears to permit partially complete
> .debug_aranges [1]. We could improve performance by starting with a
> potentially incomplete list built from .debug_aranges. If a lookup
> fails then search the CUs for missing aranges and add to the list
> when found.
>
> This approach would complicate the dwarf_get_aranges interface. The
> list it initially provides could no longer be assumed to be complete.
> The number of elements in the list could change during calls to
> dwarf_getarange{info, _addr}. This would invalidate the naranges value
> set by dwarf_getaranges. The current API doesn't include a way to
> communicate to the caller when narages changes and by how much.
>
> Due to these complications I think it's better to simply ignore
> .debug_aranges altogether and build the aranges table via CU iteration,
> as is done in this patch.
Might it be an idea to leave dwarf_getaranges as it is and introduce a
new (internal) function to get "dynamic" ranges? It looks like what
programs (like eu-stack and eu-addr2line) really use is dwarf_addrdie
and dwfl_module_addrdie. These are currently build on dwarf_getaranges,
but could maybe use a new interface?
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=22288#c5
So this comment says that "parsing CUs lightly (just enough to get
their CU ranges) should be fairly cheap", but it seems that isn't
really true. Or at least parsing .debug_aranges is a lot (2.5 times)
faster (measured in seconds).
It would be good to better understand why this is.
Cheers,
Mark
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 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 \
>
Re: [PATCH v2 1/5] strip: Adapt src/strip -o -f on mips
Hi Ying, Sorry I keep postponing this. I don't have access to a mips64le box, the cfarm only has 64bit big endian mips machines. But the part I am struggling with is the relocation data conversion needed in the mips64le case. On Fri, Nov 03, 2023 at 01:18:12PM +0100, Mark Wielaard wrote: > On Thu, 2023-11-02 at 14:55 +0800, Ying Huang wrote: > > In mips64 little-endian, r_info consists of four byte fields(contains > > three reloc types) and a 32-bit symbol index. In order to adapt > > GELF_R_SYM and GELF_R_TYPE, need convert raw data to get correct symbol > > index and type. > > This part and the new backends hooks look OK. So to make progress could you split this part? Just a patch that adds the initial mips backend (and the libebl and libelfP.h parts). And another that introduces the libelf/elf_update and elf_getdata parts? Also could you take a look at CONTRIBUTING https://sourceware.org/cgit/elfutils/tree/CONTRIBUTING And provide a Signed-off-by line if you can/agree with that? Which MIPS variant(s) have you tested this against? Is it supposed to only work for mips64le? Or also maps64[be] and/or mips32 bits? Thanks, Mark
Re: [PATCH] unstrip: Call adjust_relocs no more than once per section.
Hi Aaron,
On Mon, 2024-02-05 at 18:11 -0500, Aaron Merey wrote:
> During symtab merging, adjust_relocs might be called multiple times on
> some SHT_REL/SHT_RELA sections. In these cases it is possible for a
> relocation's symbol index to be correctly mapped from X to Y during the
> first call to adjust_relocs but then wrongly remapped from Y to Z during
> the second call.
>
> Fix this by adjusting relocation symbol indices just once per section.
>
> Also add stable sorting for symbols during symtab merging so that the
> symbol order in the output file's symtab does not depend on undefined
> behaviour in qsort.
>
> Note that adjust_relocs still might be called a second time on a section
> during add_new_section_symbols. However since add_new_section_symbols
> generates its own distinct symbol index map, this should not trigger the
> bug described above.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=31097
>
> Signed-off-by: Aaron Merey
> ---
>
> I added some tests to run-unstrip-test.sh to verify that strip+unstrip
> does not alter the symbols referred to by relocations. I tried to use
> eu-elfcmp for this purpose. However for a pair of i386 ET_REL binaries
> I did some testing with, eu-elfcmp skips checking the SHT_REL/SHT_RELA
> sections because the sh_flags do not contain SHF_ALLOC (the flags only
> contain SHF_INFO_LINK in this case).
>
> I'm not sure if this eu-elfcmp behaviour is intentional or if eu-elfcmp
> should start comparing REL/RELA sections even if they aren't allocated.
So it looks like elfcmp explicitly checks ebl_section_strip_p and
doesn't compare sections that are strippable. Maybe we should add an
eu-elfcmp --all-sections flag?
We should probably also check that it handles the new SHT_RELR sections
correctly. I see it only checks for SHT_REL and SHT_RELA in the code.
Although RELR is really simple and doesn't have symbol references. So
it is probably fine.
> src/unstrip.c| 81
> tests/.gitignore | 1 +
> tests/Makefile.am| 3 +-
> tests/elf-print-reloc-syms.c | 144 +++
> tests/run-unstrip-test.sh| 8 ++
> 5 files changed, 221 insertions(+), 16 deletions(-)
> create mode 100644 tests/elf-print-reloc-syms.c
>
> diff --git a/src/unstrip.c b/src/unstrip.c
> index d5bd1821..f37d6c58 100644
> --- a/src/unstrip.c
> +++ b/src/unstrip.c
> @@ -598,21 +598,30 @@ adjust_relocs (Elf_Scn *outscn, Elf_Scn *inscn, const
> GElf_Shdr *shdr,
> /* Adjust all the relocation sections in the file. */
> static void
> adjust_all_relocs (Elf *elf, Elf_Scn *symtab, const GElf_Shdr *symshdr,
> -size_t map[], size_t map_size)
> +size_t map[], size_t map_size, bool *scn_filter)
> {
Maybe bool scn_filter[] since it really is an array?
>size_t new_sh_link = elf_ndxscn (symtab);
>Elf_Scn *scn = NULL;
>while ((scn = elf_nextscn (elf, scn)) != NULL)
> if (scn != symtab)
>{
> + if (scn_filter != NULL)
> + {
> + size_t ndx = elf_ndxscn (scn);
> +
> + /* Skip relocations that were already mapped during adjust_relocs
> +for the stripped symtab. This is to avoid mapping a relocation's
> +symbol index from X to Y during the first adjust_relocs and then
> +wrongly remapping it from Y to Z during the second call. */
> + if (scn_filter[ndx])
> + continue;
> + }
> +
> GElf_Shdr shdr_mem;
> GElf_Shdr *shdr = gelf_getshdr (scn, _mem);
> ELF_CHECK (shdr != NULL, _("cannot get section header: %s"));
> - /* Don't redo SHT_GROUP, groups are in both the stripped and debug,
> -it will already have been done by adjust_relocs for the
> -stripped_symtab. */
> - if (shdr->sh_type != SHT_NOBITS && shdr->sh_type != SHT_GROUP
> - && shdr->sh_link == new_sh_link)
> +
> + if (shdr->sh_type != SHT_NOBITS && shdr->sh_link == new_sh_link)
> adjust_relocs (scn, scn, shdr, map, map_size, symshdr);
>}
> }
OK.
> @@ -697,7 +706,7 @@ add_new_section_symbols (Elf_Scn *old_symscn, size_t
> old_shnum,
> }
>
>/* Adjust any relocations referring to the old symbol table. */
> - adjust_all_relocs (elf, symscn, shdr, symndx_map, nsym - 1);
> + adjust_all_relocs (elf, symscn, shdr, symndx_map, nsym - 1, NULL);
>
>return symdata;
> }
OK.
> @@ -874,6 +883,7 @@ collect_symbols (Elf *outelf, bool rel, Elf_Scn *symscn,
> Elf_Scn *strscn,
>s->shndx = shndx;
>s->info.info = sym->st_info;
>s->info.other = sym->st_other;
> + s->duplicate = NULL;
>
>if (scnmap != NULL && shndx != SHN_UNDEF && shndx < SHN_LORESERVE)
> s->shndx = scnmap[shndx - 1];
> @@ -903,8 +913,7 @@ collect_symbols (Elf *outelf, bool rel, Elf_Scn *symscn,
> Elf_Scn *strscn,
>if (s1->value > s2->value) \
> return 1
>
> -/* Compare
Re: [PATCH] PR 30991: srcfiles tarball feature
Hi,
On Mon, Feb 05, 2024 at 07:24:33PM -0500, Aaron Merey wrote:
> > diff --git a/NEWS b/NEWS
> > index 0420d3b8..3391d6a1 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,3 +1,8 @@
> > +Version 0.191 (after 0.189)
> > +
> > +srcfiles: Can now fetch the source files of a DWARF/ELF file and
> > + place them into a zip.
> > +
> > Version 0.190 "Woke!"
> >
> > CONTRIBUTING: Switch from real name policy to known identity policy.
> > @@ -9,6 +14,9 @@ libelf: Add RELR support.
> >
> > libdw: Recognize .debug_[ct]u_index sections
> >
> > +srcfiles: added srcfiles tool that lists all the source files of a given
> > + DWARF/ELF file.
> > +
> > readelf: Support readelf -Ds, --use-dynamic --symbol.
> > Support .gdb_index version 9
> >
This looks like a merge issue after the 0.190 release. I updated the
version at the top and removed the duplicate entry under 0.190.
> > diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> > index 524be948..6b21f46f 100644
> > --- a/debuginfod/debuginfod.cxx
> > +++ b/debuginfod/debuginfod.cxx
> > @@ -2996,8 +2996,6 @@ dwarf_extract_source_paths (Elf *elf, set&
> > debug_sourcefiles)
> >
> >if (comp_dir[0] == '\0' && cuname[0] != '/')
> > {
> > - // This is a common symptom for dwz-compressed debug files,
> > - // where the altdebug file cannot be resolved.
> >if (verbose > 3)
> > obatched(clog) << "skipping cu=" << cuname << " due to empty
> > comp_dir" << endl;
> >continue;
Was it intended to remove this comment?
Cheers,
Mark>From 71794afb54fd9668a02a361134f15180ce85f3ad Mon Sep 17 00:00:00 2001
From: Mark Wielaard
Date: Tue, 6 Feb 2024 12:53:54 +0100
Subject: [PATCH] NEWS: Update version number and remove duplicate 0.190 entry
---
NEWS | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/NEWS b/NEWS
index e2393e77..98dc78f5 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,4 @@
-Version 0.191 (after 0.189)
+Version 0.191 (after 0.190)
srcfiles: Can now fetch the source files of a DWARF/ELF file and
place them into a zip.
@@ -14,9 +14,6 @@ libelf: Add RELR support.
libdw: Recognize .debug_[ct]u_index sections
-srcfiles: added srcfiles tool that lists all the source files of a given
- DWARF/ELF file.
-
readelf: Support readelf -Ds, --use-dynamic --symbol.
Support .gdb_index version 9
--
2.39.3
[COMMITTED] srcfiles: Fix --enable-gcov (BUILD_STATIC) build
When configuring with --enable-gcov we build most things static. Including libdebuginfod. The src Makefile was only setup for a shared library build of libdebuginfod.so. Fix this by providing a static libdebuginfod in case of BUILD_STATIC. This fixes the builder.sourceware.org elfutils-snapshots-coverage and provides fresh coverage reports again at https://snapshots.sourceware.org/elfutils/coverage/latest/ * Makefile.am (BUILD_STATIC): Provide libdebuginfod.a Signed-off-by: Mark Wielaard --- src/Makefile.am | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 28fab5f5..1d592d4d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -43,18 +43,23 @@ if BUILD_STATIC libasm = ../libasm/libasm.a libdw = ../libdw/libdw.a -lz $(zip_LIBS) $(libelf) -ldl -lpthread libelf = ../libelf/libelf.a -lz $(zstd_LIBS) +if LIBDEBUGINFOD +libdebuginfod = ../debuginfod/libdebuginfod.a -lpthread $(libcurl_LIBS) +else +libdebuginfod = +endif else libasm = ../libasm/libasm.so libdw = ../libdw/libdw.so libelf = ../libelf/libelf.so -endif -libebl = ../libebl/libebl.a ../backends/libebl_backends.a ../libcpu/libcpu.a -libeu = ../lib/libeu.a if LIBDEBUGINFOD libdebuginfod = ../debuginfod/libdebuginfod.so else libdebuginfod = endif +endif +libebl = ../libebl/libebl.a ../backends/libebl_backends.a ../libcpu/libcpu.a +libeu = ../lib/libeu.a if DEMANGLE demanglelib = -lstdc++ -- 2.39.3
[PATCH] libelf: Treat elf_memory as if using ELF_C_READ_MMAP
An Elf handle created through elf_memory was treated as if opened with
ELF_C_READ. Which means libelf believed it had read the memory itself
and could simply write to it if it wanted (because it wasn't mmaped
directly on top of a file). This causes issues when that memory was
actually read-only. Work around this by pretending the memory was
actually read with ELF_C_READ_MMAP (so directly readable, but not
writable).
Add extra tests to elfgetzdata to check using elf_memory with
read-only memory works as expected.
* libelf/elf_memory.c (elf_memory): Call
__libelf_read_mmaped_file with ELF_C_READ_MMAP.
* tests/elfgetzdata.c (main): Add new "mem" option.
* tests/run-elfgetzdata.sh: Also run all tests with new
"mem" option.
https://sourceware.org/bugzilla/show_bug.cgi?id=31225
Reported-by: Derek Bruening
Signed-off-by: Mark Wielaard
---
libelf/elf_memory.c | 2 +-
tests/elfgetzdata.c | 70 +++---
tests/run-elfgetzdata.sh | 92
3 files changed, 158 insertions(+), 6 deletions(-)
diff --git a/libelf/elf_memory.c b/libelf/elf_memory.c
index a47f1d24..13d77cb7 100644
--- a/libelf/elf_memory.c
+++ b/libelf/elf_memory.c
@@ -46,5 +46,5 @@ elf_memory (char *image, size_t size)
return NULL;
}
- return __libelf_read_mmaped_file (-1, image, 0, size, ELF_C_READ, NULL);
+ return __libelf_read_mmaped_file (-1, image, 0, size, ELF_C_READ_MMAP, NULL);
}
diff --git a/tests/elfgetzdata.c b/tests/elfgetzdata.c
index 82afbe52..0af6c223 100644
--- a/tests/elfgetzdata.c
+++ b/tests/elfgetzdata.c
@@ -1,4 +1,5 @@
/* Copyright (C) 2015 Red Hat, Inc.
+ Copyright (C) 2024 Mark J. Wielaard
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -18,16 +19,19 @@
# include
#endif
+#include
#include
#include
#include
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
int
@@ -38,21 +42,62 @@ main (int argc, char *argv[])
if (argc < 3
|| (strcmp (argv[1], "read") != 0
- && strcmp (argv[1], "mmap") != 0))
+ && strcmp (argv[1], "mmap") != 0
+ && strcmp (argv[1], "mem") != 0))
{
- printf ("Usage: (read|mmap) files...\n");
+ printf ("Usage: (read|mmap|mem) files...\n");
return -1;
}
- bool mmap = strcmp (argv[1], "mmap") == 0;
+ bool do_read = strcmp (argv[1], "read") == 0;
+ bool do_mmap = strcmp (argv[1], "mmap") == 0;
+ bool do_mem = strcmp (argv[1], "mem") == 0;
elf_version (EV_CURRENT);
for (cnt = 2; cnt < argc; ++cnt)
{
int fd = open (argv[cnt], O_RDONLY);
-
- Elf *elf = elf_begin (fd, mmap ? ELF_C_READ_MMAP : ELF_C_READ, NULL);
+ void *map_address = NULL;
+ size_t map_size = 0;
+
+ Elf *elf;
+ if (do_read)
+ elf = elf_begin (fd, ELF_C_READ, NULL);
+ else if (do_mmap)
+ elf = elf_begin (fd, ELF_C_READ_MMAP, NULL);
+ else
+{
+ assert (do_mem);
+ // We mmap the memory ourselves, explicitly PROT_READ only
+ struct stat st;
+ if (fstat (fd, ) != 0)
+ {
+ printf ("%s cannot stat %s\n", argv[cnt], strerror (errno));
+ result = 1;
+ close (fd);
+ continue;
+ }
+ map_size = st.st_size;
+ map_address = mmap (NULL, map_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (map_address == MAP_FAILED)
+ {
+ printf ("%s cannot mmap %s\n", argv[cnt], strerror (errno));
+ result = 1;
+ close (fd);
+ continue;
+ }
+ if (map_size < EI_NIDENT
+ || memcmp (map_address, ELFMAG, SELFMAG) != 0)
+ {
+ printf ("%s isn't an ELF file\n", argv[cnt]);
+ result = 1;
+ munmap (map_address, map_size);
+ close (fd);
+ continue;
+ }
+ elf = elf_memory (map_address, map_size);
+}
if (elf == NULL)
{
printf ("%s not usable %s\n", argv[cnt], elf_errmsg (-1));
@@ -107,6 +152,21 @@ main (int argc, char *argv[])
elf_end (elf);
close (fd);
+
+ if (do_mem)
+{
+ /* Make sure we can still get at the memory. */
+ if (memcmp (map_address, ELFMAG, SELFMAG) != 0)
+ {
+ printf ("%s isn't an ELF file anymore???\n", argv[cnt]);
+ result = 1;
+ }
+ if (munmap (map_address, map_size) != 0)
+ {
+ printf ("%s cannot munmap %s\n", argv[cnt], strerror (errno));
+ result = 1;
+ }
+}
}
return result;
Re: [PATCH] libdwfl: Add some extra space to buffer to read kernel image header
Hi, On Sun, 2024-01-21 at 20:54 +0100, Mark Wielaard wrote: > GCC 14 notices we play some tricks with the array into which we try > to read the kernel image header. > > image-header.c: In function ‘__libdw_image_header’: > image-header.c:77:18: error: array subscript -496 is outside array bounds of > ‘char[96]’ [-Werror=array-bounds=] >77 | header = header_buffer - H_START; > | ^ > image-header.c:67:12: note: at offset -496 into object ‘header_buffer’ of > size 96 >67 | char header_buffer[H_READ_SIZE]; > |^ > > GCC is correct. The new header pointer is before the actually buffer we > want to read from. Later in the code we "correct" the address again by > adding the "offset" off the elements we want to read. Such pointer > arithmetic is technically invalid. Make it valid by making the buffer > a little bigger, so all pointer arithmetic stays inside the header_buffer. > This does waste 496 bytes on the stack at the front of the buffer that > is never used. > > * libdwfl/image-header.c (__libdw_image_header): Add H_START > to header_buffer size and return Pushed after briefly discussing with Aaron on irc. Cheers, Mark
[PATCH] libdwfl: Add some extra space to buffer to read kernel image header
GCC 14 notices we play some tricks with the array into which we try
to read the kernel image header.
image-header.c: In function ‘__libdw_image_header’:
image-header.c:77:18: error: array subscript -496 is outside array bounds of
‘char[96]’ [-Werror=array-bounds=]
77 | header = header_buffer - H_START;
| ^
image-header.c:67:12: note: at offset -496 into object ‘header_buffer’ of size
96
67 | char header_buffer[H_READ_SIZE];
|^
GCC is correct. The new header pointer is before the actually buffer we
want to read from. Later in the code we "correct" the address again by
adding the "offset" off the elements we want to read. Such pointer
arithmetic is technically invalid. Make it valid by making the buffer
a little bigger, so all pointer arithmetic stays inside the header_buffer.
This does waste 496 bytes on the stack at the front of the buffer that
is never used.
* libdwfl/image-header.c (__libdw_image_header): Add H_START
to header_buffer size and return
Signed-off-by: Mark Wielaard
---
libdwfl/image-header.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/libdwfl/image-header.c b/libdwfl/image-header.c
index c777cc84..03cd7a76 100644
--- a/libdwfl/image-header.c
+++ b/libdwfl/image-header.c
@@ -1,6 +1,6 @@
/* Linux kernel image support for libdwfl.
Copyright (C) 2009-2011 Red Hat, Inc.
- Copyright (C) 2022 Mark J. Wielaard
+ Copyright (C) 2022, 2024 Mark J. Wielaard
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -64,17 +64,17 @@ __libdw_image_header (int fd, off_t *start_offset,
if (likely (mapped_size > H_END))
{
const void *header = mapped;
- char header_buffer[H_READ_SIZE];
+ char header_buffer[H_READ_SIZE + H_START];
if (header == NULL)
{
- ssize_t n = pread_retry (fd, header_buffer, H_READ_SIZE,
+ ssize_t n = pread_retry (fd, header_buffer + H_START, H_READ_SIZE,
*start_offset + H_START);
if (n < 0)
return DWFL_E_ERRNO;
if (n < H_READ_SIZE)
return DWFL_E_BADELF;
- header = header_buffer - H_START;
+ header = header_buffer;
}
uint16_t magic1;
--
2.39.3
Re: Porting pahole from dwarf_next_unit() to dwarf_get_units()
Hi Dimitri, Sorry, this arrived before my vacation and then the new year happened. On Tue, Dec 05, 2023 at 01:03:01PM +, Dimitri John Ledkov wrote: > Currently pahole warns and does nothing upon hitting > DW_TAG_skeleton_unit as implemented at > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?id=0135ccd632796ab3aff65b7c99b374c4682c2bcf > > In elfutils, a while back a new API got added that aids with discovery > and processing of such tags - > https://sourceware.org/git/?p=elfutils.git;a=commitdiff;h=79f0e623dcde4b042bb72f636a2211d67d5c0ade > > It seems to me if pahole is ported from using dwarf_next_unit() to > instead use dwarf_get_units() native support can be added for > split-dwarf (dwo) files. > > I am trying to write such a port, but it is proving to be very > difficult. I am entirely unfamiliar with neither pahole nor libdw nor > the dwarf file format. Thus it is very confusing when both pahole and > dwarf library use very similar type names and structs. For example > libdw has struct Dwarf_CU and pahole has unrelated dwarf_cu struct. > > What are the differences between dwarf_nextcu(), dwarf_next_unit(), > dwarf_get_units() and when should one use each one of them? (or nest > them?) The dwarf_nextcu was the original way to iterate over the CUs from .debug_info. Then dwarf_next_unit was added when type units could come from a .debug_types section. Both functions use and return offsets to iterate through the section and then get the CU DIE using dwarf_offdie (or dwarf_offdie_types). This requires the user to know beforehand where to DIE data is stored (in the .debug_info or .debug_types section). For type units one also needs to use the type offset to create the actual type DIE. In DWARF5 DIEs can come from even more data locations. And there are also skeleton units which require the user to find the associated split compile unit DIE (which would come from a different file). The new dwarf_get_units function simplifies iterating over the units in a DWARF file. It doesn't require the user to know where the DIE data is stored, it will automagically iterate over all know data sources (sections) returning the Dwarf_CU and the associated Dwarf_Die if requested. If the user requests to know the associated "subdie" it will also be resolved. A subdie is either a type DIE for a type unit or a split unit DIE for a skeleton unit. The same (and some more) info about DWARF_CUs can also be gotten through the dwarf_cu_info function. You should either use dwarf_nextcu or dwarf_next_unit with dwarf_offdie to get the (top-level) DIE. Or use dwarf_get_units and possibly dwarf_cu_info. In general you shouldn't mix them. Hope this helps and let me know if you need more info. Cheers, Mark
Re: Noop round trip through elf_update() causes segfaults
Hi Daniel, On Wed, Dec 27, 2023 at 08:40:09PM -0600, Daniel Xu wrote: > I was working on code that adds an ELF section containing custom > metadata to ELF binaries when I started getting odd segfaults > in the added-to binary. > > I've managed to create a minimal reproducer with a couple interesting > discoveries. The reproducer is available here: > > https://github.com/danobi/elf-segfault > > Basically it does a noop round trip between elf_begin() and elf_update(). > But the resulting binary, when run, outputs: > > $ ./testprog_copy > fish: Job 1, './testprog_copy' terminated by signal SIGSEGV (Address > boundary error) > > Furthermore, I built and ran tests/addsections.c [0] against my testbinary > and I still get: > > $ ./testprog_copy_elfutils > fish: Job 1, './testprog_copy_elfutils' terminated by signal SIGSEGV > (Address boundary error) > > I've also tried linking against upstream libelf built from source > with the same results. > > This leads me to believe I'm doing something very wrong or > I'm hitting a bug. You aren't doing something very wrong, but libelf does something you aren't expecting. When you are calling elf_update () it will rearrange the elf sections making sure there are no unnecessary gaps between the sections in the file, that alignment is correct, etc. libelf only cares about the section headers. It doesn't know/care about the program headers. The program headers describe how the segments have to be loaded at runtime. Since some data has moved around the program data isn't loaded correctly anymore which causes the crash. To prevent libelf from doing this, and take responsibility of how the sections are layed out yourself you have to call: elf_flagelf (elf, ELF_C_SET, ELF_F_LAYOUT); Before calling elf_update. Note that in that case you are responsible for setting/updating the sh_offset fields of the Shdrs yourself. See for example the elfutils src/elfcompress.c program to see what it does in case the Elf file has program headers. Hope that helps, Mark
Re: [PATCH 1/2] libdw: Use INTUSE with dwarf_get_units
Hi Aaron, On Wed, Dec 06, 2023 at 08:35:03PM -0500, Aaron Merey wrote: > Add INTDECL for dwarf_get_units and call dwarf_get_units with INTUSE. This is obviously OK. Although it is a bit of a micro-optimization. Thanks, Mark
Re: [PATCH] tests: fix build against upcoming `gcc-14` (`-Werror=calloc-transposed-args`)
Hi Sergei, On Thu, 2023-12-21 at 09:23 +, Sergei Trofimovich wrote: > `gcc-14` added a new `-Wcalloc-transposed-args` warning recently. It > detected minor infelicity in `calloc()` API usage in `elfutils`: > > elfstrmerge.c: In function 'main': > elfstrmerge.c:450:32: error: > 'calloc' sizes specified with 'sizeof' in the earlier argument and not > in the later argument [-Werror=calloc-transposed-args] > 450 | newscnbufs = calloc (sizeof (void *), newshnums); > |^~~~ > elfstrmerge.c:450:32: note: earlier argument should specify number of > elements, later size of each element > Thanks, looks good. Pushed. Cheers, Mark
Re: [PATCH] Add helper function for basename
Hi Khem, On Wed, Dec 20, 2023 at 08:43:57AM -0800, Khem Raj wrote: > This patch seem to work fine Thanks for double checking. I pushed it as: commit a2194f6b305bf0d0b9dd49dccd0a5c21994c8eea Author: Khem Raj Date: Sun Dec 10 12:20:33 2023 -0800 Add helper function for basename musl does not provide GNU version of basename and lately have removed the definiton from string.h [1] which exposes this problem. It can be made to work by providing a local implementation of basename which implements the GNU basename behavior, this makes it work across C libraries which have POSIX implementation only. [1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7 * lib/system.h (xbasename): New static inline functions. Poison basename. * libdw/dwarf_getsrc_file.c (dwarf_getsrc_file): Use xbasename. * libdwfl/core-file.c (dwfl_core_file_report): Likewise. * libdwfl/dwfl_module_getsrc_file.c (dwfl_module_getsrc_file): Likewise. * libdwfl/dwfl_segment_report_module.c (dwfl_segment_report_module): Likewise. * libdwfl/find-debuginfo.c (find_debuginfo_in_path): Likewise. * libdwfl/link_map.c (report_r_debug): Likewise. * libdwfl/linux-kernel-modules.c (try_kernel_name): Likewise. * src/addr2line.c (print_dwarf_function): Likewise. (print_src): Likewise. * src/ar.c (do_oper_insert): Likewise. And cast away const in entry.key assignment. * src/nm.c (show_symbols): Use xbasename. * src/stack.c (module_callback): Likewise. * src/strip.c (handle_elf): Likewise. * tests/show-die-info.c: Include system.h. (dwarf_tag_string): Use xbasename. * tests/varlocs.c: Likewise. * debuginfod/debuginfod.cxx: Move include system.h to the end. (register_file_name): Rename basename to filename. Signed-off-by: Khem Raj Signed-off-by: Mark Wielaard BTW. There is a musl tracking bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21002 Could you take a peek at that and say if there are still patches needed either in elfutils or musl? Thanks, Mark
Re: [PATCH] tests: Don't redirect output to /dev/null in run-native-test.sh
On Tue, 2023-12-12 at 10:49 +0100, Mark Wielaard wrote: > By redirecting all output to /dev/null in run-native-test.sh the > run-native-test.sh.log file will be empty on failures. This makes > it hard to figure out what went wrong. > > * tests/run-native-test.sh: Remove /dev/null redirects. I pushed this. Cheers, Mark P.S. There is now also a riscv builder.sourceware.org worker: https://builder.sourceware.org/buildbot/#/workers/40
Re: [PATCH] Add helper function for basename
Hi Khem, On Thu, Dec 14, 2023 at 10:15:00AM -0800, Khem Raj wrote: > Overall, this looks a good improvement on top of my patch so good to go. > I will also try it in my local distro builds and see how it goes. Thanks. I didn't actually test it against musl. So if you could and report whether or not it works as intended that would be really appreciated. Cheers, Mark
Re: [PATCH] Add helper function for basename
Hi Khem,
On Sun, 2023-12-10 at 12:20 -0800, Khem Raj wrote:
> musl does not provide GNU version of basename and lately have removed
> the definiton from string.h [1] which exposes this problem. It can be
> made to work by providing a local implementation of basename which
> implements the GNU basename behavior, this makes it work across C
> libraries which have POSIX implementation only.
>
> Upstream-Status: Pending
> [1]
> https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7
> Signed-off-by: Khem Raj
Our discussion showed we really need this if we want to support musl
(or any other alternative libc without the string.h basename variant).
I would have liked a configure check, but old musl makes that kind of
impossible. So I agree we should use our own implementation.
I did structure it slightly differently though. Instead of adding it to
libeu I added it to system.h as static inline function. And I poisoned
the basename symbol. That found two other places where basename was
used (and now replaced by xbasename). Sadly it means we have to rename
a variable in debuginfod.cxx from basename to filename, but I think
that is acceptable.
I don't like the const cast away in ar.c, but that seems necessary
because we are using search.h and that interface just takes non-cast
char pointers (even though they really are const).
What do you think of the attached variant of your patch?
Thanks,
Mark
From 3dfbc0f4381c835004544f7c5ee596845bb17044 Mon Sep 17 00:00:00 2001
From: Khem Raj
Date: Sun, 10 Dec 2023 12:20:33 -0800
Subject: [PATCH] Add helper function for basename
musl does not provide GNU version of basename and lately have removed
the definiton from string.h [1] which exposes this problem. It can be
made to work by providing a local implementation of basename which
implements the GNU basename behavior, this makes it work across C
libraries which have POSIX implementation only.
[1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7
* lib/system.h (xbasename): New static inline functions.
Poison basename.
* libdw/dwarf_getsrc_file.c (dwarf_getsrc_file): Use xbasename.
* libdwfl/core-file.c (dwfl_core_file_report): Likewise.
* libdwfl/dwfl_module_getsrc_file.c (dwfl_module_getsrc_file):
Likewise.
* libdwfl/dwfl_segment_report_module.c (dwfl_segment_report_module):
Likewise.
* libdwfl/find-debuginfo.c (find_debuginfo_in_path): Likewise.
* libdwfl/link_map.c (report_r_debug): Likewise.
* libdwfl/linux-kernel-modules.c (try_kernel_name): Likewise.
* src/addr2line.c (print_dwarf_function): Likewise.
(print_src): Likewise.
* src/ar.c (do_oper_insert): Likewise.
And cast away const in entry.key assignment.
* src/nm.c (show_symbols): Use xbasename.
* src/stack.c (module_callback): Likewise.
* src/strip.c (handle_elf): Likewise.
* tests/show-die-info.c: Include system.h.
(dwarf_tag_string): Use xbasename.
* tests/varlocs.c: Likewise.
* debuginfod/debuginfod.cxx: Move include system.h to the end.
(register_file_name): Rename basename to filename.
Signed-off-by: Khem Raj
Signed-off-by: Mark Wielaard
---
debuginfod/debuginfod.cxx| 22 +++---
lib/system.h | 14 ++
libdw/dwarf_getsrc_file.c| 2 +-
libdwfl/core-file.c | 2 +-
libdwfl/dwfl_module_getsrc_file.c| 2 +-
libdwfl/dwfl_segment_report_module.c | 4 ++--
libdwfl/find-debuginfo.c | 6 +++---
libdwfl/link_map.c | 2 +-
libdwfl/linux-kernel-modules.c | 2 +-
src/addr2line.c | 4 ++--
src/ar.c | 4 ++--
src/nm.c | 4 ++--
src/stack.c | 2 +-
src/strip.c | 2 +-
tests/show-die-info.c| 3 ++-
tests/varlocs.c | 2 +-
16 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index c11aeda1..524be948 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -50,11 +50,6 @@ extern "C" {
}
#endif
-extern "C" {
-#include "printversion.h"
-#include "system.h"
-}
-
#include "debuginfod.h"
#include
@@ -135,6 +130,11 @@ using namespace std;
#define tid() pthread_self()
#endif
+extern "C" {
+#include "printversion.h"
+#include "system.h"
+}
+
inline bool
string_endswith(const string& haystack, const string& needle)
@@ -3194,16 +3194,16 @@ register_file_name(sqlite_ps& ps_upsert_fileparts,
const string& name)
{
std::size_t slash = name.rfind('/');
- string dirname, basename;
+ string dirname, filename;
if (slash == std::string::npos)
{
dirname = &qu
Re: [PATCH] Add helper function for basename
On Wed, Dec 13, 2023 at 08:29:01AM -0800, Khem Raj wrote: > On Wed, Dec 13, 2023 at 7:10 AM Mark Wielaard wrote: > > > > Hi Khem, > > > > On Tue, 2023-12-12 at 09:16 -0800, Khem Raj wrote: > > > On Tue, Dec 12, 2023 at 5:18 AM Mark Wielaard wrote: > > > > On Sun, 2023-12-10 at 12:20 -0800, Khem Raj wrote: > > > > > musl does not provide GNU version of basename and lately have removed > > > > > the definiton from string.h [1] which exposes this problem. It can be > > > > > made to work by providing a local implementation of basename which > > > > > implements the GNU basename behavior, this makes it work across C > > > > > libraries which have POSIX implementation only. > > > > > > > > Thanks, this should work, but wouldn't it be easier to add a configure > > > > test for having basename defined in string.h and then only define > > > > basename in libeu.h (and build basename.c) if it isn't. So that all the > > > > code can just keep using basename (we just have to make sure libeu.h is > > > > included)? > > > > > > we could do that but it will not work as expected with older musl releases > > > where the prototype in string.h will exist. > > > > But that is good isn't it? Or did musl define basename in string.h with > > different semantics (where the given input string is modified)? > > basename was declared like this till lately > https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7 Urgh, that is pretty bad. So it declared it with the wrong signature and then it called an implementation that modified its argument... > > In the second case various elfutils libraries and tools probably just > > segfaulted when build/run against musl. And your patch would indeed fix > > both old and new musl versions. Do people using musl already use some > > variant of your patch? > > This is not yet tried widely in distros as the musl patch above is > till new and not part of > a release yet. I understand that. But I don't understand how an elfutils build/ran against current musl even worked given that it would be using the wrong basename implementation. It seems it cannot without your patch. Did we just get lucky because most paths don't end with one or more slashes? Cheers, Mark
Re: [PATCH] Add helper function for basename
Hi Khem, On Tue, 2023-12-12 at 09:16 -0800, Khem Raj wrote: > On Tue, Dec 12, 2023 at 5:18 AM Mark Wielaard wrote: > > On Sun, 2023-12-10 at 12:20 -0800, Khem Raj wrote: > > > musl does not provide GNU version of basename and lately have removed > > > the definiton from string.h [1] which exposes this problem. It can be > > > made to work by providing a local implementation of basename which > > > implements the GNU basename behavior, this makes it work across C > > > libraries which have POSIX implementation only. > > > > Thanks, this should work, but wouldn't it be easier to add a configure > > test for having basename defined in string.h and then only define > > basename in libeu.h (and build basename.c) if it isn't. So that all the > > code can just keep using basename (we just have to make sure libeu.h is > > included)? > > we could do that but it will not work as expected with older musl releases > where the prototype in string.h will exist. But that is good isn't it? Or did musl define basename in string.h with different semantics (where the given input string is modified)? In the second case various elfutils libraries and tools probably just segfaulted when build/run against musl. And your patch would indeed fix both old and new musl versions. Do people using musl already use some variant of your patch? Thanks, Mark
Re: [PATCH] Add helper function for basename
Hi Khem, On Sun, 2023-12-10 at 12:20 -0800, Khem Raj wrote: > musl does not provide GNU version of basename and lately have removed > the definiton from string.h [1] which exposes this problem. It can be > made to work by providing a local implementation of basename which > implements the GNU basename behavior, this makes it work across C > libraries which have POSIX implementation only. Thanks, this should work, but wouldn't it be easier to add a configure test for having basename defined in string.h and then only define basename in libeu.h (and build basename.c) if it isn't. So that all the code can just keep using basename (we just have to make sure libeu.h is included)? Cheers, Mark
[PATCH] tests: Don't redirect output to /dev/null in run-native-test.sh
By redirecting all output to /dev/null in run-native-test.sh the
run-native-test.sh.log file will be empty on failures. This makes
it hard to figure out what went wrong.
* tests/run-native-test.sh: Remove /dev/null redirects.
Signed-off-by: Mark Wielaard
---
tests/run-native-test.sh | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/tests/run-native-test.sh b/tests/run-native-test.sh
index 042a51c6..01a52e82 100755
--- a/tests/run-native-test.sh
+++ b/tests/run-native-test.sh
@@ -57,10 +57,10 @@ trap native_exit 0
for cc in "$HOSTCC" "$HOST_CC" cc gcc "$CC"; do
test "x$cc" != x || continue
- $cc -o native -g native.c > /dev/null 2>&1 &&
+ $cc -o native -g native.c &&
# Some shell versions don't do this right without the braces.
- { ./native > /dev/null 2>&1 & native=$! ; } &&
- sleep 1 && kill -0 $native 2> /dev/null &&
+ { ./native & native=$! ; } &&
+ sleep 1 && kill -0 $native &&
break ||
native=0
done
@@ -68,14 +68,17 @@ done
native_test()
{
# Try the build against itself, i.e. $config_host.
- testrun "$@" -e $1 > /dev/null
+ echo "Try the build against itself: $@ -e $1"
+ testrun "$@" -e $1
# Try the build against a presumed native process, running this sh.
# For tests requiring debug information, this may not test anything.
- testrun "$@" -p $$ > /dev/null
+ echo "Try the build against a presumed native process: $@ -p $$"
+ testrun "$@" -p $$
# Try the build against the trivial native program we just built with -g.
- test $native -eq 0 || testrun "$@" -p $native > /dev/null
+ echo "Try the build against the trivial native program: $@ -p $native"
+ test $native -eq 0 || testrun "$@" -p $native
}
native_test ${abs_builddir}/allregs
--
2.39.3
Sourceware infrastructure updates for Q4 2023
Sourceware infrastructure community updates for Q4 2023 - 6 months with the Software Freedom Conservancy - Sourceware @ Fosdem - OSUOSL provides extra larger arm64 and x86_64 buildbot servers - No more From rewriting for patches mailinglists = 6 months with the Software Freedom Conservancy Sourceware thanks Conservancy for their support and urges the community to support Conservancy. Sourceware has only been a Software Freedom Conservancy member project for just 6 months. But the story started a long time ago and a lot has happened in that time: https://sfconservancy.org/blog/2023/nov/27/sourceware-thanks-conservancy/ We hope the community will support the Software Freedom Conservancy 2023 Fundraiser and become a Conservancy Sustainer https://sfconservancy.org/sustainer = Sourceware @ Fosdem Various Sourceware projects will be present at Fosdem plus some overseers and of course Conservancy staff. Get your talk submissions in before end of the week (December 1st) to these developer rooms: Debuggers and Analysis tools gdb, libabigail, systemtap, valgrind, binutils, elfutils, gnupoke, cgen https://inbox.sourceware.org/6a2e8cbf-0d63-24e7-e3c2-c3d286e2e...@redhat.com/ GCC compiler devroom gcc, binutils, glibc, newlib https://inbox.sourceware.org/36fadb0549c3dca716eb3b923d66a11be2c67a61.ca...@redhat.com/ And if you like to organize an online virtual mini-BoF around some topic or project then the @conservancy BBB server is available for all Sourceware projects. https://inbox.sourceware.org/9ca90cd013675a960d47ee09fa4403f69405e9f2.ca...@klomp.org/ = OSUOSL provides extra larger arm64 and x86_64 buildbot servers There have been complaints about overloaded builders. So OSUOSL have provided us with another arm64 and x86_64 server. The new servers do the larger gcc and glibc builds so the other builders can do quicker (smaller) CI builds without having to wait on the big jobs. This also frees up the other container builders to do more automated jobs like the recently added autotools generated files checker for gcc, binutils and gdb: https://inbox.sourceware.org/20231115194803.gw31...@gnu.wildebeest.org/ Please contact the builder project build...@sourceware.org if you want to run some automated jobs on https://builder.sourceware.org/ = No more From rewriting for patches mailinglists Because of dkim, strict dmarc policies and an old mailman setup Sourceware mailinglists used From rewriting. No more! We upgraded mailman, gave up subject prefixes, mail footers, html stripping and reply-to mangling. After the libc-alpha and gcc-patches mailinglist tests to avoid From rewriting worked out nicely we enabled the same settings to some other mailinglists. The gcc patches lists for libstdc++, libgccjit, fortran and gcc-rust. And for those projects that use patchwork, newlib, elfutils, libabigail and gdb. This hopefully makes mailing patches and using git am on them a bit nicer. Outgoing sourceware email now also includes ARC headers. https://en.wikipedia.org/wiki/Authenticated_Received_Chain Feedback on whether this helps email delivery appreciated. Please contact overseers if you would like the new setting for any other Sourceware mailinglist. Thanks to the FSF tech-team for walking us through their setup for lists.gnu.org
Re: [PATCH] libelf: check decompressed ZSTD size
Hi Aleksei, On Thu, Nov 23, 2023 at 03:31:47PM +, Aleksei Vetrov wrote: > Decompression functions like __libelf_decompress_zlib check that > decompressed data has the same size as it was declared in the header > (size_out argument). The same check is now added to > __libelf_decompress_zstd to make sure that the whole allocated buffer is > initialized. > > * libelf/elf_compress.c (__libelf_decompress_zstd): Use return value > of ZSTD_decompress to check that decompressed data size is the > same as size_out of the buffer that was allocated. Thanks, this makes sense. If the decompressed size isn't what was encoded in the Chdr then we could reduce the size of the d_buf/d_size, but that probably is not what the user expects. Flagging it as bad/inconsistent data makes sense. Especially since we do the same for zlib compressed data. Pushed, Mark
Re: [PATCH v2 2/2] tests: Add test for duplicate entries in archive
Hi Aleksei, On Mon, 2023-11-20 at 17:44 +, Aleksei Vetrov wrote: > Test dwfl-report-offline-memory against an archive that contains > non-relocatable ELFs with the same name and contents. > > * tests/test-ar-duplicates.a.bz2: New test file. > * tests/run-dwfl-report-offline-memory.sh: Test new > test-ar-duplicates.a.bz2. > * tests/Makefile.am (EXTRA_DIST): Add test-ar-duplicates.a.bz2. Thanks. Also tested it fails before your fix. Pushed, Mark
Re: [PATCH v2 1/2] libdwfl: handle duplicate ELFs when reporting archives
Hi Aleksei, On Mon, 2023-11-20 at 17:44 +, Aleksei Vetrov wrote: > When archive is processed in process_archive (libdwfl/offline.c), it > creates an Elf object for each archive member. Then in > process_archive_member it calls process_file to create a Dwfl_Module > through __libdwfl_report_elf. > > The ownership of the Elf object is expected to be: > > * either transfered to the Dwfl_Module, if __libdwfl_report_elf returns > not NULL; > > * or handled at the end of process_archive_member by calling elf_end. > > Moreover, Elf object is expected to be alive, if __libdwfl_report_elf > returns not NULL, because at the end of process_archive_member it > advances to the next member through the elf_next call. > > The problem happens when __libdwfl_report_elf encounters Elf with the > same name and content as it seen before. In that case dwfl_report_module > will reuse existing Dwfl_Module object. This leads to a codepath that > calls elf_end on the Elf object, while returning not NULL, breaking the > elf_next call to the next member. > > The fix is to destroy m->main.elf instead and put the new Elf object in > the already existing Dwfl_Module. > > * libdwfl/dwfl_report_elf.c (__libdwfl_report_elf): Replace Elf in > the Dwfl_Module in case of duplicate modules to prolong its > lifetime for subsequent processing. Thanks, pushed. Mark
Re: [PATCH v2] libdwfl: Correctly handle corefile non-contiguous segments
Hi Aaron,
On Fri, 2023-11-17 at 21:48 -0500, Aaron Merey wrote:
> v1: https://sourceware.org/pipermail/elfutils-devel/2023q4/006644.html
>
> v2 changes:
>
> The size of the uncompressed testcore-noncontig has been reduced from
> 736M to 54K.
Uncompressed it is 580K. Still a bit large, but much more reasonable.
We even have a couple of larger test files in the repo. Thanks.
And in theory it can be replicated.
> dwfl_addrsegment tests have been added to verify correct handling of
> non-contiguous segments.
BTW. Adding extra comments after the --- makes it easier to post a
commit as you will apply it because comments after the --- will be
ignored by git am.
Please restore the original commit message before pushing.
The description of the issue was really good.
> ---
> libdwfl/dwfl_segment_report_module.c | 37 ++-
> tests/.gitignore | 1 +
> tests/Makefile.am| 8 ++--
> tests/dwfl-core-noncontig.c | 67 +++
> tests/run-dwfl-core-noncontig.sh | 63 +
> tests/testcore-noncontig.bz2 | Bin 0 -> 54684 bytes
> 6 files changed, 162 insertions(+), 14 deletions(-)
> create mode 100644 tests/dwfl-core-noncontig.c
> create mode 100755 tests/run-dwfl-core-noncontig.sh
> create mode 100644 tests/testcore-noncontig.bz2
>
> diff --git a/libdwfl/dwfl_segment_report_module.c
> b/libdwfl/dwfl_segment_report_module.c
> index 3ef62a7d..09ee37b3 100644
> --- a/libdwfl/dwfl_segment_report_module.c
> +++ b/libdwfl/dwfl_segment_report_module.c
> @@ -737,17 +737,34 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
> char *name,
> && invalid_elf (module->elf, module->disk_file_has_build_id,
> _id))
> {
> - elf_end (module->elf);
> - close (module->fd);
> - module->elf = NULL;
> - module->fd = -1;
> + /* If MODULE's build-id doesn't match the disk file's
> +build-id, close ELF only if MODULE and ELF refer to
> +different builds of files with the same name. This
> +prevents premature closure of the correct ELF in cases
> +where segments of a module are non-contiguous in memory. */
> + if (name != NULL && module->name[0] != '\0'
> + && strcmp (basename (module->name), basename (name)) == 0)
> + {
> + elf_end (module->elf);
> + close (module->fd);
> + module->elf = NULL;
> + module->fd = -1;
> + }
> }
> - if (module->elf != NULL)
> + else if (module->elf != NULL)
> {
> - /* Ignore this found module if it would conflict in address
> -space with any already existing module of DWFL. */
> + /* This module has already been reported. */
> skip_this_module = true;
> }
> + else
> + {
> + /* Only report this module if we haven't already done so. */
> + for (Dwfl_Module *mod = dwfl->modulelist; mod != NULL;
> + mod = mod->next)
> + if (mod->low_addr == module_start
> + && mod->high_addr == module_end)
> + skip_this_module = true;
> + }
> }
>if (skip_this_module)
> goto out;
> @@ -781,10 +798,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
> char *name,
> }
> }
>
> - /* Our return value now says to skip the segments contained
> - within the module. */
> - ndx = addr_segndx (dwfl, segment, module_end, true);
> -
>/* Examine its .dynamic section to get more interesting details.
> If it has DT_SONAME, we'll use that as the module name.
> If it has a DT_DEBUG, then it's actually a PIE rather than a DSO.
> @@ -929,6 +942,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
> char *name,
>ndx = -1;
>goto out;
> }
> + else
> +ndx++;
>
>/* We have reported the module. Now let the caller decide whether we
> should read the whole thing in right now. */
Code looks the same.
> diff --git a/tests/.gitignore b/tests/.gitignore
> index b9aa22ba..5bebb2c4 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -49,6 +49,7 @@
> /dwfl-report-elf-align
> /dwfl-report-offline-memory
> /dwfl-report-segment-contiguous
> +/dwfl-core-noncontig
> /dwfllines
> /dwflmodtest
> /dwflsyms
Ack.
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 7fb8efb1..9f8f7698 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -42,7 +42,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx
> scnnames sectiondump \
> dwfl-bug-addr-overflow arls dwfl-bug-fd-leak \
> dwfl-addr-sect dwfl-bug-report early-offscn \
>
Re: [PATCH] readelf: Don't print average number of tests when no tests are done
Hi,
On Wed, 2023-11-15 at 17:41 +0100, Mark Wielaard wrote:
> If the symbol hash table only contains lenght zero chains, no lookup
> tests need to be done and eu-readelf -I would print out bogus numbers
> for the number of tests that were successful/unsuccessful.
>
> e.g. for an "empty" program like
> int main() {}
> eu-readelf -I would print:
>
> Histogram for bucket list length in section [ 5] '.gnu.hash' (total of 1
> bucket):
> Addr: 0x004003c0 Offset: 0x0003c0 Link to section: [ 6] '.dynsym'
> Symbol Bias: 1
> Bitmask Size: 8 bytes 0% bits set 2nd hash shift: 0
> Length Number % of total Coverage
> 0 1 100.0%
> Average number of tests: successful lookup: -nan
> unsuccessful lookup: 0.00
>
> Only print out the Average number of tests when there were actual
> tests to do.
Pushed,
Mark
Re: [PATCH] tests: Restructure run-debuginfod-response-headers.sh
Hi,
On Sun, Nov 19, 2023 at 03:11:28PM +0100, Mark Wielaard wrote:
> run-debuginfod-response-headers.sh does occassionally fail because
> it might scan an rpm more than once. Try to fix this by making sure
> all files that debuginfod is supposed to scan are ready before the
> server starts. And to explicitly wait till the first scan is ready
> and done before testing 'scanned_files_total{source=".rpm archive"}'
> instead of sending an kill -USR1.
The try builds look good and on irc Frank said the code looks good.
Pushed. Lets hope this gets rid of the flakyiness of the testcase.
Cheers,
Mark
> Signed-off-by: Mark Wielaard
> ---
> tests/run-debuginfod-response-headers.sh | 23 ++-
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tests/run-debuginfod-response-headers.sh
> b/tests/run-debuginfod-response-headers.sh
> index fbb6a484..ea516ced 100755
> --- a/tests/run-debuginfod-response-headers.sh
> +++ b/tests/run-debuginfod-response-headers.sh
> @@ -32,14 +32,6 @@ export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
> base=9500
> get_ports
> mkdir F R
> -env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS=
> ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p $PORT1 -t0
> -g0 -v R F > vlog$PORT1 2>&1 &
> -PID1=$!
> -tempfiles vlog$PORT1
> -errfiles vlog$PORT1
> -# Server must become ready
> -wait_ready $PORT1 'ready' 1
> -export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1/ # or without trailing /
> -
>
> # Compile a simple program, strip its debuginfo and save the build-id.
> # Also move the debuginfo into another directory so that elfutils
> @@ -57,12 +49,25 @@ if [ "$zstd" = "false" ]; then # nuke the zstd fedora 31
> ones
> rm -vrf R/debuginfod-rpms/fedora31
> fi
>
> -kill -USR1 $PID1
> +env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS=
> ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p $PORT1 -t0
> -g0 -v R F > vlog$PORT1 2>&1 &
> +PID1=$!
> +tempfiles vlog$PORT1
> +errfiles vlog$PORT1
> +# Server must become ready
> +wait_ready $PORT1 'ready' 1
> +export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1/ # or without trailing /
> +
> +
> # Wait till both files are in the index and scan/index fully finished
> +wait_ready $PORT1 'ready' 1
> wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
> +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> +wait_ready $PORT1 'thread_busy{role="scan"}' 0
> +
> # All rpms need to be in the index, except the dummy permission-000 one
> rpms=$(find R -name \*rpm | grep -v nothing | wc -l)
> wait_ready $PORT1 'scanned_files_total{source=".rpm archive"}' $rpms
> +
> kill -USR1 $PID1 # two hits of SIGUSR1 may be needed to resolve
> .debug->dwz->srefs
> # Wait till both files are in the index and scan/index fully finished
> wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
> --
> 2.39.3
>
[PATCH] tests: Restructure run-debuginfod-response-headers.sh
run-debuginfod-response-headers.sh does occassionally fail because
it might scan an rpm more than once. Try to fix this by making sure
all files that debuginfod is supposed to scan are ready before the
server starts. And to explicitly wait till the first scan is ready
and done before testing 'scanned_files_total{source=".rpm archive"}'
instead of sending an kill -USR1.
Signed-off-by: Mark Wielaard
---
tests/run-debuginfod-response-headers.sh | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/tests/run-debuginfod-response-headers.sh
b/tests/run-debuginfod-response-headers.sh
index fbb6a484..ea516ced 100755
--- a/tests/run-debuginfod-response-headers.sh
+++ b/tests/run-debuginfod-response-headers.sh
@@ -32,14 +32,6 @@ export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
base=9500
get_ports
mkdir F R
-env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS=
${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p $PORT1 -t0
-g0 -v R F > vlog$PORT1 2>&1 &
-PID1=$!
-tempfiles vlog$PORT1
-errfiles vlog$PORT1
-# Server must become ready
-wait_ready $PORT1 'ready' 1
-export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1/ # or without trailing /
-
# Compile a simple program, strip its debuginfo and save the build-id.
# Also move the debuginfo into another directory so that elfutils
@@ -57,12 +49,25 @@ if [ "$zstd" = "false" ]; then # nuke the zstd fedora 31
ones
rm -vrf R/debuginfod-rpms/fedora31
fi
-kill -USR1 $PID1
+env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS=
${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p $PORT1 -t0
-g0 -v R F > vlog$PORT1 2>&1 &
+PID1=$!
+tempfiles vlog$PORT1
+errfiles vlog$PORT1
+# Server must become ready
+wait_ready $PORT1 'ready' 1
+export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1/ # or without trailing /
+
+
# Wait till both files are in the index and scan/index fully finished
+wait_ready $PORT1 'ready' 1
wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
+wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
+wait_ready $PORT1 'thread_busy{role="scan"}' 0
+
# All rpms need to be in the index, except the dummy permission-000 one
rpms=$(find R -name \*rpm | grep -v nothing | wc -l)
wait_ready $PORT1 'scanned_files_total{source=".rpm archive"}' $rpms
+
kill -USR1 $PID1 # two hits of SIGUSR1 may be needed to resolve
.debug->dwz->srefs
# Wait till both files are in the index and scan/index fully finished
wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
--
2.39.3
Re: [PATCH 2/2] tests: Add test for duplicate entries in archive
Hi Aleksei, On Fri, Nov 17, 2023 at 10:35:41PM +, vvv...@google.com wrote: > Test dwfl-report-offline-memory against an archive that contains > non-relocatable ELFs with the same name and contents. Nice testcase. Do note that you also have to add the new test file to EXTRA_DIST so it actually gets into the dist. * tests/test-ar-duplicates.a.bz2: New test file. * tests/run-dwfl-report-offline-memory.sh: Test new test-ar-duplicates.a.bz2. * tests/Makefile.am (EXTRA_DIST): Add test-ar-duplicates.a.bz2. diff --git a/tests/Makefile.am b/tests/Makefile.am index 7fb8efb1..80f6cb59 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -632,7 +632,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ run-nvidia-extended-linemap-libdw.sh run-nvidia-extended-linemap-readelf.sh \ testfile_nvidia_linemap.bz2 \ testfile-largealign.o.bz2 run-strip-largealign.sh \ -run-funcretval++11.sh +run-funcretval++11.sh \ +test-ar-duplicates.a.bz2 make distcheck catches these things by creating a dist and doing a build and run various tests and make check. Cheers, Mark
Re: [PATCH 1/2] libdwfl: handle duplicate ELFs when reporting archives
Hi Aleksei,
On Fri, Nov 17, 2023 at 10:35:40PM +, vvv...@google.com wrote:
> When archive is processed in process_archive (libdwfl/offline.c), it
> creates an Elf object for each archive member. Then in
> process_archive_member it calls process_file to create a Dwfl_Module
> through __libdwfl_report_elf.
>
> The ownership of the Elf object is expected to be:
>
> * either transfered to the Dwfl_Module, if __libdwfl_report_elf returns
> not NULL;
>
> * or handled at the end of process_archive_member by calling elf_end.
>
> Moreover, Elf object is expected to be alive, if __libdwfl_report_elf
> returns not NULL, because at the end of process_archive_member it
> advances to the next member through the elf_next call.
>
> The problem happens when __libdwfl_report_elf encounters Elf with the
> same name and content as it seen before. In that case dwfl_report_module
> will reuse existing Dwfl_Module object. This leads to a codepath that
> calls elf_end on the Elf object, while returning not NULL, breaking the
> elf_next call to the next member.
>
> The fix is to destroy m->main.elf instead and put the new Elf object in
> the already existing Dwfl_Module.
>
> * libdwfl/dwfl_report_elf.c (__libdwfl_report_elf): Replace Elf in
> the Dwfl_Module in case of overlapping or duplicate modules to
> prolong its lifetime for subsequent processing.
Thanks for that analysis and proposed solution. The ownership
reasoning makes sense. But I do have one question.
> diff --git a/libdwfl/dwfl_report_elf.c b/libdwfl/dwfl_report_elf.c
> index 581f4079..58b06aea 100644
> --- a/libdwfl/dwfl_report_elf.c
> +++ b/libdwfl/dwfl_report_elf.c
> @@ -276,7 +276,8 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const
> char *file_name,
> }
>else
> {
> - elf_end (elf);
> + elf_end (m->main.elf);
> + m->main.elf = elf;
> if (m->main_bias != bias
> || m->main.vaddr != vaddr || m->main.address_sync != address_sync)
> goto overlap;
If we goto overlap here don't we still have a problem? overlap will
set m->gc = true; and return NULL. So the caller will think they
still owns the elf handle and will probably close it. But then when
the module is GCed in dwfl_report_end it will close the elf handle
again.
Should we instead move the elf_end and reassignment of main.elf to
after this if statement?
Thanks,
Mark
