[PATCH] libtraceevent: Add __rel_loc support

[PATCH] libtraceevent: Add __rel_loc support

[Masami Hiramatsu]

Hello,

Here is the patch to add new '__rel_loc' relative dynamic
array attribute support. The patch series for linux kernel
corresponding to this patch is here;

https://lore.kernel.org/all/163757340321.510314.9399950115238632705.stgit@devnote2/T/#u

(I've CC'ed to this mailing list too)

This patch is the porting of [4/5] in above patch to the
libtraceevent.

Thank you,

---

Masami Hiramatsu (1):
      libtraceevent: Add __rel_loc relative location attribute support


 src/event-parse.c  |   54 +++++++++++++++++++++++++++++++++-------------------
 src/event-parse.h  |    5 +++--
 src/parse-filter.c |    5 ++++-
 3 files changed, 41 insertions(+), 23 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH] libtraceevent: Add __rel_loc relative location attribute support

[Masami Hiramatsu]

Add '__rel_loc' new dynamic data location attribute which encodes
the data location from the next to the field itself. This is similar
to the '__data_loc' but the location offset is not from the event
entry but from the next of the field.

This patch adds '__rel_loc' decoding support in the libtraceevent.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 src/event-parse.c  |   54 +++++++++++++++++++++++++++++++++-------------------
 src/event-parse.h  |    5 +++--
 src/parse-filter.c |    5 ++++-
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/src/event-parse.c b/src/event-parse.c
index 70637586bc9a..2781cac26eeb 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -1529,6 +1529,14 @@ static int field_is_dynamic(struct tep_format_field *field)
 	return 0;
 }
 
