Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: Ian Rogers <irogers@google.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Andi Kleen <ak@linux.intel.com>,
	clang-built-linux <clang-built-linux@googlegroups.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Alexey Budankov <alexey.budankov@linux.intel.com>,
	Palmer Dabbelt <palmer@sifive.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Ingo Molnar <mingo@redhat.com>, Guo Ren <guoren@kernel.org>,
	Stephane Eranian <eranian@google.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Namhyung Kim <namhyung@kernel.org>,
	linux-riscv@lists.infradead.org, Mao Han <han_mao@c-sky.com>,
	Kan Liang <kan.liang@linux.intel.com>
Subject: Re: [PATCH v3] perf tools: avoid sample_reg_masks being const + weak
Date: Wed, 9 Oct 2019 16:07:37 -0700
Message-ID: <CAP-5=fUSgjyLkZJaHTvdFbzZijy6Gzmx5UZHK_brxVEhFpMG8g@mail.gmail.com> (raw)
In-Reply-To: <20191008123104.GA16241@krava>

On Tue, Oct 8, 2019 at 5:31 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Sep 30, 2019 at 05:36:23PM -0700, Ian Rogers wrote:
> > Being const + weak breaks with some compilers that constant-propagate
> > from the weak symbol. This behavior is outside of the specification, but
> > in LLVM is chosen to match GCC's behavior.
> >
> > LLVM's implementation was set in this patch:
> > https://github.com/llvm/llvm-project/commit/f49573d1eedcf1e44893d5a062ac1b72c8419646
> > A const + weak symbol is set to be weak_odr:
> > https://llvm.org/docs/LangRef.html
> > ODR is one definition rule, and given there is one constant definition
> > constant-propagation is possible. It is possible to get this code to
> > miscompile with LLVM when applying link time optimization. As compilers
> > become more aggressive, this is likely to break in more instances.
>
> is this just aprecaution or you actualy saw some breakage?

We saw a breakage with clang with thinlto enabled for linking. Our
compiler team had recently seen, and were surprised by, a similar
issue and were able to dig out the weak ODR issue.

Thanks,
Ian

