Hi, On Thu, 2019-11-07 at 12:13 -0600, Jonathon Anderson wrote: > On Thu, Nov 7, 2019 at 18:20, Mark Wielaard <m...@klomp.org> wrote: > > Do we really need this? > > We already use __thread unconditionally in the rest of the code. > > The usage of threads.h seems to imply we actually want C11 > > _Thread_local. Is that what you really want, or can we just use > > __thread in libdw_alloc.c for thread_id? > > We don't really need it, I just got in the habit of writing > thread_local (and, proper C11 compat). __thread is perfectly fine > for thread_id.
Great, removed. > > I think if you include helgrind.h you won't get the drd.h > > ANNOTATE_HAPPENS_BEFORE/AFTER. So do you also need to include > > drd.h? > > Not really, just another habit. Since this is file only needs > HAPPENS_* helgrind.h is sufficient. Thanks. drd.h include removed. > > > > > +#else > > > +#define ANNOTATE_HAPPENS_BEFORE(X) > > > +#define ANNOTATE_HAPPENS_AFTER(X) > > > +#endif > > > > Could you explain the usage of the happens_before/after annotations in > > this code. I must admit that I don't fully understand why/how it works > > in this case. Specifically since realloc might change the address that > > mem_tails points to. > > Reader-writer locks ensure no "readers" are present whenever a "writer" > is around. In this case we use the "write" side for resizing mem_tails > and the "read" side when mem_tails needs to stay stable. Which is why > most of the time we have a read lock and then promote to a write lock > when we need to reallocate. > > The annotations are to clean up a minor deficiency in Helgrind: for > whatever reason if you do writes under a read lock it reports races > with the writes from under the write lock (in this case, > __libdw_allocate and the realloc). I haven't dug deep enough to know > exactly why it happens, just that it does and adding this H-B arc seems > to fix the issue. OK, lets keep them in for now. They are disabled by default anyway. For now people who want a "helgrindable" libdw will need to rebuild libdw with them enabled. > > > +#define THREAD_ID_UNSET ((size_t) -1) > > > +static thread_local size_t thread_id = THREAD_ID_UNSET; > > > +static atomic_size_t next_id = ATOMIC_VAR_INIT(0); > > > > OK, but maybe use static __thread size_t thread_id as explained > > above? > > Fine by me. Done. > > O, and I now think you would then also need something for dwarf_begin > > to reset any set thread_ids... bleah. So probably way too complicated. > > So lets not, unless you think this is actually simple. > > Which is why I didn't want to do that. > > The other option was to have a sort of free list for ids, but in that > case the cleanup isn't great (sometime after all threads have > completed... if you consider detached threads things get hairy). Plus > it requires a fully concurrent stack or queue, which is a complicated > data structure itself. Yeah, agreed, lets keep it with a simple monotonically increasing next_id. Things need to get really big before this ever gets a problem. And I don't think programs will keep spawning new threads and using Dwarfs on each of them anyway. I expect longer running processes that do need to handle Dwarfs in a concurrent fashion to use thread pools. Pushed with the small changes noted above. Thanks, Mark
