[DTrace-devel] [PATCH 12/15] dlib: Ensure dt_dlib_add_sym_id() does not add duplicates

[Kris Van Hees]

On Thu, Feb 23, 2023 at 01:10:26PM +0000, Nick Alcock wrote:
> On 23 Feb 2023, Kris Van Hees via DTrace-devel verbalised:
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> 
> Reviewed-by: Nick Alcock <nick.alcock at oracle.com>
> 
> (with semantic niggle below)
> 
> > ---
> >  libdtrace/dt_dlibs.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> > index e3196b63..eea45d8b 100644
> > --- a/libdtrace/dt_dlibs.c
> > +++ b/libdtrace/dt_dlibs.c
> > @@ -152,6 +152,7 @@ dt_dlib_get_xsym(dtrace_hdl_t *dtp, const char *name, int kind)
> >  
> >  /*
> >   * Add a BPF identifier of a given kind with given id.
> > + * If the identifier already exists (with the proper id, if any), return it.
> >   */
> >  static dt_ident_t *
> >  dt_dlib_add_sym_id(dtrace_hdl_t *dtp, const char *name, int kind, uint_t id)
> > @@ -159,6 +160,10 @@ dt_dlib_add_sym_id(dtrace_hdl_t *dtp, const char *name, int kind, uint_t id)
> >  	dt_idhash_t	*dhp = dtp->dt_bpfsyms;
> >  	dt_ident_t	*idp;
> >  
> > +	idp = dt_dlib_get_xsym(dtp, name, kind);
> > +	if (idp != NULL && id != DT_IDENT_UNDEF && idp->di_id == id)
> > +		return idp;
> 
> ... if an id is already provided, and is different from what's in the
> entry in the symtab, we add another one (with, no doubt, yet another id)
> and return it, rather than looking up the symbol with the right id, and
> returning that? (And future calls would go even more wrong since now we
> have duplicate entries and dt_dlib_get_xsym is only ever going to return
> one of them, so we might get duplicates added with the *same* id if this
> is called again with the exact same args).
> 
> Not really sure that particular behaviour is beneficial unless adding
> duplicate symbols with different ids, and then more with identical ids,
> is something we really want to do.

Well, yes, there are still potential semantic issues but it requires more
analysis what other cases we need to cover.  Most of the code that uses these
symbols actually works with the identifier pointer rather than doing lookups
all the time, so in the end, it is unclear whether duplicate symbols with
explicitly assigned ids that differ is expected to work or not.  I didn't
want to go down that road just yet with this patch because the issue at hand
was really the addition of true duplicates (same name, same id) and undefined
id duplicates.

So, I expect we'll be doing more work in this area in the future to firm up
what should work and what should not.