Hi Steve, On 18/09/15 14:45, Steven Rostedt wrote: > On Fri, 18 Sep 2015 11:55:47 +0100 > Kapileshwar Singh <kapileshwar.si...@arm.com> wrote: > >>>>> Perhaps we need to make addr into a unsigned long long, and then add: >>>>> >>>>> addr = (pevent->long_size == 8) ? >>>>> *(unsigned long long *)(data + field->offset) : >>>>> (unsigned long long )*(unsigned int *)(data + >>>>> field->offset); >>> >>> What about this? (untested) >>> >>> addr = *(uint64_t *)(data + field->offset) & >>> ((1ULL << pevent->long_size * 8) - 1); >> >> I tested this and it works fine. > > Except that I think it may be buggy. > >> >>> >>> Do we also need to consider byte endians? Maybe it'd be better adding >>> a helper to dereference pointers then.. > > Yes and no. > >> >> In this particular case, since the address is just a key for a lookup into >> the >> printk_map, which seems like a (addr -> const char *) mapping for string >> literals in the trace file, the endian-ness should not matter (I could be >> wrong though). > > Correct, which is why I said "no", BUT! this is why I think Namhyung's > version may be buggy (besides the overflow of the buffer). > > If this is a 64 bit big endian reading a 32 bit little endian file, I > think the result will be incorrect. > > The *(uint64_t *) will return a 64bit number, but the address (with > long_size == 4) only needs 32bits. Thus, we are getting 32 more bits > than needed. Let's say the address is 0x12345678 that is loaded in the > file. Being little endian, it would be loaded as "78 56 34 12". Let's > say the 32bits after that is 0xDEADBEEF, loaded as "EF BE AD DE". Now > the number returned to addr (being a 64 bit big endian) would be: > 0x785643412EFBEADDE But then we do the shift: > > (1ULL << pevent->long_size * 8) - 1; which would leave us with: > > 0xEFBEADDE > > Not what we wanted.
Agreed. > > My version only reads the necessary bytes, and also wont suffer from > reading past the data size of the buffer (which is another bug). > Thanks for noticing and explaining this, makes perfect sense now! Will submit a v3 for this. Regards, KP > -- Steve > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
