Re: [PATCH v3] perf tools: avoid sample_reg_masks being const + weak

From: Jiri Olsa
Date: Tue Oct 08 2019 - 08:31:13 EST

Next message: Changbin Du: "Re: [PATCH v3 0/2] perf: add support for logging debug messages to file"
Previous message: Jonathan Cameron: "Re: [PATCH 1/2] dt-bindings: iio: light: Add binding for ADUX1020"
Next in thread: Ian Rogers: "Re: [PATCH v3] perf tools: avoid sample_reg_masks being const + weak"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

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

Next message: Changbin Du: "Re: [PATCH v3 0/2] perf: add support for logging debug messages to file"
Previous message: Jonathan Cameron: "Re: [PATCH 1/2] dt-bindings: iio: light: Add binding for ADUX1020"
Next in thread: Ian Rogers: "Re: [PATCH v3] perf tools: avoid sample_reg_masks being const + weak"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]