[DTrace-devel] [PATCH 2/7] types: populate intrinsics from the kernel if possible

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
but...

On 5/2/23 13:12, Nick Alcock via DTrace-devel wrote:
> The primary purpose of types in the C container, like types in DTrace in
> general, is not just to write D programs and #include C headers -- it's
> also to interoperate with the type system used by the kernel.
>
> As such, we really should be getting types in this dict*from*  the
> kernel if at all possible.  This also means we are insulated from the
> kernel suddenly changing its type system on us (being freestanding,
> the kernel can break platform ABI like that fairly freely, as it
> recently did with the unconditional unsignednessing of char); whatever
> the types are, we just pick them up, and then any types the users (or
> the D container, or the syslibs, or...) base on them will be compatible
> with whatever the kernel's definition is.

This commit message is so long, it's hard to follow.  How about:

     Types in the C container must interoperate with those
     used by the kernel.  Meanwhile, the kernel is freestanding;
     its type system is independent from user space's.

     Therefore, get types in this dict from the kernel where possible.

> A lot of types in the_dtrace_intrinsics_* arrays are now redundant, but
> precisely which those*are*  depends on the running kernel, so we leave
> the lot so that we can always use those types in D even if the kernel
> doesn't define them at all.  (But in practice all the useful ones will
> always come from vmlinux from now on -- except for void, which we
> define ourselves because of the IS_VOID() macro. Nobody's ever going to
> check void for assignment-compatibility anyway, and the definition of
> void in particular is woolly in CTF and different implementations have
> defined it in rather different ways.  Let's stick to one consistent
> intrinsic instead.)

How about replacing this paragraph with:

     Many types in the _dtrace_intrinsics_* arrays are now
     redundant, but which ones are already defined by the
     kernel depend on the kernel.  Retain them all for a
     consistent set.

As usual, a test would be nice, but again might in this case be impractical.

> Signed-off-by: Nick Alcock<nick.alcock at oracle.com>
> ---
>   libdtrace/dt_open.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index f0dc2575377f2..f0ef053f5274d 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -939,10 +939,31 @@ dt_vopen(int version, int flags, int *errp,
>   	dmp->dm_flags = DT_DM_LOADED; /* fake up loaded bit */
>   
>   	/*
> -	 * Fill the dynamic "C" CTF container with all of the intrinsic
> -	 * integer and floating-point types appropriate for this data model.
> +	 * Fill the dynamic "C" CTF container with all of the intrinsic integer
> +	 * and floating-point types appropriate for this data model; but first,
> +	 * try to prepopulate them all from the vmlinux` container, so that our
> +	 * types are compatible with the kernel's type system.  Or at least,
> +	 * almost all of them: "void" has special hardwired behaviour in
> +	 * dt_parser.h and nobody's likely to check it for
> +	 * assignment-compatibility, so use the hardwired intrinsic for that
> +	 * unconditionally.
>   	 */

How about just making this comment simply /* Fill the dynamic "C" CTF 
container. */.  Then break the remainder of the comment block into two 
paragraphs, one for int/fp and one for prepopulate, moving those 
paragraphs into the for() loop, each paragraph next to its associated code.

>   	for (; dinp->din_name != NULL; dinp++) {
> +		dtrace_typeinfo_t ti;
> +
> +		if (strcmp(dinp->din_name, "void") != 0 &&
> +		    dtrace_lookup_by_type(dtp, "vmlinux", dinp->din_name,
> +			&ti) == 0) {
> +			if (ctf_add_type(dmp->dm_ctfp, ti.dtt_ctfp,
> +				ti.dtt_type) != CTF_ERR)
> +				continue;
> +			else
> +				dt_dprintf("Attempt to add %s from the kernel "
> +				    "CTF failed: %s.  Using hardwired def.)\n",
> +				    dinp->din_name,
> +				    ctf_errmsg(ctf_errno(dmp->dm_ctfp)));

Can you remove the "else" and save yourself a level of indentation in 
the dt_printf()?  Also, in the dt_printf(), do you mean for some 
punctuation after "kernel"?  E.g., s/kernel /kernel.  /?  Or s/kernel 
/kernel.\n/?

> +		}
> +
>   		if (dinp->din_kind == CTF_K_INTEGER) {
>   			err = ctf_add_integer(dmp->dm_ctfp, CTF_ADD_ROOT,
>   			    dinp->din_name, &dinp->din_data);
> -- 2.39.1.268.g9de2f9a303