Re: [PATCH 4/6] cg: add argument remapping in the trampoline

[Kris Van Hees <kris.van.hees@oracle.com>]

Concerning the mapping in the trampoline - yes, as I said before, I believe
the caller (the trampoline) should save the reg, and then the
dt_cg_tramp_map_args() function can use those saved copies to do the mapping.

As I said... caller should save.  I see no reason why dt_cg_tramp_map_args()
cannot depend on that.  You can add a comment before it saying that this is
expected.

On Mon, Oct 28, 2024 at 09:11:42PM +0000, Nick Alcock wrote:
> 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)