2016-03-29 07:00:19

by Anton Blanchard

[permalink] [raw]
Subject: [PATCH] perf jit: genelf makes assumptions about endian

Commit 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
incorrectly assumed that PowerPC is big endian only.

Simplify things by consolidating the define of GEN_ELF_ENDIAN and checking
for __BYTE_ORDER == __BIG_ENDIAN.

The PowerPC checks were also incorrect, they do not match what gcc
emits. We should first look for __powerpc64__, then __powerpc__.

Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
Signed-off-by: Anton Blanchard <[email protected]>
---

diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
index cd67e64..2fbeb59 100644
--- a/tools/perf/util/genelf.h
+++ b/tools/perf/util/genelf.h
@@ -9,36 +9,32 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent

#if defined(__arm__)
#define GEN_ELF_ARCH EM_ARM
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS32
#elif defined(__aarch64__)
#define GEN_ELF_ARCH EM_AARCH64
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS64
#elif defined(__x86_64__)
#define GEN_ELF_ARCH EM_X86_64
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS64
#elif defined(__i386__)
#define GEN_ELF_ARCH EM_386
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS32
-#elif defined(__ppcle__)
-#define GEN_ELF_ARCH EM_PPC
-#define GEN_ELF_ENDIAN ELFDATA2LSB
-#define GEN_ELF_CLASS ELFCLASS64
-#elif defined(__powerpc__)
-#define GEN_ELF_ARCH EM_PPC64
-#define GEN_ELF_ENDIAN ELFDATA2MSB
-#define GEN_ELF_CLASS ELFCLASS64
-#elif defined(__powerpcle__)
+#elif defined(__powerpc64__)
#define GEN_ELF_ARCH EM_PPC64
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS64
+#elif defined(__powerpc__)
+#define GEN_ELF_ARCH EM_PPC
+#define GEN_ELF_CLASS ELFCLASS32
#else
#error "unsupported architecture"
#endif

+#if __BYTE_ORDER == __BIG_ENDIAN
+#define GEN_ELF_ENDIAN ELFDATA2MSB
+#else
+#define GEN_ELF_ENDIAN ELFDATA2LSB
+#endif
+
#if GEN_ELF_CLASS == ELFCLASS64
#define elf_newehdr elf64_newehdr
#define elf_getshdr elf64_getshdr


2016-03-30 02:38:23

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH] perf jit: genelf makes assumptions about endian

On Tue, 2016-03-29 at 17:59 +1100, Anton Blanchard wrote:

> Commit 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
> incorrectly assumed that PowerPC is big endian only.
>
> Simplify things by consolidating the define of GEN_ELF_ENDIAN and checking
> for __BYTE_ORDER == __BIG_ENDIAN.
>
> The PowerPC checks were also incorrect, they do not match what gcc
> emits. We should first look for __powerpc64__, then __powerpc__.
>
> Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
> Signed-off-by: Anton Blanchard <[email protected]>

The diff's a little hard to read because you're pulling the endian logic out,
if I remove that I get something like:

> #elif defined(__i386__)
> #define GEN_ELF_ARCH EM_386
> #define GEN_ELF_CLASS ELFCLASS32
> -#elif defined(__ppcle__)
> -#define GEN_ELF_ARCH EM_PPC
> -#define GEN_ELF_CLASS ELFCLASS64
> -#elif defined(__powerpc__)
> -#define GEN_ELF_ARCH EM_PPC64
> -#define GEN_ELF_CLASS ELFCLASS64
> -#elif defined(__powerpcle__)
> +#elif defined(__powerpc64__)
> #define GEN_ELF_ARCH EM_PPC64
> #define GEN_ELF_CLASS ELFCLASS64
> +#elif defined(__powerpc__)
> +#define GEN_ELF_ARCH EM_PPC
> +#define GEN_ELF_CLASS ELFCLASS32
> #else
> #error "unsupported architecture"
> #endif

Which looks correct to me.

And the consolidation of the endian logic is "obviously correct", so:

Acked-by: Michael Ellerman <[email protected]>

cheers

2016-03-30 21:15:26

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH] perf jit: genelf makes assumptions about endian

Em Wed, Mar 30, 2016 at 01:38:20PM +1100, Michael Ellerman escreveu:
> On Tue, 2016-03-29 at 17:59 +1100, Anton Blanchard wrote:
>
> > Commit 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
> > incorrectly assumed that PowerPC is big endian only.
> >
> > Simplify things by consolidating the define of GEN_ELF_ENDIAN and checking
> > for __BYTE_ORDER == __BIG_ENDIAN.
> >
> > The PowerPC checks were also incorrect, they do not match what gcc
> > emits. We should first look for __powerpc64__, then __powerpc__.
> >
> > Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
> > Signed-off-by: Anton Blanchard <[email protected]>
>
> The diff's a little hard to read because you're pulling the endian logic out,
> if I remove that I get something like:

