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

[Eugene Loh]

On 1/10/25 00:07, Kris Van Hees wrote:

> 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.

Right.  I considered that, but decided against it.  I think the reason 
was that, although we don't really need a function name for naming the 
underlying uprobe (as you point out), it's nice to have the function for 
the whole "USDT ignore clause" stuff.  Put another way, we *do* use the 
function name of the underlying probe... not so much for naming the 
uprobe but for an extra USDT ignore_clause() check.  If we wanted to, we 
could get rid of that check, but... well, that's the trade-off that 
we're facing here.

>> 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.