lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 8 Aug 2018 13:08:43 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Thomas-Mich Richter <tmricht@...ux.ibm.com>
Cc:     mpe@...erman.id.au, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org, brueckner@...ux.vnet.ibm.com,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com
Subject: Re: [PATCH 2/3] perf report: Add raw report support for s390
 auxiliary trace

Em Wed, Aug 08, 2018 at 12:59:21PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 08, 2018 at 12:53:28PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Aug 08, 2018 at 08:39:58AM +0200, Thomas-Mich Richter escreveu:
> > > On 08/08/2018 05:37 AM, mpe@...erman.id.au wrote:
> > > > Thomas Richter <tmricht@...ux.ibm.com> writes:
> > > >> Add support for s390 auxiliary trace support.
> > > >> Use 'perf record -e rbd000' to create the perf.data file.
> > > >> The event also has the symbolic name SF_CYCLES_BASIC_DIAG,
> > > >> using 'perf record -e SF_CYCLES_BASIC_DIAG' is equivalent.
> > > > ...
> > > >>
> > > >> Signed-off-by: Thomas Richter <tmricht@...ux.ibm.com>
> > > >> Reviewed-by: Hendrik Brueckner <brueckner@...ux.ibm.com>
> > > >> ---
> > > >>  tools/perf/util/s390-cpumsf-kernel.h |  71 ++++++++
> > > >>  tools/perf/util/s390-cpumsf.c        | 244 ++++++++++++++++++++++++++-
> > > >>  2 files changed, 314 insertions(+), 1 deletion(-)
> > > >>  create mode 100644 tools/perf/util/s390-cpumsf-kernel.h
> > > > 
> > > > 
> > > > I'm seeing a build break on ppc64le which seems to be caused by this
> > > > commit (I haven't bisected):
> > > > 
> > > >   util/s390-cpumsf.c: In function ‘trailer_timestamp’:
> > > >   util/s390-cpumsf.c:222:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
> > > >     return *((unsigned long long *) &te->timestamp[te->t]);
> > > >     ^
> > > > 
> > > > 
> > > > And on powerpc big endian:
> > > >   In file included from util/cpumap.h:10:0,
> > > >                    from util/s390-cpumsf.c:150:
> > > >   util/s390-cpumsf.c: In function ‘s390_cpumsf_basic_show’:
> > > >   util/s390-cpumsf.c:187:10: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘size_t {aka unsigned int}’ [-Werror=format=]
> > > >      pr_err("Invalid AUX trace basic entry [%#08lx]\n", pos);
> > > >             ^
> > > > 
> > > > cheers
> > > > 
> > > 
> > > Can you please try this patch. Thanks a lot
> > 
> > Ok, this was making the build fail as well on some containers here:
> 
> Ok, failed in another container:
> 
>   11 centos:6                      : gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-18)
> 
>   CC       /tmp/build/perf/tests/sample-parsing.o
> cc1: warnings being treated as errors
> In file included from util/s390-cpumsf.c:161:
> util/s390-cpumsf-kernel.h:16: error: packed attribute is unnecessary for 'def'
> util/s390-cpumsf-kernel.h:25: error: packed attribute is unnecessary for 'CL'
> util/s390-cpumsf-kernel.h:28: error: packed attribute is unnecessary for 'ia'
> util/s390-cpumsf-kernel.h:29: error: packed attribute is unnecessary for 'gpp'
> util/s390-cpumsf-kernel.h:30: error: packed attribute is unnecessary for 'hpp'
> util/s390-cpumsf-kernel.h:34: error: packed attribute is unnecessary for 'def'
> mv: cannot stat `/tmp/build/perf/util/.s390-cpumsf.o.tmp': No such file or directory
> make[4]: *** [/tmp/build/perf/util/s390-cpumsf.o] Error 1

So, this is how it shows using that __packed you have:

[acme@...enth perf]$ pahole -C hws_basic_entry ~/bin/perf
struct hws_basic_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:4;                  /*     0:12  4 */
	unsigned int               U:4;                  /*     0: 8  4 */
	unsigned int               z:2;                  /*     0: 6  4 */
	unsigned int               T:1;                  /*     0: 5  4 */
	unsigned int               W:1;                  /*     0: 4  4 */
	unsigned int               P:1;                  /*     0: 3  4 */
	unsigned int               AS:2;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	unsigned int               CL:2;                 /*     4:30  4 */
	unsigned int               prim_asn:16;          /*     4: 0  4 */

	/* XXX 14 bits hole, try to pack */

	long long unsigned int     ia;                   /*     8     8 */
	long long unsigned int     gpp;                  /*    16     8 */
	long long unsigned int     hpp;                  /*    24     8 */

	/* size: 32, cachelines: 1, members: 14 */
	/* bit holes: 1, sum bit holes: 14 bits */
	/* last cacheline: 32 bytes */
};
[acme@...enth perf]$

And:

[acme@...enth perf]$ pahole -C hws_diag_entry ~/bin/perf
struct hws_diag_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:15;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	u8                         data[0];              /*     4     0 */

	/* size: 4, cachelines: 1, members: 4 */
	/* last cacheline: 4 bytes */
};
[acme@...enth perf]$ 

Now, if we apply this patch:

[acme@...enth perf]$ pahole -C hws_basic_entry ~/bin/perf
struct hws_basic_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:4;                  /*     0:12  4 */
	unsigned int               U:4;                  /*     0: 8  4 */
	unsigned int               z:2;                  /*     0: 6  4 */
	unsigned int               T:1;                  /*     0: 5  4 */
	unsigned int               W:1;                  /*     0: 4  4 */
	unsigned int               P:1;                  /*     0: 3  4 */
	unsigned int               AS:2;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	unsigned int               CL:2;                 /*     4:30  4 */
	unsigned int               prim_asn:16;          /*     4: 0  4 */

	/* XXX 14 bits hole, try to pack */

	long long unsigned int     ia;                   /*     8     8 */
	long long unsigned int     gpp;                  /*    16     8 */
	long long unsigned int     hpp;                  /*    24     8 */

	/* size: 32, cachelines: 1, members: 14 */
	/* bit holes: 1, sum bit holes: 14 bits */
	/* last cacheline: 32 bytes */
};
[acme@...enth perf]$ 

See, no need for packing, that old gcc version is right, and the other
struct:

[acme@...enth perf]$ pahole -C hws_diag_entry ~/bin/perf
struct hws_diag_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:15;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	u8                         data[0];              /*     4     0 */

	/* size: 4, cachelines: 1, members: 4 */
	/* last cacheline: 4 bytes */
};
[acme@...enth perf]$

No need for __packed.

I'm removing that to avoid having this failling in compilers that have
such a warning, since we default to Werror.

Holler if there is something I'missing :-)

- Arnaldo

Powered by blists - more mailing lists