tool lib traceevent: Fix some parsing errors

PATCH Applied Steven Rostedt 3 participants 7 messages Mar 24, 2020 – Jun 18, 2020 Memory management

lore

Share & tools

lore

Steven Rostedt

author

2020-03-24 20:10 UTC #1 lore raw

PATCH 1/3 tools lib traceevent: Add append() function helper for appending strings

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

There's several locations that open code realloc and strcat() to append text to strings. Add an append() function that takes a delimiter and a string to append to another string.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 tools/lib/traceevent/event-parse.c | 98 ++++++++++++------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

tools/lib/traceevent/event-parse.c +40 ~~-58~~

index e1bd2a93c6db..eec96c31ea9e 100644
@@ -1425,6 +1425,19 @@ static unsigned int type_size(const char *name)
 	return 0;
 }
 
+static int append(char **buf, const char *delim, const char *str)
+{
+	char *new_buf;
+
+	new_buf = realloc(*buf, strlen(*buf) + strlen(delim) + strlen(str) + 1);
+	if (!new_buf)
+		return -1;
+	strcat(new_buf, delim);
+	strcat(new_buf, str);
+	*buf = new_buf;
+	return 0;
+}
+
 static int event_read_fields(struct tep_event *event, struct tep_format_field **fields)
 {
 	struct tep_format_field *field = NULL;

@@ -1432,6 +1445,7 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 	char *token;
 	char *last_token;
 	int count = 0;
+	int ret;
 
 	do {
 		unsigned int size_dynamic = 0;

@@ -1490,24 +1504,15 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 					field->flags |= TEP_FIELD_IS_POINTER;
 
 				if (field->type) {
-					char *new_type;
-					new_type = realloc(field->type,
-							   strlen(field->type) +
-							   strlen(last_token) + 2);
-					if (!new_type) {
-						free(last_token);
-						goto fail;
-					}
-					field->type = new_type;
-					strcat(field->type, " ");
-					strcat(field->type, last_token);
+					ret = append(&field->type, " ", last_token);
 					free(last_token);
+					if (ret < 0)
+						goto fail;
 				} else
 					field->type = last_token;
 				last_token = token;
 				continue;
 			}
-
 			break;
 		}

