[PATCH 0/3] tool lib traceevent: Fix some parsing errors

[PATCH 0/3] tool lib traceevent: Fix some parsing errors

[Steven Rostedt]

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(-)

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

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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) <rostedt@goodmis.org>
---
 tools/lib/traceevent/event-parse.c | 98 ++++++++++++------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index e1bd2a93c6db..eec96c31ea9e 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -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);
 		}
-- 
2.25.1

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

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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) <rostedt@goodmis.org>
---
 tools/lib/traceevent/event-parse.c | 39 +++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index eec96c31ea9e..010e60d5a081 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -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;
-- 
2.25.1

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

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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/20200320055823.27089-3-jaewon31.kim@samsung.com/

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/lib/traceevent/event-parse.c | 35 ++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 010e60d5a081..5b36c589a029 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -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) {
-- 
2.25.1

Re: [PATCH 0/3] tool lib traceevent: Fix some parsing errors

[Steven Rostedt]

Hi Arnaldo,

I guess these patches were missed.

-- Steve


On Tue, 24 Mar 2020 16:08:45 -0400
Steven Rostedt <rostedt@goodmis.org> 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(-)

Re: [PATCH 0/3] tool lib traceevent: Fix some parsing errors

[Arnaldo Carvalho de Melo]

On June 17, 2020 6:28:04 PM GMT-03:00, Steven Rostedt <rostedt@goodmis.org> wrote:
>Hi Arnaldo,
>
>I guess these patches were missed.

I'll pick those tomorrow,

Thanks for the reminder,


- Arnaldo

>
>-- Steve
>
>
>On Tue, 24 Mar 2020 16:08:45 -0400
>Steven Rostedt <rostedt@goodmis.org> 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(-)

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

Re: [PATCH 0/3] tool lib traceevent: Fix some parsing errors

[Arnaldo Carvalho de Melo]

On June 17, 2020 6:28:04 PM GMT-03:00, Steven Rostedt <rostedt@goodmis.org> wrote:
>Hi Arnaldo,
>
>I guess these patches were missed.

I'll pick those tomorrow,

Thanks for the reminder,


- Arnaldo

>
>-- Steve
>
>
>On Tue, 24 Mar 2020 16:08:45 -0400
>Steven Rostedt <rostedt@goodmis.org> 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(-)

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

Re: [PATCH 0/3] tool lib traceevent: Fix some parsing errors

[Arnaldo Carvalho de Melo]

Em Wed, Jun 17, 2020 at 07:55:29PM -0300, Arnaldo Carvalho de Melo escreveu:
> 
> 
> On June 17, 2020 6:28:04 PM GMT-03:00, Steven Rostedt <rostedt@goodmis.org> wrote:
> >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
 
> 
> - Arnaldo
> 
> >
> >-- Steve
> >
> >
> >On Tue, 24 Mar 2020 16:08:45 -0400
> >Steven Rostedt <rostedt@goodmis.org> 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(-)
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> 

-- 

- Arnaldo