lkml.org 
[lkml]   [2019]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3] perf tools: avoid sample_reg_masks being const + weak
    On Mon, Sep 30, 2019 at 5:36 PM 'Ian Rogers' via Clang Built Linux
    <clang-built-linux@googlegroups.com> 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.
    >
    > 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.

    s/sample_regs_masks/sample_reg_masks/
    (otherwise I thought for a second that my grep was broken)

    So powerpc and x86 set `NO_PERF_REGS := 0` AND declare `const struct
    sample_reg sample_reg_masks[]`.

    From what I can tell, it makes the below architectures match the way
    x86 and powerpc are structured.
    Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

    >
    > 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
    >
    > --

    --
    Thanks,
    ~Nick Desaulniers

    \
     
     \ /
      Last update: 2019-10-07 22:50    [W:3.011 / U:0.768 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site