* [PATCH] libtraceevent: Add __rel_loc support
@ 2021-11-22 9:36 Masami Hiramatsu
2021-11-22 9:36 ` [PATCH] libtraceevent: Add __rel_loc relative location attribute support Masami Hiramatsu
0 siblings, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2021-11-22 9:36 UTC (permalink / raw)
To: Steven Rostedt
Cc: Beau Belgrave, Masami Hiramatsu, linux-trace-devel, Tzvetomir Stoyanov
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH] libtraceevent: Add __rel_loc relative location attribute support 2021-11-22 9:36 [PATCH] libtraceevent: Add __rel_loc support Masami Hiramatsu @ 2021-11-22 9:36 ` Masami Hiramatsu 2021-11-23 3:39 ` Steven Rostedt 0 siblings, 1 reply; 13+ messages in thread From: Masami Hiramatsu @ 2021-11-22 9:36 UTC (permalink / raw) To: Steven Rostedt Cc: Beau Belgrave, Masami Hiramatsu, linux-trace-devel, Tzvetomir Stoyanov 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; } /* ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] libtraceevent: Add __rel_loc relative location attribute support 2021-11-22 9:36 ` [PATCH] libtraceevent: Add __rel_loc relative location attribute support Masami Hiramatsu @ 2021-11-23 3:39 ` Steven Rostedt 2021-11-23 12:05 ` Masami Hiramatsu 2021-11-23 13:36 ` [PATCH v2] " Masami Hiramatsu 0 siblings, 2 replies; 13+ messages in thread From: Steven Rostedt @ 2021-11-23 3:39 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Beau Belgrave, linux-trace-devel, Tzvetomir Stoyanov 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 > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libtraceevent: Add __rel_loc relative location attribute support 2021-11-23 3:39 ` Steven Rostedt @ 2021-11-23 12:05 ` Masami Hiramatsu 2021-11-23 14:49 ` Steven Rostedt 2021-11-23 13:36 ` [PATCH v2] " Masami Hiramatsu 1 sibling, 1 reply; 13+ messages in thread From: Masami Hiramatsu @ 2021-11-23 12:05 UTC (permalink / raw) To: Steven Rostedt; +Cc: Beau Belgrave, linux-trace-devel, Tzvetomir Stoyanov 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libtraceevent: Add __rel_loc relative location attribute support 2021-11-23 12:05 ` Masami Hiramatsu @ 2021-11-23 14:49 ` Steven Rostedt 0 siblings, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2021-11-23 14:49 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Beau Belgrave, linux-trace-devel, Tzvetomir Stoyanov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-23 3:39 ` Steven Rostedt 2021-11-23 12:05 ` Masami Hiramatsu @ 2021-11-23 13:36 ` Masami Hiramatsu 2021-11-24 23:07 ` Steven Rostedt 2021-11-29 20:15 ` Beau Belgrave 1 sibling, 2 replies; 13+ messages in thread From: Masami Hiramatsu @ 2021-11-23 13:36 UTC (permalink / raw) To: Steven Rostedt Cc: Beau Belgrave, Masami Hiramatsu, linux-trace-devel, Tzvetomir Stoyanov 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; } /* ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-23 13:36 ` [PATCH v2] " Masami Hiramatsu @ 2021-11-24 23:07 ` Steven Rostedt 2021-11-26 12:20 ` Masami Hiramatsu 2021-11-29 20:15 ` Beau Belgrave 1 sibling, 1 reply; 13+ messages in thread From: Steven Rostedt @ 2021-11-24 23:07 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Beau Belgrave, linux-trace-devel, Tzvetomir Stoyanov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-24 23:07 ` Steven Rostedt @ 2021-11-26 12:20 ` Masami Hiramatsu 2021-11-26 18:23 ` Steven Rostedt 0 siblings, 1 reply; 13+ messages in thread From: Masami Hiramatsu @ 2021-11-26 12:20 UTC (permalink / raw) To: Steven Rostedt; +Cc: Beau Belgrave, linux-trace-devel, Tzvetomir Stoyanov 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-26 12:20 ` Masami Hiramatsu @ 2021-11-26 18:23 ` Steven Rostedt 0 siblings, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2021-11-26 18:23 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Beau Belgrave, linux-trace-devel, Tzvetomir Stoyanov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-23 13:36 ` [PATCH v2] " Masami Hiramatsu 2021-11-24 23:07 ` Steven Rostedt @ 2021-11-29 20:15 ` Beau Belgrave 2021-11-29 20:27 ` Steven Rostedt 1 sibling, 1 reply; 13+ messages in thread From: Beau Belgrave @ 2021-11-29 20:15 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Steven Rostedt, linux-trace-devel, Tzvetomir Stoyanov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-29 20:15 ` Beau Belgrave @ 2021-11-29 20:27 ` Steven Rostedt 2021-11-29 21:26 ` Beau Belgrave 0 siblings, 1 reply; 13+ messages in thread From: Steven Rostedt @ 2021-11-29 20:27 UTC (permalink / raw) To: Beau Belgrave; +Cc: Masami Hiramatsu, linux-trace-devel, Tzvetomir Stoyanov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-29 20:27 ` Steven Rostedt @ 2021-11-29 21:26 ` Beau Belgrave 2021-11-30 0:16 ` Masami Hiramatsu 0 siblings, 1 reply; 13+ messages in thread From: Beau Belgrave @ 2021-11-29 21:26 UTC (permalink / raw) To: Steven Rostedt; +Cc: Masami Hiramatsu, linux-trace-devel, Tzvetomir Stoyanov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] libtraceevent: Add __rel_loc relative location attribute support 2021-11-29 21:26 ` Beau Belgrave @ 2021-11-30 0:16 ` Masami Hiramatsu 0 siblings, 0 replies; 13+ messages in thread From: Masami Hiramatsu @ 2021-11-30 0:16 UTC (permalink / raw) To: Beau Belgrave Cc: Steven Rostedt, Masami Hiramatsu, linux-trace-devel, Tzvetomir Stoyanov 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-30 0:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-22 9:36 [PATCH] libtraceevent: Add __rel_loc support Masami Hiramatsu 2021-11-22 9:36 ` [PATCH] libtraceevent: Add __rel_loc relative location attribute support Masami Hiramatsu 2021-11-23 3:39 ` Steven Rostedt 2021-11-23 12:05 ` Masami Hiramatsu 2021-11-23 14:49 ` Steven Rostedt 2021-11-23 13:36 ` [PATCH v2] " Masami Hiramatsu 2021-11-24 23:07 ` Steven Rostedt 2021-11-26 12:20 ` Masami Hiramatsu 2021-11-26 18:23 ` Steven Rostedt 2021-11-29 20:15 ` Beau Belgrave 2021-11-29 20:27 ` Steven Rostedt 2021-11-29 21:26 ` Beau Belgrave 2021-11-30 0:16 ` Masami Hiramatsu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.