[DTrace-devel] [PATCH 4/4] Make sure assignment expressions work correctly with tstrings

[Eugene Loh]

On 11/1/21 4:32 PM, Kris Van Hees wrote:

> On Mon, Nov 01, 2021 at 03:33:01PM -0400, Eugene Loh wrote:
>>> @@ -2830,6 +2841,35 @@ dt_cg_ternary_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>>    
>>>    	emitl(dlp, lbl_post,
>>>    		   BPF_NOP());
>>> +
>>> +	/*
>>> +	 * Strings complicate things a bit because dn_left and dn_right might
>>> +	 * actually be temporary strings (tstring) *and* in different slots.
>>> +	 * We need to allocate a new tstring to hold the result, and copy the
>>> +	 * value into the new tstring (and free any tstrings in dn_left and
>>> +	 * dn_right).
>>> +	 */
>>> +	if (dt_node_is_string(dnp)) {
>>> +		/*
>>> +		 * At this point, dnp->dn_reg holds a pointer to the string we
>>> +		 * need to copy.  But we want to copy it into a tstring which
>>> +		 * location is to be stored in dnp->dn_reg.  So, we need to
>>> +		 * shuffle things a bit.
>>> +		 */
>>> +		emit(dlp,  BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
>>> +		dt_cg_tstring_alloc(yypcb, dnp);
>>> +
>>> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, DT_STK_DCTX));
>>> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, dnp->dn_reg, DCTX_MEM));
>>> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, dnp->dn_tstring->dn_value));
>>> +
>>> +		dt_cg_memcpy(dlp, drp, dnp->dn_reg, BPF_REG_0,
>>> +			     DT_STRLEN_BYTES +
>>> +			     yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE]);
>>> +
>>> +		dt_cg_tstring_free(yypcb, dnp->dn_left);
>>> +		dt_cg_tstring_free(yypcb, dnp->dn_right);
>>> +	}
>>>    }
>> Should the memcpy not include an extra byte for the NULL terminator?
> Excellent question!  The answer here is 'no' mainly because we don't do that in
> other places either but whether that is the perfect behaviour or not is a bit
> unclear.  The documentation states:
>
>    In DTrace, strings are represented as an array of characters terminated by a
>    null byte (that is, a byte whose value is zero, usually written as '\0').
>    The visible part of the string is of variable length, depending on the
>    location of the null byte, but DTrace stores each string in a fixed-size
>    array so that each probe traces a consistent amount of data. Strings cannot
>    exceed the length of the predefined string limit. However, the limit can be
>    modified in your D program or on the dtrace command line by tuning the
>    strsize option.
>
> The terminology shifts a bit near the end of the paragraph when we go from
> a "fixed-size array" and "consistent amount of data" to "string limit".  The
> implementation seems to be a bit inconsistent on what the interpretation ought
> to be.  Whether the setting of strsize is meant to be the limit on the
> "consistent amount of data" or whether it is that value - 1 to account for
> the null byte is not clear.

Okay, but to be clear, regardless of whether we make space for that NULL 
char internally, we do expect that strsize should be the cap on the 
number of visible chars, right?  E.g., if Solaris is the reference 
implementation:

# uname -r
5.11
# /usr/sbin/dtrace -xstrsize=8 -n 'BEGIN { x = "123456789"; 
printf("|%s|\n", x); exit(0) }'
dtrace: description 'BEGIN ' matched 1 probe
  CPU     ID                    FUNCTION:NAME
   62      1                           :BEGIN |12345678|

# /usr/sbin/dtrace -xstrsize=7 -n 'BEGIN { x = "123456789"; 
printf("|%s|\n", x); exit(0) }'
dtrace: description 'BEGIN ' matched 1 probe
  CPU     ID                    FUNCTION:NAME
   62      1                           :BEGIN |1234567|

# /usr/sbin/dtrace -n 'BEGIN {
     x = " 123456";
     printf("%d |%s| |%s|\n", strlen(x), substr(x, 250), x);
     exit(0);
}'
dtrace: description 'BEGIN ' matched 1 probe
  CPU     ID                    FUNCTION:NAME
   12      1                           :BEGIN 256 |123456| | 123456|

That is, 7 chars are visible with strsize=7, 8 with strsize=8, and 256 
by default.