From 180223ea9fdce6bf495a67f019c2067955bc87d3 Mon Sep 17 00:00:00 2001 From: Sergej Breiter Date: Mon, 18 Nov 2024 18:39:20 +0100 Subject: [PATCH 1/5] Implementation and draft integration of binary expression tree This replaces the stack-based calculator with a binary expression tree. The tree is constructed only once per metric when a perfgroup is read and can be evaluated arbitrarily often. --- src/calculator_exptree.c | 271 ++++++++++++++++++++++++++++++ src/includes/calculator_exptree.h | 19 +++ src/includes/likwid.h | 3 + src/includes/perfgroup.h | 6 +- src/nvmon.c | 36 +++- src/perfgroup.c | 64 ++++++- src/perfmon.c | 21 +++ 7 files changed, 403 insertions(+), 17 deletions(-) create mode 100644 src/calculator_exptree.c create mode 100644 src/includes/calculator_exptree.h diff --git a/src/calculator_exptree.c b/src/calculator_exptree.c new file mode 100644 index 000000000..cd8b8beec --- /dev/null +++ b/src/calculator_exptree.c @@ -0,0 +1,271 @@ +#include +#include +#include /* CounterList */ +#include "calculator_exptree.h" + +#include +#include +#include +#include +#include + + +struct exptree_node { + struct exptree_node *left; // Left child + struct exptree_node *right; // Right child + + double value; // Operand value (if it's a number) + char *counter_name; + char operator; // Operator: '+', '-', '*', '/' +}; + +// Forward declarations +static struct exptree_node *_make_expression_tree(const char **expr); +static struct exptree_node *_make_term_tree(const char **expr); +static struct exptree_node *_make_factor_tree(const char **expr); + +#define NODE_NULL_VALUE 0.0 +#define NODE_NULL_OPERATOR '\0' + +static void _skip_spaces(const char **expr) +{ + while (isspace(**expr)) { + (*expr)++; + } +} + +// Set value and create a leaf node +static struct exptree_node *_make_value_node(double value) +{ + struct exptree_node *node = malloc(sizeof(struct exptree_node)); + if (!node) { + return NULL; + } + *node = (struct exptree_node){.left = NULL, + .right = NULL, + .value = value, + .counter_name = NULL, + .operator= NODE_NULL_OPERATOR }; + return node; +} + +// Set counter and create a leaf node +static struct exptree_node *_make_counter_node(char *counter) +{ + struct exptree_node *node = + (struct exptree_node *)malloc(sizeof(struct exptree_node)); + if (!node) { + return NULL; + } + *node = (struct exptree_node){.left = NULL, + .right = NULL, + .value = NODE_NULL_VALUE, + .counter_name = counter, + .operator= NODE_NULL_OPERATOR }; + return node; +} + +// Parse an operator and create an operator node +static struct exptree_node * +_make_operator_node(char operator, struct exptree_node *left, struct exptree_node *right) +{ + struct exptree_node *node = + (struct exptree_node *)malloc(sizeof(struct exptree_node)); + if (!node) { + return NULL; + } + *node = (struct exptree_node){.left = left, + .right = right, + .value = NODE_NULL_VALUE, + .counter_name = NULL, + .operator= operator}; + return node; +} + +// Parse factors: numbers or subexpressions in parentheses +static struct exptree_node *_make_factor_tree(const char **expr) +{ + _skip_spaces(expr); + if (**expr == '(') { + (*expr)++; // Skip '(' + // Recursively parse the subexpression: + struct exptree_node *subtree = _make_expression_tree(expr); + _skip_spaces(expr); + if (**expr == ')') { + (*expr)++; // Skip ')' + } else { + fprintf(stderr, "Error: Mismatched parentheses\n"); + exit(EXIT_FAILURE); + } + return subtree; + } else { + char *endptr; + double value = strtod(*expr, &endptr); + if (*expr == endptr) { + // no conversion performed + char *counter_name; + if (sscanf(*expr, " %m[^()+-*/ \n] %*s", &counter_name) == 1) { + *expr += strlen(counter_name); + return _make_counter_node(counter_name); + } else { + fprintf(stderr, "Error: Could not parse: %s\n", *expr); + exit(EXIT_FAILURE); + } + } + *expr = endptr; + return _make_value_node(value); + } +} + +// Parse terms: handles multiplication and division +static struct exptree_node *_make_term_tree(const char **expr) +{ + struct exptree_node *left = _make_factor_tree(expr); + while (1) { + _skip_spaces(expr); + if (**expr == '*' || **expr == '/') { + char operator= ** expr; + (*expr)++; + struct exptree_node *right = _make_factor_tree(expr); + left = _make_operator_node(operator, left, right); + } else { + break; + } + } + return left; +} + +// Parse expressions: handles addition and subtraction +static struct exptree_node *_make_expression_tree(const char **expr) +{ + struct exptree_node *left = _make_term_tree(expr); + while (1) { + _skip_spaces(expr); + if (**expr == '+' || **expr == '-') { + char operator= ** expr; + (*expr)++; + struct exptree_node *right = _make_term_tree(expr); + left = _make_operator_node(operator, left, right); + } else { + break; + } + } + return left; +} + +struct exptree_node *make_expression_tree(const char *expr) +{ + return _make_expression_tree(&expr); +} + +// Print the expression tree in in-order traversal +static void _print_expression_tree(const struct exptree_node *node) +{ + if (!node) { + return; + } + if (node->operator) { + printf("("); + } + _print_expression_tree(node->left); + if (node->operator) { + printf(" %c ", node->operator); + } else if (node->counter_name) { + printf("%s", node->counter_name); + } else { + printf("%g", node->value); + } + _print_expression_tree(node->right); + if (node->operator) { + printf(")"); + } +} + +// Print the expression tree in in-order traversal +void print_expression_tree(const struct exptree_node *node) +{ + if (!node) { + printf("Empty expression tree\n"); + return; + } + _print_expression_tree(node); + printf("\n"); +} + +// Free the memory used by the tree +void free_expression_tree(struct exptree_node *node) +{ + if (!node) { + return; + } + free_expression_tree(node->left); + free_expression_tree(node->right); + free(node->counter_name); + free(node); +} + +// Get node value +static double _get_value(const struct exptree_node *node, const CounterList *clist) +{ + if (!node->counter_name) { + return node->value; + } + + size_t len = strlen(node->counter_name); + + /* TODO: set counter index when making the counter node to avoid redundant search */ + /* only ok if order does not change */ + for (int ctr = 0; clist->counters; ++ctr) { + const char *cname = bdata(clist->cnames->entry[ctr]); + + if (len == strlen(cname) && !strncmp(node->counter_name, cname, len)) { + const char *val_str = bdata(clist->cvalues->entry[ctr]); + /* TODO: why are counter values stored as strings instead of unsigned long + * long ? */ + double val = strtod(val_str, NULL); + /* TODO error handling of strtod */ + return val; + } + } + + fprintf(stderr, "Error: counter not found: %s\n", node->counter_name); + return NODE_NULL_VALUE; // TODO: error handling +} + +// Evaluate the expression tree recursively +double evaluate_expression_tree(const struct exptree_node *node, const CounterList *clist) +{ + // TODO: maybe return NAN to indicate error ? + // need to check for NULL in child node evaluation in this case + if (!node) { + return 0.0; + } + + // If it's a leaf node (number/counter), return its value + if (node->operator== NODE_NULL_OPERATOR) { + return _get_value(node, clist); + } + + // Recursively evaluate left and right subtrees + double val_left = evaluate_expression_tree(node->left, clist); + double val_right = evaluate_expression_tree(node->right, clist); + + // Apply the operator + switch (node->operator) { + case '+': + return val_left + val_right; + case '-': + return val_left - val_right; + case '*': + return val_left * val_right; + case '/': + if (val_right == 0.0) { + fprintf(stderr, "Error: Division by zero\n"); + exit(EXIT_FAILURE); + } + return val_left / val_right; + default: + fprintf(stderr, "Error: Unknown operator '%c'\n", node->operator); + exit(EXIT_FAILURE); + } +} diff --git a/src/includes/calculator_exptree.h b/src/includes/calculator_exptree.h new file mode 100644 index 000000000..0aa638c48 --- /dev/null +++ b/src/includes/calculator_exptree.h @@ -0,0 +1,19 @@ +// calculator_exptree.h + +struct exptree_node; // fwd declaration + +// cannot fwd declare CounterList because it was an anonymous struct (changed) +// thus we named CounterList to avoid unnecessary cyclic inclusion dependency with: +// #include "perfgroup.h" +struct CounterList; // fwd declaration + +// TODO: documentation of interfaces +// TODO: do we want "print_expression_tree"? + +extern struct exptree_node *make_expression_tree(const char *expr); + +extern void free_expression_tree(struct exptree_node *root); + +extern double evaluate_expression_tree(const struct exptree_node *node, const struct CounterList *clist); + +extern void print_expression_tree(const struct exptree_node *root); diff --git a/src/includes/likwid.h b/src/includes/likwid.h index 0ce0dc0f2..5039048e2 100644 --- a/src/includes/likwid.h +++ b/src/includes/likwid.h @@ -35,6 +35,8 @@ #include #include +#include "calculator_exptree.h" // fwd declaration of struct exptree_node (maybe move to tree_types.h ?) + #define DEBUGLEV_ONLY_ERROR 0 #define DEBUGLEV_INFO 1 #define DEBUGLEV_DETAIL 2 @@ -1284,6 +1286,7 @@ typedef struct { int nmetrics; /*!< \brief Number of metrics */ char **metricnames; /*!< \brief Metric names */ char **metricformulas; /*!< \brief Metric formulas */ + struct exptree_node **metrictrees; /*!< \brief Metric expression trees */ char *longinfo; /*!< \brief Descriptive text about the group or empty */ } GroupInfo; diff --git a/src/includes/perfgroup.h b/src/includes/perfgroup.h index 4ba1c5669..48e1bf7c4 100644 --- a/src/includes/perfgroup.h +++ b/src/includes/perfgroup.h @@ -34,6 +34,7 @@ #include #include +#include "calculator_exptree.h" typedef enum { GROUP_NONE = 0, @@ -54,7 +55,7 @@ static char* groupFileSectionNames[MAX_GROUP_FILE_SECTIONS] = { "LUA" }; -typedef struct { +typedef struct CounterList { int counters; /*!< \brief Number of entries in the list */ struct bstrList* cnames; /*!< \brief List of counter names */ struct bstrList* cvalues; /*!< \brief List of counter values */ @@ -79,8 +80,7 @@ extern int update_clist(CounterList* clist, char* counter, double result); extern void destroy_clist(CounterList* clist); extern int calc_metric(char* formula, CounterList* clist, double *result); - - +extern int calc_metric_new(const struct exptree_node* tree, const CounterList* clist, double *result); #endif /* PERFGROUP_H */ diff --git a/src/nvmon.c b/src/nvmon.c index af9b575de..142050703 100644 --- a/src/nvmon.c +++ b/src/nvmon.c @@ -1188,8 +1188,7 @@ double nvmon_getMetric(int groupId, int metricId, int gpuId) { return NAN; } - - char* f = ginfo->metricformulas[metricId]; + NvmonDevice_t device = &nvGroupSet->gpus[gpuId]; if (groupId < 0 || groupId >= device->numNvEventSets) { @@ -1207,7 +1206,14 @@ double nvmon_getMetric(int groupId, int metricId, int gpuId) add_to_clist(&clist, "true", 1); add_to_clist(&clist, "false", 0); - e = calc_metric(f, &clist, &result); + double result2; + e = calc_metric(ginfo->metricformulas[metricId], &clist, &result); + e = calc_metric_new(ginfo->metrictrees[metricId], &clist, &result2); + if(fabs(result - result2) > 1.0E-6) { + fprintf(stderr "Error: results don't match"); + exit(EXIT_FAILURE); + } + if (e < 0) { result = NAN; @@ -1251,7 +1257,7 @@ double nvmon_getLastMetric(int groupId, int metricId, int gpuId) { return NAN; } - char* f = ginfo->metricformulas[metricId]; + NvmonDevice_t device = &nvGroupSet->gpus[gpuId]; if (groupId < 0 || groupId >= device->numNvEventSets) { @@ -1268,9 +1274,14 @@ double nvmon_getLastMetric(int groupId, int metricId, int gpuId) add_to_clist(&clist, "inverseClock", 1.0/timer_getCycleClock()); add_to_clist(&clist, "true", 1); add_to_clist(&clist, "false", 0); - - - e = calc_metric(f, &clist, &result); + double result2; + e = calc_metric(ginfo->metricformulas[metricId], &clist, &result); + e = calc_metric_new(ginfo->metrictrees[metricId], &clist, &result2); + if(fabs(result - result2) > 1.0E-6) { + fprintf(stderr "Error: results don't match"); + exit(EXIT_FAILURE); + } + if (e < 0) { result = NAN; @@ -1685,7 +1696,7 @@ double nvmon_getMetricOfRegionGpu(int region, int metricId, int gpuId) { return NAN; } - char *f = ginfo->metricformulas[metricId]; + timer_init(); init_clist(&clist); for (e = 0; e < gMarkerResults[region].eventCount; e++) @@ -1699,7 +1710,14 @@ double nvmon_getMetricOfRegionGpu(int region, int metricId, int gpuId) add_to_clist(&clist, "true", 1); add_to_clist(&clist, "false", 0); - err = calc_metric(f, &clist, &result); + double result2; + e = calc_metric(ginfo->metricformulas[metricId], &clist, &result); + e = calc_metric_new(ginfo->metrictrees[metricId], &clist, &result2); + if(fabs(result - result2) > 1.0E-6) { + fprintf(stderr "Error: results don't match"); + exit(EXIT_FAILURE); + } + if (err < 0) { ERROR_PRINT(Cannot calculate formula %s, f); diff --git a/src/perfgroup.c b/src/perfgroup.c index 42bd4bb72..df1bc2ec4 100644 --- a/src/perfgroup.c +++ b/src/perfgroup.c @@ -51,7 +51,7 @@ #include #include - +#include "calculator_exptree.h" static int totalgroups = 0; @@ -879,6 +879,7 @@ perfgroup_readGroup( ginfo->nmetrics = 0; ginfo->metricformulas = NULL; ginfo->metricnames = NULL; + ginfo->metrictrees = NULL; ginfo->longinfo = NULL; ginfo->groupname = (char*)malloc((strlen(groupname)+10)*sizeof(char)); if (ginfo->groupname == NULL) @@ -1161,6 +1162,23 @@ perfgroup_readGroup( bdestroy(REQUIRE); bdestroy(homepath); bdestroy(fullpath); + + // build expression trees from formulas: + if(ginfo->nmetrics > 0) { + ginfo->metrictrees = calloc(ginfo->nmetrics, sizeof(struct exptree_node *)); + if(!ginfo->metrictrees) { + err = -ENOMEM; + goto cleanup; + } + for(int i = 0; i < ginfo->nmetrics; ++i) { + ginfo->metrictrees[i] = make_expression_tree(ginfo->metricformulas[i]); + if(!ginfo->metrictrees[i]) { + err = -ENOMEM; /* err = ? TODO: proper error handling */ + goto cleanup; + } + } + } + return 0; cleanup: bdestroy(REQUIRE); @@ -1186,11 +1204,16 @@ perfgroup_readGroup( { for(i=0;inmetrics; i++) { - if (ginfo->metricformulas[i]) + if (ginfo->metricformulas[i]) /* ? do you mean ginfo->metricformulas ? */ free(ginfo->metricformulas[i]); - if (ginfo->metricnames[i]) + if (ginfo->metricnames[i]) /* ? do you mean ginfo->metricnames ? */ free(ginfo->metricnames[i]); + + if(ginfo->metrictrees) { + free_expression_tree(ginfo->metrictrees[i]); + } } + free(ginfo->metrictrees); } /*ginfo->shortinfo = NULL; ginfo->nevents = 0; @@ -1217,6 +1240,7 @@ perfgroup_new(GroupInfo* ginfo) ginfo->nmetrics = 0; ginfo->metricformulas = NULL; ginfo->metricnames = NULL; + ginfo->metrictrees = NULL; ginfo->longinfo = NULL; return 0; } @@ -1324,6 +1348,13 @@ perfgroup_addMetric(GroupInfo* ginfo, char* mname, char* mcalc) ERROR_PRINT(Cannot increase space for metricformulas to %d bytes, (ginfo->nmetrics + 1) * sizeof(char*)); return -ENOMEM; } + ginfo->metrictrees = realloc(ginfo->metrictrees, (ginfo->nmetrics + 1) * sizeof(struct exptree_node*)); + if (!ginfo->metrictrees) + { + ERROR_PRINT(Cannot increase space for metrictrees to %d bytes, (ginfo->nmetrics + 1) * sizeof(char*)); + return -ENOMEM; + } + ginfo->metricnames[ginfo->nmetrics] = malloc((strlen(mname) + 1) * sizeof(char)); if (!ginfo->metricnames[ginfo->nmetrics]) { @@ -1347,6 +1378,14 @@ perfgroup_addMetric(GroupInfo* ginfo, char* mname, char* mcalc) { ginfo->metricformulas[ginfo->nmetrics][ret] = '\0'; } + ginfo->metrictrees[ginfo->nmetrics] = make_expression_tree(mcalc); + if (!ginfo->metrictrees[ginfo->nmetrics]) + { + /* TODO: proper error handling */ + // ERROR_PRINT(?); + return -ENOMEM; + } + ginfo->nmetrics++; return 0; } @@ -1517,13 +1556,17 @@ perfgroup_returnGroup(GroupInfo* ginfo) { for(i=0;inmetrics; i++) { - if (ginfo->metricformulas[i]) + if (ginfo->metricformulas[i]) /* ? */ free(ginfo->metricformulas[i]); - if (ginfo->metricnames[i]) + if (ginfo->metricnames[i]) /* ? */ free(ginfo->metricnames[i]); + if (ginfo->metrictrees) { + free_expression_tree(ginfo->metrictrees[i]); + } } free(ginfo->metricformulas); free(ginfo->metricnames); + free(ginfo->metrictrees); } ginfo->groupname = NULL; ginfo->shortinfo = NULL; @@ -1532,6 +1575,7 @@ perfgroup_returnGroup(GroupInfo* ginfo) ginfo->events = NULL; ginfo->metricformulas = NULL; ginfo->metricnames = NULL; + ginfo->metrictrees = NULL; ginfo->nevents = 0; ginfo->nmetrics = 0; } @@ -1656,6 +1700,16 @@ destroy_clist(CounterList* clist) } } +int +calc_metric_new(const struct exptree_node *root, const CounterList* clist, double *result) +{ + double val = evaluate_expression_tree(root, clist); + *result = val; + return 0; + /* TODO: error handling */ + // return -EINVAL; /* negative result means error ? */ +} + int calc_metric(char* formula, CounterList* clist, double *result) { diff --git a/src/perfmon.c b/src/perfmon.c index 248e88344..5ea2a3b96 100644 --- a/src/perfmon.c +++ b/src/perfmon.c @@ -3272,7 +3272,14 @@ perfmon_getMetric(int groupId, int metricId, int threadId) } } } + + double result2; + e = calc_metric_new(groupSet->groups[groupId].group.metrictrees[metricId], &clist, &result2); e = calc_metric(groupSet->groups[groupId].group.metricformulas[metricId], &clist, &result); + if(fabs(result - result2) > 1.0E-6) { + fprintf(stderr, "Error: results don't match"); + exit(EXIT_FAILURE); + } if (e < 0) { result = 0.0; @@ -3362,7 +3369,14 @@ perfmon_getLastMetric(int groupId, int metricId, int threadId) } } } + + double result2; + e = calc_metric_new(groupSet->groups[groupId].group.metrictrees[metricId], &clist, &result2); e = calc_metric(groupSet->groups[groupId].group.metricformulas[metricId], &clist, &result); + if(fabs(result - result2) > 1.0E-6) { + fprintf(stderr, "Error: results don't match"); + exit(EXIT_FAILURE); + } if (e < 0) { result = 0.0; @@ -4032,7 +4046,14 @@ perfmon_getMetricOfRegionThread(int region, int metricId, int threadId) } } } + double result2; + err = calc_metric_new(groupSet->groups[markerResults[region].groupID].group.metrictrees[metricId], &clist, &result2); err = calc_metric(groupSet->groups[markerResults[region].groupID].group.metricformulas[metricId], &clist, &result); + if(fabs(result - result2) > 1.0E-6) { + fprintf(stderr, "Error: results don't match"); + exit(EXIT_FAILURE); + } + if (err < 0) { ERROR_PRINT(Cannot calculate formula %s, groupSet->groups[markerResults[region].groupID].group.metricformulas[metricId]); From 15eff12d4ea2287279f31e946bf2d405528ce861 Mon Sep 17 00:00:00 2001 From: Sergej Breiter Date: Tue, 19 Nov 2024 01:43:10 +0100 Subject: [PATCH 2/5] Fix typos --- src/nvmon.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/nvmon.c b/src/nvmon.c index 142050703..f773c645d 100644 --- a/src/nvmon.c +++ b/src/nvmon.c @@ -1210,7 +1210,7 @@ double nvmon_getMetric(int groupId, int metricId, int gpuId) e = calc_metric(ginfo->metricformulas[metricId], &clist, &result); e = calc_metric_new(ginfo->metrictrees[metricId], &clist, &result2); if(fabs(result - result2) > 1.0E-6) { - fprintf(stderr "Error: results don't match"); + fprintf(stderr, "Error: results don't match"); exit(EXIT_FAILURE); } @@ -1278,7 +1278,7 @@ double nvmon_getLastMetric(int groupId, int metricId, int gpuId) e = calc_metric(ginfo->metricformulas[metricId], &clist, &result); e = calc_metric_new(ginfo->metrictrees[metricId], &clist, &result2); if(fabs(result - result2) > 1.0E-6) { - fprintf(stderr "Error: results don't match"); + fprintf(stderr, "Error: results don't match"); exit(EXIT_FAILURE); } @@ -1711,10 +1711,11 @@ double nvmon_getMetricOfRegionGpu(int region, int metricId, int gpuId) add_to_clist(&clist, "false", 0); double result2; - e = calc_metric(ginfo->metricformulas[metricId], &clist, &result); + char *f = ginfo->metricformulas[metricId]; + e = calc_metric(f, &clist, &result); e = calc_metric_new(ginfo->metrictrees[metricId], &clist, &result2); if(fabs(result - result2) > 1.0E-6) { - fprintf(stderr "Error: results don't match"); + fprintf(stderr, "Error: results don't match"); exit(EXIT_FAILURE); } From 88b3b67d72050c32c359722364a2ea46561a6cc8 Mon Sep 17 00:00:00 2001 From: Sergej Breiter Date: Tue, 19 Nov 2024 14:28:25 +0100 Subject: [PATCH 3/5] Implement calculator error handling --- src/calculator_exptree.c | 81 ++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 33 deletions(-) diff --git a/src/calculator_exptree.c b/src/calculator_exptree.c index cd8b8beec..85e437351 100644 --- a/src/calculator_exptree.c +++ b/src/calculator_exptree.c @@ -1,14 +1,20 @@ #include #include -#include /* CounterList */ #include "calculator_exptree.h" +#include /* CounterList */ +#include /* isspace */ +#include /* errno */ +#include /* NAN / isnan */ +#include /* printf / fprintf */ +#include /* malloc / free */ +#include /* strncmp / strlen */ -#include -#include -#include -#include -#include +#ifndef NAN +# define NAN (0.0/0.0) +#endif +#define NODE_NULL_VALUE NAN +#define NODE_NULL_OPERATOR '\0' struct exptree_node { struct exptree_node *left; // Left child @@ -24,9 +30,6 @@ static struct exptree_node *_make_expression_tree(const char **expr); static struct exptree_node *_make_term_tree(const char **expr); static struct exptree_node *_make_factor_tree(const char **expr); -#define NODE_NULL_VALUE 0.0 -#define NODE_NULL_OPERATOR '\0' - static void _skip_spaces(const char **expr) { while (isspace(**expr)) { @@ -52,8 +55,7 @@ static struct exptree_node *_make_value_node(double value) // Set counter and create a leaf node static struct exptree_node *_make_counter_node(char *counter) { - struct exptree_node *node = - (struct exptree_node *)malloc(sizeof(struct exptree_node)); + struct exptree_node *node = malloc(sizeof(struct exptree_node)); if (!node) { return NULL; } @@ -61,7 +63,7 @@ static struct exptree_node *_make_counter_node(char *counter) .right = NULL, .value = NODE_NULL_VALUE, .counter_name = counter, - .operator= NODE_NULL_OPERATOR }; + .operator = NODE_NULL_OPERATOR }; return node; } @@ -69,8 +71,7 @@ static struct exptree_node *_make_counter_node(char *counter) static struct exptree_node * _make_operator_node(char operator, struct exptree_node *left, struct exptree_node *right) { - struct exptree_node *node = - (struct exptree_node *)malloc(sizeof(struct exptree_node)); + struct exptree_node *node = malloc(sizeof(struct exptree_node)); if (!node) { return NULL; } @@ -78,7 +79,7 @@ _make_operator_node(char operator, struct exptree_node *left, struct exptree_nod .right = right, .value = NODE_NULL_VALUE, .counter_name = NULL, - .operator= operator}; + .operator = operator}; return node; } @@ -100,9 +101,10 @@ static struct exptree_node *_make_factor_tree(const char **expr) return subtree; } else { char *endptr; + errno = 0; double value = strtod(*expr, &endptr); if (*expr == endptr) { - // no conversion performed + // No conversion performed char *counter_name; if (sscanf(*expr, " %m[^()+-*/ \n] %*s", &counter_name) == 1) { *expr += strlen(counter_name); @@ -111,6 +113,9 @@ static struct exptree_node *_make_factor_tree(const char **expr) fprintf(stderr, "Error: Could not parse: %s\n", *expr); exit(EXIT_FAILURE); } + } else if (errno) { + /* Underflow or Overflow */ + /* value is DBL_MIN / HUGE_VAL (TODO: is this ok?) */ } *expr = endptr; return _make_value_node(value); @@ -124,7 +129,7 @@ static struct exptree_node *_make_term_tree(const char **expr) while (1) { _skip_spaces(expr); if (**expr == '*' || **expr == '/') { - char operator= ** expr; + char operator = **expr; (*expr)++; struct exptree_node *right = _make_factor_tree(expr); left = _make_operator_node(operator, left, right); @@ -142,7 +147,7 @@ static struct exptree_node *_make_expression_tree(const char **expr) while (1) { _skip_spaces(expr); if (**expr == '+' || **expr == '-') { - char operator= ** expr; + char operator = **expr; (*expr)++; struct exptree_node *right = _make_term_tree(expr); left = _make_operator_node(operator, left, right); @@ -204,7 +209,7 @@ void free_expression_tree(struct exptree_node *node) free(node); } -// Get node value +// Gets node's value (returns NAN on error) static double _get_value(const struct exptree_node *node, const CounterList *clist) { if (!node->counter_name) { @@ -219,33 +224,43 @@ static double _get_value(const struct exptree_node *node, const CounterList *cli const char *cname = bdata(clist->cnames->entry[ctr]); if (len == strlen(cname) && !strncmp(node->counter_name, cname, len)) { - const char *val_str = bdata(clist->cvalues->entry[ctr]); - /* TODO: why are counter values stored as strings instead of unsigned long - * long ? */ - double val = strtod(val_str, NULL); - /* TODO error handling of strtod */ - return val; + const char *value_str = bdata(clist->cvalues->entry[ctr]); + /* TODO: why are counter values stored as strings instead of + * unsigned long long ? */ + + char *endptr; + errno = 0; + double value = strtod(value_str, &endptr); + if (value_str == endptr) { + // no conversion performed + fprintf(stderr, "Error: no conversion performed on %s\n", value_str); + return NODE_NULL_VALUE; + } else if (errno) { + /* Underflow or Overflow */ + /* value is DBL_MIN / HUGE_VAL (TODO: is this ok?) */ + } + return value; } } fprintf(stderr, "Error: counter not found: %s\n", node->counter_name); - return NODE_NULL_VALUE; // TODO: error handling + return NODE_NULL_VALUE; } -// Evaluate the expression tree recursively +// Evaluates the expression tree recursively (returns NAN on error) double evaluate_expression_tree(const struct exptree_node *node, const CounterList *clist) { - // TODO: maybe return NAN to indicate error ? - // need to check for NULL in child node evaluation in this case if (!node) { - return 0.0; + return NODE_NULL_VALUE; } // If it's a leaf node (number/counter), return its value - if (node->operator== NODE_NULL_OPERATOR) { + if (node->operator == NODE_NULL_OPERATOR) { return _get_value(node, clist); } + // If it's not a leaf node, it must have two child (may change in case of unary operators) + // Recursively evaluate left and right subtrees double val_left = evaluate_expression_tree(node->left, clist); double val_right = evaluate_expression_tree(node->right, clist); @@ -261,11 +276,11 @@ double evaluate_expression_tree(const struct exptree_node *node, const CounterLi case '/': if (val_right == 0.0) { fprintf(stderr, "Error: Division by zero\n"); - exit(EXIT_FAILURE); + return NODE_NULL_VALUE; } return val_left / val_right; default: fprintf(stderr, "Error: Unknown operator '%c'\n", node->operator); - exit(EXIT_FAILURE); + return NODE_NULL_VALUE; } } From 6364d4f9dfa885824801e063dace0a0cd5f4e0cf Mon Sep 17 00:00:00 2001 From: Sergej Breiter Date: Tue, 19 Nov 2024 14:52:57 +0100 Subject: [PATCH 4/5] Add expression tree interface documentation and include guards --- src/calculator_exptree.c | 3 ++ src/includes/calculator_exptree.h | 46 +++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/calculator_exptree.c b/src/calculator_exptree.c index 85e437351..faf5a0c9d 100644 --- a/src/calculator_exptree.c +++ b/src/calculator_exptree.c @@ -1,3 +1,6 @@ +// calculator_exptree.c +// TODO: file description + #include #include #include "calculator_exptree.h" diff --git a/src/includes/calculator_exptree.h b/src/includes/calculator_exptree.h index 0aa638c48..54d33be5b 100644 --- a/src/includes/calculator_exptree.h +++ b/src/includes/calculator_exptree.h @@ -1,19 +1,55 @@ // calculator_exptree.h +// TODO: file description -struct exptree_node; // fwd declaration +#ifndef CALCULATOR_EXPTREE_H_INCLUDED +#define CALCULATOR_EXPTREE_H_INCLUDED + +/** + * @brief Forward declaration of struct exptree_node + * + */ +struct exptree_node; // cannot fwd declare CounterList because it was an anonymous struct (changed) -// thus we named CounterList to avoid unnecessary cyclic inclusion dependency with: +// thus we named CounterList to avoid unnecessary inclusion dependency with: // #include "perfgroup.h" -struct CounterList; // fwd declaration +/** + * @brief Forward declaration of struct CounterList + * + */ +struct CounterList; -// TODO: documentation of interfaces // TODO: do we want "print_expression_tree"? +/** + * @brief Builds a binary expression tree from expression expr + * + * @param expr The expression + * @return struct exptree_node* Expression tree node on success, NULL on error + */ extern struct exptree_node *make_expression_tree(const char *expr); +/** + * @brief Deallocates the binary expression tree + * + * @param root The root node + */ extern void free_expression_tree(struct exptree_node *root); -extern double evaluate_expression_tree(const struct exptree_node *node, const struct CounterList *clist); +/** + * @brief Evaluates the expression tree with values from counter list + * + * @param root The root node + * @param clist The counter list + * @return double The value of the expression tree evaluation on success, NAN on error + */ +extern double evaluate_expression_tree(const struct exptree_node *root, const struct CounterList *clist); +/** + * @brief Prints the expression tree in infix order + * + * @param root The root node + */ extern void print_expression_tree(const struct exptree_node *root); + +#endif /* CALCULATOR_EXPTREE_H_INCLUDED */ From c4a3d78845781db15713ec27a6ce068391bf5ada Mon Sep 17 00:00:00 2001 From: Sergej Breiter Date: Wed, 20 Nov 2024 09:06:20 +0100 Subject: [PATCH 5/5] Fix missing bstrlib.h include --- src/cpustring.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cpustring.c b/src/cpustring.c index 65082b20a..2005a9157 100644 --- a/src/cpustring.c +++ b/src/cpustring.c @@ -34,6 +34,7 @@ #include #include +#include #include #define MAX(a, b) (((a) > (b)) ? (a) : (b))