Hi Aaron, On Thu, 2025-05-22 at 18:28 -0400, Aaron Merey wrote: > Add new internal static library libthread.a that provides infrastructure > for eu-* tools to run functions concurrently using pthreads. > > threadlib.c manages per-job threads as well as per-job buffers for stdout > output. Output for each job is printed to stdout in the order that the > jobs were added to the job queue. This helps preserve the order of > output when parallelization is added to an eu-* tool. > > threadlib.h declares functions add_job and run_jobs. Jobs are added to > a threadlib.c internal job queue using add_job. run_jobs concurrently > executes jobs in parallel.
It isn't stated anywhere explicitly but I think this means all jobs must be added first with add_job and then when run_jobs will be called no new jobs can be added (concurrently)? I assumed this while reviewing, because if add_job can be called after run_jobs has already started by head would explode :} Could you document this and/or maybe even add an assert somewhere? > eu-readelf now links against libthread.a when elfutils is configured > with --enable-thread-safety. > > * src/Makefile.am: libthread.a is compiled and and linked with > readelf when USE_LOCKS is defined. > * src/threadlib.c: New file. Manages job creation, concurrent > execution and output handling. > * src/threadlib.h: New file. Declares functions add_job and > run_jobs. > > Signed-off-by: Aaron Merey <ame...@redhat.com> > > --- > v2: > Remove unnecessary thread_LDADD variable in src/Makefile.am. Minor comment > rewordings. > > src/Makefile.am | 16 +++ > src/threadlib.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++ > src/threadlib.h | 34 +++++++ > 3 files changed, 302 insertions(+) > create mode 100644 src/threadlib.c > create mode 100644 src/threadlib.h > > diff --git a/src/Makefile.am b/src/Makefile.am > index 4a3fb957..90f2d55f 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -52,6 +52,19 @@ libar.manifest: $(libar_a_OBJECTS) > MOSTLYCLEANFILES = *.gconv > CLEANFILES = $(bin_SCRIPTS) $(EXTRA_libar_a_DEPENDENCIES) > > +if USE_LOCKS > +noinst_LIBRARIES += libthread.a > + > +libthread_a_SOURCES = threadlib.c > + > +EXTRA_DIST += threadlib.h I think this should be be added to noinst_HEADERS which isn't defined yet in this Makefile.am, so it probably should be: noinst_HEADERS = threadlib.h Please double check with make distcheck and/or make rpmbuild > +libthread.manifest: $(libthread_a_OBJECTS) > + $(AM_V_GEN)echo $^ > $@ > + > +CLEANFILES += $(EXTRA_libthread_a_DEPENDENCIES) > +endif EXTRA_libthread_a_DEPENDENCIES doesn't seem to be defined anywhere, should it be defined first as: EXTRA_libtreahd_a_DEPENDENCIES = libthread.manifest ? > + > if BUILD_STATIC > libasm = ../libasm/libasm.a > libdw = ../libdw/libdw.a -lz $(zip_LIBS) $(libelf) -ldl -lpthread > @@ -91,6 +104,9 @@ ar_no_Wstack_usage = yes > unstrip_no_Wstack_usage = yes > > readelf_LDADD = $(libdw) $(libebl) $(libelf) $(libeu) $(argp_LDADD) > +if USE_LOCKS > +readelf_LDADD += libthread.a > +endif > nm_LDADD = $(libdw) $(libebl) $(libelf) $(libeu) $(argp_LDADD) > $(obstack_LIBS) \ > $(demanglelib) > size_LDADD = $(libelf) $(libeu) $(argp_LDADD) OK. > diff --git a/src/threadlib.c b/src/threadlib.c > new file mode 100644 > index 00000000..ab158b69 > --- /dev/null > +++ b/src/threadlib.c > @@ -0,0 +1,252 @@ > +/* Functions for running jobs concurrently. > + Copyright (C) 2025 Red Hat, Inc. > + 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/>. */ > + > +#ifdef HAVE_CONFIG_H > +# include <config.h> > +#endif > + > +#include <error.h> > +#include <stdbool.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <stdatomic.h> > + > +#include "threadlib.h" > + > +/* Dynamic buffer for thread output. */ > +typedef struct { > + size_t sizeloc; > + char *buf; > + FILE *file; > +} output_stream_t; > + > +/* Allocate resources for STREAM. */ > +static void > +init_thread_output_stream (output_stream_t *stream) > +{ > + stream->buf = NULL; > + stream->sizeloc = 0; > + stream->file = open_memstream (&(stream->buf), &(stream->sizeloc)); > + > + if (stream->file == NULL) > + error (1, 0, _("cannot open thread output stream")); > +} > + > +/* Print and deallocate resources for STREAM. */ > +static void > +print_thread_output_stream (output_stream_t *stream) > +{ > + /* fclose may update stream->buf. */ > + if (fclose (stream->file) != 0) > + error (1, 0, _("cannot close thread output stream")); > + > + printf ("%s", stream->buf); > + free (stream->buf); > +} I think it would be slightly more efficient to use fwrite here. printf "%s" has to first look for the end of buf. And you do have the stream- >sizeloc already. Sorry for stopping the review here suddenly. Will continue later, but hopefully this is already helpful. Cheers, Mark
