On Tue, Jan 25, 2022 at 04:28:59PM -0500, Steven Rostedt wrote:
> On Tue, 25 Jan 2022 16:23:26 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > > > #undef __get_rel_dynamic_array
> > > > -#define __get_rel_dynamic_array(field) \
> > > > - ((void *)(&__entry->__rel_loc_##field) + \
> > > > - sizeof(__entry->__rel_loc_##field) + \
> > > > +#define __get_rel_dynamic_array(field) \
> > > > + ((void *)__entry + \
> > > > + offsetof(typeof(*__entry), __rel_loc_##field) + \
> > > > + sizeof(__entry->__rel_loc_##field) + \
> > > > (__entry->__rel_loc_##field & 0xffff))
> > > >
> > > > #undef __get_rel_dynamic_array_len
> > >
>
> I also do not like the the inconsistency between
> sizeof(__entry->__rel_loc_##field) and sizeof(u32) that is used in the
> calculation part:
>
>
> #define __rel_dynamic_array(type, item, len) \
> __item_length = (len) * sizeof(type); \
> __data_offsets->item = __data_size + \
> offsetof(typeof(*entry), __data) - \
> offsetof(typeof(*entry), __rel_loc_##item) - \
> sizeof(u32); \
> __data_offsets->item |= __item_length << 16; \
> __data_size += __item_length;
>
> Why is one using sizeof(u32) and the other using the size of the field?
It might make more sense to calculate everything as an offset within
__data[] instead of from the start of __entry. The patch I sent just did
in perf.h exactly what Masami did in trace_event.h. That worked. I had
an earlier version that did this horrible thing which could probably be
significantly improved, since I just subtract the offset of __data:
#undef __get_rel_dynamic_array
#define __get_rel_dynamic_array(field) \
- ((void *)(&__entry->__rel_loc_##field) + \
- sizeof(__entry->__rel_loc_##field) + \
- (__entry->__rel_loc_##field & 0xffff))
+ ((void *)&__entry->__data[ \
+ offsetof(typeof(*__entry), __rel_loc_##field) \
+ + sizeof(__entry->__rel_loc_##field) \
+ + (__entry->__rel_loc_##field & 0xffff) \
+ - offsetof(typeof(*__entry), __data) \
+ ])
> Just to let you know what is happening. As dynamic elements of the trace
> event needs to be appended at the end of the event, the above macros are
> defined and then run through the TRACE_EVENT() macro, where the
> TP_STRUCT__entry() is parsed to calculate where each item will be for that
> event.
>
> static inline notrace int trace_event_get_offsets_##call( \
> struct trace_event_data_offsets_##call *__data_offsets, proto) \
> { \
> int __data_size = 0; \
> int __maybe_unused __item_length; \
> struct trace_event_raw_##call __maybe_unused *entry; \
> \
> tstruct; \
> \
> return __data_size; \
> }
>
>
> The tstruct is the TP_STRUCT__entry() and for each __rel_dynamic_array() or
> __dynamic_array(), the __data_size gets updated and saved into the
> __data_offsets that holds where each item is.
>
> The rel versions sets the offset from its location to the data, where as
> the non rel versions sets the offset from the beginning of the event to the
> data.
Could this just be
#define __get_rel_dynamic_array(field) \
((void *)(&__entry->data[__entry->__rel_loc_##field & 0xffff])
?
--
Kees Cook
