| Message ID | 20200810122835.2309026-1-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [bpf] libbpf: Handle GCC built-in types for Arm NEON | expand |
On 8/10/20 2:28 PM, Jean-Philippe Brucker wrote: > When building Arm NEON (SIMD) code, GCC emits built-in types __PolyXX_t, > which are not recognized by Clang. This causes build failures when > including vmlinux.h generated from a kernel built with CONFIG_RAID6_PQ=y > and CONFIG_KERNEL_MODE_NEON. Emit typedefs for these built-in types, > based on the Clang definitions. poly64_t is unsigned long because it's > only defined for 64-bit Arm. > > Including linux/kernel.h to use ARRAY_SIZE() incidentally redefined > max(), causing a build bug due to different types, hence the seemingly > unrelated change. > > Reported-by: Jakov Petrina <jakov.petrina@sartura.hr> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Looks like this was fixed here [0], but not available on older clang/LLVM versions, right? [0] https://reviews.llvm.org/D79711 [...] > > +static const char *builtin_types[][2] = { > + /* > + * GCC emits typedefs to its internal __PolyXX_t types when compiling > + * Arm SIMD intrinsics. Alias them to the same standard types as Clang. > + */ > + { "__Poly8_t", "unsigned char" }, > + { "__Poly16_t", "unsigned short" }, > + { "__Poly64_t", "unsigned long" }, > + { "__Poly128_t", "unsigned __int128" }, In that above LLVM link [0], they typefdef this to signed types ... which one is correct now? // For now, signedness of polynomial types depends on target OS << "#ifdef __aarch64__\n"; OS << "typedef uint8_t poly8_t;\n"; OS << "typedef uint16_t poly16_t;\n"; OS << "typedef uint64_t poly64_t;\n"; OS << "typedef __uint128_t poly128_t;\n"; OS << "#else\n"; OS << "typedef int8_t poly8_t;\n"; OS << "typedef int16_t poly16_t;\n"; OS << "typedef int64_t poly64_t;\n"; OS << "#endif\n"; > +}; > + > +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, > + const struct btf_type *t) > +{ > + const char *name = btf_dump_type_name(d, id); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(builtin_types); i++) { > + if (strcmp(name, builtin_types[i][0]) == 0) { > + btf_dump_printf(d, "typedef %s %s;\n\n", > + builtin_types[i][1], name); > + break; > + } > + } > +} > + > static void btf_dump_emit_enum_fwd(struct btf_dump *d, __u32 id, > const struct btf_type *t) > { >
On Tue, Aug 11, 2020 at 04:10:31PM +0200, Daniel Borkmann wrote: > On 8/10/20 2:28 PM, Jean-Philippe Brucker wrote: > > When building Arm NEON (SIMD) code, GCC emits built-in types __PolyXX_t, > > which are not recognized by Clang. This causes build failures when > > including vmlinux.h generated from a kernel built with CONFIG_RAID6_PQ=y > > and CONFIG_KERNEL_MODE_NEON. Emit typedefs for these built-in types, > > based on the Clang definitions. poly64_t is unsigned long because it's > > only defined for 64-bit Arm. > > > > Including linux/kernel.h to use ARRAY_SIZE() incidentally redefined > > max(), causing a build bug due to different types, hence the seemingly > > unrelated change. > > > > Reported-by: Jakov Petrina <jakov.petrina@sartura.hr> > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > Looks like this was fixed here [0], but not available on older clang/LLVM > versions, right? > > [0] https://reviews.llvm.org/D79711 No, that issue is unrelated. Here the problem is with the DWARF information generated by GCC. In Linux, lib/raid6/neon.uc uses poly8x16_t, and the DWARF information provided by GCC for that type uses a base type named "__Poly8_t", which is only understood by GCC. So after transforming DWARF->BTF->vmlinux.h, the generated vmlinux.h uses this "__Poly8_t" without typedefing it to unsigned char. Passing this vmlinux.h to GCC works because GCC recognizes "__Poly8_t" as one of its internal types, but passing it to clang fails: test.h:20:9: error: unknown type name '__Poly8_t' typedef __Poly8_t poly8x8_t[8]; ^ On the other hand a kernel built with Clang will have DWARF information that defines poly8x16_t to be an array of 16 unsigned char. > [...] > > +static const char *builtin_types[][2] = { > > + /* > > + * GCC emits typedefs to its internal __PolyXX_t types when compiling > > + * Arm SIMD intrinsics. Alias them to the same standard types as Clang. > > + */ > > + { "__Poly8_t", "unsigned char" }, > > + { "__Poly16_t", "unsigned short" }, > > + { "__Poly64_t", "unsigned long" }, > > + { "__Poly128_t", "unsigned __int128" }, > > In that above LLVM link [0], they typefdef this to signed types ... which one > is correct now? > > // For now, signedness of polynomial types depends on target > OS << "#ifdef __aarch64__\n"; > OS << "typedef uint8_t poly8_t;\n"; > OS << "typedef uint16_t poly16_t;\n"; > OS << "typedef uint64_t poly64_t;\n"; > OS << "typedef __uint128_t poly128_t;\n"; > OS << "#else\n"; > OS << "typedef int8_t poly8_t;\n"; > OS << "typedef int16_t poly16_t;\n"; > OS << "typedef int64_t poly64_t;\n"; > OS << "#endif\n"; I don't know why they typedef it to signed types on non-64bit, perhaps legacy support? The official doc linked in [0] (https://developer.arm.com/docs/101028/latest) states that they are unsigned: "poly8_t, poly16_t, poly64_t and poly128_t are defined as unsigned integer types." Thanks, Jean > > +}; > > + > > +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, > > + const struct btf_type *t) > > +{ > > + const char *name = btf_dump_type_name(d, id); > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(builtin_types); i++) { > > + if (strcmp(name, builtin_types[i][0]) == 0) { > > + btf_dump_printf(d, "typedef %s %s;\n\n", > > + builtin_types[i][1], name); > > + break; > > + } > > + } > > +} > > + > > static void btf_dump_emit_enum_fwd(struct btf_dump *d, __u32 id, > > const struct btf_type *t) > > { > > >
On Mon, Aug 10, 2020 at 5:41 AM Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > > When building Arm NEON (SIMD) code, GCC emits built-in types __PolyXX_t, > which are not recognized by Clang. This causes build failures when > including vmlinux.h generated from a kernel built with CONFIG_RAID6_PQ=y > and CONFIG_KERNEL_MODE_NEON. Emit typedefs for these built-in types, > based on the Clang definitions. poly64_t is unsigned long because it's > only defined for 64-bit Arm. > > Including linux/kernel.h to use ARRAY_SIZE() incidentally redefined > max(), causing a build bug due to different types, hence the seemingly > unrelated change. > > Reported-by: Jakov Petrina <jakov.petrina@sartura.hr> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- Thanks for sending small binaries (in little-endian, double thanks!) for me to look at generated DWARF and BTF. I have a bunch of naming nits below and a grudge against "long", but the approach looks reasonable to me overall. It's unfortunate we have to deal with GCC quirks like this. > tools/lib/bpf/btf_dump.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > index cf711168d34a..3162d7b1880c 100644 > --- a/tools/lib/bpf/btf_dump.c > +++ b/tools/lib/bpf/btf_dump.c > @@ -13,6 +13,7 @@ > #include <errno.h> > #include <linux/err.h> > #include <linux/btf.h> > +#include <linux/kernel.h> > #include "btf.h" > #include "hashmap.h" > #include "libbpf.h" > @@ -549,6 +550,9 @@ static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr) > } > } > > +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, > + const struct btf_type *t); > + > static void btf_dump_emit_struct_fwd(struct btf_dump *d, __u32 id, > const struct btf_type *t); > static void btf_dump_emit_struct_def(struct btf_dump *d, __u32 id, > @@ -671,6 +675,9 @@ static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id) > > switch (kind) { > case BTF_KIND_INT: > + /* Emit type alias definitions if necessary */ > + btf_dump_emit_int_def(d, id, t); let's call it btf_dump_emit_missing_aliases() or something like that, so it's clear that it's some sort of compatibility/legacy compiler handling. "emit_int_def" is way too generic and normal-looking. > + > tstate->emit_state = EMITTED; > break; > case BTF_KIND_ENUM: > @@ -870,7 +877,7 @@ static void btf_dump_emit_struct_def(struct btf_dump *d, > btf_dump_printf(d, ": %d", m_sz); > off = m_off + m_sz; > } else { > - m_sz = max(0, btf__resolve_size(d->btf, m->type)); > + m_sz = max(0LL, btf__resolve_size(d->btf, m->type)); > off = m_off + m_sz * 8; > } > btf_dump_printf(d, ";"); > @@ -890,6 +897,32 @@ static void btf_dump_emit_struct_def(struct btf_dump *d, > btf_dump_printf(d, " __attribute__((packed))"); > } > > +static const char *builtin_types[][2] = { again, something like "missing_base_types" would be a bit more prominent > + /* > + * GCC emits typedefs to its internal __PolyXX_t types when compiling > + * Arm SIMD intrinsics. Alias them to the same standard types as Clang. > + */ > + { "__Poly8_t", "unsigned char" }, > + { "__Poly16_t", "unsigned short" }, > + { "__Poly64_t", "unsigned long" }, In the diff ([0]) that Daniel referenced, seems like they are adding poly64_t to ARM32. What prevents GCC from doing that (or maybe they've already done that). So instead of making unreliable assumptions, let's define it as "unsigned long long" instead? [0] https://reviews.llvm.org/D79711 > + { "__Poly128_t", "unsigned __int128" }, > +}; > + > +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, > + const struct btf_type *t) > +{ > + const char *name = btf_dump_type_name(d, id); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(builtin_types); i++) { > + if (strcmp(name, builtin_types[i][0]) == 0) { > + btf_dump_printf(d, "typedef %s %s;\n\n", > + builtin_types[i][1], name); > + break; > + } > + } > +} > + > static void btf_dump_emit_enum_fwd(struct btf_dump *d, __u32 id, > const struct btf_type *t) > { > -- > 2.27.0 >
On Tue, Aug 11, 2020 at 08:30:06PM -0700, Andrii Nakryiko wrote: [...] > > + /* > > + * GCC emits typedefs to its internal __PolyXX_t types when compiling > > + * Arm SIMD intrinsics. Alias them to the same standard types as Clang. > > + */ > > + { "__Poly8_t", "unsigned char" }, > > + { "__Poly16_t", "unsigned short" }, > > + { "__Poly64_t", "unsigned long" }, > > In the diff ([0]) that Daniel referenced, seems like they are adding > poly64_t to ARM32. What prevents GCC from doing that (or maybe they've > already done that). So instead of making unreliable assumptions, let's > define it as "unsigned long long" instead? Agreed. When writing this I had an older version of the ACLE doc referenced in [0] and wanted to be consistent with the older clang typedefs. Thanks, Jean > > [0] https://reviews.llvm.org/D79711 > > > + { "__Poly128_t", "unsigned __int128" }, > > +}; > > + > > +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, > > + const struct btf_type *t) > > +{ > > + const char *name = btf_dump_type_name(d, id); > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(builtin_types); i++) { > > + if (strcmp(name, builtin_types[i][0]) == 0) { > > + btf_dump_printf(d, "typedef %s %s;\n\n", > > + builtin_types[i][1], name); > > + break; > > + } > > + } > > +} > > + > > static void btf_dump_emit_enum_fwd(struct btf_dump *d, __u32 id, > > const struct btf_type *t) > > { > > -- > > 2.27.0 > >
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index cf711168d34a..3162d7b1880c 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -13,6 +13,7 @@ #include <errno.h> #include <linux/err.h> #include <linux/btf.h> +#include <linux/kernel.h> #include "btf.h" #include "hashmap.h" #include "libbpf.h" @@ -549,6 +550,9 @@ static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr) } } +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, + const struct btf_type *t); + static void btf_dump_emit_struct_fwd(struct btf_dump *d, __u32 id, const struct btf_type *t); static void btf_dump_emit_struct_def(struct btf_dump *d, __u32 id, @@ -671,6 +675,9 @@ static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id) switch (kind) { case BTF_KIND_INT: + /* Emit type alias definitions if necessary */ + btf_dump_emit_int_def(d, id, t); + tstate->emit_state = EMITTED; break; case BTF_KIND_ENUM: @@ -870,7 +877,7 @@ static void btf_dump_emit_struct_def(struct btf_dump *d, btf_dump_printf(d, ": %d", m_sz); off = m_off + m_sz; } else { - m_sz = max(0, btf__resolve_size(d->btf, m->type)); + m_sz = max(0LL, btf__resolve_size(d->btf, m->type)); off = m_off + m_sz * 8; } btf_dump_printf(d, ";"); @@ -890,6 +897,32 @@ static void btf_dump_emit_struct_def(struct btf_dump *d, btf_dump_printf(d, " __attribute__((packed))"); } +static const char *builtin_types[][2] = { + /* + * GCC emits typedefs to its internal __PolyXX_t types when compiling + * Arm SIMD intrinsics. Alias them to the same standard types as Clang. + */ + { "__Poly8_t", "unsigned char" }, + { "__Poly16_t", "unsigned short" }, + { "__Poly64_t", "unsigned long" }, + { "__Poly128_t", "unsigned __int128" }, +}; + +static void btf_dump_emit_int_def(struct btf_dump *d, __u32 id, + const struct btf_type *t) +{ + const char *name = btf_dump_type_name(d, id); + int i; + + for (i = 0; i < ARRAY_SIZE(builtin_types); i++) { + if (strcmp(name, builtin_types[i][0]) == 0) { + btf_dump_printf(d, "typedef %s %s;\n\n", + builtin_types[i][1], name); + break; + } + } +} + static void btf_dump_emit_enum_fwd(struct btf_dump *d, __u32 id, const struct btf_type *t) {
When building Arm NEON (SIMD) code, GCC emits built-in types __PolyXX_t, which are not recognized by Clang. This causes build failures when including vmlinux.h generated from a kernel built with CONFIG_RAID6_PQ=y and CONFIG_KERNEL_MODE_NEON. Emit typedefs for these built-in types, based on the Clang definitions. poly64_t is unsigned long because it's only defined for 64-bit Arm. Including linux/kernel.h to use ARRAY_SIZE() incidentally redefined max(), causing a build bug due to different types, hence the seemingly unrelated change. Reported-by: Jakov Petrina <jakov.petrina@sartura.hr> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- tools/lib/bpf/btf_dump.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)