Re: [PATCH] Introduce public dwfl_get_debuginfod_client API

[Mark Wielaard <mark@klomp.org>]

[-- Attachment #1: Type: text/plain, Size: 3013 bytes --]

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

[-- Attachment #2: 0001-Move-dwfl_get_debuginfod_client-to-ELFUTILS_0.188.patch --]
[-- Type: text/x-diff, Size: 3564 bytes --]

From 2fb8571e78852cc71c0128705f500490d79f8a94 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Wed, 13 Jul 2022 22:34:36 +0200
Subject: [PATCH] Move dwfl_get_debuginfod_client to ELFUTILS_0.188

0.187 was already released, so add new function to 0.188. Also add
NEWS entry and INTUSE.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 ChangeLog                   | 4 ++++
 NEWS                        | 2 ++
 libdw/ChangeLog             | 5 +++++
 libdw/libdw.map             | 2 +-
 libdwfl/ChangeLog           | 7 +++++++
 libdwfl/debuginfod-client.c | 5 +++--
 6 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c5e43e8c..eb2a35f5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2022-07-13  Mark Wielaard  <mark@klomp.org>
+
+	* NEWS: Add dwfl_get_debuginfod_client.
+
 2022-06-02  Mark Wielaard  <mark@klomp.org>
 
 	* configure.ac (OLD_LIBMICROHTTPD): New AM_CONDITIONAL based on
diff --git a/NEWS b/NEWS
index c133fcbe..392f2edc 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,8 @@ Version 0.188 some time after 0.187
 
 debuginfod: Add --disable-source-scan option.
 
+libdwfl: Add new function dwfl_get_debuginfod_client.
+
 Version 0.187
 
 debuginfod: Support -C option for connection thread pooling.
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index b254d9cd..6a8f7e51 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,8 @@
+2022-07-13  Mark Wielaard  <mark@klomp.org>
+
+	* libdw.map (ELFUTILS_0.187): Renamed to...
+	(ELFUTILS_0.188): ... this.
+
 2022-05-09  Mark Wielaard  <mark@klomp.org>
 
 	* dwarf_getlocation.c (store_implicit_value): Check block length.
diff --git a/libdw/libdw.map b/libdw/libdw.map
index 3fdf3f93..6da25561 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -367,7 +367,7 @@ ELFUTILS_0.186 {
     dwarf_linefunctionname;
 } ELFUTILS_0.177;
 
-ELFUTILS_0.187 {
+ELFUTILS_0.188 {
   global:
     dwfl_get_debuginfod_client;
 } ELFUTILS_0.186;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 890df156..6ade1afc 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,10 @@
+2022-07-13  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (dwfl_get_debuginfod_client): Add INTDEF.
+	(__libdwfl_debuginfod_find_executable): Use
+	INTUSE (dwfl_get_debuginfod_client).
+	(__libdwfl_debuginfod_find_debuginfo): Likewise.
+
 2022-06-22  Milian Wolff <mail@milianw.de>
 
 	* libdwfl.h, debuginfod-client.c (dwfl_get_debuginfod_client):
diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
index 813043b1..87af73a7 100644
--- a/libdwfl/debuginfod-client.c
+++ b/libdwfl/debuginfod-client.c
@@ -65,6 +65,7 @@ dwfl_get_debuginfod_client (Dwfl *dwfl)
 
   return NULL;
 }
+INTDEF(dwfl_get_debuginfod_client)
 
 int
 __libdwfl_debuginfod_find_executable (Dwfl *dwfl,
@@ -74,7 +75,7 @@ __libdwfl_debuginfod_find_executable (Dwfl *dwfl,
   int fd = -1;
   if (build_id_len > 0)
     {
-      debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
+      debuginfod_client *c = INTUSE (dwfl_get_debuginfod_client) (dwfl);
       if (c != NULL)
 	fd = (*fp_debuginfod_find_executable) (c, build_id_bits,
 					       build_id_len, NULL);
@@ -91,7 +92,7 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl,
   int fd = -1;
   if (build_id_len > 0)
     {
-      debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
+      debuginfod_client *c = INTUSE (dwfl_get_debuginfod_client) (dwfl);
       if (c != NULL)
 	fd = (*fp_debuginfod_find_debuginfo) (c, build_id_bits,
 					      build_id_len, NULL);
-- 
2.30.2