Index | Thread | Search

From:
Martin Pieuchot <mpi@openbsd.org>
Subject:
Re: btrace: Implement else-branch for if-statements in bt(5)
To:
Christian Ludwig <christian_ludwig@genua.de>
Cc:
tech@openbsd.org
Date:
Wed, 13 Mar 2024 17:00:19 +0100

Download raw body.

Thread
Hello Christian,

On 13/03/24(Wed) 13:45, Christian Ludwig wrote:
> Hi,
> 
> btrace has support for if-statements already implemented, but it lacks
> support for an else-branch. This diff implements that, including
> 'else if'.

Really nice.

> Although it has a regress test attached, it certainly needs more
> testing.

Could you extend the regressions instead of modifying it?  I'd like to
keep the existing cases and I'd also like to have if/else blocks where
all (else) branches are executed. 

> Is the abuse of a struct bt_arg as container for the two statement
> lists acceptable, or do we want a proper type?

I'd prefer a proper type with two statements pointers.  I'd be happy if
we can start improving the "design" of the various types used internally.

> So long,

Thanks for your work!

> ---
>  regress/usr.sbin/btrace/if.bt |  8 ++++++++
>  regress/usr.sbin/btrace/if.ok |  1 +
>  usr.sbin/btrace/bt_parse.y    | 15 ++++++++++-----
>  usr.sbin/btrace/btrace.c      | 14 +++++++++++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/regress/usr.sbin/btrace/if.bt b/regress/usr.sbin/btrace/if.bt
> index 053801cac54..89cdc3f9f47 100644
> --- a/regress/usr.sbin/btrace/if.bt
> +++ b/regress/usr.sbin/btrace/if.bt
> @@ -1,6 +1,8 @@
>  BEGIN {
>  	if (0)
>  		printf("nothing");
> +	else
> +		printf("printed!\n");
>  
>  	@var = 0;
>  	if (@var)
> @@ -8,10 +10,16 @@ BEGIN {
>  
>  	if (1) {
>  		printf("printed!\n");
> +	} else if (1) {
> +		printf("not printed\n");
> +	} else {
> +		printf("not printed\n");
>  	}
>  }
>  
>  END {
> +	if (1) { ; } else printf("not printed\n");
> +
>  	if (42) {
>  		printf("multiple ");
>  		@var = 4;
> diff --git a/regress/usr.sbin/btrace/if.ok b/regress/usr.sbin/btrace/if.ok
> index c2d5a91bc90..7b26ae9cd7c 100644
> --- a/regress/usr.sbin/btrace/if.ok
> +++ b/regress/usr.sbin/btrace/if.ok
> @@ -1,2 +1,3 @@
>  printed!
> +printed!
>  multiple (4) statements
> diff --git a/usr.sbin/btrace/bt_parse.y b/usr.sbin/btrace/bt_parse.y
> index 075eaa5b7f8..19b3e5029c6 100644
> --- a/usr.sbin/btrace/bt_parse.y
> +++ b/usr.sbin/btrace/bt_parse.y
> @@ -119,7 +119,7 @@ static int 	 beflag = 0;		/* BEGIN/END parsing context flag */
>  %token	<v.i>		ERROR ENDFILT
>  %token	<v.i>		OP_EQ OP_NE OP_LE OP_LT OP_GE OP_GT OP_LAND OP_LOR
>  /* Builtins */
> -%token	<v.i>		BUILTIN BEGIN END HZ IF STR
> +%token	<v.i>		BUILTIN BEGIN ELSE END HZ IF STR
>  /* Functions and Map operators */
>  %token  <v.i>		F_DELETE F_PRINT
>  %token	<v.i>		MFUNC FUNC0 FUNC1 FUNCN OP1 OP2 OP4 MOP0 MOP1
> @@ -248,7 +248,9 @@ stmt	: ';' NL			{ $$ = NULL; }
>  	| GVAR '=' OP4 '(' expr ',' vargs ')'	{ $$ = bh_inc($1, $5, $7); }
>  	;
>  
> -stmtblck: IF '(' expr ')' block			{ $$ = bt_new($3, $5); }
> +stmtblck: IF '(' expr ')' block			{ $$ = bt_new($3, $5, NULL); }
> +	| IF '(' expr ')' block ELSE block	{ $$ = bt_new($3, $5, $7); }
> +	| IF '(' expr ')' block ELSE stmtblck	{ $$ = bt_new($3, $5, $7); }
>  	;
>  
>  stmtlist: stmtlist stmtblck		{ $$ = bs_append($1, $2); }
> @@ -340,13 +342,15 @@ bc_new(struct bt_arg *term, enum bt_argtype op, struct bt_arg *ba)
>  
>  /* Create a new if/else test */
>  struct bt_stmt *
> -bt_new(struct bt_arg *ba, struct bt_stmt *condbs)
> +bt_new(struct bt_arg *ba, struct bt_stmt *condbs, struct bt_stmt *elsebs)
>  {
> -	struct bt_arg *bop;
> +	struct bt_arg *bop, *lnk;
>  
>  	bop = ba_op(B_AT_OP_NE, NULL, ba);
> +	lnk = ba_new(condbs, 0);
> +	lnk->ba_key = (struct bt_arg *)elsebs;
>  
> -	return bs_new(B_AC_TEST, bop, (struct bt_var *)condbs);
> +	return bs_new(B_AC_TEST, bop, (struct bt_var *)lnk);
>  
>  }
>  /* Create a new probe */
> @@ -714,6 +718,7 @@ lookup(char *s)
>  		{ "count",	MOP0, 		B_AT_MF_COUNT },
>  		{ "cpu",	BUILTIN,	B_AT_BI_CPU },
>  		{ "delete",	F_DELETE,	B_AC_DELETE },
> +		{ "else",	ELSE,		0 },
>  		{ "exit",	FUNC0,		B_AC_EXIT },
>  		{ "hist",	OP1,		0 },
>  		{ "hz",		HZ,		0 },
> diff --git a/usr.sbin/btrace/btrace.c b/usr.sbin/btrace/btrace.c
> index ce32989be91..ba3efb2983a 100644
> --- a/usr.sbin/btrace/btrace.c
> +++ b/usr.sbin/btrace/btrace.c
> @@ -474,8 +474,11 @@ rules_action_scan(struct bt_stmt *bs)
>  			evtflags |= ba2dtflags(ba);
>  			break;
>  		case B_AC_TEST:
> +			ba = (struct bt_arg *)bs->bs_var;
>  			evtflags |= rules_action_scan(
> -			    (struct bt_stmt *)bs->bs_var);
> +			    (struct bt_stmt *)ba->ba_value);
> +			evtflags |= rules_action_scan(
> +			    (struct bt_stmt *)ba->ba_key);
>  			break;
>  		default:
>  			break;
> @@ -669,8 +672,13 @@ rule_eval(struct bt_rule *r, struct dt_evt *dtev)
>  	}
>  
>  	SLIST_FOREACH(bs, &r->br_action, bs_next) {
> -		if ((bs->bs_act == B_AC_TEST) && stmt_test(bs, dtev) == true) {
> -			struct bt_stmt *bbs = (struct bt_stmt *)bs->bs_var;
> +		if (bs->bs_act == B_AC_TEST) {
> +			struct bt_arg *lnk = (struct bt_arg *)bs->bs_var;
> +			struct bt_stmt *bbs;
> +			if (stmt_test(bs, dtev) == true)
> +				bbs = (struct bt_stmt *)lnk->ba_value;
> +			else
> +				bbs = (struct bt_stmt *)lnk->ba_key;
>  
>  			while (bbs != NULL) {
>  				if (stmt_eval(bbs, dtev))
> -- 
> 2.34.1
>