[DTrace-devel] [PATCH 4/6] cg: add argument remapping in the trampoline

[Nick Alcock]

On 25 Oct 2024, Kris Van Hees said:

> On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
>> > However, I think it might be a good idea to have this function actually rewrite
>> > the mapping data to become a pure i -> i mapping.
>> 
>> Can we guarantee that trampoline() is only ever invoked once, and that
>> it's invoked after probe_info is called? If it changes the remapping
>> array, any subsequent invocation will do the wrong thing. If it's
>> invoked before probe_info, it becomes impossible for probe_info to
>> adjust the xlated types appropriately.
>> 
>> This all seems like way too much coupling to me. At least done
>> this way trampoline() is idempotent and doesn't depend on being called
>> in the right (undocumented) order wrt probe_info.
>
> If the actual probe arguments (argN) are being reshuffled (which is what you
> are doing by shuffling the arguments in the trampoline), then the mapping
> needs to be updated to become an identity mapping i -> i).  Otherwise there
> could be other code at some point that tries to apply the argument mapping
> and that would be really bad.

OK so I tried to implement this and it all fell over as soon as I
thought about it in depth. The problem is -- and you might not have
realised this because it was different when I started and I had to move
things around before anything would work -- trampoline() and
probe_info() are functions defined on *different probes*. trampoline()
is called on the underlying probes, and emits code for them that
reshuffles the args according to the mapping info (while ignoring the
types entirely); but probe_info() is called on the *overlying* probes.
It digs down to *one* underlying probe to get the dt_uprobe_t, but it
has no idea which one (if there is more than one): it's basically
arbitrary.

So we cannot possibly have the trampoline() for the underlying probe go
changing the dt_uprobe_t's mapping unless we can be absolutely sure that
the trampoline() for all the underlying probes is called before the
probe_info() for *any* of the overlying probes. Are we really sorting
things like that? I thought this was a TODO, if that...


I tried storing the arg info in dt_argdesc_t structures in the
dt_uprobe_t too and it rapidly turned into *another* nightmare, because
now instead of one allocation for all the args at once we have one
allocation per arg, plus up to two more for its arg type strings -- but
we can't just hand the argdesc back to probe_info()s caller wholesale
because it frees it after it's done with it! So we'd actually have to
loop through them and copy the whole thing (including all the arg
strings) and then do the *same thing* in the underlying probe freeing
function, and all this complexity buys us what? The ability to save four
lines in probe_info() going through the nargv so we can index it like an
array (which we'd have to do the nargc *and* xargc in your scheme).

Sorry, this seems again to have zero benefits and a great deal of extra
complexity.

-- 
NULL && (void)