From: Martin Pieuchot Subject: Re: btrace: Implement else-branch for if-statements in bt(5) To: Christian Ludwig Cc: tech@openbsd.org Date: Wed, 13 Mar 2024 17:00:19 +0100 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 ERROR ENDFILT > %token OP_EQ OP_NE OP_LE OP_LT OP_GE OP_GT OP_LAND OP_LOR > /* Builtins */ > -%token BUILTIN BEGIN END HZ IF STR > +%token BUILTIN BEGIN ELSE END HZ IF STR > /* Functions and Map operators */ > %token F_DELETE F_PRINT > %token 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 >