Re: [DTrace-devel] [PATCH v4 3/5] cg: add argument mapping in the trampoline

[Nick Alcock <nick.alcock@oracle.com>]

On 2 Nov 2024, Kris Van Hees uttered the following:

> On Fri, Nov 01, 2024 at 03:57:10PM +0000, Nick Alcock via DTrace-devel wrote:
>> +	/*
>> +	 * Work from saved args so we don't need to worry about overwriting regs
>> +	 * we're about to map into different positions.
>> +	 */
>> +	dt_cg_tramp_save_args(pcb);
>
> I still maintain that this should be done in the caller.  (See patch 4/5 for
> more info on that.)  As mentioned before, you can mention in the comment before
> this function that the caller must save the arguments, just like you already
> mention that %r7 must contain a value set by dt_cg_tramp_prologue*().

I think this is gross and just asking for trouble later, but fixed for
the sake of avoiding argument.

>> @@ -5060,7 +5089,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>  	 * array index according to the static argument mapping (if any),
>>  	 * unless the argument reference is provided by a dynamic translator.
>>  	 * If we're using a dynamic translator for args[], then just set dn_reg
>> -	 * to an invalid reg and return: DIF_OP_XLARG will fetch the arg later.
>
> You dropped the end of a sentence without replacing it with anything?

Argh! Fixed.

>> +	 *
>> +	 * If dt_cg_tramp_map_args is in use, you should apply the mapping at
>> +	 * the probe_info stage, since the effective "native" arg positions will
>> +	 * be changed.
>
> This does not belong here.  If anything, clauses get compiled separete from
> trampolines and a given clause (compiled once) may get used in more than one
> BPF program, i.e. it may get associated with more than one trampoline.  So
> you can't really depend on what a specific trampoline does when generating code
> for a clause.

... well, we *do*. All over the place.

But instructions to the caller don't really belong here in any case:
dropped. dt_cg_tramp_map_args's header comment already notes it, and
that's the right place for it.

-- 
NULL && (void)