[DTrace-devel] [PATCH 6/6] Add support for pid function "-" with absolute offset

[Kris Van Hees]

On Thu, Jan 09, 2025 at 04:00:42PM -0500, Eugene Loh wrote:
> On 1/9/25 15:52, Eugene Loh wrote:
> 
> > On 1/9/25 14:12, Kris Van Hees wrote:
> > 
> > > On Fri, Dec 20, 2024 at 05:27:16PM -0500, eugene.loh--- via
> > > DTrace-devel wrote:
> > > > diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> > > > @@ -47,6 +47,7 @@ typedef struct pid_probespec {
> > > >       /*
> > > >        * Fields below this point do not apply to underlying probes.
> > > >        */
> > > > +    int pps_absoff;                /* use "-" for overlying
> > > > probe function */
> > > Why use this?  All you do with it is record that func is "-", and
> > > then later
> > > in the pid provider, you actually use that to force the function
> > > name in the
> > > probe description to be "-".  But that function name in the probe
> > > desc is (in
> > > the pid provider) already coming from pps_fun, and the code was already
> > > assigning func to pps_fun, so it would be "-" anyway.  So the
> > > pps_absoff seems
> > > to serve no purpose, and you could just rely on pps_fun being "-"
> > > when needed.
> > 
> > Yeah, good point.  Let me see if I can remember/understand/explain
> > what's going on here.
> > 
> > In essence, the problem is we have to pass two function names: "-" (for
> > the overlying probe) and the actual function name (for the underlying
> > probe);  up until now, they were both the same.  I tossed around
> > different ideas for this.  E.g., pass two function names in. Or, get the
> > actual function name inside dt_prov_uprobe. It seemed to make most sense
> > (given the alternatives) to pass in the actual function name and add
> > this flag to say when the overlying probe's function name should be
> > "-".  Does that make sense?
> > 
> > Note that pps_fun is not "-".  Although we set it to func ("-"), the
> > code that processes this case then fills pps_fun in with the actual
> > function name.  Again, one option is not to do this until
> > dt_prov_uprobe.c, but that introduces some other hassles.

Ok, I see what you mean...  because we rely on underlying probes, we need to
track the function name that the absolute offset falls in in order to be able
to determine the underlying uprobe that the PID probe maps onto (especially
since there might be another probe on it already).

However, as far as I can see there is not really any need to have a function
name on the underlying probe.  After all, it is actually a uprobe that is
identified by dev/inode/offset where offset is an offset into the mapping at
dev/inode, i.e. not relative to any particular function at all.  So, not
having the function name associated with the underlying probe would make more
sense than what we are doing now.

And if we drop the function name from the underlying probe, we suddenly no
longer need to carry it across for absolute offset probe, and this entire
two-function name thing goes away.

> Hmm.  Having said all that, maybe another (better?!) way of solving this
> problem is to add another probe type to pid_probetype_t.  Maybe
> DTPPT_ABSOFFSETS.  After all, we already have this problem of a "special"
> name for the overlying probe while requiring the "real" name for the
> underlying probe:  we have that problem for probe names.  There, we solve
> the problem by checking the pid_probetype_t.  So maybe that same approach
> should be used here, for probe function.
> 
> Let me know.  If you agree/prefer, I can revise the patch.
> 
> > 
> > > >       pid_t pps_pid;                /* task PID */
> > > >       uint64_t pps_nameoff;            /* offset to use for name */
> > > >   } pid_probespec_t;
> > > > diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> > > > @@ -182,8 +181,50 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const
> > > > GElf_Sym *symp, const char *func)
> > > >       psp->pps_fun = (char *) func;
> > > >       psp->pps_nameoff = 0;
> > > >       psp->pps_off = symp->st_value - pp->dpp_vaddr;
> > > > +    psp->pps_absoff = 0;
> > > >   -    if (!isdash && gmatch("return", pp->dpp_name)) {
> > > > +    /*
> > > > +     * The special function "-" means the probe name is an absolute
> > > > +     * virtual address.
> > > > +     */
> > > > +    if (strcmp("-", func) == 0) {
> > > > +        char *end;
> > > > +        GElf_Sym sym;
> > > > +
> > > > +        off = strtoull(pp->dpp_name, &end, 16);
> > > > +
> > > > +        psp->pps_absoff = 1;
> > > > +        psp->pps_nameoff = off;
> > > > +
> > > > +        if (dt_Plookup_by_addr(dtp, pid, off, (const char
> > > > **)&psp->pps_fun, &sym)) {
> > > > +            rc = dt_pid_error(dtp, pcb, dpr, D_PROC_NAME,
> > > > +                 "failed to lookup 0x%lx in module '%s'", off,
> > > > pp->dpp_mod);
> > > > +            if (psp->pps_fun != func && psp->pps_fun != NULL)
> > > > +                free(psp->pps_fun);
> > > > +            goto out;
> > > > +        }
> > > > +
> > > > +        psp->pps_prb = (char*)pp->dpp_name;  // make sure
> > > > dt_prov_uprobe uses it
> > > > +        psp->pps_off = off - sym.st_value;   // make sure
> > > > dt_prov_uprobe uses it // dump this
> > > > +        psp->pps_off = off - pp->dpp_vaddr;  // make sure
> > > > dt_prov_uprobe uses it
> > > > +
> > > > +        if (dt_pid_create_one_probe(pp->dpp_pr, dtp, psp,
> > > > DTPPT_OFFSETS) < 0)
> > > > +            rc = dt_pid_error(dtp, pcb, dpr, D_PROC_CREATEFAIL,
> > > > +                "failed to create probes at '%s+0x%llx': %s",
> > > > +                func, (unsigned long long)off,
> > > > dtrace_errmsg(dtp, dtrace_errno(dtp)));
> > > > +        else
> > > > +            pp->dpp_nmatches++;
> > > > +        free(psp->pps_fun);
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (gmatch("return", pp->dpp_name)) {
> > > >           if (dt_pid_create_one_probe(pp->dpp_pr, dtp, psp,
> > > > DTPPT_RETURN) < 0) {
> > > >               rc = dt_pid_error(
> > > >                   dtp, pcb, dpr, D_PROC_CREATEFAIL,
> > > > diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> > > > index acf1c914f..ae4b262ac 100644
> > > > --- a/libdtrace/dt_prov_uprobe.c
> > > > +++ b/libdtrace/dt_prov_uprobe.c
> > > > @@ -735,7 +735,7 @@ static int provide_probe(dtrace_hdl_t *dtp,
> > > > const pid_probespec_t *psp,
> > > >       pd.id = DTRACE_IDNONE;
> > > >       pd.prv = prv;
> > > >       pd.mod = psp->pps_mod;
> > > > -    pd.fun = psp->pps_fun;
> > > > +    pd.fun = psp->pps_absoff ? "-" : psp->pps_fun;
> > > >       pd.prb = prb;
> > > >         /* Get (or create) the provider for the PID of the probe. */