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

[Nick Alcock]

On 25 Oct 2024, Kris Van Hees verbalised:
> On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
>> > I also disagree that the function performs the saving of arguments itself.
>> > I think that the caller ought to take care of that, because the caller has a
>> > much better idea about when/if to save arguments, and when/if to restore them.
>> 
>> Well, the reshuffling is *done* by saving arguments and then copying
>> them out of the saved orig_arg area into the args area, so either it
>> does the saving itself, or it silently malfunctions if the caller
>> doesn't save, or it reimplements all the saving infrastructure for no
>> other reason than to use a *different name* so the caller can save.
>> 
>> The latter two options seem, well, entirely terrible, so I went the
>> former route.
>
> The way this has been implemented already for SDT and the design behind that
> is that the underlying probe saves arguments before overlying probes do
> anything with them.  The reshuffling is only one of the things that might
> get done to arguments, and since there may be multiple overlying probes that
> have different arguments, the caller needs to preserve the arguments it was
> given in case any of the overlying ones changes them.

... that doesn't help me figure out what you want me to do. Can I save
the args and then pick them out in a reshuffled fashion, or what? I'm
trying to avoid having to write some sort of nightmarish in-place
shuffler in raw BPF here, because I'm fairly sure doing so would drive
me quite mad and take forever. Saving all the args and picking them out
in the reshuffled order is only a couple of lines. (See below.)

I still have no idea what's wrong with saving the args in
dt_cg_tramp_remap_args(): it will only cause trouble if something else
has saved the args before trampoline() is called, then modified them,
then expects to restore the originals after the trampoline(). Surely
nothing does that?

>> >> The remapping info is still propagated up out of per-provider state so that
>> >> the parser can use it to tell what types the remapped arguments are and
>> >> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
>> >> the probe remapping to 1:1 and shuffle the native types correspondingly,
>> >> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
>> >> moved the native types around).
>> >
>> > I would drop this paragraph.  It is unnecessary and speculative at best about
>> > what other code should or should not do.
>> 
>> Hardly. It's documentation about how other code can get the arg types
>> right if they are using dt_cg_tramp_remap_args(). Would you prefer I
>> leave this entirely undescribed?! I could put it in a comment above
>> dt_cg_tramp_remap_args() (or rather the renamed dt_cg_tramp_map_args())
>> instead.
>> 
>> (Shifted into a comment, for now.)
>
> But that is not relevant to this patch, and other code shouldn't be doing this
> because it is almost certainly wrong that USDT is remapping the arguments in
> the trampoline because of the argN vs args[N] thng discussed above.
> 
> So, let's not add comments or commit message info that might point people to
> do the wrong thing.

It's correct *iff* you're using the in-trampoline remapping (the comment
is above dt_cg_tramp_remap_args). Obviously if we end up removing that
function, we'll remove the comment too :)

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

Yes, the code does exactly that, in probe_info.

I don't know the relative ordering of the calls to probe_info() versus
trampoline(), but when probe_info() is called the mapping *must* be
fixed up, because it's probe_info()s caller that stashes it away -- so
it's safest to set up that revised mapping there. If I set it up in
trampoline(), I have no idea whether we'd reliably have the remapping
worked out by the time probe_info() is called or not. Yes, this means
there is an implied coupling between trampoline() calling
dt_cg_tramp_remap_args() and between probe_info() having to change the
mapping in the args -- but given that you've been suggesting similar
coupling between parts of *dt_pid* and dt_prov_uprobe, I was assuming
you'd find such a minor coupling (between, really, trampoline() and
probe_info() in dt_prov_uprobe) uncontroversial.

>                                                             Otherwise there
> could be other code at some point that tries to apply the argument mapping
> and that would be really bad.

Well, yes. Is there anything anywhere that documents the order in which
the dt_provimpl_t callbacks are called? They're invoked from all over
the place and their call order is distinctly obscure, and deciding
whether it's safe to play with DTrace-side arg mapping info from
trampoline() is core to that. (As opposed to merely *generating code*
that shuffles args. That can obviously be done at any time.)

>> (Or are you suggesting that dt_cg_tramp_map_args(), or its trampoline()
>> caller, shuffle the xargs too?! This seems even *further* from anything
>> a trampoline() function should do to me.)
>
> Why would xargs ever be shuffled?  There is nothing in DTrace that does that.

Good. I thought you were suggesting it, and was rather confused.

>> >> + * arguments are overwritten (but saved).
>> >
>> > Caller should save.
>> 
>> As far as I can see this is not implementable without massive disruptive
>> ugly (or at least pointlessly duplicative) changes, see above. It
>> doesn't seem to buy us anything, either.
>
> How so?  Just put the dt_cg_tramp_save_args() call in the trampoline right
> before you call dt_cg_tramp_map_args().  How is that a massive disruptive ugly
> change?

Here's the code again:

	/*
	 * Work from saved args so we don't need to worry about overwriting regs
	 * we're about to remap.
	 */
	dt_cg_tramp_save_args(pcb);

	for (i = 0; i < nmappings; i++) {
		if (mappings[i] != i) {
			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(mappings[i])));
			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
		}
	}

i.e. it's *using* the save area. It relies on the args being saved in
it. Either it does so itself, or it silently relies on callers doing it
(! ew), or you're asking me to *not use* the save area but instead
implement arg mapping in some other way (and this is what I thought you
were asking for, because having dt_cg_tramp_map_args() not save the args
but nonetheless rely on something else having saved them already is
horrible, surely you don't mean that).

At this point I have no idea if you want me to reimplement this in terms
of something else, add *another* saved arg area just for the sake of arg
mapping, implement some completely different sort of arg mapping
algorithm in raw BPF (no thank you), or what.

>> >> +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
>> >> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
>> >
>> > I would add:
>> > 			argmap[i] = i;
>> > to convert the mapping into a pure i -> i mapping, so no other code will apply
>> > argument mapping again.
>> 
>> I think not: see above.
>
> I think so, above also :)

See above: I have no idea if this is even slightly safe, because I don't
know the relative order in which trampoline and arg_info are called. I
at least know what I'm doing now works.

-- 
NULL && (void)