[DTrace-devel] refactoring trampoline code generation

[Kris Van Hees]

On Tue, Jun 09, 2020 at 04:42:13PM -0700, Eugene Loh wrote:
> I'm thinking about a profile provider, but was considering some code 
> refactoring in anticipation.
> 
> One of the current maintenance issues is that the various providers 
> replicate so much code, notably in trampoline generation.  The various 
> trampolines must all fill in the "struct dt_bpf_context", which 
> basically means the EPID and args.  They must also step around those 
> values, zero-filling the other contents of that context.  They then set 
> up and call dt_program, finally exiting.  (Currently, regs are not 
> copied, with some redesign of the BPF stack layout in the works.)
> 
> So really, the only difference among the trampolines is filling in the 
> probe arguments.

Well, with the reference made to the regs, that would make two differences
since not all BPF contexts provide the registers in a manner that we can
access from BPF code.  In that case we need a more complex method (if at all
possible) to obtain the register values.  More complex code will also require
a well defined contract between the common code and the provider specific code
to ensure that register are allocated and used correctly, etc.

> How would it be if I were to submit a patch that adds a "static void 
> dt_cg_trampoline(dt_pcb_t *pcb)" to dt_cg.c.  Then, dt_cg() would call 
> this function instead of the provider-specific versions.  The function 
> would generate most of the trampoline itself.  For probe arguments, it 
> would call the provider-specific function 
> pcb->pcb_probe->prov->impl->trampoline(). Common boiler-plate code 
> generation would be removed from the provider-specific trampolines.

I feel that this is a bit of a premature optimization.  Since not all the
required providers have been implemented yet we don't quite know whether the
commonality between trampolines is going to remain as close as you describe.
Also (as I mention above) there is likely to be more complex coding needed
in the provider-specific portion which requires a more careful contract between
your dt_cg_trampoline() and the provider trampoline() e.g. in terms of what
registers are alive at which point.

On a more minor point, I do not believe that a generic trampoline generating
function would belong in the code generator because this has more to do with
the specific BPF program types than with code generation.  One of the reasons
for having trampoline generating functions per provider despite the duplication
of code is the fact that each trampoline generates a BPF program of a specific
BPF program type.  I do think there is value in making that distinction very
clear in the implementation.  Even though the generated trampolines are very
similar and only differ (right now) in two areas, they are not in fact generic
programs that have some provider specific parts.  They are to be seen as
programs of different BPF program types.  I do not want to lose that aspect
of the implementation.

> Let me know.  A simple patch that removes a bunch of replicated code.