Messages in this thread | | | Subject | Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV | From | Adrian Hunter <> | Date | Mon, 26 Apr 2021 10:26:12 +0300 |
| |
On 26/04/21 9:44 am, Leo Yan wrote: > Hi Adrian, > > On Mon, Apr 26, 2021 at 08:40:00AM +0300, Adrian Hunter wrote: > > [...] > >>> +static void perf_event__time_conv_swap(union perf_event *event, >>> + bool sample_id_all __maybe_unused) >>> +{ >>> + size_t time_zero_size; >>> + >>> + event->time_conv.time_shift = bswap_64(event->time_conv.time_shift); >>> + event->time_conv.time_mult = bswap_64(event->time_conv.time_mult); >>> + event->time_conv.time_zero = bswap_64(event->time_conv.time_zero); >>> + >>> + time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event; >>> + if (event->header.size > time_zero_size) { >> >> I wonder if we could have a helper for this e.g. (untested) >> >> #define event_contains(obj, mem) (obj.header.size > offsetof(typeof(obj), mem)) >> >> if (event_contains(event->time_conv, time_cycles)) { > > Yeah, this is a better implementation. Will refine patch for this. > >>> + event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles); >>> + event->time_conv.time_mask = bswap_64(event->time_conv.time_mask); >>> + event->time_conv.cap_user_time_zero = >>> + bswap_32(event->time_conv.cap_user_time_zero); >>> + event->time_conv.cap_user_time_short = >>> + bswap_32(event->time_conv.cap_user_time_short); >> >> 'struct perf_record_time_conv' contains bool, the sizeof which, AFAIK, is not defined. >> Is it really 4 bytes on your implementation? It is only 1 byte with gcc on x86. > > On Arm64, bool is also 1 byte with GCC. > > When working on this patch, I checked the size for structure > perf_record_time_conv, it gave out the structure size is 56; I wrongly > thought the bool size is 4 bytes and there have no padding. In fact, > bool size is 1 byte and GCC pads 6 bytes at the end of structure. > >> Either way, you should change 'struct perf_record_time_conv' so it uses a type of known size. >> Since you are the only one using it, it should match your implementation. > > Will change to "unsigned int" value.
If it is 1 byte, I'd have thought __u8 i.e.
struct perf_record_time_conv { struct perf_event_header header; __u64 time_shift; __u64 time_mult; __u64 time_zero; __u64 time_cycles; __u64 time_mask; __u8 cap_user_time_zero; __u8 cap_user_time_short; __u8 unused[6]; };
| |