[DTrace-devel] [PATCH v2 dtrace 1/3] fbt: support "."-suffixed functions for kprobes

[Alan Maguire]

On 11/10/2024 16:00, Kris Van Hees wrote:
> On Fri, Oct 11, 2024 at 11:28:08AM +0100, Alan Maguire wrote:
>> gcc adds suffixes when it carries out optimizations, but often
>> these leave parameters to functions intact.  Many of these functions
>> (like finish_task_switch()) are important for tracing (and they
>> are present in available_filter_functions so are traceable) so it is
>> valuable to support probing them.  For kprobes, all that is needed
>> is to ensure that the event name does not contain a ".".
> 
> The challenge here is that not everything that is present in
> available_filter_functions is actually traceable.  I.e. anything with a
> compiler-optimization-generated name (*.*) cannot be traced with fprobes.
> Also, at least with older kernels, my testing has shown that not all that
> is listed in that file can even be traced with kprobes (probably due to a
> kernel bug that has since been fixed).

Yeah the results of wildcarding on all fbt functions are pretty varied
unfortunately. I wonder if there's a way we could come up with a test
and feed back the results upstream; maybe a few more functions just need
to be annotated notrace. Unfortunately it's such a fast-moving target..

The logic in this patch is coupled with that in the next in a way; so we
present fprobes for everything in available_filter_functions, and if any
probes are _used_ that do not have a BTF representation, we can then
fall back to kprobes. Previously I just added "."-suffixed functions as
kprobes initially, but that then raises the issue of intermixing of
kprobe- and fprobe-backed fbt at a clause.

> 
> This leaves us with an interesting challenge.  Since we prefer fprobes
> because they are faster (and give us argument type info), using the content
> of that file indiscriminately is not a good idea because it will cause us
> to list probes that cannot be used.  On the other hand, if probes with "."
> in their names *can* be probed using kprobe, then it would certainly be
> beneficial to make use of that.
> 
> Now, since the .-names are compiler generated we do not necessarily need to
> list those as probes.  We could just list the base function *if* we can
> determine (for FBT) which suffixes refer to variants that actually constitute
> a valid alternative (where function entry and return are valid representations
> of the original function).
>

Right, we could eliminate for example .cold. suffixes and .part.
suffixes as these do not represent function boundaries; we'd want to
keep .isra and .constprop as these do correspond to function boundaries.
As you say, after doing that, it might be better to represent the probe
to the user without the suffix, as this would mean they could trace the
function in a stable manner regardless of what the compiler did for a
specific kernel.

>> Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
>> ---
>>  libdtrace/dt_prov_fbt.c | 31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
>> index 21f63ddf..0ba0fb71 100644
>> --- a/libdtrace/dt_prov_fbt.c
>> +++ b/libdtrace/dt_prov_fbt.c
>> @@ -106,10 +106,6 @@ static int populate(dtrace_hdl_t *dtp)
>>  				p++;
>>  		}
>>  
>> -		/* Weed out synthetic symbol names (that are invalid). */
>> -		if (strchr(buf, '.') != NULL)
>> -			continue;
>> -
>>  #define strstarts(var, x) (strncmp(var, x, strlen (x)) == 0)
>>  		/* Weed out __ftrace_invalid_address___* entries. */
>>  		if (strstarts(buf, "__ftrace_invalid_address__") ||
>> @@ -363,7 +359,7 @@ static int kprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>  static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>>  {
>>  	if (!dt_tp_probe_has_info(prp)) {
>> -		char	*fn;
>> +		char	*fn, *event, *s;
>>  		FILE	*f;
>>  		size_t	len;
>>  		int	fd, rc = -1;
>> @@ -376,22 +372,37 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>>  		if (fd == -1)
>>  			return -ENOENT;
>>  
>> +		if (asprintf(&event, "%s", prp->desc->fun) < 0)
>> +			return -ENOMEM;
>> +		/* replace possible "." in event name as "." is not supported
>> +		 * in trace event names, and function could be named foo.isra.0
>> +		 */
>> +		for (s = strchr(event, '.'); s != NULL && *s != '\0'; s++) {
>> +			if (*s == '.')
>> +				*s = '_';
>> +		}
>> +
>>  		rc = dprintf(fd, "%c:" FBT_GROUP_FMT "/%s %s\n",
>>  			     prp->desc->prb[0] == 'e' ? 'p' : 'r',
>> -			     FBT_GROUP_DATA, prp->desc->fun, prp->desc->fun);
>> +			     FBT_GROUP_DATA, event, prp->desc->fun);
>>  		close(fd);
>> -		if (rc == -1)
>> +		if (rc == -1) {
>> +			free(event);
>>  			return -ENOENT;
>> +		}
>>  
>>  		/* create format file name */
>>  		len = snprintf(NULL, 0, "%s" FBT_GROUP_FMT "/%s/format",
>> -			       EVENTSFS, FBT_GROUP_DATA, prp->desc->fun) + 1;
>> +			       EVENTSFS, FBT_GROUP_DATA, event) + 1;
>>  		fn = dt_alloc(dtp, len);
>> -		if (fn == NULL)
>> +		if (fn == NULL) {
>> +			free(event);
>>  			return -ENOENT;
>> +		}
>>  
>>  		snprintf(fn, len, "%s" FBT_GROUP_FMT "/%s/format", EVENTSFS,
>> -			 FBT_GROUP_DATA, prp->desc->fun);
>> +			 FBT_GROUP_DATA, event);
>> +		free(event);
>>  
>>  		/* open format file */
>>  		f = fopen(fn, "r");
>> -- 
>> 2.43.5
>>