[DTrace-devel] [PATCH 2/7] htab reduction: providers

[Nick Alcock]

On 8 Oct 2021, Eugene Loh spake thusly:

> A nit:  in dt_provider_lookup(), these function names should be flush 
> against their parentheses:
>      strlen (name)
>      sizeof (tmpl.desc.dtvd_name)
>      strcpy (tmpl.desc.dtvd_name, name)
> For stylistic purposes, DTrace-on-BPF applies this rule to sizeof().  
> See CODING-STYLE.

augh I meant to fix that. Sorry, fixed. (I still think this rule is
dreadfully inconsistent, and either it should apply to if/while/etc too,
or to nothing. The GNU style is far better here. Still this style is
older than I am, so it's not changing now...)

There is one instance of this in the parser changes for libctf and one
in the speculations stuff too. I'll fix the lot in one go once it's all
applied.

> Otherwise, I have the same high-level comments as before:

I'd fix it the same way (and mostly have, moving to a destructor so you
don't need to hand-free providers when freeing the htab), but...

> *)  Instead of maintaining dtp->dt_provlist, should we just have dt_htab 
> provide an iterator?

The provlist is used elsewhere, in dt_pcb_pop, to destroy not only
providers but any associated probes, and because we only destroy probes
created at the same dt_pcb_push/pop level, we can't push that down into
a dt_provider-level destructor: it's pcb-specific.

... it turns out that *not* implementing an iterator is dead annoying,
because we can't remove from the dt_provlist in the prov htab
destructor, so the constructor and destructor become asymmetric.

So I've implemented an iterator, in the libctf _next style because it's
*so much* nicer to use than the function-calling _iter form. It is only
tested insofar as building dtrace does a bunch of provider iteration so
might still have bugs, but is surely fixable because I've been using
this style of iterators for ages in libctf and it's simply lovely to
use:

extern void *dt_htab_next(const dt_htab_t *htab, dt_htab_next_t **it);
extern void dt_htab_next_destroy(dt_htab_next_t *i);
extern void dt_htab_next_del(dt_htab_next_t *i);

Call dt_htab_next with the htab to iterate over and a dt_htab_next_t
initialized to NULL. It allocates an iterator, returns new elements from
the hash until iteration is complete, then frees the iterator and
re-annuls it, ready for another iteration cycle. If you need to exit
early, dt_htab_next_destroy frees the iterator for you (it's just a
free() for now, but see binutils-gdb/libctf/ctf-util.c:ctf_next_destroy
for how this can change over time if your iterator family can iterate
over many different things, like the ctf_next_t can.)

There are no restrictions whatsoever on what you can do inside iteration
(iterate over other things, fork, longjmp out of it, you name it) except
for deleting the hash entry you're iterating over: for that, we have
dt_htab_next_del. (You might repeat some elements you've already seen
after calling this, but for most use cases I can think of this is much
safer than missing some. This is not a fundamental limitation: I just
need to add a .prev htab operation, symmetric with .next, to fix it. For
now this feels unnecessary.)

Downside: iteration over providers is no longer in insertion order (but
is still deterministic). Does anyone care? If so, this too should be
fixed by adding an ordered htab with guaranteed iteration order (which
is easy).

This is probably the most debatable set of changes I've done so far :P

> *)  Instead of maintaining dtp->dt_nprovs, how about destroying the htab 
> with an iterator or with a function that calls back a destructor?

The destructor approach is definitely preferable, even if we sometimes
also want to destroy only a subset of provs (as in dt_pcb).

Unfortunately, the lack of an easily-available dtp pointer means I need
to move more and more towards free() rather than dt_free(). What a
SHAME. dt_free should die in a fire. I'm tired of adding pointless dtp
back-pointers to things for no reason other than to placate it, dammit.