[DTrace-devel] [PATCH 16/16] PID provider implementation

[Eugene Loh]

On 3/23/21 5:53 PM, Kris Van Hees wrote:

> On Mon, Mar 22, 2021 at 10:52:36PM -0400, Eugene Loh wrote:
>> On 3/19/21 12:55 AM, Kris Van Hees wrote:
>>> +	for (pprp = dt_list_next(&pp->probes); pprp != NULL;
>>> +	     pprp = dt_list_next(pprp)) {
>>> +		uint_t	lbl_next = dt_irlist_label(dlp);
>>> +		pid_t	pid = strtoul(pprp->desc->prv + 3, NULL, 10);
>>> +
>>> +		/*
>>> +		 * Check whether this pid-provider probe serves the current
>>> +		 * process.
>>> +		 */
>>> +		emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, pid, lbl_next));
>>> +		dt_cg_tramp_call_clauses(pcb, pprp, DT_ACTIVITY_ACTIVE);
>>> +		emitl(dlp, lbl_next,
>>> +			   BPF_NOP());
>>> +	}
>> The main reason for this email is a question I had about the pid
>> trampoline.  We enter that loop with %r0 holding the firing pid.  We
>> then loop over probes, comparing %r0 with the probe pid.  That's great,
>> but if ever the firing and probe pids match, then we call the code
>> that's generated in dt_cg_tramp_call_clauses().  That generated BPF code
>> taints %r0, which will no longer contain the firing pid. Then, the next
>> %r0!= test will not be doing what we want it to do.
> Well, it is actually really doing what it is meant to do...  This is a
> composite conditional (if { ... } else if { ... } else if { ... } and so
> on, so a chain where only one conditional will ever evaluate as true, and
> it is possible none will).  But it is cleaner to add an explicit jump to
> the trampoline exit label, and that also avoids evaluating subsequent
> conditionals that we know won't match anyway.

I think we're squared away on this, but I'll add one clarification as I clean out my inbox.
The code was implementing:

	%r0 = mypid
	if (%r0 == pid1) call_clauses(...)
	if (%r0 == pid2) call_clauses(...)
	if (%r0 == pid3) call_clauses(...)
	if (%r0 == pid4) call_clauses(...)
	if (%r0 == pid5) call_clauses(...)
	if (%r0 == pid6) call_clauses(...)

The thing is, if any of those %r0==pid matches, you do call_clauses(...) and that changes the value of %r0.  So the remaining conditionals are no longer checking the condition they were supposed to be checking.

Anyhow, I think the value of %r0 changes to %r0=0.  And in any case your proposal to jump to the tramp exit label resolves all the issues.