@@ -1523,8 +1528,6 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 		if (strcmp(token, "[") == 0) {
 			enum tep_event_type last_type = type;
 			char *brackets = token;
-			char *new_brackets;
-			int len;
 
 			field->flags |= TEP_FIELD_IS_ARRAY;

@@ -1536,29 +1539,27 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 				field->arraylen = 0;
 
 		        while (strcmp(token, "]") != 0) {
+				const char *delim;
+
 				if (last_type == TEP_EVENT_ITEM &&
 				    type == TEP_EVENT_ITEM)
-					len = 2;
+					delim = " ";
 				else
-					len = 1;
+					delim = "";
+
 				last_type = type;
 
-				new_brackets = realloc(brackets,
-						       strlen(brackets) +
-						       strlen(token) + len);
-				if (!new_brackets) {
+				ret = append(&brackets, delim, token);
+				if (ret < 0) {
 					free(brackets);
 					goto fail;
 				}
-				brackets = new_brackets;
-				if (len == 2)
-					strcat(brackets, " ");
-				strcat(brackets, token);
 				/* We only care about the last token */
 				field->arraylen = strtoul(token, NULL, 0);
 				free_token(token);
 				type = read_token(&token);
 				if (type == TEP_EVENT_NONE) {
+					free(brackets);
 					do_warning_event(event, "failed to find token");
 					goto fail;
 				}

@@ -1566,13 +1567,11 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 
 			free_token(token);
 
-			new_brackets = realloc(brackets, strlen(brackets) + 2);
-			if (!new_brackets) {
+			ret = append(&brackets, "", "]");
+			if (ret < 0) {
 				free(brackets);
 				goto fail;
 			}
-			brackets = new_brackets;
-			strcat(brackets, "]");
 
 			/* add brackets to type */

@@ -1582,34 +1581,23 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 			 * the format: type [] item;
 			 */
 			if (type == TEP_EVENT_ITEM) {
-				char *new_type;
-				new_type = realloc(field->type,
-						   strlen(field->type) +
-						   strlen(field->name) +
-						   strlen(brackets) + 2);
-				if (!new_type) {
+				ret = append(&field->type, " ", field->name);
+				if (ret < 0) {
 					free(brackets);
 					goto fail;
 				}
-				field->type = new_type;
-				strcat(field->type, " ");
-				strcat(field->type, field->name);
+				ret = append(&field->type, "", brackets);
+
 				size_dynamic = type_size(field->name);
 				free_token(field->name);
-				strcat(field->type, brackets);
 				field->name = field->alias = token;
 				type = read_token(&token);
 			} else {
-				char *new_type;
-				new_type = realloc(field->type,
-						   strlen(field->type) +
-						   strlen(brackets) + 1);
-				if (!new_type) {
+				ret = append(&field->type, "", brackets);
+				if (ret < 0) {
 					free(brackets);
 					goto fail;
 				}
-				field->type = new_type;
-				strcat(field->type, brackets);
 			}
 			free(brackets);
 		}

@@ -2046,19 +2034,16 @@ process_op(struct tep_event *event, struct tep_print_arg *arg, char **tok)
 		/* could just be a type pointer */
 		if ((strcmp(arg->op.op, "*") == 0) &&
 		    type == TEP_EVENT_DELIM && (strcmp(token, ")") == 0)) {
-			char *new_atom;
+			int ret;
 
 			if (left->type != TEP_PRINT_ATOM) {
 				do_warning_event(event, "bad pointer type");
 				goto out_free;
 			}
-			new_atom = realloc(left->atom.atom,
-					    strlen(left->atom.atom) + 3);
-			if (!new_atom)
+			ret = append(&left->atom.atom, " ", "*");
+			if (ret < 0)
 				goto out_warn_free;
 
-			left->atom.atom = new_atom;
-			strcat(left->atom.atom, " *");
 			free(arg->op.op);
 			*arg = *left;
 			free(left);

@@ -3151,18 +3136,15 @@ process_arg_token(struct tep_event *event, struct tep_print_arg *arg,
 		}
 		/* atoms can be more than one token long */
 		while (type == TEP_EVENT_ITEM) {
-			char *new_atom;
-			new_atom = realloc(atom,
-					   strlen(atom) + strlen(token) + 2);
-			if (!new_atom) {
+			int ret;
+
+			ret = append(&atom, " ", token);
+			if (ret < 0) {
 				free(atom);
 				*tok = NULL;
 				free_token(token);
 				return TEP_EVENT_ERROR;
 			}
-			atom = new_atom;
-			strcat(atom, " ");
-			strcat(atom, token);
 			free_token(token);
 			type = read_token_item(&token);
 		}

Signature · 1 line

2.25.1

Steven Rostedt

author

2020-03-24 20:10 UTC #2 lore raw

PATCH 2/3 tools lib traceevent: Handle __attribute__((user)) in field names

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

Commit c61f13eaa1ee1 ("gcc-plugins: Add structleak for more stack initialization") added "__attribute__((user))" to the user when stackleak detector is enabled. This now appears in the field format of system call trace events for system calls that have user buffers. The "__attribute__((user))" breaks the parsing in libtraceevent. That needs to be handled.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 tools/lib/traceevent/event-parse.c | 39 +++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

tools/lib/traceevent/event-parse.c +38 -1

index eec96c31ea9e..010e60d5a081 100644
@@ -1444,6 +1444,7 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 	enum tep_event_type type;
 	char *token;
 	char *last_token;
+	char *delim = " ";
 	int count = 0;
 	int ret;

@@ -1504,13 +1505,49 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 					field->flags |= TEP_FIELD_IS_POINTER;
 
 				if (field->type) {
-					ret = append(&field->type, " ", last_token);
+					ret = append(&field->type, delim, last_token);
 					free(last_token);
 					if (ret < 0)
 						goto fail;
 				} else
 					field->type = last_token;
 				last_token = token;
+				delim = " ";
+				continue;
+			}
+
+			/* Handle __attribute__((user)) */
+			if ((type == TEP_EVENT_DELIM) &&
+			    strcmp("__attribute__", last_token) == 0 &&
+			    token[0] == '(') {
+				int depth = 1;
+				int ret;
+
+				ret = append(&field->type, " ", last_token);
+				ret |= append(&field->type, "", "(");
+				if (ret < 0)
+					goto fail;
+
+				delim = " ";
+				while ((type = read_token(&token)) != TEP_EVENT_NONE) {
+					if (type == TEP_EVENT_DELIM) {
+						if (token[0] == '(')
+							depth++;
+						else if (token[0] == ')')
+							depth--;
+						if (!depth)
+							break;
+						ret = append(&field->type, "", token);
+						delim = "";
+					} else {
+						ret = append(&field->type, delim, token);
+						delim = " ";
+					}
+					if (ret < 0)
+						goto fail;
+					free(last_token);
+					last_token = token;
+				}
 				continue;
 			}
 			break;

Signature · 1 line

2.25.1

Steven Rostedt

author

2020-03-24 20:10 UTC #0 lore raw

cover tool lib traceevent: Fix some parsing errors

Arnaldo,

Here's a few patches for lib traceevent.

The first one adds an "append()" helper function for appending to strings, which will also simplify the next patch.

The second patch handles "__attribute__((user))" in the field of a trace event. This is needed after the stack leak code added this to parameters of system call events.

The last patch adds handling of __builtin_expect(), as someone wanted to move IS_ERR_VALUE() from the fast path into the print fmt section, which breaks the parsing. This should also be useful for other macros that may evaluate down to a __builtin_expect().

Steven Rostedt (VMware) (3):
tools lib traceevent: Add append() function helper for appending strings tools lib traceevent: Handle __attribute__((user)) in field names tools lib traceevent: Add handler for __builtin_expect()

----

 tools/lib/traceevent/event-parse.c | 168 ++++++++++++++++++++++++-------------
 1 file changed, 111 insertions(+), 57 deletions(-)

Steven Rostedt

author

2020-03-24 20:10 UTC #3 lore raw

PATCH 3/3 tools lib traceevent: Add handler for __builtin_expect()

From: "Steven Rostedt (VMware)" <[email protected]>

In order to move pointer checks like IS_ERR_VALUE() out of the hotpath and into the reader path of a trace event, user space tools need to be able to parse that. IS_ERR_VALUE() is defined as:

#define IS_ERR_VALUE() unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)

Which eventually turns into:

__builtin_expect(!!((unsigned long)(void *)(x) >= (unsigned long)-4095), 0)

Now the traceevent parser can handle most of that except for the __builtin_expect(), which needs to be added.

Link https://lore.kernel.org/linux-mm/[email protected]/ open here

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 tools/lib/traceevent/event-parse.c | 35 ++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

tools/lib/traceevent/event-parse.c +35 -0

index 010e60d5a081..5b36c589a029 100644
@@ -3084,6 +3084,37 @@ process_func_handler(struct tep_event *event, struct tep_function_handler *func,
 	return TEP_EVENT_ERROR;
 }
 
+static enum tep_event_type
+process_builtin_expect(struct tep_event *event, struct tep_print_arg *arg, char **tok)
+{
+	enum tep_event_type type;
+	char *token = NULL;
+
+	/* Handle __builtin_expect( cond, #) */
+	type = process_arg(event, arg, &token);
+
+	if (type != TEP_EVENT_DELIM || token[0] != ',')
+		goto out_free;
+
+	free_token(token);
+
+	/* We don't care what the second parameter is of the __builtin_expect() */
+	if (read_expect_type(TEP_EVENT_ITEM, &token) < 0)
+		goto out_free;
+
+	if (read_expected(TEP_EVENT_DELIM, ")") < 0)
+		goto out_free;
+
+	free_token(token);
+	type = read_token_item(tok);
+	return type;
+
+out_free:
+	free_token(token);
+	*tok = NULL;
+	return TEP_EVENT_ERROR;
+}
+
 static enum tep_event_type
 process_function(struct tep_event *event, struct tep_print_arg *arg,
 		 char *token, char **tok)

@@ -3128,6 +3159,10 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
 		free_token(token);
 		return process_dynamic_array_len(event, arg, tok);
 	}
+	if (strcmp(token, "__builtin_expect") == 0) {
+		free_token(token);
+		return process_builtin_expect(event, arg, tok);
+	}
 
 	func = find_func_handler(event->tep, token);
 	if (func) {

Signature · 1 line

2.25.1

Steven Rostedt

author

2020-06-17 21:28 UTC #4 lore raw

In reply to Steven Rostedt

Hi Arnaldo,

I guess these patches were missed.

-- Steve

Quoted 24 lines · On Tue, 24 Mar 2020 16:08:45 -0400 Steven Rostedt <[email protected]> wrote:

Arnaldo,
Here's a few patches for lib traceevent.
The first one adds an "append()" helper function for appending to strings, which will also simplify the next patch.
The second patch handles "__attribute__((user))" in the field of a trace event. This is needed after the stack leak code added this to parameters of system call events.
The last patch adds handling of __builtin_expect(), as someone wanted to move IS_ERR_VALUE() from the fast path into the print fmt section, which breaks the parsing. This should also be useful for other macros that may evaluate down to a __builtin_expect().
Steven Rostedt (VMware) (3):
tools lib traceevent: Add append() function helper for appending strings tools lib traceevent: Handle __attribute__((user)) in field names tools lib traceevent: Add handler for __builtin_expect()
----
 tools/lib/traceevent/event-parse.c | 168 ++++++++++++++++++++++++-------------
 1 file changed, 111 insertions(+), 57 deletions(-)

Arnaldo Carvalho de Melo

2020-06-17 22:56 UTC #5 lore raw

Quoted 3 lines · Steven Rostedt

Hi Arnaldo,
I guess these patches were missed.

I'll pick those tomorrow,

Thanks for the reminder,

- Arnaldo

Quoted 35 lines

-- Steve
Quoted 5 lines · On Tue, 24 Mar 2020 16:08:45 -0400 Steven Rostedt <[email protected]> wrote:
Arnaldo,
Here's a few patches for lib traceevent.
The first one adds an "append()" helper function for appending to
strings,
Quoted 6 lines
which will also simplify the next patch.
The second patch handles "__attribute__((user))" in the field of a trace event. This is needed after the stack leak code added this to parameters of system call events.
Quoted 7 lines
The last patch adds handling of __builtin_expect(), as someone wanted to move IS_ERR_VALUE() from the fast path into the print fmt section, which breaks the parsing. This should also be useful for other macros that may evaluate down to a __builtin_expect().
Steven Rostedt (VMware) (3):
tools lib traceevent: Add append() function helper for
appending strings
Quoted 1 line
tools lib traceevent: Handle __attribute__((user)) in field
names
Quoted 4 lines
tools lib traceevent: Add handler for __builtin_expect()
----
 tools/lib/traceevent/event-parse.c | 168
++++++++++++++++++++++++-------------
Quoted 1 line
 1 file changed, 111 insertions(+), 57 deletions(-)

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Arnaldo Carvalho de Melo

Applied

2020-06-18 13:24 UTC #6 lore raw

Em Wed, Jun 17, 2020 at 07:55:29PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 10 lines · Steven Rostedt

Quoted 3 lines
Hi Arnaldo,
I guess these patches were missed.
I'll pick those tomorrow,
Thanks for the reminder,

Thanks, applied to perf/urgent, for v5.8.

- Arnaldo

Quoted 42 lines

- Arnaldo
Quoted 35 lines
-- Steve
Quoted 5 lines · On Tue, 24 Mar 2020 16:08:45 -0400 Steven Rostedt <[email protected]> wrote:
Arnaldo,
Here's a few patches for lib traceevent.
The first one adds an "append()" helper function for appending to
strings,
Quoted 6 lines
which will also simplify the next patch.
The second patch handles "__attribute__((user))" in the field of a trace event. This is needed after the stack leak code added this to parameters of system call events.
Quoted 7 lines
The last patch adds handling of __builtin_expect(), as someone wanted to move IS_ERR_VALUE() from the fast path into the print fmt section, which breaks the parsing. This should also be useful for other macros that may evaluate down to a __builtin_expect().
Steven Rostedt (VMware) (3):
tools lib traceevent: Add append() function helper for
appending strings
Quoted 1 line
tools lib traceevent: Handle __attribute__((user)) in field
names
Quoted 4 lines
tools lib traceevent: Add handler for __builtin_expect()
----
 tools/lib/traceevent/event-parse.c | 168
++++++++++++++++++++++++-------------
Quoted 1 line
 1 file changed, 111 insertions(+), 57 deletions(-)
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Signature · 1 line

- Arnaldo