* [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r.
@ 2020-12-16 22:30 Érico Nogueira
2020-12-16 22:30 ` [PATCH v2 2/2] src/readelf: use qsort instead of qsort_r Érico Nogueira
2020-12-17 0:08 ` [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Dmitry V. Levin
0 siblings, 2 replies; 6+ messages in thread
From: Érico Nogueira @ 2020-12-16 22:30 UTC (permalink / raw)
To: elfutils-devel; +Cc: Érico Rolim
From: Érico Rolim <erico.erc@gmail.com>
The Linux man pages recommend this version of the function for portable
applications.
Signed-off-by: Érico Rolim <erico.erc@gmail.com>
---
Only difference from the initial patch is that this includes the
Signed-off-by line.
libdwfl/ChangeLog | 4 ++++
libdwfl/dwfl_error.c | 11 ++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index f9f6f01f..d22f9892 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,7 @@
+2020-12-16 Érico Nogueira <ericonr@disroot.org>
+
+ * dwfl_error.c (strerror_r): Always use the XSI-compliant version.
+
2020-12-16 Dmitry V. Levin <ldv@altlinux.org>
* argp-std.c (_): Remove.
diff --git a/libdwfl/dwfl_error.c b/libdwfl/dwfl_error.c
index 7bcf61cc..e5db1217 100644
--- a/libdwfl/dwfl_error.c
+++ b/libdwfl/dwfl_error.c
@@ -30,6 +30,11 @@
# include <config.h>
#endif
+/* Guarantee that we get the XSI compliant strerror_r */
+#ifdef _GNU_SOURCE
+#undef _GNU_SOURCE
+#endif
+
#include <assert.h>
#include <libintl.h>
#include <stdbool.h>
@@ -136,6 +141,8 @@ __libdwfl_seterrno (Dwfl_Error error)
global_error = canonicalize (error);
}
+/* To store the error message from strerror_r */
+static __thread char errormsg[128];
const char *
dwfl_errmsg (int error)
@@ -154,7 +161,9 @@ dwfl_errmsg (int error)
switch (error &~ 0xffff)
{
case OTHER_ERROR (ERRNO):
- return strerror_r (error & 0xffff, "bad", 0);
+ if (strerror_r (error & 0xffff, errormsg, sizeof errormsg))
+ return "strerror_r() failed()";
+ return errormsg;
case OTHER_ERROR (LIBELF):
return elf_errmsg (error & 0xffff);
case OTHER_ERROR (LIBDW):
--
2.29.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] src/readelf: use qsort instead of qsort_r.
2020-12-16 22:30 [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Érico Nogueira
@ 2020-12-16 22:30 ` Érico Nogueira
2021-01-10 15:37 ` Mark Wielaard
2020-12-17 0:08 ` [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Dmitry V. Levin
1 sibling, 1 reply; 6+ messages in thread
From: Érico Nogueira @ 2020-12-16 22:30 UTC (permalink / raw)
To: elfutils-devel; +Cc: Érico Rolim
From: Érico Rolim <erico.erc@gmail.com>
This program is single threaded, so using qsort with a global variable
isn't a danger. The interface for qsort_r isn't standardized (and
diverges between glibc and FreeBSD, for example), which makes usage of
qsort, where possible, preferrable.
Signed-off-by: Érico Rolim <erico.erc@gmail.com>
---
Only difference from the initial patch is that this includes the
Signed-off-by line.
src/ChangeLog | 4 ++++
src/readelf.c | 14 ++++++++++----
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/ChangeLog b/src/ChangeLog
index 2e428e0b..5c1ad1a2 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,7 @@
+2020-12-16 Érico Nogueira <ericonr@disroot.org>
+
+ * readelf.c (qsort_r): Use qsort for improved portability.
+
2020-12-12 Mark Wielaard <mark@klomp.org>
* elflint.c (check_sections): Handle SHF_GNU_RETAIN.
diff --git a/src/readelf.c b/src/readelf.c
index 829a418d..0001a3d8 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -4831,10 +4831,13 @@ listptr_base (struct listptr *p)
return cudie_base (&cu);
}
+/* To store the name used in compare_listptr */
+static const char *sort_listptr_name;
+
static int
-compare_listptr (const void *a, const void *b, void *arg)
+compare_listptr (const void *a, const void *b)
{
- const char *name = arg;
+ const char *name = sort_listptr_name;
struct listptr *p1 = (void *) a;
struct listptr *p2 = (void *) b;
@@ -4944,8 +4947,11 @@ static void
sort_listptr (struct listptr_table *table, const char *name)
{
if (table->n > 0)
- qsort_r (table->table, table->n, sizeof table->table[0],
- &compare_listptr, (void *) name);
+ {
+ sort_listptr_name = name;
+ qsort (table->table, table->n, sizeof table->table[0],
+ &compare_listptr);
+ }
}
static bool
--
2.29.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r.
2020-12-16 22:30 [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Érico Nogueira
2020-12-16 22:30 ` [PATCH v2 2/2] src/readelf: use qsort instead of qsort_r Érico Nogueira
@ 2020-12-17 0:08 ` Dmitry V. Levin
2020-12-17 4:17 ` Érico Nogueira
1 sibling, 1 reply; 6+ messages in thread
From: Dmitry V. Levin @ 2020-12-17 0:08 UTC (permalink / raw)
To: Érico Nogueira; +Cc: elfutils-devel, Érico Rolim
On Wed, Dec 16, 2020 at 07:30:11PM -0300, Érico Nogueira via Elfutils-devel wrote:
> From: Érico Rolim <erico.erc@gmail.com>
>
> The Linux man pages recommend this version of the function for portable
> applications.
>
> Signed-off-by: Érico Rolim <erico.erc@gmail.com>
I'd rather not do it this way because __xpg_strerror_r in glibc is a
wrapper around GNU strerror_r which does this almost always unneeded
string copying. Instead of penalizing GNU systems, I'd suggest checking
at configure time what kind of strerror_r do we have, and choosing the
code appropriately.
> ---
>
> Only difference from the initial patch is that this includes the
> Signed-off-by line.
>
> libdwfl/ChangeLog | 4 ++++
> libdwfl/dwfl_error.c | 11 ++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> index f9f6f01f..d22f9892 100644
> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,7 @@
> +2020-12-16 Érico Nogueira <ericonr@disroot.org>
> +
> + * dwfl_error.c (strerror_r): Always use the XSI-compliant version.
> +
> 2020-12-16 Dmitry V. Levin <ldv@altlinux.org>
>
> * argp-std.c (_): Remove.
> diff --git a/libdwfl/dwfl_error.c b/libdwfl/dwfl_error.c
> index 7bcf61cc..e5db1217 100644
> --- a/libdwfl/dwfl_error.c
> +++ b/libdwfl/dwfl_error.c
> @@ -30,6 +30,11 @@
> # include <config.h>
> #endif
>
> +/* Guarantee that we get the XSI compliant strerror_r */
> +#ifdef _GNU_SOURCE
> +#undef _GNU_SOURCE
> +#endif
> +
> #include <assert.h>
> #include <libintl.h>
> #include <stdbool.h>
> @@ -136,6 +141,8 @@ __libdwfl_seterrno (Dwfl_Error error)
> global_error = canonicalize (error);
> }
>
> +/* To store the error message from strerror_r */
> +static __thread char errormsg[128];
>
> const char *
> dwfl_errmsg (int error)
> @@ -154,7 +161,9 @@ dwfl_errmsg (int error)
> switch (error &~ 0xffff)
> {
> case OTHER_ERROR (ERRNO):
> - return strerror_r (error & 0xffff, "bad", 0);
> + if (strerror_r (error & 0xffff, errormsg, sizeof errormsg))
> + return "strerror_r() failed()";
> + return errormsg;
> case OTHER_ERROR (LIBELF):
> return elf_errmsg (error & 0xffff);
> case OTHER_ERROR (LIBDW):
What if we had something like this:
static const char *
errnomsg(int error)
{
static const char unknown[] = "unknown error";
#ifdef HAVE_STRERROR_R_POSIX_SIGNATURE
static __thread char msg[128];
return strerror_r (error, msg, sizeof (msg)) ? unknown : msg;
#else
return strerror_r (error, unknown, 0);
#endif
}
and use it in dwfl_errmsg instead of strerror_r?
--
ldv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r.
2020-12-17 0:08 ` [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Dmitry V. Levin
@ 2020-12-17 4:17 ` Érico Nogueira
2020-12-17 19:06 ` Dmitry V. Levin
0 siblings, 1 reply; 6+ messages in thread
From: Érico Nogueira @ 2020-12-17 4:17 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: elfutils-devel, Érico Rolim
On Wed Dec 16, 2020 at 9:08 PM -03, Dmitry V. Levin wrote:
> On Wed, Dec 16, 2020 at 07:30:11PM -0300, Érico Nogueira via
> Elfutils-devel wrote:
> > From: Érico Rolim <erico.erc@gmail.com>
> >
> > The Linux man pages recommend this version of the function for portable
> > applications.
> >
> > Signed-off-by: Érico Rolim <erico.erc@gmail.com>
>
> I'd rather not do it this way because __xpg_strerror_r in glibc is a
> wrapper around GNU strerror_r which does this almost always unneeded
> string copying. Instead of penalizing GNU systems, I'd suggest checking
> at configure time what kind of strerror_r do we have, and choosing the
> code appropriately.
Fair enough. The GNU version of strerror_r does have a nicer API :)
>
> > ---
> >
> > Only difference from the initial patch is that this includes the
> > Signed-off-by line.
> >
> > libdwfl/ChangeLog | 4 ++++
> > libdwfl/dwfl_error.c | 11 ++++++++++-
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> > index f9f6f01f..d22f9892 100644
> > --- a/libdwfl/ChangeLog
> > +++ b/libdwfl/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2020-12-16 Érico Nogueira <ericonr@disroot.org>
> > +
> > + * dwfl_error.c (strerror_r): Always use the XSI-compliant version.
> > +
> > 2020-12-16 Dmitry V. Levin <ldv@altlinux.org>
> >
> > * argp-std.c (_): Remove.
> > diff --git a/libdwfl/dwfl_error.c b/libdwfl/dwfl_error.c
> > index 7bcf61cc..e5db1217 100644
> > --- a/libdwfl/dwfl_error.c
> > +++ b/libdwfl/dwfl_error.c
> > @@ -30,6 +30,11 @@
> > # include <config.h>
> > #endif
> >
> > +/* Guarantee that we get the XSI compliant strerror_r */
> > +#ifdef _GNU_SOURCE
> > +#undef _GNU_SOURCE
> > +#endif
> > +
> > #include <assert.h>
> > #include <libintl.h>
> > #include <stdbool.h>
> > @@ -136,6 +141,8 @@ __libdwfl_seterrno (Dwfl_Error error)
> > global_error = canonicalize (error);
> > }
> >
> > +/* To store the error message from strerror_r */
> > +static __thread char errormsg[128];
> >
> > const char *
> > dwfl_errmsg (int error)
> > @@ -154,7 +161,9 @@ dwfl_errmsg (int error)
> > switch (error &~ 0xffff)
> > {
> > case OTHER_ERROR (ERRNO):
> > - return strerror_r (error & 0xffff, "bad", 0);
> > + if (strerror_r (error & 0xffff, errormsg, sizeof errormsg))
> > + return "strerror_r() failed()";
> > + return errormsg;
> > case OTHER_ERROR (LIBELF):
> > return elf_errmsg (error & 0xffff);
> > case OTHER_ERROR (LIBDW):
>
> What if we had something like this:
>
> static const char *
> errnomsg(int error)
> {
> static const char unknown[] = "unknown error";
>
> #ifdef HAVE_STRERROR_R_POSIX_SIGNATURE
> static __thread char msg[128];
> return strerror_r (error, msg, sizeof (msg)) ? unknown : msg;
> #else
> return strerror_r (error, unknown, 0);
> #endif
> }
Would you prefer this go in lib/ as a "general usage" function or do I
just leave it in libdwfl/ for now?
As a side note, the trick of passing a constant string and buflen=0 is
quite neat, and isn't obvious from reading the man page :)
>
> and use it in dwfl_errmsg instead of strerror_r?
>
>
> --
> ldv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r.
2020-12-17 4:17 ` Érico Nogueira
@ 2020-12-17 19:06 ` Dmitry V. Levin
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry V. Levin @ 2020-12-17 19:06 UTC (permalink / raw)
To: Érico Nogueira; +Cc: elfutils-devel, Érico Rolim
On Thu, Dec 17, 2020 at 01:17:40AM -0300, Érico Nogueira wrote:
> On Wed Dec 16, 2020 at 9:08 PM -03, Dmitry V. Levin wrote:
> > On Wed, Dec 16, 2020 at 07:30:11PM -0300, Érico Nogueira via
> > Elfutils-devel wrote:
> > > From: Érico Rolim <erico.erc@gmail.com>
> > >
> > > The Linux man pages recommend this version of the function for portable
> > > applications.
> > >
> > > Signed-off-by: Érico Rolim <erico.erc@gmail.com>
> >
> > I'd rather not do it this way because __xpg_strerror_r in glibc is a
> > wrapper around GNU strerror_r which does this almost always unneeded
> > string copying. Instead of penalizing GNU systems, I'd suggest checking
> > at configure time what kind of strerror_r do we have, and choosing the
> > code appropriately.
>
> Fair enough. The GNU version of strerror_r does have a nicer API :)
[...]
> > What if we had something like this:
> >
> > static const char *
> > errnomsg(int error)
> > {
> > static const char unknown[] = "unknown error";
> >
> > #ifdef HAVE_STRERROR_R_POSIX_SIGNATURE
> > static __thread char msg[128];
> > return strerror_r (error, msg, sizeof (msg)) ? unknown : msg;
> > #else
> > return strerror_r (error, unknown, 0);
> > #endif
> > }
>
> Would you prefer this go in lib/ as a "general usage" function or do I
> just leave it in libdwfl/ for now?
Is there any more potential users besides dwfl_errmsg? If yes, then it
would make sense to have it in lib/, but then you'd have to come up with a
good name for the function. :)
> As a side note, the trick of passing a constant string and buflen=0 is
> quite neat, and isn't obvious from reading the man page :)
I'd suggest to make it explicitly documented in the manual page.
--
ldv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] src/readelf: use qsort instead of qsort_r.
2020-12-16 22:30 ` [PATCH v2 2/2] src/readelf: use qsort instead of qsort_r Érico Nogueira
@ 2021-01-10 15:37 ` Mark Wielaard
0 siblings, 0 replies; 6+ messages in thread
From: Mark Wielaard @ 2021-01-10 15:37 UTC (permalink / raw)
To: Érico Nogueira, elfutils-devel; +Cc: Érico Rolim
Hi Érico,
Sorry for the late review.
On Wed, 2020-12-16 at 19:30 -0300, Érico Nogueira via Elfutils-devel wrote:
> From: Érico Rolim <erico.erc@gmail.com>
>
> This program is single threaded, so using qsort with a global variable
> isn't a danger. The interface for qsort_r isn't standardized (and
> diverges between glibc and FreeBSD, for example), which makes usage of
> qsort, where possible, preferrable.
I don't really like this one. Currently readelf isn't multi-threaded,
but precisely this, processing of different section data, could in
theory be done in parallel. But it isn't, so for now assuming single-
threadedness is OK. If we make this really parallel, and don't revert
back to using qsort_r, a better solution would be to have (6!)
different sort_listptr compar callback variants.
That said, applied since it does look OK for now.
Cheers,
Mark
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-10 15:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-16 22:30 [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Érico Nogueira
2020-12-16 22:30 ` [PATCH v2 2/2] src/readelf: use qsort instead of qsort_r Érico Nogueira
2021-01-10 15:37 ` Mark Wielaard
2020-12-17 0:08 ` [PATCH v2 1/2] libdwfl: use XSI-compliant strerror_r Dmitry V. Levin
2020-12-17 4:17 ` Érico Nogueira
2020-12-17 19:06 ` Dmitry V. Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).