Hi Andres,
On Fri, Jul 26, 2019 at 6:58 AM Andres Freund <andres@xxxxxxxxxxx> wrote:
>
> Hi,
>
...
>
> Is just plain wrong, as:
>
> > - return pevent->events[idx];
> > + return (all_events + idx);
>
> that's not a valid conversion. ->events isn't an array of tep_handle,
> it's an array of tep_handle* (and even if it were, the previous notation
> would have needed to dereference the value to make it comparable). What
> this does is look idx behind the individually allocated event. Which is
> incorrect.
>
You are right, it is a bug.
>
...
>
> The following patch fixes this for me. I can polish it up, but I'm
> wondering if I'm missing something here?
>
> diff --git i/tools/lib/traceevent/event-parse.h w/tools/lib/traceevent/event-parse.h
> index 642f68ab5fb2..7ebc9b5308d4 100644
> --- i/tools/lib/traceevent/event-parse.h
> +++ w/tools/lib/traceevent/event-parse.h
> @@ -517,6 +517,7 @@ int tep_read_number_field(struct tep_format_field *field, const void *data,
> unsigned long long *value);
>
> struct tep_event *tep_get_first_event(struct tep_handle *tep);
> +struct tep_event *tep_get_event(struct tep_handle *tep, int index);
> int tep_get_events_count(struct tep_handle *tep);
> struct tep_event *tep_find_event(struct tep_handle *tep, int id);
>
> diff --git i/tools/perf/util/trace-event-parse.c w/tools/perf/util/trace-event-parse.c
> index 62bc61155dd1..6a035ffd58ac 100644
> --- i/tools/perf/util/trace-event-parse.c
> +++ w/tools/perf/util/trace-event-parse.c
> @@ -179,28 +179,26 @@ struct tep_event *trace_find_next_event(struct tep_handle *pevent,
> {
> static int idx;
> int events_count;
> - struct tep_event *all_events;
>
> - all_events = tep_get_first_event(pevent);
> events_count = tep_get_events_count(pevent);
> - if (!pevent || !all_events || events_count < 1)
> + if (!pevent || events_count < 1)
> return NULL;
>
> if (!event) {
> idx = 0;
> - return all_events;
> + return tep_get_event(pevent, 0);
> }
>
> - if (idx < events_count && event == (all_events + idx)) {
> + if (idx < events_count && event == tep_get_event(pevent, idx)) {
> idx++;
> if (idx == events_count)
> return NULL;
> - return (all_events + idx);
> + return tep_get_event(pevent, idx);
> }
>
> for (idx = 1; idx < events_count; idx++) {
> - if (event == (all_events + (idx - 1)))
> - return (all_events + idx);
> + if (event == tep_get_event(pevent, idx - 1))
> + return tep_get_event(pevent, idx);
> }
> return NULL;
> }
>
>
I'm OK with the first part of the patch, the changes in
tools/lib/traceevent/event-parse.h.
tep_get_event() is an API and is omitted in the header by mistake, it
should be there.
As for the fix for the crash itself, I think it would be better to
implement your suggestion -
removing trace_find_next_event() at all and replacing it with
tep_get_events_count() and
tep_get_event() loop.
>
>
> Not related to this crash, but it also seems that the whole
> trace_find_next_event() API ought to be removed. Back when it was
>
> -struct event *trace_find_next_event(struct event *event)
> -{
> - if (!event)
> - return event_list;
> -
> - return event->next;
> -}
>
> it made some sense, but the changes in
>
> commit aaf045f72335653b24784d6042be8e4aee114403
> Author: Steven Rostedt <srostedt@xxxxxxxxxx>
> Date: 2012-04-06 00:47:56 +0200
>
> perf: Have perf use the new libtraceevent.a library
>
> seem to make the current API somewhat absurd, as evidenced by the
> complexity in trace_find_next_event(). I mean even just removing that
> function and changing the two callsites to simple for loops with
> tep_get_events_count() & tep_get_event() ought to be a lot better.
>
> Greetings,
>
> Andres Freund
Thanks!
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|