Re: [PATCH 5/5] perf, tool: Fix endianity trick for adds_features bitmask

[David Ahern <dsahern@gmail.com>]

On 5/7/12 5:29 AM, Jiri Olsa wrote:
> Addons bitmask is stored as array of unsigned long values. The size
> of the unsigned long is same as pointer size for architecture, so it
> could differ for each architecture.
>
> To handle the endianity for adds_features bitmask, we first swap the
> bitmaks as u64 values and check for HEADER_HOSTNAME bit. If not set we
> want to unswap the u64 values and swap the adds_features as u32 values.
>
> This is currently buggy, since we swap just first 32bits of each u64
> value. Adding swap of the next 32 bits as well.  Also adding&  using
> BITS_TO_U64 instead of BITS_TO_LONGS as counter max due to the different
> size of unsigned longs per architecture.
>
> Note, running following to test perf endianity handling:
>    - origin system:
>      # perf record -a -- sleep 10 (any perf record will do)
>      # perf report>  report.origin
>      # perf archive perf.data
>
>    - copy the perf.data, report.origin and perf.data.tar.bz2
>      to a target system and run:
>      # tar xjvf perf.data.tar.bz2 -C ~/.debug
>      # perf report>  report.target
>      # diff -u report.origin report.target
>
>    - the diff should produce no output
>      (besides some white space stuff and possibly different
>       date/TZ output)

Did you cross bitness and architectures in your test? When I added the 
code you are modifying I verified it worked by analyzing 32-bit PPC 
files on 64-bit x86 and 32-bit x86.

Did you also test old versus new perf commands? perf data collected with 
older command and analyzed with a newer version with this patch? Be sure 
to add the -I switch to get all the processing of the feature mask.

David

>
> Signed-off-by: Jiri Olsa<jolsa@redhat.com>
> ---
>   tools/perf/util/header.c               |   20 +++++++++++++++-----
>   tools/perf/util/include/linux/bitops.h |    1 +
>   2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c0b70c6..a56db7e 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1963,13 +1963,23 @@ int perf_file_header__read(struct perf_file_header *header,
>   		 * file), punt and fallback to the original behavior --
>   		 * clearing all feature bits and setting buildid.
>   		 */
> -		for (i = 0; i<  BITS_TO_LONGS(HEADER_FEAT_BITS); ++i)
> -			header->adds_features[i] = bswap_64(header->adds_features[i]);
> +
> +		mem_bswap_64(&header->adds_features,
> +			     BITS_TO_U64(HEADER_FEAT_BITS));
>
>   		if (!test_bit(HEADER_HOSTNAME, header->adds_features)) {
> -			for (i = 0; i<  BITS_TO_LONGS(HEADER_FEAT_BITS); ++i) {
> -				header->adds_features[i] = bswap_64(header->adds_features[i]);
> -				header->adds_features[i] = bswap_32(header->adds_features[i]);
> +			u64 *v = (u64 *)&header->adds_features;
> +
> +			for (i = 0; i<  BITS_TO_U64(HEADER_FEAT_BITS); ++i) {
> +				union {
> +					u64 val64;
> +					u32 val32[2];
> +				} u;
> +
> +				u.val64 = *v++;
> +				u.val64 = bswap_64(u.val64);
> +				u.val32[0] = bswap_32(u.val32[0]);
> +				u.val32[1] = bswap_32(u.val32[1]);
>   			}
>   		}
>
> diff --git a/tools/perf/util/include/linux/bitops.h b/tools/perf/util/include/linux/bitops.h
> index f1584833..10096cb 100644
> --- a/tools/perf/util/include/linux/bitops.h
> +++ b/tools/perf/util/include/linux/bitops.h
> @@ -8,6 +8,7 @@
>   #define BITS_PER_LONG __WORDSIZE
>   #define BITS_PER_BYTE           8
>   #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
> +#define BITS_TO_U64(nr)         DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(u64))
>
>   #define for_each_set_bit(bit, addr, size) \
>   	for ((bit) = find_first_bit((addr), (size));		\