> > Move the definition of sample_reg_masks to the conditional part of
> > perf_regs.h and guard usage with HAVE_PERF_REGS_SUPPORT. This avoids the
> > weak symbol.
> >
> > Fix an issue when HAVE_PERF_REGS_SUPPORT isn't defined from patch v1.
> > In v3, add perf_regs.c for architectures that HAVE_PERF_REGS_SUPPORT but
> > don't declare sample_regs_masks.
>
> looks good to me (again ;-)), let's see if it passes Arnaldo's farm
>
> thanks,
> jirka
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/arm/util/Build         | 2 ++
> >  tools/perf/arch/arm/util/perf_regs.c   | 6 ++++++
> >  tools/perf/arch/arm64/util/Build       | 1 +
> >  tools/perf/arch/arm64/util/perf_regs.c | 6 ++++++
> >  tools/perf/arch/csky/util/Build        | 2 ++
> >  tools/perf/arch/csky/util/perf_regs.c  | 6 ++++++
> >  tools/perf/arch/riscv/util/Build       | 2 ++
> >  tools/perf/arch/riscv/util/perf_regs.c | 6 ++++++
> >  tools/perf/arch/s390/util/Build        | 1 +
> >  tools/perf/arch/s390/util/perf_regs.c  | 6 ++++++
> >  tools/perf/util/parse-regs-options.c   | 8 ++++++--
> >  tools/perf/util/perf_regs.c            | 4 ----
> >  tools/perf/util/perf_regs.h            | 4 ++--
> >  13 files changed, 46 insertions(+), 8 deletions(-)
> >  create mode 100644 tools/perf/arch/arm/util/perf_regs.c
> >  create mode 100644 tools/perf/arch/arm64/util/perf_regs.c
> >  create mode 100644 tools/perf/arch/csky/util/perf_regs.c
> >  create mode 100644 tools/perf/arch/riscv/util/perf_regs.c
> >  create mode 100644 tools/perf/arch/s390/util/perf_regs.c
> >
> > diff --git a/tools/perf/arch/arm/util/Build b/tools/perf/arch/arm/util/Build
> > index 296f0eac5e18..37fc63708966 100644
> > --- a/tools/perf/arch/arm/util/Build
> > +++ b/tools/perf/arch/arm/util/Build
> > @@ -1,3 +1,5 @@
> > +perf-y += perf_regs.o
> > +
> >  perf-$(CONFIG_DWARF) += dwarf-regs.o
> >
> >  perf-$(CONFIG_LOCAL_LIBUNWIND)    += unwind-libunwind.o
> > diff --git a/tools/perf/arch/arm/util/perf_regs.c b/tools/perf/arch/arm/util/perf_regs.c
> > new file mode 100644
> > index 000000000000..2864e2e3776d
> > --- /dev/null
> > +++ b/tools/perf/arch/arm/util/perf_regs.c
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "../../util/perf_regs.h"
> > +
> > +const struct sample_reg sample_reg_masks[] = {
> > +     SMPL_REG_END
> > +};
> > diff --git a/tools/perf/arch/arm64/util/Build b/tools/perf/arch/arm64/util/Build
> > index 3cde540d2fcf..0a7782c61209 100644
> > --- a/tools/perf/arch/arm64/util/Build
> > +++ b/tools/perf/arch/arm64/util/Build
> > @@ -1,4 +1,5 @@
> >  perf-y += header.o
> > +perf-y += perf_regs.o
> >  perf-y += sym-handling.o
> >  perf-$(CONFIG_DWARF)     += dwarf-regs.o
> >  perf-$(CONFIG_LOCAL_LIBUNWIND) += unwind-libunwind.o
> > diff --git a/tools/perf/arch/arm64/util/perf_regs.c b/tools/perf/arch/arm64/util/perf_regs.c
> > new file mode 100644
> > index 000000000000..2864e2e3776d
> > --- /dev/null
> > +++ b/tools/perf/arch/arm64/util/perf_regs.c
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "../../util/perf_regs.h"
> > +
> > +const struct sample_reg sample_reg_masks[] = {
> > +     SMPL_REG_END
> > +};
> > diff --git a/tools/perf/arch/csky/util/Build b/tools/perf/arch/csky/util/Build
> > index 1160bb2332ba..7d3050134ae0 100644
> > --- a/tools/perf/arch/csky/util/Build
> > +++ b/tools/perf/arch/csky/util/Build
> > @@ -1,2 +1,4 @@
> > +perf-y += perf_regs.o
> > +
> >  perf-$(CONFIG_DWARF) += dwarf-regs.o
> >  perf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
> > diff --git a/tools/perf/arch/csky/util/perf_regs.c b/tools/perf/arch/csky/util/perf_regs.c
> > new file mode 100644
> > index 000000000000..2864e2e3776d
> > --- /dev/null
> > +++ b/tools/perf/arch/csky/util/perf_regs.c
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "../../util/perf_regs.h"
> > +
> > +const struct sample_reg sample_reg_masks[] = {
> > +     SMPL_REG_END
> > +};
> > diff --git a/tools/perf/arch/riscv/util/Build b/tools/perf/arch/riscv/util/Build
> > index 1160bb2332ba..7d3050134ae0 100644
> > --- a/tools/perf/arch/riscv/util/Build
> > +++ b/tools/perf/arch/riscv/util/Build
> > @@ -1,2 +1,4 @@
> > +perf-y += perf_regs.o
> > +
> >  perf-$(CONFIG_DWARF) += dwarf-regs.o
> >  perf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
> > diff --git a/tools/perf/arch/riscv/util/perf_regs.c b/tools/perf/arch/riscv/util/perf_regs.c
> > new file mode 100644
> > index 000000000000..2864e2e3776d
> > --- /dev/null
> > +++ b/tools/perf/arch/riscv/util/perf_regs.c
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "../../util/perf_regs.h"
> > +
> > +const struct sample_reg sample_reg_masks[] = {
> > +     SMPL_REG_END
> > +};
> > diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build
> > index 22797f043b84..3d9d0f4f72ca 100644
> > --- a/tools/perf/arch/s390/util/Build
> > +++ b/tools/perf/arch/s390/util/Build
> > @@ -1,5 +1,6 @@
> >  perf-y += header.o
> >  perf-y += kvm-stat.o
> > +perf-y += perf_regs.o
> >
> >  perf-$(CONFIG_DWARF) += dwarf-regs.o
> >  perf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
> > diff --git a/tools/perf/arch/s390/util/perf_regs.c b/tools/perf/arch/s390/util/perf_regs.c
> > new file mode 100644
> > index 000000000000..2864e2e3776d
> > --- /dev/null
> > +++ b/tools/perf/arch/s390/util/perf_regs.c
> > @@ -0,0 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "../../util/perf_regs.h"
> > +
> > +const struct sample_reg sample_reg_masks[] = {
> > +     SMPL_REG_END
> > +};
> > diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c
> > index ef46c2848808..e687497b3aac 100644
> > --- a/tools/perf/util/parse-regs-options.c
> > +++ b/tools/perf/util/parse-regs-options.c
> > @@ -13,7 +13,7 @@ static int
> >  __parse_regs(const struct option *opt, const char *str, int unset, bool intr)
> >  {
> >       uint64_t *mode = (uint64_t *)opt->value;
> > -     const struct sample_reg *r;
> > +     const struct sample_reg *r = NULL;
> >       char *s, *os = NULL, *p;
> >       int ret = -1;
> >       uint64_t mask;
> > @@ -46,19 +46,23 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr)
> >
> >                       if (!strcmp(s, "?")) {
> >                               fprintf(stderr, "available registers: ");
> > +#ifdef HAVE_PERF_REGS_SUPPORT
> >                               for (r = sample_reg_masks; r->name; r++) {
> >                                       if (r->mask & mask)
> >                                               fprintf(stderr, "%s ", r->name);
> >                               }
> > +#endif
> >                               fputc('\n', stderr);
> >                               /* just printing available regs */
> >                               return -1;
> >                       }
> > +#ifdef HAVE_PERF_REGS_SUPPORT
> >                       for (r = sample_reg_masks; r->name; r++) {
> >                               if ((r->mask & mask) && !strcasecmp(s, r->name))
> >                                       break;
> >                       }
> > -                     if (!r->name) {
> > +#endif
> > +                     if (!r || !r->name) {
> >                               ui__warning("Unknown register \"%s\", check man page or run \"perf record %s?\"\n",
> >                                           s, intr ? "-I" : "--user-regs=");
> >                               goto error;
> > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> > index 2774cec1f15f..5ee47ae1509c 100644
> > --- a/tools/perf/util/perf_regs.c
> > +++ b/tools/perf/util/perf_regs.c
> > @@ -3,10 +3,6 @@
> >  #include "perf_regs.h"
> >  #include "event.h"
> >
> > -const struct sample_reg __weak sample_reg_masks[] = {
> > -     SMPL_REG_END
> > -};
> > -
> >  int __weak arch_sdt_arg_parse_op(char *old_op __maybe_unused,
> >                                char **new_op __maybe_unused)
> >  {
> > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> > index 47fe34e5f7d5..e014c2c038f4 100644
> > --- a/tools/perf/util/perf_regs.h
> > +++ b/tools/perf/util/perf_regs.h
> > @@ -15,8 +15,6 @@ struct sample_reg {
> >  #define SMPL_REG2(n, b) { .name = #n, .mask = 3ULL << (b) }
> >  #define SMPL_REG_END { .name = NULL }
> >
> > -extern const struct sample_reg sample_reg_masks[];
> > -
> >  enum {
> >       SDT_ARG_VALID = 0,
> >       SDT_ARG_SKIP,
> > @@ -27,6 +25,8 @@ uint64_t arch__intr_reg_mask(void);
> >  uint64_t arch__user_reg_mask(void);
> >
> >  #ifdef HAVE_PERF_REGS_SUPPORT
> > +extern const struct sample_reg sample_reg_masks[];
> > +
> >  #include <perf_regs.h>
> >
> >  #define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP))
> > --
> > 2.23.0.444.g18eeb5a265-goog
> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190927214341.170683-1-irogers@google.com>
2019-10-01  0:36 ` Ian Rogers
2019-10-07 20:49   ` Nick Desaulniers
2019-10-08 12:31   ` Jiri Olsa
2019-10-09 23:07     ` Ian Rogers [this message]
2019-10-10 12:29       ` Arnaldo Carvalho de Melo
2019-10-15  5:31   ` [tip: perf/core] perf tools: Avoid 'sample_reg_masks' " tip-bot2 for Ian Rogers

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP-5=fUSgjyLkZJaHTvdFbzZijy6Gzmx5UZHK_brxVEhFpMG8g@mail.gmail.com' \
    --to=irogers@google.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=alexey.budankov@linux.intel.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=clang-built-linux@googlegroups.com \
    --cc=eranian@google.com \
    --cc=guoren@kernel.org \
    --cc=han_mao@c-sky.com \
    --cc=jolsa@redhat.com \
    --cc=kan.liang@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=palmer@sifive.com \
    --cc=paul.walmsley@sifive.com \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git