[DTrace-devel] [PATCH 12/12] Implement TLS variables

[Eugene Loh]

On 11/23/21 2:45 AM, Kris Van Hees wrote:

> On Mon, Nov 22, 2021 at 06:08:18PM -0500, Eugene Loh via DTrace-devel wrote:
>> On 11/20/21 12:44 AM, Kris Van Hees via DTrace-devel wrote:
>>> diff --git a/bpf/get_tvar.c b/bpf/get_tvar.c
>>> @@ -10,12 +10,40 @@
>>>    # define noinline	__attribute__((noinline))
>>>    #endif
>>> -extern struct bpf_map_def tvars;
>>> +extern struct bpf_map_def dvars;
>>> +extern uint64_t NCPUS;
>>> -noinline uint64_t dt_get_tvar(uint32_t id)
>>> +noinline void *dt_get_tvar(uint32_t id)
>>>    {
>>> -	uint64_t	*val;
>>> +	uint64_t	key;
>>> +	uint64_t	dflt_key = 0;;
Double semicolon.
>>> +	void		*val;
>>> -	val = bpf_map_lookup_elem(&tvars, &id);
>>> -	return val ? *val : 0;
>>> +	key = bpf_get_current_pid_tgid();
>>> +	key &= 0x00000000ffffffffUL;
>>> +	if (key == 0)
>>> +		key = bpf_get_smp_processor_id();
>>> +	else
>>> +		key += (uint32_t)(uint64_t)&NCPUS;
>>> +
>>> +	key++;
>>> +	key = (key << 32) | id;
>>> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
>>> @@ -213,36 +213,21 @@ populate_probes_map(dtrace_hdl_t *dtp, int fd)
>>>     *		use.
>>>     * - gvars:	Global variables map.  This is a global map with a singleton
>>>     *		element (key 0) addressed by variable offset.
>>> + * - dvars:	Dynamic variables map.  This is a global hash map index with
>>> + *		a unique numeric identifier for each variable.
>> Well, variable/pid combo.
s/index/indexed/
and again not just per variable but also per pid
>>> @@ -371,10 +354,19 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>>>    			sizeof(uint32_t), lvarsz, 1) == -1)
>>>    		return -1;		/* dt_errno is set for us */
>>> -	if (tvarc > 0 &&
>>> -	    create_gmap(dtp, "tvars", BPF_MAP_TYPE_ARRAY,
>>> -			sizeof(uint32_t), sizeof(uint64_t), tvarc) == -1)
>>> -		return -1;		/* dt_errno is set for us */
>>> +	if (dvarc > 0) {
>> Speaking of which, is there a plan to get fancier on the sizing? We're
>> allocating string space for everyone.  In many cases, that will be 256/4 or
>> 256/8 times too much.  Meanwhile, for some structs, I guess it'll be too
>> small.  Doesn't need to be fixed in the first patch, but maybe worth
>> mentioning?
> This is the challenge of doing tracing and supporting dynamic variables.  You
> cannot really get into implementing fancy memory management.  And yes, later on
> we will need to make the size be based on the largest datatype a variable can
> have rather than strsize, so indeed it can be larger than strsize.
But this should be mentioned in the commit message or somewhere.  If 
there are major limitations, they should be acknowledged.
>>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>>> @@ -2351,23 +2377,48 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>>>    	/* TLS var */
>>>    	varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>>> -	idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_set_tvar");
>>> +	size = idp->di_size;
>>> +
>>> +	idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_tvar");
>>>    	assert(idp != NULL);
>>>    	if (dt_regset_xalloc_args(drp) == -1)
>>>    		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>>> -	/*
>>> -	 * We assign the varid in the instruction preceding the call because
>>> -	 * the disassembler expects this sequence in support for annotating
>>> -	 * the disassembly with variables names.
>>> -	 */
>>> -	emit(dlp,  BPF_MOV_REG(BPF_REG_2, dnp->dn_reg));
>>>    	emit(dlp,  BPF_MOV_IMM(BPF_REG_1, varid));
>>>    	dt_regset_xalloc(drp, BPF_REG_0);
>>>    	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
>>> -	dt_regset_free(drp, BPF_REG_0);
>>>    	dt_regset_free_args(drp);
>>> +
>>> +	if ((reg = dt_regset_alloc(drp)) == -1)
>>> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>>> +
>>> +	emit(dlp, BPF_MOV_REG(reg, BPF_REG_0));
>>> +	dt_regset_free(drp, BPF_REG_0);
>>> +
>>> +	dt_cg_check_notnull(dlp, drp, reg);
>>> +
>>> +	if (dnp->dn_flags & DT_NF_REF) {
>>> +		size_t		srcsz;
>>> +
>>> +		/*
>>> +		 * Determine the amount of data to be copied.  It is
>>> +		 * the lesser of the size of the identifier and the
>>> +		 * size of the data being copied in.
>>> +		 */
>>> +		srcsz = dt_node_type_size(dnp->dn_right);
>>> +		if (dt_node_is_string(dnp))
>>> +			srcsz += DT_STRLEN_BYTES;
>>> +		size = MIN(srcsz, size);
>> I think neither "srcsz = dt_node_type_size(dnp->dn_right)" nor "size =
>> idp->di_size" includes the NUL terminating byte.  Same with the pre-existing
>> gvar/lvar code.
> This is actually an interesting situation because dt_node_type_size() does
> include the terminating NUL byte for string constants but not for regular
> string types.  And di_size generally will not contain it either.  And as you
> say, this applies to gvar and lvar as well.  So, I am inclined to leave it as
> is in this patch and simply fix it for all three cases with a single patch.
How do these bugs get tracked?
>>> +
>>> +		dt_cg_memcpy(dlp, drp, reg, dnp->dn_reg, size);
>>> +	} else {
>>> +		assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
>>> +
>>> +		emit(dlp, BPF_STORE(ldstw[size], reg, 0, dnp->dn_reg));
>>> +	}
>>> +
>>> +	dt_regset_free(drp, reg);
>>>    }