Yeah, I'm taking this patch, but would be better next time to break it
down in two, one doing the reorg, the other doing the actual fix...

Thanks,

- Arnaldo

> > #elif defined(__i386__)
> > #define GEN_ELF_ARCH EM_386
> > #define GEN_ELF_CLASS ELFCLASS32
> > -#elif defined(__ppcle__)
> > -#define GEN_ELF_ARCH EM_PPC
> > -#define GEN_ELF_CLASS ELFCLASS64
> > -#elif defined(__powerpc__)
> > -#define GEN_ELF_ARCH EM_PPC64
> > -#define GEN_ELF_CLASS ELFCLASS64
> > -#elif defined(__powerpcle__)
> > +#elif defined(__powerpc64__)
> > #define GEN_ELF_ARCH EM_PPC64
> > #define GEN_ELF_CLASS ELFCLASS64
> > +#elif defined(__powerpc__)
> > +#define GEN_ELF_ARCH EM_PPC
> > +#define GEN_ELF_CLASS ELFCLASS32
> > #else
> > #error "unsupported architecture"
> > #endif
>
> Which looks correct to me.
>
> And the consolidation of the endian logic is "obviously correct", so:
>
> Acked-by: Michael Ellerman <[email protected]>
>
> cheers

Subject: [tip:perf/urgent] perf jit: genelf makes assumptions about endian

Commit-ID: 9f56c092b99b40ce3cf4c6d0134ff7e513c9f1a6
Gitweb: http://git.kernel.org/tip/9f56c092b99b40ce3cf4c6d0134ff7e513c9f1a6
Author: Anton Blanchard <[email protected]>
AuthorDate: Tue, 29 Mar 2016 17:59:44 +1100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 30 Mar 2016 18:12:06 -0300

perf jit: genelf makes assumptions about endian

Commit 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
incorrectly assumed that PowerPC is big endian only.

Simplify things by consolidating the define of GEN_ELF_ENDIAN and checking
for __BYTE_ORDER == __BIG_ENDIAN.

The PowerPC checks were also incorrect, they do not match what gcc
emits. We should first look for __powerpc64__, then __powerpc__.

Signed-off-by: Anton Blanchard <[email protected]>
Acked-by: Michael Ellerman <[email protected]>
Cc: Carl Love <[email protected]>
Cc: Stephane Eranian <[email protected]>
Cc: Sukadev Bhattiprolu <[email protected]>
Cc: [email protected]
Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
Link: http://lkml.kernel.org/r/20160329175944.33a211cc@kryten
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/genelf.h | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
index cd67e64..2fbeb59 100644
--- a/tools/perf/util/genelf.h
+++ b/tools/perf/util/genelf.h
@@ -9,36 +9,32 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent

#if defined(__arm__)
#define GEN_ELF_ARCH EM_ARM
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS32
#elif defined(__aarch64__)
#define GEN_ELF_ARCH EM_AARCH64
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS64
#elif defined(__x86_64__)
#define GEN_ELF_ARCH EM_X86_64
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS64
#elif defined(__i386__)
#define GEN_ELF_ARCH EM_386
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS32
-#elif defined(__ppcle__)
-#define GEN_ELF_ARCH EM_PPC
-#define GEN_ELF_ENDIAN ELFDATA2LSB
-#define GEN_ELF_CLASS ELFCLASS64
-#elif defined(__powerpc__)
-#define GEN_ELF_ARCH EM_PPC64
-#define GEN_ELF_ENDIAN ELFDATA2MSB
-#define GEN_ELF_CLASS ELFCLASS64
-#elif defined(__powerpcle__)
+#elif defined(__powerpc64__)
#define GEN_ELF_ARCH EM_PPC64
-#define GEN_ELF_ENDIAN ELFDATA2LSB
#define GEN_ELF_CLASS ELFCLASS64
+#elif defined(__powerpc__)
+#define GEN_ELF_ARCH EM_PPC
+#define GEN_ELF_CLASS ELFCLASS32
#else
#error "unsupported architecture"
#endif

+#if __BYTE_ORDER == __BIG_ENDIAN
+#define GEN_ELF_ENDIAN ELFDATA2MSB
+#else
+#define GEN_ELF_ENDIAN ELFDATA2LSB
+#endif
+
#if GEN_ELF_CLASS == ELFCLASS64
#define elf_newehdr elf64_newehdr
#define elf_getshdr elf64_getshdr