Introduce public dwfl_get_debuginfod_client API
Commit Message
Dwfl can use debuginfod internally, which was so far totally opaque
to the outside. While the functionality is great for users of the
dwfl API, the long wait times induced by downloading of data over
debuginfod lead to complaints by endusers. To offer them a bit more
insight into the internal ongoings, one can now use e.g.
`debuginfod_set_progressfn` on the handle returned by
`dwfl_get_debuginfod_client` to report download progress.
Rename get_client to dwfl_get_debuginfod_client and make it public.
Unconditionally compile debuginfod-client.c and stub the new public
function and always return NULL when debuginfod integration was
disabled.
Signed-off-by: Milian Wolff <mail@milianw.de>
---
libdw/libdw.map | 5 +++++
libdwfl/ChangeLog | 5 +++++
libdwfl/Makefile.am | 5 +----
libdwfl/debuginfod-client.c | 23 ++++++++++++++++++-----
libdwfl/libdwfl.h | 10 ++++++++++
libdwfl/libdwflP.h | 1 +
6 files changed, 40 insertions(+), 9 deletions(-)
Comments
Thanks. Let's wait for a maintainer to give it the ok before merging.
Aaron
Hi Milian,
There weren't any concerns with the patch so I've gone ahead and merged
it as commit a4b1839c3c46.
Thanks,
Aaron
On Mittwoch, 13. Juli 2022 20:20:04 CEST Aaron Merey wrote:
> Hi Milian,
>
> There weren't any concerns with the patch so I've gone ahead and merged
> it as commit a4b1839c3c46.
Thank you Aaron!
I got spammed by a flood of mails from buildbot, but from what I can tell the
issues are all unrelated to my patch? Or did I break something? See e.g.:
[1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41
Thanks
On Wed, Jul 13, 2022 at 3:36 PM Milian Wolff <mail@milianw.de> wrote:
> I got spammed by a flood of mails from buildbot, but from what I can tell the
> issues are all unrelated to my patch? Or did I break something? See e.g.:
>
> [1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41
These issues are unrelated to your patch. One has been fixed and we
are looking into the ppc/s390x failures.
Aaron
Hi Milian,
On Wed, Jul 13, 2022 at 09:36:45PM +0200, Milian Wolff wrote:
> On Mittwoch, 13. Juli 2022 20:20:04 CEST Aaron Merey wrote:
> > There weren't any concerns with the patch so I've gone ahead and merged
> > it as commit a4b1839c3c46.
Yeah, sorry, I should have spoken up on the list not just mumbled
something on irc. My only concern was that I was afraid to pull in
libdebuginfod.h api by default. But you handled to by making
debuginfod_client handle an opaque pointer, which it of course is. So
my worry was unfounded.
But I did just now see it has a typo in libdw.map:
ELFUTILS_0.187 {
global:
dwfl_get_debuginfod_client;
} ELFUTILS_0.186;
That should be ELFUTILS_0.188 we already released 0.187.
Also (micro optimazition to prevent a PLT call) the internal calls
should use INTUSE (dwfl_get_debuginfod_client).
I made both changes and added a NEWS entry. See attached.
> I got spammed by a flood of mails from buildbot, but from what I can tell the
> issues are all unrelated to my patch? Or did I break something? See e.g.:
>
> [1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41
Those are unrelated to your patch, but a typo in the buildbot step:
https://sourceware.org/git/?p=builder.git;a=commitdiff;h=cc655e64acb45b2904cb4ce3169c8be92422e2c4;hp=7de05787acb62998c0fbbeec7ee6489700910962
Most builders are now green again:
https://builder.sourceware.org/buildbot/#/builders?tags=elfutils
There is however a new failure because of a new glibc/gcc
fortification on ppc64le and s390x:
https://builder.sourceware.org/buildbot/#/builders/43/builds/41
https://builder.sourceware.org/buildbot/#/builders/55/builds/40
These are also unrelated to your patch. The error is:
In file included from /usr/include/ar.h:22,
from ../libelf/libelfP.h:33,
from core-file.c:31:
In function ‘pread’,
inlined from ‘pread_retry’ at ../lib/system.h:188:21,
inlined from ‘elf_begin_rand’ at core-file.c:86:16,
inlined from ‘core_file_read_eagerly’ at core-file.c:205:15:
/usr/include/bits/unistd.h:74:10: error: ‘__pread_alias’ writing 58 or more bytes into a region of size 10 overflows the destination [-Werror=stringop-overflow=]
74 | return __glibc_fortify (pread, __nbytes, sizeof (char),
| ^~~~~~~~~~~~~~~
/usr/include/ar.h: In function ‘core_file_read_eagerly’:
/usr/include/ar.h:41:10: note: destination object ‘ar_size’ of size 10
41 | char ar_size[10]; /* File size, in ASCII decimal. */
| ^~~~~~~
/usr/include/bits/unistd.h:50:16: note: in a call to function ‘__pread_alias’ declared with attribute ‘access (write_only, 2, 3)’
50 | extern ssize_t __REDIRECT (__pread_alias,
| ^~~~~~~~~~
cc1: all warnings being treated as errors
That needs investigation. The ar file format is a little odd, those
fields are in (non-zero terminated) ASCII decimal notation. Which
probably confuses the fortification.
Cheers,
Mark
On Mittwoch, 13. Juli 2022 22:38:51 CEST Mark Wielaard wrote:
> Hi Milian,
>
> On Wed, Jul 13, 2022 at 09:36:45PM +0200, Milian Wolff wrote:
> > On Mittwoch, 13. Juli 2022 20:20:04 CEST Aaron Merey wrote:
> > > There weren't any concerns with the patch so I've gone ahead and merged
> > > it as commit a4b1839c3c46.
>
> Yeah, sorry, I should have spoken up on the list not just mumbled
> something on irc. My only concern was that I was afraid to pull in
> libdebuginfod.h api by default. But you handled to by making
> debuginfod_client handle an opaque pointer, which it of course is. So
> my worry was unfounded.
>
> But I did just now see it has a typo in libdw.map:
>
> ELFUTILS_0.187 {
> global:
> dwfl_get_debuginfod_client;
> } ELFUTILS_0.186;
>
> That should be ELFUTILS_0.188 we already released 0.187.
Ah, thanks for spotting that.
> Also (micro optimazition to prevent a PLT call) the internal calls
> should use INTUSE (dwfl_get_debuginfod_client).
TIL, thanks. I probably will never feel at home in C code bases with all these
arcane macros :D
> I made both changes and added a NEWS entry. See attached.
LGTM.
> > I got spammed by a flood of mails from buildbot, but from what I can tell
> > the issues are all unrelated to my patch? Or did I break something? See
> > e.g.:
> >
> > [1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41
>
> Those are unrelated to your patch
OK, thanks.
@@ -366,3 +366,8 @@ ELFUTILS_0.186 {
dwarf_linecontext;
dwarf_linefunctionname;
} ELFUTILS_0.177;
+
+ELFUTILS_0.187 {
+ global:
+ dwfl_get_debuginfod_client;
+} ELFUTILS_0.186;
@@ -1,3 +1,8 @@
+2022-06-22 Milian Wolff <mail@milianw.de>
+
+ * libdwfl.h, debuginfod-client.c (dwfl_get_debuginfod_client):
+ Rename get_client to dwfl_get_debuginfod_client and make it public.
+
2022-05-15 Mark Wielaard <mark@klomp.org>
* libdwfl.h (dwfl_module_addrinfo): Update docs and nonnull
@@ -70,7 +70,7 @@ libdwfl_a_SOURCES = dwfl_begin.c dwfl_end.c dwfl_error.c dwfl_version.c \
link_map.c core-file.c open.c image-header.c \
dwfl_frame.c frame_unwind.c dwfl_frame_pc.c \
linux-pid-attach.c linux-core-attach.c dwfl_frame_regs.c \
- gzip.c
+ gzip.c debuginfod-client.c
if BZLIB
libdwfl_a_SOURCES += bzip2.c
@@ -81,9 +81,6 @@ endif
if ZSTD
libdwfl_a_SOURCES += zstd.c
endif
-if LIBDEBUGINFOD
-libdwfl_a_SOURCES += debuginfod-client.c
-endif
libdwfl = $(libdw)
libdw = ../libdw/libdw.so
@@ -32,6 +32,9 @@
#endif
#include "libdwflP.h"
+
+#ifdef ENABLE_LIBDEBUGINFOD
+
#include <pthread.h>
#include <dlfcn.h>
@@ -46,8 +49,8 @@ static pthread_once_t init_control = PTHREAD_ONCE_INIT;
/* NB: this is slightly thread-unsafe */
-static debuginfod_client *
-get_client (Dwfl *dwfl)
+debuginfod_client *
+dwfl_get_debuginfod_client (Dwfl *dwfl)
{
if (dwfl->debuginfod != NULL)
return dwfl->debuginfod;
@@ -71,7 +74,7 @@ __libdwfl_debuginfod_find_executable (Dwfl *dwfl,
int fd = -1;
if (build_id_len > 0)
{
- debuginfod_client *c = get_client (dwfl);
+ debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
if (c != NULL)
fd = (*fp_debuginfod_find_executable) (c, build_id_bits,
build_id_len, NULL);
@@ -88,7 +91,7 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl,
int fd = -1;
if (build_id_len > 0)
{
- debuginfod_client *c = get_client (dwfl);
+ debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
if (c != NULL)
fd = (*fp_debuginfod_find_debuginfo) (c, build_id_bits,
build_id_len, NULL);
@@ -105,7 +108,7 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
}
/* Try to get the libdebuginfod library functions.
- Only needs to be called once from get_client. */
+ Only needs to be called once from dwfl_get_debuginfod_client. */
static void
__libdwfl_debuginfod_init (void)
{
@@ -134,3 +137,13 @@ __libdwfl_debuginfod_init (void)
}
}
}
+
+#else // ENABLE_LIBDEBUGINFOD
+
+debuginfod_client *
+dwfl_get_debuginfod_client (Dwfl *)
+{
+ return NULL;
+}
+
+#endif // ENABLE_LIBDEBUGINFOD
@@ -49,6 +49,9 @@ typedef struct Dwfl_Thread Dwfl_Thread;
PC location described by an FDE belonging to Dwfl_Thread. */
typedef struct Dwfl_Frame Dwfl_Frame;
+/* Handle for debuginfod-client connection. */
+typedef struct debuginfod_client debuginfod_client;
+
/* Callbacks. */
typedef struct
{
@@ -795,6 +798,13 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
__nonnull_attribute__ (1, 2);
+/* Return the internal debuginfod-client connection handle for the DWFL session.
+ When the client connection has not yet been initialized, it will be done on the
+ first call to this function. If elfutils is compiled without support for debuginfod,
+ NULL will be returned.
+ */
+extern debuginfod_client *dwfl_get_debuginfod_client (Dwfl *dwfl);
+
#ifdef __cplusplus
}
#endif
@@ -786,6 +786,7 @@ INTDECL (dwfl_getthread_frames)
INTDECL (dwfl_getthreads)
INTDECL (dwfl_thread_getframes)
INTDECL (dwfl_frame_pc)
+INTDECL (dwfl_get_debuginfod_client)
/* Leading arguments standard to callbacks passed a Dwfl_Module. */
#define MODCB_ARGS(mod) (mod), &(mod)->userdata, (mod)->name, (mod)->low_addr