On Mon, Dec 30, 2024 at 08:03:38AM +0100, Patrick Steinhardt wrote:
> > - we could turn it on only for LSan builds. But that would break
> > builds on non-Linux platforms (like macOS) that otherwise should
> > support sanitizers.
>
> Mh. I'm not a huge fan of having extra code for just a subset of our
> builds and think that having the code generally enabled on platforms
> that support it is preferable to reduce the number of build variants.
> But...
>
> > - we could trigger only on the combination of Linux and LSan together.
> > This isn't too hard to do, but the uname check isn't completely
> > accurate. It is really about what your libc supports, and non-glibc
> > systems might not have it (though at least musl seems to).
> >
> > So we'd risk breaking builds on those systems, which would need to
> > add a new knob. Though the upside would be that running local "make
> > SANITIZE=leak test" would be protected automatically.
>
> ... this is a fair remark. So I dunno.
Yeah. I do not like having the behavior differ only for LSan, or only on
Linux. But I also do not like having tricky threading code that we do
not otherwise need in all of the other builds. So it is really about
picking the least-bad option, and they all seem similarly bad to me. So
I picked the one that involved writing the least amount of code. ;)
> Okay. The Meson equivalent would be:
> [...]
Yeah, I figured it would need something similar (but for our CI it does
not yet matter). Do you want to prepare that as a patch on top? (Though
also see the message I'm about to send that we might be able to avoid
this series entirely!).
> > +#ifdef THREAD_BARRIER_PTHREAD
> > +#define maybe_thread_barrier_t pthread_barrier_t
> > +#define maybe_thread_barrier_init pthread_barrier_init
> > +#define maybe_thread_barrier_wait pthread_barrier_wait
> > +#define maybe_thread_barrier_destroy pthread_barrier_destroy
> > +#else
> > +#define maybe_thread_barrier_t int
>
> Out of curiosity: why did you pick a define here and not a typedef?
That's what we do for all of the NO_PTHREADS fallbacks, so I was just
following that style. I suspect it matters more there, because you would
not want to typedef pthread_t on a system that is building with
NO_PTHREADS but actually does define that type (whereas the "maybe"
variants are our own invention, so we can be confident those names won't
conflict).
I don't think it matters too much either way.
> > +static inline int maybe_thread_barrier_init(maybe_thread_barrier_t *b UNUSED,
> > + void *attr UNUSED,
> > + unsigned nr UNUSED)
> > +{
> > + errno = ENOSYS;
> > + return -1;
> > +}
> > +#define maybe_thread_barrier_wait(barrier)
> > +#define maybe_thread_barrier_destroy(barrier)
>
> So the way these wrappers are implemented it is not possible to check
> for errors of `pthread_barrier_init()` et al. When the implementation
> exists we do have return codes, but if it's stubbed out we don't.
Yeah, again I was following the NO_PTHREADS fallbacks defined above.
Perhaps those are different in that we wouldn't generally expect to ever
call them if we don't have threads at all (whereas these "maybe" ones
are meant to be quiet noops).
> I think we should align these two implementations so that it does become
> possible to check for errors, or otherwise we wouldn't be using the
> pthread APIs correctly. It does raise the question though whether we
> should really return `-1` in the stubbed-out variant or whether we
> should instead pretend as if things were alright.
You'd have to return a fake success if you expect to check errors, I'd
think. Unless you want to introduce a bunch of conditional code to say
"well, we tried to initialize a barrier but it didn't work, so let's not
actually wait".
For all of the existing pthread calls, we simply assume init stuff like
pthread_mutex_init() doesn't fail. Possibly we should change that
globally, but this is just following the same pattern.
> An alternative would be to die in case the pthread-functions return an
> error.
That's certainly akin to xmalloc(). But probably we don't want to go
that direction, simply because it works against libification in the long
run.
-Peff
