On Wed, Oct 20, 2021 at 8:48 AM Steven Rostedt rostedt@goodmis.org wrote:
On Tue, 19 Oct 2021 18:31:40 -0700 Kalesh Singh kaleshsingh@google.com wrote:
@@ -2391,60 +2460,61 @@ static int check_expr_operands(struct trace_array *tr, static struct hist_field *parse_expr(struct hist_trigger_data *hist_data, struct trace_event_file *file, char *str, unsigned long flags,
char *var_name, unsigned int level)
char *var_name, unsigned int *n_subexprs){ struct hist_field *operand1 = NULL, *operand2 = NULL, *expr = NULL; unsigned long operand_flags; int field_op, ret = -EINVAL; char *sep, *operand1_str;
if (level > 3) {
if (*n_subexprs > 3) {Why limit the sub expressions, and not just keep the limit of the level of recursion. We allow 3 levels of recursion, but we could have more than 3 sub expressions.
The main reason for this is that it's predictable behavior form the user's perspective. Before this patch the recursion always walked down a single branch so limiting by level worked out the same as limiting by sub expressions and is in line with the error the user would see ("Too many sub-expressions (3 max)"). Now that we take multiple paths in the recursion, using the level to reflect the number of sub-expressions would lead to only seeing the error in some of the cases (Sometimes we allow 4, 5, 6 sub-expressions depending on how balanced the tree is, and sometimes we error out on 4 - when the tree is list-like). Limiting by sub-expression keeps this consistent (always error out if we have more than 3 sub-expressions) and is in line with the previous behavior.
- Kalesh
If we have: a * b + c / d - e * f / h
It would break down into: - + /
/ * ha b c d e f
Which I believe is 6 "sub expressions", but never goes more than three deep in recursion:
"a * b + c / d - e * f / h"
Step 1:
op = "-" operand1 = "a * b + c / d" operand2 = "e * f / h"
Process operand1: (recursion level 1)
op = "+" operand1a = "a * b" operand2a = "c / d"
Process operand1a: (recursion level 2)
op = "*" operand1b = "a" operand2b = "b"
return;
Process operand1b: (recursion level 2)
op = "/" operand1b = "c" operand2b = "d"
return;
return;
Process operand2: (recursion level 1)
op = "/" operand1c = "e * f" operand2c = "h"
Process operand1c: (recursion level 2)
op = "*" operand1c = "e" operand2c = "f"
return;
return;
/* LHS of string is an expression e.g. a+b in a+b+c */operand1 = parse_expr(hist_data, file, operand1_str, operand_flags, NULL, n_subexprs); if (IS_ERR(operand1)) { ret = PTR_ERR(operand1); operand1 = NULL;I wonder if we should look for optimizations, in case of operand1 and operand2 are both constants?
Just perform the function, and convert it into a constant as well.
-- Steve