Re: [PATCH v2 03/19] Deprecate enabled probe ID (epid)

[Kris Van Hees <kris.van.hees@oracle.com>]

On Sun, Sep 08, 2024 at 12:31:12PM -0400, Eugene Loh wrote:
> On 9/5/24 23:31, Kris Van Hees wrote:
> 
> > Great work!  Some comments below...
> > 
> > Note that with this work, we are breaking the libdtrace API
> 
> Okay, but perhaps it was already broken, at least through disuse and
> changing ioctl stuff???

There probably were already smoe breakage changes - I am actually not sure.
But this definitely is one - but as I wrote, I don't think we should really
care too much right now.

> I'll post a revised patch (and small revisions of two other patches due to
> the index/id naming change).  In a few cases, however, I comment below:
> 
> >   - but that might
> > be a small price to pay for cleaning up some needed things.  However, that
> > also means that we could just break it a tiny bit more and make things easier,
> > perhaps.  (Yes, that goes a bit against my earlier advice but it really makes
> > no sense (in retrospect) to keep supporting something that we are breaking in
> > others ways anyway.)
> > 
> > See below for details...
> > 
> > On Tue, Sep 03, 2024 at 01:16:43AM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > However, its value was opaque and therefore difficult to use.
> > I would drop this sentence.  I think the preceding paragraph sufficiently
> > describes it.  And you risk confusing the reader concerning whether the epipd
> > was difficult to use in the dtrace internals or as a user (who won't read this
> > anyway).
> 
> Difficult for the user, but even if a user doesn't read this text the point
> still matters for the reader.  I think it's worth building the case for
> deprecating something that, after all, was part of D. Noting that the value
> to the user was poor helps build this case.  I added a couple of words to
> clarify "to the user."
> 
> > > Deprecate the use of EPID:
> > > 
> > > *)  So we rearrange as follows:
> > > 
> > >              old                        new
> > > 
> > >           0: pad (size)                 pad (size) <= buffer start
> > >           4: pad (additional)           specid
> > >           8: epid <= buffer start       prid
> > >          12: specid                     stid
> > >          16: data[n]                    data[n]
> > > 
> > >      So now, we say there is no longer an 8-byte pad before the
> > >      buffer start;  rather, the buffer starts with a 4-byte pad.
> > I am not sure whether this layout comparison is comprehensible to anyone not
> > familiar with this code.
> 
> Yeah.  Incidentally, I had moved specid so that prid/stid were in the same
> 8-byte block so that someone could read it as a single 8-byte EPID if they
> wanted to.  I'm assuming you see no value in that.  So I moved specid back
> to where it used to be and I think this simplifies the description of what's
> going on.  Anyhow, I revised this explanation.  Hopefully it's sufficient
> now.

I like the specid first actually :)  Since it reflects the designation of the
data.

> > It may need a bit more explanation concerning how
> > this is used.  E.g. the size that is referred to above is a 32-bit integer
> > emitted by the perf ring buffer code, giving the size of the data blob that
> > follows.  And this is the reason that we need to do this little shuffle with
> > the padding because (1) data needs to be written to the temporary buffer
> > storage with proper alignment, (2) data should be aligned in the ring buffer,
> > and (3) the perf event header + this size results in the data starting at
> > a multiple of 8 + 4, i.e. not a 8-byte boundary.
> > 
> > > diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> > > @@ -156,12 +157,12 @@ dt_clause_create(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
> > >   	/*
> > >   	 * Generate a symbol name.
> > >   	 */
> > > -	len = snprintf(NULL, 0, "dt_clause_%d", dtp->dt_clause_nextid) + 1;
> > > +	len = snprintf(NULL, 0, "dt_clause_%d", sdp->dtsd_index) + 1;
> > >   	name = dt_alloc(dtp, len);
> > >   	if (name == NULL)
> > >   		longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> > > -	snprintf(name, len, "dt_clause_%d", dtp->dt_clause_nextid++);
> > > +	snprintf(name, len, "dt_clause_%d", sdp->dtsd_index);
> > Here (and elsewhere) this should be replaced by:
> > 
> > 	if (asprintf(&name, "dt_clause_%d", sdp->dtsd_id) == -1)
> > 		longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> 
> Hmm.  But:
> 1)  These were pre-existing constructs.
> 2)  The other other two sites used alloca() and handling the free()s would
> require a bit more rearranging.
> 
> So, okay, I updated this case but left the other two cases alone.  I don't
> object to asprintf() but those other two cases would seem to be material for
> their own patch.

