[DTrace-devel] [PATCH 01/14] Have dt_cg_load_scalar report runtime error if it fails

[Eugene Loh]

On 5/2/23 11:25, Nick Alcock wrote:

> On 2 May 2023, eugene loh said:
>
>> From: Eugene Loh <eugene.loh at oracle.com>
> Much needed, if you ask me. (I really should go through and add a lot
> more error checking to DTrace's CTF operations as well. Too many things
> which can fail are assumed to be impossible to fail...)
>
> Reviewed-by: Nick Alcock <nick.alcock at oracle.com>
>
> (with a couple of stylistic nits below)
>
>> -	dt_regset_free(drp, BPF_REG_0);
>>   	dt_regset_free_args(drp);
>> +
>> +        /* check if we were successful */
>> +	emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, Lokay));
>> +	dt_regset_free(drp, BPF_REG_0);
>> +	dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, DT_ISREG, dnp->dn_reg);
>> +	emitl(dlp, Lokay,
>> +		   BPF_NOP());
>> +
>> +	/* load the copy of the data in "safe" memory (D stack) */
> The code looks fine, but DTrace comments are of the style
>
> /*
>   * Load the copy of the data in "safe" memory (D stack).
>   */

Sometimes.  But not exclusively.  The comments in this patch are 
stylistically consistent with commits of the last month/s, existing 
comments (including in dt_cg.c), and guidelines described in the file 
CODING-STYLE.

> I happen to think this is annoyingly verbose and needlessly wastes
> vertical space, discouraging comments, and would be very happy to move
> to a scheme where one-line (or one paragraph?) comments are of the style
>
> /* Load the copy of the data in "safe" memory (D stack). */
>
> But at the very least I think we want a capital letter :)

Okay, I capitalized some letters but left the comments as one-liners.

Thanks for the reviews.

> Nice tests!
>