+static int field_is_relative_dynamic(struct tep_format_field *field)
+{
+	if (strncmp(field->type, "__rel_loc", 9) == 0)
+		return 1;
+
+	return 0;
+}
+
 static int field_is_long(struct tep_format_field *field)
 {
 	/* includes long long */
@@ -1784,6 +1792,8 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 			field->flags |= TEP_FIELD_IS_STRING;
 		if (field_is_dynamic(field))
 			field->flags |= TEP_FIELD_IS_DYNAMIC;
+		if (field_is_relative_dynamic(field))
+			field->flags |= TEP_FIELD_IS_DYNAMIC | TEP_FIELD_IS_RELATIVE;
 		if (field_is_long(field))
 			field->flags |= TEP_FIELD_IS_LONG;
 
@@ -3113,7 +3123,7 @@ process_str(struct tep_event *event __maybe_unused, struct tep_print_arg *arg,
 
 	arg->type = TEP_PRINT_STRING;
 	arg->string.string = token;
-	arg->string.offset = -1;
+	arg->string.field = NULL;
 
 	if (read_expected(TEP_EVENT_DELIM, ")") < 0)
 		goto out_err;
@@ -3142,7 +3152,7 @@ process_bitmask(struct tep_event *event __maybe_unused, struct tep_print_arg *ar
 
 	arg->type = TEP_PRINT_BITMASK;
 	arg->bitmask.bitmask = token;
-	arg->bitmask.offset = -1;
+	arg->bitmask.field = NULL;
 
 	if (read_expected(TEP_EVENT_DELIM, ")") < 0)
 		goto out_err;
@@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
 		free_token(token);
 		return process_int_array(event, arg, tok);
 	}
-	if (strcmp(token, "__get_str") == 0) {
+	if (strcmp(token, "__get_str") == 0 ||
+	    strcmp(token, "__get_rel_str") == 0) {
 		free_token(token);
 		return process_str(event, arg, tok);
 	}
-	if (strcmp(token, "__get_bitmask") == 0) {
+	if (strcmp(token, "__get_bitmask") == 0 ||
+	    strcmp(token, "__get_rel_bitmask") == 0) {
 		free_token(token);
 		return process_bitmask(event, arg, tok);
 	}
-	if (strcmp(token, "__get_dynamic_array") == 0) {
+	if (strcmp(token, "__get_dynamic_array") == 0 ||
+	    strcmp(token, "__get_rel_dynamic_array") == 0) {
 		free_token(token);
 		return process_dynamic_array(event, arg, tok);
 	}
-	if (strcmp(token, "__get_dynamic_array_len") == 0) {
+	if (strcmp(token, "__get_dynamic_array_len") == 0 ||
+	    strcmp(token, "__get_rel_dynamic_array_len") == 0) {
 		free_token(token);
 		return process_dynamic_array_len(event, arg, tok);
 	}
@@ -3886,6 +3900,8 @@ static inline void dynamic_offset_field(struct tep_handle *tep,
 					unsigned int *len)
 {
 	dynamic_offset(tep, field->size, data + field->offset, offset, len);
+	if (field->flags & TEP_FIELD_IS_RELATIVE)
+		offset += field->offset + field->size;
 }
 
 static unsigned long long
@@ -4398,13 +4414,11 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
 	case TEP_PRINT_TYPE:
 		break;
 	case TEP_PRINT_STRING: {
-		if (arg->string.offset == -1) {
-			struct tep_format_field *f;
-
-			f = tep_find_any_field(event, arg->string.string);
-			arg->string.offset = f->offset;
-		}
-		dynamic_offset(tep, 4, data + arg->string.offset, &offset, &len);
+		if (!arg->string.field)
+			arg->string.field = tep_find_any_field(event, arg->string.string);
+		if (!arg->string.field)
+			break;
+		dynamic_offset_field(tep, arg->string.field, data, &offset, &len);
 		/* Do not attempt to save zero length dynamic strings */
 		if (!len)
 			break;
@@ -4415,13 +4429,11 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
 		print_str_to_seq(s, format, len_arg, arg->string.string);
 		break;
 	case TEP_PRINT_BITMASK: {
-		if (arg->bitmask.offset == -1) {
-			struct tep_format_field *f;
-
-			f = tep_find_any_field(event, arg->bitmask.bitmask);
-			arg->bitmask.offset = f->offset;
-		}
-		dynamic_offset(tep, 4, data + arg->bitmask.offset, &offset, &len);
+		if (!arg->bitmask.field)
+			arg->bitmask.field = tep_find_any_field(event, arg->bitmask.bitmask);
+		if (!arg->bitmask.field)
+			break;
+		dynamic_offset_field(tep, arg->bitmask.field, data, &offset, &len);
 		print_bitmask_to_seq(tep, s, format, len_arg,
 				     data + offset, len);
 		break;
@@ -7343,6 +7355,8 @@ void *tep_get_field_raw(struct trace_seq *s, struct tep_event *event,
 					 data + offset, field->size);
 		*len = offset >> 16;
 		offset &= 0xffff;
+		if (field->flags & TEP_FIELD_IS_RELATIVE)
+			offset += field->offset + field->size;
 	} else
 		*len = field->size;
 
diff --git a/src/event-parse.h b/src/event-parse.h
index 083389358127..db8bc6db5431 100644
--- a/src/event-parse.h
+++ b/src/event-parse.h
@@ -125,6 +125,7 @@ enum tep_format_flags {
 	TEP_FIELD_IS_LONG	= 32,
 	TEP_FIELD_IS_FLAG	= 64,
 	TEP_FIELD_IS_SYMBOLIC	= 128,
+	TEP_FIELD_IS_RELATIVE	= 256,
 };
 
 struct tep_format_field {
@@ -153,12 +154,12 @@ struct tep_print_arg_atom {
 
 struct tep_print_arg_string {
 	char			*string;
-	int			offset;
+	struct tep_format_field	*field;
 };
 
 struct tep_print_arg_bitmask {
 	char			*bitmask;
-	int			offset;
+	struct tep_format_field	*field;
 };
 
 struct tep_print_arg_field {
diff --git a/src/parse-filter.c b/src/parse-filter.c
index 368826bb5a57..5df177070d53 100644
--- a/src/parse-filter.c
+++ b/src/parse-filter.c
@@ -1712,8 +1712,11 @@ static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record *
 
 		if (arg->str.field->flags & TEP_FIELD_IS_DYNAMIC) {
 			addr = *(unsigned int *)val;
-			val = record->data + (addr & 0xffff);
 			size = addr >> 16;
+			addr &= 0xffff;
+			if (arg->str.field->flags & TEP_FIELD_IS_RELATIVE)
+				addr += arg->str.field->offset + arg->str.field->size;
+			val = record->data + addr;
 		}
 
 		/*

Re: [PATCH] libtraceevent: Add __rel_loc relative location attribute support

[Steven Rostedt]

On Mon, 22 Nov 2021 18:36:57 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> @@ -3886,6 +3900,8 @@ static inline void dynamic_offset_field(struct tep_handle *tep,
>  					unsigned int *len)
>  {
>  	dynamic_offset(tep, field->size, data + field->offset, offset, len);
> +	if (field->flags & TEP_FIELD_IS_RELATIVE)
> +		offset += field->offset + field->size;

I think you want *offset += field->offset + field->size;

-- Steve

>  }
>  

Re: [PATCH] libtraceevent: Add __rel_loc relative location attribute support

[Masami Hiramatsu]

On Mon, 22 Nov 2021 22:39:09 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 22 Nov 2021 18:36:57 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > @@ -3886,6 +3900,8 @@ static inline void dynamic_offset_field(struct tep_handle *tep,
> >  					unsigned int *len)
> >  {
> >  	dynamic_offset(tep, field->size, data + field->offset, offset, len);
> > +	if (field->flags & TEP_FIELD_IS_RELATIVE)
> > +		offset += field->offset + field->size;
> 
> I think you want *offset += field->offset + field->size;

Oops, good catch! Yes, that's right.

BTW, are there any tool I can use for testing this library?

Thank you,

> 
> -- Steve
> 
> >  }
> >  


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Masami Hiramatsu]

Add '__rel_loc' new dynamic data location attribute which encodes
the data location from the next to the field itself. This is similar
to the '__data_loc' but the location offset is not from the event
entry but from the next of the field.

This patch adds '__rel_loc' decoding support in the libtraceevent.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
   - Fix to adjust '*offset' instead of 'offset' in dynamic_offset_field()
---
 src/event-parse.c  |   54 +++++++++++++++++++++++++++++++++-------------------
 src/event-parse.h  |    5 +++--
 src/parse-filter.c |    5 ++++-
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/src/event-parse.c b/src/event-parse.c
index 70637586bc9a..a88c73a8d0fb 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -1529,6 +1529,14 @@ static int field_is_dynamic(struct tep_format_field *field)
 	return 0;
 }
 
+static int field_is_relative_dynamic(struct tep_format_field *field)
+{
+	if (strncmp(field->type, "__rel_loc", 9) == 0)
+		return 1;
+
+	return 0;
+}
+
 static int field_is_long(struct tep_format_field *field)
 {
 	/* includes long long */
@@ -1784,6 +1792,8 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field **
 			field->flags |= TEP_FIELD_IS_STRING;
 		if (field_is_dynamic(field))
 			field->flags |= TEP_FIELD_IS_DYNAMIC;
+		if (field_is_relative_dynamic(field))
+			field->flags |= TEP_FIELD_IS_DYNAMIC | TEP_FIELD_IS_RELATIVE;
 		if (field_is_long(field))
 			field->flags |= TEP_FIELD_IS_LONG;
 
@@ -3113,7 +3123,7 @@ process_str(struct tep_event *event __maybe_unused, struct tep_print_arg *arg,
 
 	arg->type = TEP_PRINT_STRING;
 	arg->string.string = token;
-	arg->string.offset = -1;
+	arg->string.field = NULL;
 
 	if (read_expected(TEP_EVENT_DELIM, ")") < 0)
 		goto out_err;
@@ -3142,7 +3152,7 @@ process_bitmask(struct tep_event *event __maybe_unused, struct tep_print_arg *ar
 
 	arg->type = TEP_PRINT_BITMASK;
 	arg->bitmask.bitmask = token;
-	arg->bitmask.offset = -1;
+	arg->bitmask.field = NULL;
 
 	if (read_expected(TEP_EVENT_DELIM, ")") < 0)
 		goto out_err;
@@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
 		free_token(token);
 		return process_int_array(event, arg, tok);
 	}
-	if (strcmp(token, "__get_str") == 0) {
+	if (strcmp(token, "__get_str") == 0 ||
+	    strcmp(token, "__get_rel_str") == 0) {
 		free_token(token);
 		return process_str(event, arg, tok);
 	}
-	if (strcmp(token, "__get_bitmask") == 0) {
+	if (strcmp(token, "__get_bitmask") == 0 ||
+	    strcmp(token, "__get_rel_bitmask") == 0) {
 		free_token(token);
 		return process_bitmask(event, arg, tok);
 	}
-	if (strcmp(token, "__get_dynamic_array") == 0) {
+	if (strcmp(token, "__get_dynamic_array") == 0 ||
+	    strcmp(token, "__get_rel_dynamic_array") == 0) {
 		free_token(token);
 		return process_dynamic_array(event, arg, tok);
 	}
-	if (strcmp(token, "__get_dynamic_array_len") == 0) {
+	if (strcmp(token, "__get_dynamic_array_len") == 0 ||
+	    strcmp(token, "__get_rel_dynamic_array_len") == 0) {
 		free_token(token);
 		return process_dynamic_array_len(event, arg, tok);
 	}
@@ -3886,6 +3900,8 @@ static inline void dynamic_offset_field(struct tep_handle *tep,
 					unsigned int *len)
 {
 	dynamic_offset(tep, field->size, data + field->offset, offset, len);
+	if (field->flags & TEP_FIELD_IS_RELATIVE)
+		*offset += field->offset + field->size;
 }
 
 static unsigned long long
@@ -4398,13 +4414,11 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
 	case TEP_PRINT_TYPE:
 		break;
 	case TEP_PRINT_STRING: {
-		if (arg->string.offset == -1) {
-			struct tep_format_field *f;
-
-			f = tep_find_any_field(event, arg->string.string);
-			arg->string.offset = f->offset;
-		}
-		dynamic_offset(tep, 4, data + arg->string.offset, &offset, &len);
+		if (!arg->string.field)
+			arg->string.field = tep_find_any_field(event, arg->string.string);
+		if (!arg->string.field)
+			break;
+		dynamic_offset_field(tep, arg->string.field, data, &offset, &len);
 		/* Do not attempt to save zero length dynamic strings */
 		if (!len)
 			break;
@@ -4415,13 +4429,11 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
 		print_str_to_seq(s, format, len_arg, arg->string.string);
 		break;
 	case TEP_PRINT_BITMASK: {
-		if (arg->bitmask.offset == -1) {
-			struct tep_format_field *f;
-
-			f = tep_find_any_field(event, arg->bitmask.bitmask);
-			arg->bitmask.offset = f->offset;
-		}
-		dynamic_offset(tep, 4, data + arg->bitmask.offset, &offset, &len);
+		if (!arg->bitmask.field)
+			arg->bitmask.field = tep_find_any_field(event, arg->bitmask.bitmask);
+		if (!arg->bitmask.field)
+			break;
+		dynamic_offset_field(tep, arg->bitmask.field, data, &offset, &len);
 		print_bitmask_to_seq(tep, s, format, len_arg,
 				     data + offset, len);
 		break;
@@ -7343,6 +7355,8 @@ void *tep_get_field_raw(struct trace_seq *s, struct tep_event *event,
 					 data + offset, field->size);
 		*len = offset >> 16;
 		offset &= 0xffff;
+		if (field->flags & TEP_FIELD_IS_RELATIVE)
+			offset += field->offset + field->size;
 	} else
 		*len = field->size;
 
diff --git a/src/event-parse.h b/src/event-parse.h
index 083389358127..db8bc6db5431 100644
--- a/src/event-parse.h
+++ b/src/event-parse.h
@@ -125,6 +125,7 @@ enum tep_format_flags {
 	TEP_FIELD_IS_LONG	= 32,
 	TEP_FIELD_IS_FLAG	= 64,
 	TEP_FIELD_IS_SYMBOLIC	= 128,
+	TEP_FIELD_IS_RELATIVE	= 256,
 };
 
 struct tep_format_field {
@@ -153,12 +154,12 @@ struct tep_print_arg_atom {
 
 struct tep_print_arg_string {
 	char			*string;
-	int			offset;
+	struct tep_format_field	*field;
 };
 
 struct tep_print_arg_bitmask {
 	char			*bitmask;
-	int			offset;
+	struct tep_format_field	*field;
 };
 
 struct tep_print_arg_field {
diff --git a/src/parse-filter.c b/src/parse-filter.c
index 368826bb5a57..5df177070d53 100644
--- a/src/parse-filter.c
+++ b/src/parse-filter.c
@@ -1712,8 +1712,11 @@ static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record *
 
 		if (arg->str.field->flags & TEP_FIELD_IS_DYNAMIC) {
 			addr = *(unsigned int *)val;
-			val = record->data + (addr & 0xffff);
 			size = addr >> 16;
+			addr &= 0xffff;
+			if (arg->str.field->flags & TEP_FIELD_IS_RELATIVE)
+				addr += arg->str.field->offset + arg->str.field->size;
+			val = record->data + addr;
 		}
 
 		/*

Re: [PATCH] libtraceevent: Add __rel_loc relative location attribute support

[Steven Rostedt]

On Tue, 23 Nov 2021 21:05:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Oops, good catch! Yes, that's right.
> 
> BTW, are there any tool I can use for testing this library?

Usually the way I test it is by running the libtracefs unit tests and the
trace-cmd test suite (which isn't public yet), but we should add unit tests
for libtraceevent as well.

-- Steve

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Steven Rostedt]

On Tue, 23 Nov 2021 22:36:18 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> +++ b/src/event-parse.h
> @@ -125,6 +125,7 @@ enum tep_format_flags {
>  	TEP_FIELD_IS_LONG	= 32,
>  	TEP_FIELD_IS_FLAG	= 64,
>  	TEP_FIELD_IS_SYMBOLIC	= 128,
> +	TEP_FIELD_IS_RELATIVE	= 256,
>  };
>  
>  struct tep_format_field {
> @@ -153,12 +154,12 @@ struct tep_print_arg_atom {
>  
>  struct tep_print_arg_string {
>  	char			*string;
> -	int			offset;
> +	struct tep_format_field	*field;
>  };
>  
>  struct tep_print_arg_bitmask {
>  	char			*bitmask;
> -	int			offset;
> +	struct tep_format_field	*field;
>  };
>  

We need to be careful about modifying content in event-parse.h. Because
this is exposed outside he library.

Although, I don't think this should be an issue.

We probably need to move some content out of that file if it isn't used in
any of the exposed APIs :-/

And since both libtracefs and trace-cmd do not reference any of the
tep_print_arg* fields, we probably should remove them before somebody does.

-- Steve

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Masami Hiramatsu]

On Wed, 24 Nov 2021 18:07:08 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 23 Nov 2021 22:36:18 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > +++ b/src/event-parse.h
> > @@ -125,6 +125,7 @@ enum tep_format_flags {
> >  	TEP_FIELD_IS_LONG	= 32,
> >  	TEP_FIELD_IS_FLAG	= 64,
> >  	TEP_FIELD_IS_SYMBOLIC	= 128,
> > +	TEP_FIELD_IS_RELATIVE	= 256,
> >  };
> >  
> >  struct tep_format_field {
> > @@ -153,12 +154,12 @@ struct tep_print_arg_atom {
> >  
> >  struct tep_print_arg_string {
> >  	char			*string;
> > -	int			offset;
> > +	struct tep_format_field	*field;
> >  };
> >  
> >  struct tep_print_arg_bitmask {
> >  	char			*bitmask;
> > -	int			offset;
> > +	struct tep_format_field	*field;
> >  };
> >  
> 
> We need to be careful about modifying content in event-parse.h. Because
> this is exposed outside he library.

Oh, I thought this is an internal header, because it is under src/ directory.
However, indeed "TEP_FIELD_IS_*" referred from perf-tools, so this might be
need to be exposed.

> 
> Although, I don't think this should be an issue.
> 
> We probably need to move some content out of that file if it isn't used in
> any of the exposed APIs :-/
> 
> And since both libtracefs and trace-cmd do not reference any of the
> tep_print_arg* fields, we probably should remove them before somebody does.

Yes, tep_print_arg seems to be used only in the event-parse.c.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Steven Rostedt]

On Fri, 26 Nov 2021 21:20:26 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > We need to be careful about modifying content in event-parse.h. Because
> > this is exposed outside he library.  
> 
> Oh, I thought this is an internal header, because it is under src/ directory.
> However, indeed "TEP_FIELD_IS_*" referred from perf-tools, so this might be
> need to be exposed.

Yeah, we need to fix this.

The issue was that this is loaded with legacy code. The code was
created in trace-cmd, ported to the kernel into tools/lib/traceevent,
then pulled out into it's own directory. The poor library had so many
foster parents, that it wasn't ready to go out on its own.

I'll fix some of this before posting a new version.

-- Steve

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Beau Belgrave]

On Tue, Nov 23, 2021 at 10:36:18PM +0900, Masami Hiramatsu wrote:
> Add '__rel_loc' new dynamic data location attribute which encodes
> the data location from the next to the field itself. This is similar
> to the '__data_loc' but the location offset is not from the event
> entry but from the next of the field.
> 
> This patch adds '__rel_loc' decoding support in the libtraceevent.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  Changes in v2:
>    - Fix to adjust '*offset' instead of 'offset' in dynamic_offset_field()
> ---
>  src/event-parse.c  |   54 +++++++++++++++++++++++++++++++++-------------------
>  src/event-parse.h  |    5 +++--
>  src/parse-filter.c |    5 ++++-
>  3 files changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/src/event-parse.c b/src/event-parse.c
> index 70637586bc9a..a88c73a8d0fb 100644
> --- a/src/event-parse.c
> +++ b/src/event-parse.c

[..]

> @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
>  		free_token(token);
>  		return process_int_array(event, arg, tok);
>  	}
> -	if (strcmp(token, "__get_str") == 0) {
> +	if (strcmp(token, "__get_str") == 0 ||
> +	    strcmp(token, "__get_rel_str") == 0) {

Should user_events use __get_rel_str vs __get_str for the print_fmt?
Both __dyn_loc and __rel_loc use __get_str currently.

Thanks,
-Beau

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Steven Rostedt]

On Mon, 29 Nov 2021 12:15:07 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
> >  		free_token(token);
> >  		return process_int_array(event, arg, tok);
> >  	}
> > -	if (strcmp(token, "__get_str") == 0) {
> > +	if (strcmp(token, "__get_str") == 0 ||
> > +	    strcmp(token, "__get_rel_str") == 0) {  
> 
> Should user_events use __get_rel_str vs __get_str for the print_fmt?
> Both __dyn_loc and __rel_loc use __get_str currently.

I'm guessing that it should use the get_rel_str(), as get_str() will use
the absolute offset and not the relative one.

-- Steve

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Beau Belgrave]

On Mon, Nov 29, 2021 at 03:27:04PM -0500, Steven Rostedt wrote:
> On Mon, 29 Nov 2021 12:15:07 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
> > >  		free_token(token);
> > >  		return process_int_array(event, arg, tok);
> > >  	}
> > > -	if (strcmp(token, "__get_str") == 0) {
> > > +	if (strcmp(token, "__get_str") == 0 ||
> > > +	    strcmp(token, "__get_rel_str") == 0) {  
> > 
> > Should user_events use __get_rel_str vs __get_str for the print_fmt?
> > Both __dyn_loc and __rel_loc use __get_str currently.
> 
> I'm guessing that it should use the get_rel_str(), as get_str() will use
> the absolute offset and not the relative one.
> 
> -- Steve

It appears both cases call into process_str() and set the
TEP_PRINT_STRING field type.

The TEP_FIELD_IS_RELATIVE bit to advance the offset to a relative position
is within dynamic_offset which is used for TEP_PRINT_STRING field types.

I'm not sure if this was intentional or if __get_rel_str is an artifact
left behind considering __get_str appears to be doing the same thing?

Thanks,
-Beau

Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support

[Masami Hiramatsu]

On Mon, 29 Nov 2021 13:26:35 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> On Mon, Nov 29, 2021 at 03:27:04PM -0500, Steven Rostedt wrote:
> > On Mon, 29 Nov 2021 12:15:07 -0800
> > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > 
> > > > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg,
> > > >  		free_token(token);
> > > >  		return process_int_array(event, arg, tok);
> > > >  	}
> > > > -	if (strcmp(token, "__get_str") == 0) {
> > > > +	if (strcmp(token, "__get_str") == 0 ||
> > > > +	    strcmp(token, "__get_rel_str") == 0) {  
> > > 
> > > Should user_events use __get_rel_str vs __get_str for the print_fmt?
> > > Both __dyn_loc and __rel_loc use __get_str currently.
> > 
> > I'm guessing that it should use the get_rel_str(), as get_str() will use
> > the absolute offset and not the relative one.

Yes, you can see an example in this patch;

https://lore.kernel.org/all/163757343050.510314.2876529802471645178.stgit@devnote2/T/#u

Thus, the user-event must tell the kernel which is used
correctly.

> > 
> > -- Steve
> 
> It appears both cases call into process_str() and set the
> TEP_PRINT_STRING field type.
> 
> The TEP_FIELD_IS_RELATIVE bit to advance the offset to a relative position
> is within dynamic_offset which is used for TEP_PRINT_STRING field types.

Correct. The field type attribute is set in event_read_fields() before
this process.

> 
> I'm not sure if this was intentional or if __get_rel_str is an artifact
> left behind considering __get_str appears to be doing the same thing?

Note that this patch is for libtraceevent, not in-kernel code.

In the kernel, for example, your user-event, you must make a flag for
each field to identify that is __rel_loc, __data_loc, or just a value.
The event filter (and histogram) will parse the "events/*/*/format"
file and identify the field is __rel_loc or __data_loc.

https://lore.kernel.org/all/163757343050.510314.2876529802471645178.stgit@devnote2/T/#m98591a73fb48e4ac79ed6d2e8eb14a13c69703c8

And __get_str()/__get_rel_str() are used in the event which statically
defined (Note that trace_event field itself has no information about
the __rel_loc/__data_loc, so print_fmt function for each field must
handle that correctly.)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>