OK

> > >   	/*
> > >   	 * Add the symbol to the BPF identifier table and associate the DIFO
> > > diff --git a/libdtrace/dt_map.c b/libdtrace/dt_map.c
> > > @@ -85,98 +85,16 @@ dt_datadesc_finalize(dtrace_hdl_t *dtp, dtrace_datadesc_t *ddp)
> > > -void
> > > -dt_epid_destroy(dtrace_hdl_t *dtp)
> > > -{
> > > -	size_t i;
> > > -
> > > -	assert((dtp->dt_pdesc != NULL && dtp->dt_ddesc != NULL &&
> > > -	    dtp->dt_maxprobe > 0) || (dtp->dt_pdesc == NULL &&
> > > -	    dtp->dt_ddesc == NULL && dtp->dt_maxprobe == 0));
> > > -
> > > -	if (dtp->dt_pdesc == NULL)
> > > -		return;
> > > -
> > > -	for (i = 0; i < dtp->dt_maxprobe; i++) {
> > > -		if (dtp->dt_ddesc[i] == NULL) {
> > > -			assert(dtp->dt_pdesc[i] == NULL);
> > > -			continue;
> > > -		}
> > > -
> > > -		dt_datadesc_release(dtp, dtp->dt_ddesc[i]);
> > > -		assert(dtp->dt_pdesc[i] != NULL);
> > > -	}
> > > -
> > > -	free(dtp->dt_pdesc);
> > > -	dtp->dt_pdesc = NULL;
> > > +	dtrace_difo_t *rdp = dt_dlib_get_func_difo(dtp, dtp->dt_stmts[stid]->dtsd_clause);
> > > +	*ddp = dt_datadesc_hold(rdp->dtdo_ddesc);             // FIXME what releases the hold?
> > Well, you are the one requesting the hold, so you decide when it is to be
> > released :-)
> > 
> > In all seriousness, I believe dtrace_stmt_destroy() is the place you are
> > looking for.
> 
> Actually, this is a short-lived usage by the consumer.  So stmt_destroy()
> would not seem to be the right place and, indeed, I just decided to drop the
> hold/release altogether.

Yes, that shouold be ok.

> > > -	free(dtp->dt_ddesc);
> > > -	dtp->dt_ddesc = NULL;
> > > -	dtp->dt_nextepid = 0;
> > > -	dtp->dt_maxprobe = 0;
> > > +	return (*ddp == NULL) ? -1 : 0;
> > >   }
> > >   uint32_t
> > > diff --git a/test/unittest/actions/setopt/tst.badopt.r b/test/unittest/actions/setopt/tst.badopt.r
> > > index 29e39fd4..e2f6d2c3 100644
> > > --- a/test/unittest/actions/setopt/tst.badopt.r
> > > +++ b/test/unittest/actions/setopt/tst.badopt.r
> > > @@ -1,16 +1,16 @@
> > >   -- @@stderr --
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > Perhaps it would be more efficient to update runtest.sh to support both the
> > old and the new error reporting output, and changing both into a single unified
> > form that substitutes changeable elements.
> 
> I don't get this.  First, there is no reason to support the old output...
> that would be a rather remarkable historical vestige to keep in the code
> base.  Second, it seems simpler and more stringent just to compare to a
> results file in some simple, straightforward way.  You say "perhaps" so I
> just left it as is.

I was more thinking about making the output we compare less dependent on how
things are phrased so that we do not have to update .r files whenever there is
a change in how we output the data.  Except for a test or two that verify the
exact phrasing we expect.

> > E.g.
> > dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > 
> > and
> > 
> > dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > 
> > could both be rewritten using pattern amtching and substitutions to be:
> > 
> > dtrace: error for probe (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > 
> > and tests that are supposed to exercise specific aspects of the error message
> > or that depend on specific details could test that explicitly?
> > 
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Harding" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Harding" to "1": Invalid option name
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Hoover" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Hoover" to "1": Invalid option name
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Bush" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Bush" to "1": Invalid option name
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "quiet" to "um, no": Invalid value for specified option
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "quiet" to "um, no": Invalid value for specified option
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "aggrate" to "0.5hz": Invalid value for specified option
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "aggrate" to "0.5hz": Invalid value for specified option
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "bufsize" to "1m": Operation illegal when tracing is active
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "bufsize" to "1m": Operation illegal when tracing is active