[DTrace-devel] [PATCH v4 2/3] libdtrace: BTF->CTF conversion for kernel modules

[Kris Van Hees]

I think I have a more minimal patch that accomplishes the same thing, and that
is actually necessary at this point for some patches that have been in queue
for a while.  I'll post my variant of the patch shortly, but I will explain
in this email what and why I changed things.

On Mon, Jul 08, 2024 at 03:54:03PM +0100, Alan Maguire wrote:
> We had logic in place to collect BTF in modules and convert vmlinux
> BTF to CTF, but were not doing the same for module BTF.  Add support
> to do this, and explicitly set parent dict to "shared_ctf".  With
> this in place (along with the support to find the appropriate module
> CTF) we can successfully print module-specific types such as:
> 
> $ dtrace -n 'fbt::ieee80211_rx_napi:entry { print((struct ieee80211_hw *)arg0); }'
> dtrace: description 'fbt::ieee80211_rx_napi:entry ' matched 1 probe
> CPU     ID                    FUNCTION:NAME
>   6 117489          ieee80211_rx_napi:entry 0xffff9fabc2e688e0 = *
>                                             (struct ieee80211_hw) {
>                                              .conf = (struct ieee80211_conf) {
>                                               .listen_interval = (u16)10,
>                                               .long_frame_max_tx_count = (u8)4,
>                                               .short_frame_max_tx_count = (u8)7,
>                                              },
>                                              .wiphy = (struct wiphy *)0xffff9fabc2e683c0,
>                                              .rate_control_algorithm = (const char *)0xffffffffc1346ae5,
> ...
> 
> Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
> ---
>  libdtrace/dt_btf.c    | 3 ++-
>  libdtrace/dt_cg.c     | 1 -
>  libdtrace/dt_module.c | 7 ++++++-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/libdtrace/dt_btf.c b/libdtrace/dt_btf.c
> index 9e67fd9d..4e1ed9f6 100644
> --- a/libdtrace/dt_btf.c
> +++ b/libdtrace/dt_btf.c
> @@ -715,6 +715,7 @@ dt_btf_to_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp, dt_btf_t *btf)
>  
>  		if (strcmp(dmp->dm_name, "vmlinux") == 0)
>  			goto out;
> +		ctf_parent_name_set(ctf, "shared_ctf");

This is not needed because the BTF-to-CTF conversion code already performs
an explicit ctf_import of the shared_ctf.  There is no need to set the parent
name.

>  	}
>  
>  	if (!btf)
> @@ -867,7 +868,7 @@ dt_btf_lookup_name_kind(dtrace_hdl_t *dtp, dt_btf_t *btf, const char *name,
>  	 * shared one.
>  	 */
>  	 if (!dtp->dt_shared_btf) {
> -		  dt_btf_load_module(dtp, dtp->dt_exec);
> +		  btf = dt_btf_load_module(dtp, dtp->dt_exec);

This is not needed.  The dt_btf_load_module() function sets dtp->dt_shared_btf
when it is called for vmlinux.  We do not want to assign it to btf unless the
dt_btf_lookup_name_kind() function was called with btf == NULL, because
otherwise we'd be performing a lookup in the shared BTF rather than in the
supplied BTF.

>  
>  		  if (!btf)
>  			   btf = dtp->dt_shared_btf;
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 2eb2df9c..4206235b 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2852,7 +2852,6 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  		dnerror(addr, D_PRINT_SIZE,
>  			"print( ) argument #1 reference has type '%s' with size 0; cannot print( ) it.\n",
>  			ctf_type_name(fp, type, n, sizeof(n)));
> -
>  	pfp = dt_printf_create(dtp, dmp->dm_name);
>  
>  	/* reserve space for addr/type, data/size */
> diff --git a/libdtrace/dt_module.c b/libdtrace/dt_module.c
> index 5e608446..d8a539af 100644
> --- a/libdtrace/dt_module.c
> +++ b/libdtrace/dt_module.c
> @@ -962,8 +962,8 @@ dt_kern_module_find_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp)
>  			dt_dprintf("Cannot open CTF archive %s: %s; "
>  				   "looking for in-module CTF instead.\n",
>  				   ctfa_name, ctf_errmsg(dtp->dt_ctferr));
> -#endif
>  			return;
> +#endif

This is valid, though technically not necessary.  The lookup for CTF data is
in practice always done for vmlinux before any other module, so it would be
safe to have the earlier return here.  But there is nothing wrong with not
taking the return, and it is probably more clear.

>  		}
>  
>  		if (dtp->dt_ctfa_path == NULL)
> @@ -982,6 +982,11 @@ dt_kern_module_find_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp)
>  			    "archive: %s; looking for out-of-tree module.\n",
>  			    dmp->dm_name, ctf_errmsg(dtp->dt_ctferr));
>  		}
> +	} else if (dtp->dt_shared_ctf) {
> +#ifdef HAVE_LIBCTF
> +		/* Have shared CTF via vmlinux BTF; now do BTF->CTF for mod. */
> +		dt_kern_module_ctf_from_btf(dtp, dmp);
> +#endif

This is not the right place to do this.  If you do, then we will trigger the
code that follows where a ctf_import of shared_ctf is performed, but we did
that already when doing the BTF-to-CTF conversion.  So, instead, we ought to
do this later in the function.  See my version of the patch to see where.

>  	}
>  
>  	if (dmp->dm_ctfp != NULL) {
> -- 
> 2.43.5
> 
>