Re: [PATCH] tools/perf: Fix the mask in regs_dump__printf

[Madhavan Srinivasan]

On Friday 17 June 2016 12:07 PM, Jiri Olsa wrote:
> On Fri, Jun 17, 2016 at 10:52:38AM +0530, Madhavan Srinivasan wrote:
>> When decoding the perf_regs mask in regs_dump__printf(),
>> we loop through the mask using find_first_bit and find_next_bit functions.
>> And mask is of type "u64". But "u64" is send as a "unsigned long *" to
>> lib functions along with sizeof().
>>
>> While the exisitng code works fine in most of the case, when using a
>> 32bit perf on a 64bit kernel (Big Endian), we end up reading the wrong word
>> in the u64 mask. Patch to fix the mask in regs_dump__printf().
>>
>> Suggested-by: Yury Norov <ynorov@xxxxxxxxxxxxxxxxxx>
>> Cc: Yury Norov <ynorov@xxxxxxxxxxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> Cc: Kan Liang <kan.liang@xxxxxxxxx>
>> Cc: Wang Nan <wangnan0@xxxxxxxxxx>
>> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>> Signed-off-by: Madhavan Srinivasan <maddy@xxxxxxxxxxxxxxxxxx>
>> ---
>>  tools/perf/util/session.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index 5214974e841a..2eaa42a4832a 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -940,8 +940,13 @@ static void branch_stack__printf(struct perf_sample *sample)
>>  static void regs_dump__printf(u64 mask, u64 *regs)
>>  {
>>  	unsigned rid, i = 0;
>> +	unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
>>  
>> -	for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
>> +	_mask[0] = mask & ULONG_MAX;
>> +	if (sizeof(mask) > sizeof(unsigned long))
>> +		_mask[1] = mask >> 32;
>> +
> I think we should do this earlier when reading the mask,
> not at the moment we just print it, like we do for other
> types.. maybe we could do this as an extra bit for

Ok. I tried something like this. Currently
mask is read in perf_evsel__parse_sample(),
so added for both sample type (PERF_SAMPLE_REGS_USER
and PERF_SAMPLE_REGS_INTR)

 
                if (data->user_regs.abi) {
-                       u64 mask = evsel->attr.sample_regs_user;
+                       u.val64 = evsel->attr.sample_regs_user;
 
-                       sz = hweight_long(mask) * sizeof(u64);
+                       if (sizeof(u64) > sizeof(unsigned long)) {
+                               u64 mask = u.val64;
+                               u.val32[1] = mask >> 32;
+                               u.val32[0] = mask & ULONG_MAX;
+                       }
+
+                       sz = hweight_long(u.val64) * sizeof(u64);
                        OVERFLOW_CHECK(array, sz, max_size);
-                       data->user_regs.mask = mask;
+                       data->user_regs.mask = u.val64;
                        data->user_regs.regs = (u64 *)array;

Issue I see is when printing the mask value in a 32bit perf
on a 64bit kernel (big endian).

442044948492 0xdc0 [0x188]: PERF_RECORD_SAMPLE(IP, 0x1): 7299/7299:
0xc000000000059200 period: 12599 addr: 0
... intr regs: mask 0xffffffff000007ff ABI 32-bit
                            ^^^ shld have been 0x7ffffffffff

I agree it is better to fix this when reading, but
we need to swap again when printing?

> perf_event__swap_ops[PERF_RECORD_SAMPLE] function
>
> also there's print_sample_iregs in builtin-script.c that's
> most likely affected as well

My bad. Should have seen this too.

Maddy
>
> thanks,
> jirka
>
>> +	for_each_set_bit(rid, _mask, sizeof(mask) * 8) {
>>  		u64 val = regs[i++];
>>  
>>  		printf(".... %-5s 0x%" PRIx64 "\n",
>> -- 
>> 1.9.1
>>