* [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
@ 2021-02-07 7:17 Yonghong Song
2021-02-07 10:32 ` Sedat Dilek
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Yonghong Song @ 2021-02-07 7:17 UTC (permalink / raw)
To: acme, dwarves
Cc: bpf, andriin, mark, ndesaulniers, sedat.dilek, Andrii Nakryiko
clang with dwarf5 may generate non-regular int base type,
i.e., not a signed/unsigned char/short/int/longlong/__int128.
Such base types are often used to describe
how an actual parameter or variable is generated. For example,
0x000015cf: DW_TAG_base_type
DW_AT_name ("DW_ATE_unsigned_1")
DW_AT_encoding (DW_ATE_unsigned)
DW_AT_byte_size (0x00)
0x00010ed9: DW_TAG_formal_parameter
DW_AT_location (DW_OP_lit0,
DW_OP_not,
DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
DW_OP_stack_value)
DW_AT_abstract_origin (0x00013984 "branch")
What it does is with a literal "0", did a "not" operation, and the converted to
one-bit unsigned int and then 8-bit unsigned int.
Another example,
0x000e97e4: DW_TAG_base_type
DW_AT_name ("DW_ATE_unsigned_24")
DW_AT_encoding (DW_ATE_unsigned)
DW_AT_byte_size (0x03)
0x000f88f8: DW_TAG_variable
DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
[0xffffffff82808812, 0xffffffff82808817):
DW_OP_breg0 RAX+0,
DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
DW_OP_stack_value,
DW_OP_piece 0x1,
DW_OP_breg0 RAX+0,
DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
DW_OP_lit8,
DW_OP_shr,
DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
DW_OP_stack_value,
DW_OP_piece 0x3
......
At one point, a right shift by 8 happens and the result is converted to
32-bit unsigned int and then to 24-bit unsigned int.
BTF does not need any of these DW_OP_* information and such non-regular int
types will cause libbpf to emit errors.
Let us sanitize them to generate BTF acceptable to libbpf and kernel.
Cc: Sedat Dilek <sedat.dilek@gmail.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/libbtf.c b/libbtf.c
index 9f76283..5843200 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
struct btf *btf = btfe->btf;
const struct btf_type *t;
uint8_t encoding = 0;
+ uint16_t byte_sz;
int32_t id;
if (bt->is_signed) {
@@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
return -1;
}
- id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
+ /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
+ * {num} is not power of 2 and may exceed 128. Such attributes
+ * are mostly used to record operation for an actual parameter
+ * or variable.
+ * For example,
+ * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
+ * [0xffffffff82808812, 0xffffffff82808817):
+ * DW_OP_breg0 RAX+0,
+ * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
+ * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
+ * DW_OP_stack_value,
+ * DW_OP_piece 0x1,
+ * DW_OP_breg0 RAX+0,
+ * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
+ * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
+ * DW_OP_lit8,
+ * DW_OP_shr,
+ * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
+ * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
+ * DW_OP_stack_value, DW_OP_piece 0x3
+ * DW_AT_name ("ebx")
+ * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
+ *
+ * In the above example, at some point, one unsigned_32 value
+ * is right shifted by 8 and the result is converted to unsigned_32
+ * and then unsigned_24.
+ *
+ * BTF does not need such DW_OP_* information so let us sanitize
+ * these non-regular int types to avoid libbpf/kernel complaints.
+ */
+ byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
+ if (!byte_sz || (byte_sz & (byte_sz - 1))) {
+ name = "__SANITIZED_FAKE_INT__";
+ byte_sz = 4;
+ }
+
+ id = btf__add_int(btf, name, byte_sz, encoding);
if (id < 0) {
btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
} else {
--
2.24.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 7:17 [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type Yonghong Song
@ 2021-02-07 10:32 ` Sedat Dilek
2021-02-08 19:55 ` Arnaldo Carvalho de Melo
2021-02-07 14:18 ` Mark Wielaard
2021-02-08 19:22 ` Nick Desaulniers
2 siblings, 1 reply; 14+ messages in thread
From: Sedat Dilek @ 2021-02-07 10:32 UTC (permalink / raw)
To: Yonghong Song
Cc: acme, dwarves, bpf, andriin, mark, Nick Desaulniers, Andrii Nakryiko
On Sun, Feb 7, 2021 at 8:17 AM Yonghong Song <yhs@fb.com> wrote:
>
> clang with dwarf5 may generate non-regular int base type,
> i.e., not a signed/unsigned char/short/int/longlong/__int128.
> Such base types are often used to describe
> how an actual parameter or variable is generated. For example,
>
> 0x000015cf: DW_TAG_base_type
> DW_AT_name ("DW_ATE_unsigned_1")
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_byte_size (0x00)
>
> 0x00010ed9: DW_TAG_formal_parameter
> DW_AT_location (DW_OP_lit0,
> DW_OP_not,
> DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> DW_OP_stack_value)
> DW_AT_abstract_origin (0x00013984 "branch")
>
> What it does is with a literal "0", did a "not" operation, and the converted to
> one-bit unsigned int and then 8-bit unsigned int.
>
> Another example,
>
> 0x000e97e4: DW_TAG_base_type
> DW_AT_name ("DW_ATE_unsigned_24")
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_byte_size (0x03)
>
> 0x000f88f8: DW_TAG_variable
> DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> [0xffffffff82808812, 0xffffffff82808817):
> DW_OP_breg0 RAX+0,
> DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> DW_OP_stack_value,
> DW_OP_piece 0x1,
> DW_OP_breg0 RAX+0,
> DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> DW_OP_lit8,
> DW_OP_shr,
> DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> DW_OP_stack_value,
> DW_OP_piece 0x3
> ......
>
> At one point, a right shift by 8 happens and the result is converted to
> 32-bit unsigned int and then to 24-bit unsigned int.
>
> BTF does not need any of these DW_OP_* information and such non-regular int
> types will cause libbpf to emit errors.
> Let us sanitize them to generate BTF acceptable to libbpf and kernel.
>
> Cc: Sedat Dilek <sedat.dilek@gmail.com>
Thanks for v2.
For both v1 and v2:
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
My development and testing environment:
1. Debian/testing AMD64
2. Linux v5.11-rc6+ with custom mostly Clang fixes
3. Debug-Info: BTF + DWARF-v5 enabled
4. Compiler and tools (incl. Clang's Integrated ASsembler IAS):
LLVM/Clang v12.0.0-rc1 (make LLVM=1 LLVM_IAS=1)
Build and boot on bare metal.
- Sedat -
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 9f76283..5843200 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> struct btf *btf = btfe->btf;
> const struct btf_type *t;
> uint8_t encoding = 0;
> + uint16_t byte_sz;
> int32_t id;
>
> if (bt->is_signed) {
> @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> return -1;
> }
>
> - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> + * {num} is not power of 2 and may exceed 128. Such attributes
> + * are mostly used to record operation for an actual parameter
> + * or variable.
> + * For example,
> + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> + * [0xffffffff82808812, 0xffffffff82808817):
> + * DW_OP_breg0 RAX+0,
> + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> + * DW_OP_stack_value,
> + * DW_OP_piece 0x1,
> + * DW_OP_breg0 RAX+0,
> + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> + * DW_OP_lit8,
> + * DW_OP_shr,
> + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> + * DW_OP_stack_value, DW_OP_piece 0x3
> + * DW_AT_name ("ebx")
> + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> + *
> + * In the above example, at some point, one unsigned_32 value
> + * is right shifted by 8 and the result is converted to unsigned_32
> + * and then unsigned_24.
> + *
> + * BTF does not need such DW_OP_* information so let us sanitize
> + * these non-regular int types to avoid libbpf/kernel complaints.
> + */
> + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> + name = "__SANITIZED_FAKE_INT__";
> + byte_sz = 4;
> + }
> +
> + id = btf__add_int(btf, name, byte_sz, encoding);
> if (id < 0) {
> btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> } else {
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 7:17 [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type Yonghong Song
2021-02-07 10:32 ` Sedat Dilek
@ 2021-02-07 14:18 ` Mark Wielaard
2021-02-07 15:15 ` Sedat Dilek
` (2 more replies)
2021-02-08 19:22 ` Nick Desaulniers
2 siblings, 3 replies; 14+ messages in thread
From: Mark Wielaard @ 2021-02-07 14:18 UTC (permalink / raw)
To: Yonghong Song, acme, dwarves
Cc: bpf, andriin, ndesaulniers, sedat.dilek, Andrii Nakryiko
Hi,
On Sat, 2021-02-06 at 23:17 -0800, Yonghong Song wrote:
> clang with dwarf5 may generate non-regular int base type,
> i.e., not a signed/unsigned char/short/int/longlong/__int128.
> Such base types are often used to describe
> how an actual parameter or variable is generated. For example,
>
> 0x000015cf: DW_TAG_base_type
> DW_AT_name ("DW_ATE_unsigned_1")
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_byte_size (0x00)
>
> 0x00010ed9: DW_TAG_formal_parameter
> DW_AT_location (DW_OP_lit0,
> DW_OP_not,
> DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> DW_OP_stack_value)
> DW_AT_abstract_origin (0x00013984 "branch")
>
> What it does is with a literal "0", did a "not" operation, and the converted to
> one-bit unsigned int and then 8-bit unsigned int.
Thanks for tracking this down. Do you have any idea why the clang
compiler emits this? You might be right that it is intended to do what
you describe it does (but then it would simply encode an unsigned
constant 1 char in a very inefficient way). But as implemented it
doesn't seem to make any sense. What would DW_OP_convert of an zero
sized base type even mean (if it is intended as a 1 bit sized typed,
then why is there no DW_AT_bit_size)?
So I do think your patch makes sense. clang clearly is emitting
something bogus. And so some fixup is needed. But maybe we should at
least give a warning about it, otherwise it might never get fixed.
BTW. If these bogus base types are only emitted as part of a location
expression and not as part of an actual function or variable type
description, then why are we even trying to encode it as a BTF type? It
might be cheaper to just skip/drop it. But maybe the code setup makes
it hard to know whether or not such a (bogus) type is actually
referenced from a function or variable description?
Cheers,
Mark
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 14:18 ` Mark Wielaard
@ 2021-02-07 15:15 ` Sedat Dilek
2021-02-07 18:14 ` Yonghong Song
2021-02-08 19:16 ` Nick Desaulniers
2 siblings, 0 replies; 14+ messages in thread
From: Sedat Dilek @ 2021-02-07 15:15 UTC (permalink / raw)
To: Mark Wielaard
Cc: Yonghong Song, acme, dwarves, bpf, andriin, Nick Desaulniers,
Andrii Nakryiko
On Sun, Feb 7, 2021 at 3:18 PM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi,
>
> On Sat, 2021-02-06 at 23:17 -0800, Yonghong Song wrote:
> > clang with dwarf5 may generate non-regular int base type,
> > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > Such base types are often used to describe
> > how an actual parameter or variable is generated. For example,
> >
> > 0x000015cf: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_1")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x00)
> >
> > 0x00010ed9: DW_TAG_formal_parameter
> > DW_AT_location (DW_OP_lit0,
> > DW_OP_not,
> > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > DW_OP_stack_value)
> > DW_AT_abstract_origin (0x00013984 "branch")
> >
> > What it does is with a literal "0", did a "not" operation, and the converted to
> > one-bit unsigned int and then 8-bit unsigned int.
>
> Thanks for tracking this down. Do you have any idea why the clang
> compiler emits this? You might be right that it is intended to do what
> you describe it does (but then it would simply encode an unsigned
> constant 1 char in a very inefficient way). But as implemented it
> doesn't seem to make any sense. What would DW_OP_convert of an zero
> sized base type even mean (if it is intended as a 1 bit sized typed,
> then why is there no DW_AT_bit_size)?
>
> So I do think your patch makes sense. clang clearly is emitting
> something bogus. And so some fixup is needed. But maybe we should at
> least give a warning about it, otherwise it might never get fixed.
>
> BTW. If these bogus base types are only emitted as part of a location
> expression and not as part of an actual function or variable type
> description, then why are we even trying to encode it as a BTF type? It
> might be cheaper to just skip/drop it. But maybe the code setup makes
> it hard to know whether or not such a (bogus) type is actually
> referenced from a function or variable description?
>
As said this is with LLVM/Clang v12.0.0-rc1 and `make LLVM=1
LLVM_IAS=1` means use all LLVM tools (do not use GNU/binutils like
GNU/ld BFD) and Clang's Integrated ASsembler (means do not use GNU
AS).
When doing an indexed search for "DW_ATE_unsigned"...
...this points to changes recently done in places like
llvm/lib/CodeGen/AsmPrinter/.
I see 16 changes there touching DWARF-x area since llvmorg-12.0.0-rc1 release:
$ git log --oneline llvmorg-12.0.0-rc1.. llvm/lib/CodeGen/AsmPrinter/
e44a10094283 .gcc_except_table: Set SHF_LINK_ORDER if binutils>=2.36,
and drop unneeded unique ID for -fno-unique-section-names
853a2649160c [AsmPrinter] __patchable_function_entries: Set
SHF_LINK_ORDER for binutils 2.36 and above
e3c0b0fe0958 [WebAssembly] locals can now be indirect in DWARF
34f3249abdff [DebugInfo] Fix error from D95893, where I accidentally
used an unsigned int in a loop and it wraps around.
a740af4de970 [CodeView][DebugInfo] Update the code for removing
template arguments from the display name of a codeview function id.
56fa34ae3570 DebugInfo: Temporarily work around -gsplit-dwarf + LTO
.debug_gnu_pubnames regression after D94976
8998f5843503 Re-land D94976 after revert in e29552c5aff6
d32deaab4d53 Revert "[DWARF] Location-less inlined variables should
not have DW_TAG_variable"
ddc2f1e3fb4f [DWARF] Location-less inlined variables should not have
DW_TAG_variable
511c9a76fb98 [AsmPrinter] Use ListSeparator (NFC)
85b7b5625a00 Fix memory leak in 4318028cd2d7633a0cdeb0b5d4d2ed81fab87864
4318028cd2d7 DebugInfo: Add a DWARF FORM extension for addrx+offset
references to reduce relocations
e29552c5aff6 Revert "[DWARF] Create subprogram's DIE in DISubprogram's unit"
dd7297e1bffe DebugInfo: Fix bug in addr+offset exprloc to use DWARFv5
addrx op instead of DWARFv4 GNU extension
7e6c87ee0454 DebugInfo: Deduplicate addresses in debug_addr
ef0dcb506300 [DWARF] Create subprogram's DIE in DISubprogram's unit
What I try to say is with LLVM-13 (git) this might look different?
Here, I have a LLVM-12 ThinLTO+PGO optimized toolchain which saves 1/3
of Linux-kernel build-time.
So, I do not want to switch to Debian's or packages from <apt.llvm.org>.
These binaries take much much longer and I do not know if I get some
new issues with Linux v5.11-rc6+.
Again, I am not a LLVM toolchain expert.
Best is to ask on llvm-dev mailing-list?
Thanks.
- Sedat -
[1] https://github.com/llvm/llvm-project/search?o=desc&q=DW_ATE_unsigned&s=indexed
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 14:18 ` Mark Wielaard
2021-02-07 15:15 ` Sedat Dilek
@ 2021-02-07 18:14 ` Yonghong Song
2021-02-08 19:16 ` Nick Desaulniers
2 siblings, 0 replies; 14+ messages in thread
From: Yonghong Song @ 2021-02-07 18:14 UTC (permalink / raw)
To: Mark Wielaard, acme, dwarves
Cc: bpf, andriin, ndesaulniers, sedat.dilek, Andrii Nakryiko
On 2/7/21 6:18 AM, Mark Wielaard wrote:
> Hi,
>
> On Sat, 2021-02-06 at 23:17 -0800, Yonghong Song wrote:
>> clang with dwarf5 may generate non-regular int base type,
>> i.e., not a signed/unsigned char/short/int/longlong/__int128.
>> Such base types are often used to describe
>> how an actual parameter or variable is generated. For example,
>>
>> 0x000015cf: DW_TAG_base_type
>> DW_AT_name ("DW_ATE_unsigned_1")
>> DW_AT_encoding (DW_ATE_unsigned)
>> DW_AT_byte_size (0x00)
>>
>> 0x00010ed9: DW_TAG_formal_parameter
>> DW_AT_location (DW_OP_lit0,
>> DW_OP_not,
>> DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
>> DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
>> DW_OP_stack_value)
>> DW_AT_abstract_origin (0x00013984 "branch")
>>
>> What it does is with a literal "0", did a "not" operation, and the converted to
>> one-bit unsigned int and then 8-bit unsigned int.
>
> Thanks for tracking this down. Do you have any idea why the clang
> compiler emits this? You might be right that it is intended to do what
No, I don't know.
> you describe it does (but then it would simply encode an unsigned
> constant 1 char in a very inefficient way). But as implemented it
> doesn't seem to make any sense. What would DW_OP_convert of an zero
> sized base type even mean (if it is intended as a 1 bit sized typed,
> then why is there no DW_AT_bit_size)?
We need to report back to llvm community about this instance.
DW_AT_byte_size = 0 only for DW_ATE_unsigned_1 does not sound right.
DW_AT_bit_size might be needed as you suggested.
>
> So I do think your patch makes sense. clang clearly is emitting
> something bogus. And so some fixup is needed. But maybe we should at
> least give a warning about it, otherwise it might never get fixed.
In pahole, to deal with dwarf weirdness, we have several other places
for workaround without warning. I think it is okay not to have
warning here as users cannot do anything about it. Also we have
this special int type with name __SANITIZED_FAKE_INT__ which will
signal the issue still exists if looking at BTF or generated vmlinux.h.
>
> BTW. If these bogus base types are only emitted as part of a location
> expression and not as part of an actual function or variable type
> description, then why are we even trying to encode it as a BTF type? It
Let us still encode it as a BTF type as an evidence that we do
changed some types. After deduplication, we will only have ONE
__SANITIZED_FAKE_INT__ type. I think leave this one type in BTF is okay.
> might be cheaper to just skip/drop it. But maybe the code setup makes
> it hard to know whether or not such a (bogus) type is actually
> referenced from a function or variable description?
I guess it is possible that we do a preprocessing to check whether
any types BTF intends to emit (var/func definition, etc.) referencing
such types or not. This will involves overhead for every CU most of
which does not have this issue. And this should not really happen given
a sane dwarf.
With __SANITIZED_FAKE_INT__ as the type name, if something e.g., a
variable definition use this type, you will have
__SANITIZED_FAKE_INT__ a;
in skeleton, or have
struct {
__SANITIZED_FAKE_INT__ member;
...
};
in vmlinux.h,
we should be able to catch such an error easily.
>
> Cheers,
>
> Mark
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 14:18 ` Mark Wielaard
2021-02-07 15:15 ` Sedat Dilek
2021-02-07 18:14 ` Yonghong Song
@ 2021-02-08 19:16 ` Nick Desaulniers
[not found] ` <CAMXQf9_Qy5tD05ax1vtETnzM9szLxm95JgpHgT0HzjktixMNNQ@mail.gmail.com>
2 siblings, 1 reply; 14+ messages in thread
From: Nick Desaulniers @ 2021-02-08 19:16 UTC (permalink / raw)
To: Mark Wielaard, David Blaikie
Cc: Yonghong Song, Arnaldo Carvalho de Melo, dwarves, bpf,
Andrii Nakryiko, Sedat Dilek, Andrii Nakryiko
On Sun, Feb 7, 2021 at 6:18 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi,
>
> On Sat, 2021-02-06 at 23:17 -0800, Yonghong Song wrote:
> > clang with dwarf5 may generate non-regular int base type,
> > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > Such base types are often used to describe
> > how an actual parameter or variable is generated. For example,
> >
> > 0x000015cf: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_1")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x00)
> >
> > 0x00010ed9: DW_TAG_formal_parameter
> > DW_AT_location (DW_OP_lit0,
> > DW_OP_not,
> > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > DW_OP_stack_value)
> > DW_AT_abstract_origin (0x00013984 "branch")
> >
> > What it does is with a literal "0", did a "not" operation, and the converted to
> > one-bit unsigned int and then 8-bit unsigned int.
>
> Thanks for tracking this down. Do you have any idea why the clang
> compiler emits this? You might be right that it is intended to do what
> you describe it does (but then it would simply encode an unsigned
> constant 1 char in a very inefficient way). But as implemented it
> doesn't seem to make any sense. What would DW_OP_convert of an zero
> sized base type even mean (if it is intended as a 1 bit sized typed,
> then why is there no DW_AT_bit_size)?
David,
Any thoughts on the above sequence of DW_OP_ entries? This is a part
of DWARF I'm unfamiliar with.
>
> So I do think your patch makes sense. clang clearly is emitting
> something bogus. And so some fixup is needed. But maybe we should at
> least give a warning about it, otherwise it might never get fixed.
>
> BTW. If these bogus base types are only emitted as part of a location
> expression and not as part of an actual function or variable type
> description, then why are we even trying to encode it as a BTF type? It
> might be cheaper to just skip/drop it. But maybe the code setup makes
> it hard to know whether or not such a (bogus) type is actually
> referenced from a function or variable description?
>
> Cheers,
>
> Mark
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 7:17 [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type Yonghong Song
2021-02-07 10:32 ` Sedat Dilek
2021-02-07 14:18 ` Mark Wielaard
@ 2021-02-08 19:22 ` Nick Desaulniers
2021-02-08 19:27 ` Sedat Dilek
2021-02-08 20:13 ` Arnaldo Carvalho de Melo
2 siblings, 2 replies; 14+ messages in thread
From: Nick Desaulniers @ 2021-02-08 19:22 UTC (permalink / raw)
To: Yonghong Song
Cc: Arnaldo Carvalho de Melo, dwarves, bpf, Andrii Nakryiko,
Mark Wielaard, Sedat Dilek, Andrii Nakryiko
On Sat, Feb 6, 2021 at 11:17 PM Yonghong Song <yhs@fb.com> wrote:
>
> clang with dwarf5 may generate non-regular int base type,
> i.e., not a signed/unsigned char/short/int/longlong/__int128.
> Such base types are often used to describe
> how an actual parameter or variable is generated. For example,
>
> 0x000015cf: DW_TAG_base_type
> DW_AT_name ("DW_ATE_unsigned_1")
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_byte_size (0x00)
>
> 0x00010ed9: DW_TAG_formal_parameter
> DW_AT_location (DW_OP_lit0,
> DW_OP_not,
> DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> DW_OP_stack_value)
> DW_AT_abstract_origin (0x00013984 "branch")
>
> What it does is with a literal "0", did a "not" operation, and the converted to
> one-bit unsigned int and then 8-bit unsigned int.
>
> Another example,
>
> 0x000e97e4: DW_TAG_base_type
> DW_AT_name ("DW_ATE_unsigned_24")
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_byte_size (0x03)
>
> 0x000f88f8: DW_TAG_variable
> DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> [0xffffffff82808812, 0xffffffff82808817):
> DW_OP_breg0 RAX+0,
> DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> DW_OP_stack_value,
> DW_OP_piece 0x1,
> DW_OP_breg0 RAX+0,
> DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> DW_OP_lit8,
> DW_OP_shr,
> DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> DW_OP_stack_value,
> DW_OP_piece 0x3
> ......
>
> At one point, a right shift by 8 happens and the result is converted to
> 32-bit unsigned int and then to 24-bit unsigned int.
>
> BTF does not need any of these DW_OP_* information and such non-regular int
> types will cause libbpf to emit errors.
> Let us sanitize them to generate BTF acceptable to libbpf and kernel.
>
> Cc: Sedat Dilek <sedat.dilek@gmail.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Yonghong Song <yhs@fb.com>
Thanks for the patch!
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 9f76283..5843200 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> struct btf *btf = btfe->btf;
> const struct btf_type *t;
> uint8_t encoding = 0;
> + uint16_t byte_sz;
> int32_t id;
>
> if (bt->is_signed) {
> @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> return -1;
> }
>
> - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> + * {num} is not power of 2 and may exceed 128. Such attributes
> + * are mostly used to record operation for an actual parameter
> + * or variable.
> + * For example,
> + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> + * [0xffffffff82808812, 0xffffffff82808817):
> + * DW_OP_breg0 RAX+0,
> + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> + * DW_OP_stack_value,
> + * DW_OP_piece 0x1,
> + * DW_OP_breg0 RAX+0,
> + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> + * DW_OP_lit8,
> + * DW_OP_shr,
> + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> + * DW_OP_stack_value, DW_OP_piece 0x3
> + * DW_AT_name ("ebx")
> + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> + *
> + * In the above example, at some point, one unsigned_32 value
> + * is right shifted by 8 and the result is converted to unsigned_32
> + * and then unsigned_24.
> + *
> + * BTF does not need such DW_OP_* information so let us sanitize
> + * these non-regular int types to avoid libbpf/kernel complaints.
> + */
> + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> + name = "__SANITIZED_FAKE_INT__";
> + byte_sz = 4;
> + }
> +
> + id = btf__add_int(btf, name, byte_sz, encoding);
> if (id < 0) {
> btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> } else {
> --
> 2.24.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-08 19:22 ` Nick Desaulniers
@ 2021-02-08 19:27 ` Sedat Dilek
2021-02-08 20:13 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 14+ messages in thread
From: Sedat Dilek @ 2021-02-08 19:27 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Yonghong Song, Arnaldo Carvalho de Melo, dwarves, bpf,
Andrii Nakryiko, Mark Wielaard, Andrii Nakryiko
On Mon, Feb 8, 2021 at 8:23 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Sat, Feb 6, 2021 at 11:17 PM Yonghong Song <yhs@fb.com> wrote:
> >
> > clang with dwarf5 may generate non-regular int base type,
> > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > Such base types are often used to describe
> > how an actual parameter or variable is generated. For example,
> >
> > 0x000015cf: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_1")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x00)
> >
> > 0x00010ed9: DW_TAG_formal_parameter
> > DW_AT_location (DW_OP_lit0,
> > DW_OP_not,
> > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > DW_OP_stack_value)
> > DW_AT_abstract_origin (0x00013984 "branch")
> >
> > What it does is with a literal "0", did a "not" operation, and the converted to
> > one-bit unsigned int and then 8-bit unsigned int.
> >
> > Another example,
> >
> > 0x000e97e4: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_24")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x03)
> >
> > 0x000f88f8: DW_TAG_variable
> > DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > [0xffffffff82808812, 0xffffffff82808817):
> > DW_OP_breg0 RAX+0,
> > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > DW_OP_stack_value,
> > DW_OP_piece 0x1,
> > DW_OP_breg0 RAX+0,
> > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > DW_OP_lit8,
> > DW_OP_shr,
> > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > DW_OP_stack_value,
> > DW_OP_piece 0x3
> > ......
> >
> > At one point, a right shift by 8 happens and the result is converted to
> > 32-bit unsigned int and then to 24-bit unsigned int.
> >
> > BTF does not need any of these DW_OP_* information and such non-regular int
> > types will cause libbpf to emit errors.
> > Let us sanitize them to generate BTF acceptable to libbpf and kernel.
> >
> > Cc: Sedat Dilek <sedat.dilek@gmail.com>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Yonghong Song <yhs@fb.com>
>
> Thanks for the patch!
>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
>
Cool, thanks for testing Nick.
- Sedat -
> > ---
> > libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 9f76283..5843200 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > struct btf *btf = btfe->btf;
> > const struct btf_type *t;
> > uint8_t encoding = 0;
> > + uint16_t byte_sz;
> > int32_t id;
> >
> > if (bt->is_signed) {
> > @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > return -1;
> > }
> >
> > - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> > + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> > + * {num} is not power of 2 and may exceed 128. Such attributes
> > + * are mostly used to record operation for an actual parameter
> > + * or variable.
> > + * For example,
> > + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > + * [0xffffffff82808812, 0xffffffff82808817):
> > + * DW_OP_breg0 RAX+0,
> > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > + * DW_OP_stack_value,
> > + * DW_OP_piece 0x1,
> > + * DW_OP_breg0 RAX+0,
> > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > + * DW_OP_lit8,
> > + * DW_OP_shr,
> > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > + * DW_OP_stack_value, DW_OP_piece 0x3
> > + * DW_AT_name ("ebx")
> > + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> > + *
> > + * In the above example, at some point, one unsigned_32 value
> > + * is right shifted by 8 and the result is converted to unsigned_32
> > + * and then unsigned_24.
> > + *
> > + * BTF does not need such DW_OP_* information so let us sanitize
> > + * these non-regular int types to avoid libbpf/kernel complaints.
> > + */
> > + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> > + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> > + name = "__SANITIZED_FAKE_INT__";
> > + byte_sz = 4;
> > + }
> > +
> > + id = btf__add_int(btf, name, byte_sz, encoding);
> > if (id < 0) {
> > btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> > } else {
> > --
> > 2.24.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-07 10:32 ` Sedat Dilek
@ 2021-02-08 19:55 ` Arnaldo Carvalho de Melo
2021-02-08 20:46 ` Sedat Dilek
0 siblings, 1 reply; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-08 19:55 UTC (permalink / raw)
To: Sedat Dilek
Cc: Yonghong Song, dwarves, bpf, andriin, mark, Nick Desaulniers,
Andrii Nakryiko
Em Sun, Feb 07, 2021 at 11:32:00AM +0100, Sedat Dilek escreveu:
> On Sun, Feb 7, 2021 at 8:17 AM Yonghong Song <yhs@fb.com> wrote:
> >
> > clang with dwarf5 may generate non-regular int base type,
> > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > Such base types are often used to describe
> > how an actual parameter or variable is generated. For example,
> >
> > 0x000015cf: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_1")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x00)
> >
> > 0x00010ed9: DW_TAG_formal_parameter
> > DW_AT_location (DW_OP_lit0,
> > DW_OP_not,
> > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > DW_OP_stack_value)
> > DW_AT_abstract_origin (0x00013984 "branch")
> >
> > What it does is with a literal "0", did a "not" operation, and the converted to
> > one-bit unsigned int and then 8-bit unsigned int.
> >
> > Another example,
> >
> > 0x000e97e4: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_24")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x03)
> >
> > 0x000f88f8: DW_TAG_variable
> > DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > [0xffffffff82808812, 0xffffffff82808817):
> > DW_OP_breg0 RAX+0,
> > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > DW_OP_stack_value,
> > DW_OP_piece 0x1,
> > DW_OP_breg0 RAX+0,
> > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > DW_OP_lit8,
> > DW_OP_shr,
> > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > DW_OP_stack_value,
> > DW_OP_piece 0x3
> > ......
> >
> > At one point, a right shift by 8 happens and the result is converted to
> > 32-bit unsigned int and then to 24-bit unsigned int.
> >
> > BTF does not need any of these DW_OP_* information and such non-regular int
> > types will cause libbpf to emit errors.
> > Let us sanitize them to generate BTF acceptable to libbpf and kernel.
> >
> > Cc: Sedat Dilek <sedat.dilek@gmail.com>
>
> Thanks for v2.
>
> For both v1 and v2:
>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Thanks, applied.
- Arnaldo
> My development and testing environment:
>
> 1. Debian/testing AMD64
> 2. Linux v5.11-rc6+ with custom mostly Clang fixes
> 3. Debug-Info: BTF + DWARF-v5 enabled
> 4. Compiler and tools (incl. Clang's Integrated ASsembler IAS):
> LLVM/Clang v12.0.0-rc1 (make LLVM=1 LLVM_IAS=1)
>
> Build and boot on bare metal.
>
> - Sedat -
>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > ---
> > libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 9f76283..5843200 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > struct btf *btf = btfe->btf;
> > const struct btf_type *t;
> > uint8_t encoding = 0;
> > + uint16_t byte_sz;
> > int32_t id;
> >
> > if (bt->is_signed) {
> > @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > return -1;
> > }
> >
> > - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> > + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> > + * {num} is not power of 2 and may exceed 128. Such attributes
> > + * are mostly used to record operation for an actual parameter
> > + * or variable.
> > + * For example,
> > + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > + * [0xffffffff82808812, 0xffffffff82808817):
> > + * DW_OP_breg0 RAX+0,
> > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > + * DW_OP_stack_value,
> > + * DW_OP_piece 0x1,
> > + * DW_OP_breg0 RAX+0,
> > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > + * DW_OP_lit8,
> > + * DW_OP_shr,
> > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > + * DW_OP_stack_value, DW_OP_piece 0x3
> > + * DW_AT_name ("ebx")
> > + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> > + *
> > + * In the above example, at some point, one unsigned_32 value
> > + * is right shifted by 8 and the result is converted to unsigned_32
> > + * and then unsigned_24.
> > + *
> > + * BTF does not need such DW_OP_* information so let us sanitize
> > + * these non-regular int types to avoid libbpf/kernel complaints.
> > + */
> > + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> > + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> > + name = "__SANITIZED_FAKE_INT__";
> > + byte_sz = 4;
> > + }
> > +
> > + id = btf__add_int(btf, name, byte_sz, encoding);
> > if (id < 0) {
> > btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> > } else {
> > --
> > 2.24.1
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-08 19:22 ` Nick Desaulniers
2021-02-08 19:27 ` Sedat Dilek
@ 2021-02-08 20:13 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-08 20:13 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Yonghong Song, dwarves, bpf, Andrii Nakryiko, Mark Wielaard,
Sedat Dilek, Andrii Nakryiko
Em Mon, Feb 08, 2021 at 11:22:48AM -0800, Nick Desaulniers escreveu:
> On Sat, Feb 6, 2021 at 11:17 PM Yonghong Song <yhs@fb.com> wrote:
> >
> > clang with dwarf5 may generate non-regular int base type,
> > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > Such base types are often used to describe
> > how an actual parameter or variable is generated. For example,
> >
> > 0x000015cf: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_1")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x00)
> >
> > 0x00010ed9: DW_TAG_formal_parameter
> > DW_AT_location (DW_OP_lit0,
> > DW_OP_not,
> > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > DW_OP_stack_value)
> > DW_AT_abstract_origin (0x00013984 "branch")
> >
> > What it does is with a literal "0", did a "not" operation, and the converted to
> > one-bit unsigned int and then 8-bit unsigned int.
> >
> > Another example,
> >
> > 0x000e97e4: DW_TAG_base_type
> > DW_AT_name ("DW_ATE_unsigned_24")
> > DW_AT_encoding (DW_ATE_unsigned)
> > DW_AT_byte_size (0x03)
> >
> > 0x000f88f8: DW_TAG_variable
> > DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > [0xffffffff82808812, 0xffffffff82808817):
> > DW_OP_breg0 RAX+0,
> > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > DW_OP_stack_value,
> > DW_OP_piece 0x1,
> > DW_OP_breg0 RAX+0,
> > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > DW_OP_lit8,
> > DW_OP_shr,
> > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > DW_OP_stack_value,
> > DW_OP_piece 0x3
> > ......
> >
> > At one point, a right shift by 8 happens and the result is converted to
> > 32-bit unsigned int and then to 24-bit unsigned int.
> >
> > BTF does not need any of these DW_OP_* information and such non-regular int
> > types will cause libbpf to emit errors.
> > Let us sanitize them to generate BTF acceptable to libbpf and kernel.
> >
> > Cc: Sedat Dilek <sedat.dilek@gmail.com>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Yonghong Song <yhs@fb.com>
>
> Thanks for the patch!
>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Thanks for testing and documenting that you tested, added the tag to
the commit,
- Arnaldo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-08 19:55 ` Arnaldo Carvalho de Melo
@ 2021-02-08 20:46 ` Sedat Dilek
2021-02-08 21:07 ` Andrii Nakryiko
0 siblings, 1 reply; 14+ messages in thread
From: Sedat Dilek @ 2021-02-08 20:46 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Yonghong Song, dwarves, bpf, andriin, mark, Nick Desaulniers,
Andrii Nakryiko
On Mon, Feb 8, 2021 at 8:55 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Sun, Feb 07, 2021 at 11:32:00AM +0100, Sedat Dilek escreveu:
> > On Sun, Feb 7, 2021 at 8:17 AM Yonghong Song <yhs@fb.com> wrote:
> > >
> > > clang with dwarf5 may generate non-regular int base type,
> > > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > > Such base types are often used to describe
> > > how an actual parameter or variable is generated. For example,
> > >
> > > 0x000015cf: DW_TAG_base_type
> > > DW_AT_name ("DW_ATE_unsigned_1")
> > > DW_AT_encoding (DW_ATE_unsigned)
> > > DW_AT_byte_size (0x00)
> > >
> > > 0x00010ed9: DW_TAG_formal_parameter
> > > DW_AT_location (DW_OP_lit0,
> > > DW_OP_not,
> > > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > > DW_OP_stack_value)
> > > DW_AT_abstract_origin (0x00013984 "branch")
> > >
> > > What it does is with a literal "0", did a "not" operation, and the converted to
> > > one-bit unsigned int and then 8-bit unsigned int.
> > >
> > > Another example,
> > >
> > > 0x000e97e4: DW_TAG_base_type
> > > DW_AT_name ("DW_ATE_unsigned_24")
> > > DW_AT_encoding (DW_ATE_unsigned)
> > > DW_AT_byte_size (0x03)
> > >
> > > 0x000f88f8: DW_TAG_variable
> > > DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > > [0xffffffff82808812, 0xffffffff82808817):
> > > DW_OP_breg0 RAX+0,
> > > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > > DW_OP_stack_value,
> > > DW_OP_piece 0x1,
> > > DW_OP_breg0 RAX+0,
> > > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > DW_OP_lit8,
> > > DW_OP_shr,
> > > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > > DW_OP_stack_value,
> > > DW_OP_piece 0x3
> > > ......
> > >
> > > At one point, a right shift by 8 happens and the result is converted to
> > > 32-bit unsigned int and then to 24-bit unsigned int.
> > >
> > > BTF does not need any of these DW_OP_* information and such non-regular int
> > > types will cause libbpf to emit errors.
> > > Let us sanitize them to generate BTF acceptable to libbpf and kernel.
> > >
> > > Cc: Sedat Dilek <sedat.dilek@gmail.com>
> >
> > Thanks for v2.
> >
> > For both v1 and v2:
> >
> > Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
>
> Thanks, applied.
>
Great.
I cannot see it yet in [1] or [2].
More important to me is is this worth a pahole v1.20.1 release?
This patch is required to successfully build with BTF and DWARF-5 and Clang-12.
I have Nathan's "bpf: Hoise pahole version checks into Kconfig" patch
in my custom patchset (together with Nick's DWARF-v5 patchset) which
makes it easier to do (depends on PAHOLE_VERSION >= 1201) via Kconfig
for example.
- Sedat -
[1] https://git.kernel.org/pub/scm/devel/pahole/pahole.git/
[2] https://github.com/acmel/dwarves/
> - Arnaldo
>
> > My development and testing environment:
> >
> > 1. Debian/testing AMD64
> > 2. Linux v5.11-rc6+ with custom mostly Clang fixes
> > 3. Debug-Info: BTF + DWARF-v5 enabled
> > 4. Compiler and tools (incl. Clang's Integrated ASsembler IAS):
> > LLVM/Clang v12.0.0-rc1 (make LLVM=1 LLVM_IAS=1)
> >
> > Build and boot on bare metal.
> >
> > - Sedat -
> >
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: Yonghong Song <yhs@fb.com>
> > > ---
> > > libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 38 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libbtf.c b/libbtf.c
> > > index 9f76283..5843200 100644
> > > --- a/libbtf.c
> > > +++ b/libbtf.c
> > > @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > > struct btf *btf = btfe->btf;
> > > const struct btf_type *t;
> > > uint8_t encoding = 0;
> > > + uint16_t byte_sz;
> > > int32_t id;
> > >
> > > if (bt->is_signed) {
> > > @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > > return -1;
> > > }
> > >
> > > - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> > > + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> > > + * {num} is not power of 2 and may exceed 128. Such attributes
> > > + * are mostly used to record operation for an actual parameter
> > > + * or variable.
> > > + * For example,
> > > + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > > + * [0xffffffff82808812, 0xffffffff82808817):
> > > + * DW_OP_breg0 RAX+0,
> > > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > > + * DW_OP_stack_value,
> > > + * DW_OP_piece 0x1,
> > > + * DW_OP_breg0 RAX+0,
> > > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > + * DW_OP_lit8,
> > > + * DW_OP_shr,
> > > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > > + * DW_OP_stack_value, DW_OP_piece 0x3
> > > + * DW_AT_name ("ebx")
> > > + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> > > + *
> > > + * In the above example, at some point, one unsigned_32 value
> > > + * is right shifted by 8 and the result is converted to unsigned_32
> > > + * and then unsigned_24.
> > > + *
> > > + * BTF does not need such DW_OP_* information so let us sanitize
> > > + * these non-regular int types to avoid libbpf/kernel complaints.
> > > + */
> > > + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> > > + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> > > + name = "__SANITIZED_FAKE_INT__";
> > > + byte_sz = 4;
> > > + }
> > > +
> > > + id = btf__add_int(btf, name, byte_sz, encoding);
> > > if (id < 0) {
> > > btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> > > } else {
> > > --
> > > 2.24.1
> > >
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-08 20:46 ` Sedat Dilek
@ 2021-02-08 21:07 ` Andrii Nakryiko
2021-02-08 21:11 ` Sedat Dilek
0 siblings, 1 reply; 14+ messages in thread
From: Andrii Nakryiko @ 2021-02-08 21:07 UTC (permalink / raw)
To: Sedat Dilek
Cc: Arnaldo Carvalho de Melo, Yonghong Song, dwarves, bpf,
Andrii Nakryiko, Mark Wielaard, Nick Desaulniers,
Andrii Nakryiko
On Mon, Feb 8, 2021 at 12:46 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 8:55 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Sun, Feb 07, 2021 at 11:32:00AM +0100, Sedat Dilek escreveu:
> > > On Sun, Feb 7, 2021 at 8:17 AM Yonghong Song <yhs@fb.com> wrote:
> > > >
> > > > clang with dwarf5 may generate non-regular int base type,
> > > > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > > > Such base types are often used to describe
> > > > how an actual parameter or variable is generated. For example,
> > > >
> > > > 0x000015cf: DW_TAG_base_type
> > > > DW_AT_name ("DW_ATE_unsigned_1")
> > > > DW_AT_encoding (DW_ATE_unsigned)
> > > > DW_AT_byte_size (0x00)
> > > >
> > > > 0x00010ed9: DW_TAG_formal_parameter
> > > > DW_AT_location (DW_OP_lit0,
> > > > DW_OP_not,
> > > > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > > > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > > > DW_OP_stack_value)
> > > > DW_AT_abstract_origin (0x00013984 "branch")
> > > >
> > > > What it does is with a literal "0", did a "not" operation, and the converted to
> > > > one-bit unsigned int and then 8-bit unsigned int.
> > > >
> > > > Another example,
> > > >
> > > > 0x000e97e4: DW_TAG_base_type
> > > > DW_AT_name ("DW_ATE_unsigned_24")
> > > > DW_AT_encoding (DW_ATE_unsigned)
> > > > DW_AT_byte_size (0x03)
> > > >
> > > > 0x000f88f8: DW_TAG_variable
> > > > DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > > > [0xffffffff82808812, 0xffffffff82808817):
> > > > DW_OP_breg0 RAX+0,
> > > > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > > > DW_OP_stack_value,
> > > > DW_OP_piece 0x1,
> > > > DW_OP_breg0 RAX+0,
> > > > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > DW_OP_lit8,
> > > > DW_OP_shr,
> > > > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > > > DW_OP_stack_value,
> > > > DW_OP_piece 0x3
> > > > ......
> > > >
> > > > At one point, a right shift by 8 happens and the result is converted to
> > > > 32-bit unsigned int and then to 24-bit unsigned int.
> > > >
> > > > BTF does not need any of these DW_OP_* information and such non-regular int
> > > > types will cause libbpf to emit errors.
> > > > Let us sanitize them to generate BTF acceptable to libbpf and kernel.
> > > >
> > > > Cc: Sedat Dilek <sedat.dilek@gmail.com>
> > >
> > > Thanks for v2.
> > >
> > > For both v1 and v2:
> > >
> > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> > > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> >
> > Thanks, applied.
> >
>
> Great.
> I cannot see it yet in [1] or [2].
>
> More important to me is is this worth a pahole v1.20.1 release?
> This patch is required to successfully build with BTF and DWARF-5 and Clang-12.
>
> I have Nathan's "bpf: Hoise pahole version checks into Kconfig" patch
> in my custom patchset (together with Nick's DWARF-v5 patchset) which
> makes it easier to do (depends on PAHOLE_VERSION >= 1201) via Kconfig
I don't think the math works out with 1201 vs 121 (in the future), so
I'd rather prefer 1.21, if necessary.
> for example.
>
> - Sedat -
>
> [1] https://git.kernel.org/pub/scm/devel/pahole/pahole.git/
> [2] https://github.com/acmel/dwarves/
>
> > - Arnaldo
> >
> > > My development and testing environment:
> > >
> > > 1. Debian/testing AMD64
> > > 2. Linux v5.11-rc6+ with custom mostly Clang fixes
> > > 3. Debug-Info: BTF + DWARF-v5 enabled
> > > 4. Compiler and tools (incl. Clang's Integrated ASsembler IAS):
> > > LLVM/Clang v12.0.0-rc1 (make LLVM=1 LLVM_IAS=1)
> > >
> > > Build and boot on bare metal.
> > >
> > > - Sedat -
> > >
> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Yonghong Song <yhs@fb.com>
> > > > ---
> > > > libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 38 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libbtf.c b/libbtf.c
> > > > index 9f76283..5843200 100644
> > > > --- a/libbtf.c
> > > > +++ b/libbtf.c
> > > > @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > > > struct btf *btf = btfe->btf;
> > > > const struct btf_type *t;
> > > > uint8_t encoding = 0;
> > > > + uint16_t byte_sz;
> > > > int32_t id;
> > > >
> > > > if (bt->is_signed) {
> > > > @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > > > return -1;
> > > > }
> > > >
> > > > - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> > > > + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> > > > + * {num} is not power of 2 and may exceed 128. Such attributes
> > > > + * are mostly used to record operation for an actual parameter
> > > > + * or variable.
> > > > + * For example,
> > > > + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > > > + * [0xffffffff82808812, 0xffffffff82808817):
> > > > + * DW_OP_breg0 RAX+0,
> > > > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > > > + * DW_OP_stack_value,
> > > > + * DW_OP_piece 0x1,
> > > > + * DW_OP_breg0 RAX+0,
> > > > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > + * DW_OP_lit8,
> > > > + * DW_OP_shr,
> > > > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > > > + * DW_OP_stack_value, DW_OP_piece 0x3
> > > > + * DW_AT_name ("ebx")
> > > > + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> > > > + *
> > > > + * In the above example, at some point, one unsigned_32 value
> > > > + * is right shifted by 8 and the result is converted to unsigned_32
> > > > + * and then unsigned_24.
> > > > + *
> > > > + * BTF does not need such DW_OP_* information so let us sanitize
> > > > + * these non-regular int types to avoid libbpf/kernel complaints.
> > > > + */
> > > > + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> > > > + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> > > > + name = "__SANITIZED_FAKE_INT__";
> > > > + byte_sz = 4;
> > > > + }
> > > > +
> > > > + id = btf__add_int(btf, name, byte_sz, encoding);
> > > > if (id < 0) {
> > > > btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> > > > } else {
> > > > --
> > > > 2.24.1
> > > >
> >
> > --
> >
> > - Arnaldo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
2021-02-08 21:07 ` Andrii Nakryiko
@ 2021-02-08 21:11 ` Sedat Dilek
0 siblings, 0 replies; 14+ messages in thread
From: Sedat Dilek @ 2021-02-08 21:11 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Arnaldo Carvalho de Melo, Yonghong Song, dwarves, bpf,
Andrii Nakryiko, Mark Wielaard, Nick Desaulniers,
Andrii Nakryiko
On Mon, Feb 8, 2021 at 10:08 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 12:46 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > On Mon, Feb 8, 2021 at 8:55 PM Arnaldo Carvalho de Melo
> > <arnaldo.melo@gmail.com> wrote:
> > >
> > > Em Sun, Feb 07, 2021 at 11:32:00AM +0100, Sedat Dilek escreveu:
> > > > On Sun, Feb 7, 2021 at 8:17 AM Yonghong Song <yhs@fb.com> wrote:
> > > > >
> > > > > clang with dwarf5 may generate non-regular int base type,
> > > > > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > > > > Such base types are often used to describe
> > > > > how an actual parameter or variable is generated. For example,
> > > > >
> > > > > 0x000015cf: DW_TAG_base_type
> > > > > DW_AT_name ("DW_ATE_unsigned_1")
> > > > > DW_AT_encoding (DW_ATE_unsigned)
> > > > > DW_AT_byte_size (0x00)
> > > > >
> > > > > 0x00010ed9: DW_TAG_formal_parameter
> > > > > DW_AT_location (DW_OP_lit0,
> > > > > DW_OP_not,
> > > > > DW_OP_convert (0x000015cf) "DW_ATE_unsigned_1",
> > > > > DW_OP_convert (0x000015d4) "DW_ATE_unsigned_8",
> > > > > DW_OP_stack_value)
> > > > > DW_AT_abstract_origin (0x00013984 "branch")
> > > > >
> > > > > What it does is with a literal "0", did a "not" operation, and the converted to
> > > > > one-bit unsigned int and then 8-bit unsigned int.
> > > > >
> > > > > Another example,
> > > > >
> > > > > 0x000e97e4: DW_TAG_base_type
> > > > > DW_AT_name ("DW_ATE_unsigned_24")
> > > > > DW_AT_encoding (DW_ATE_unsigned)
> > > > > DW_AT_byte_size (0x03)
> > > > >
> > > > > 0x000f88f8: DW_TAG_variable
> > > > > DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > > > > [0xffffffff82808812, 0xffffffff82808817):
> > > > > DW_OP_breg0 RAX+0,
> > > > > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > > DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > > > > DW_OP_stack_value,
> > > > > DW_OP_piece 0x1,
> > > > > DW_OP_breg0 RAX+0,
> > > > > DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > > DW_OP_lit8,
> > > > > DW_OP_shr,
> > > > > DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > > DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > > > > DW_OP_stack_value,
> > > > > DW_OP_piece 0x3
> > > > > ......
> > > > >
> > > > > At one point, a right shift by 8 happens and the result is converted to
> > > > > 32-bit unsigned int and then to 24-bit unsigned int.
> > > > >
> > > > > BTF does not need any of these DW_OP_* information and such non-regular int
> > > > > types will cause libbpf to emit errors.
> > > > > Let us sanitize them to generate BTF acceptable to libbpf and kernel.
> > > > >
> > > > > Cc: Sedat Dilek <sedat.dilek@gmail.com>
> > > >
> > > > Thanks for v2.
> > > >
> > > > For both v1 and v2:
> > > >
> > > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> > > > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> > >
> > > Thanks, applied.
> > >
> >
> > Great.
> > I cannot see it yet in [1] or [2].
> >
> > More important to me is is this worth a pahole v1.20.1 release?
> > This patch is required to successfully build with BTF and DWARF-5 and Clang-12.
> >
> > I have Nathan's "bpf: Hoise pahole version checks into Kconfig" patch
> > in my custom patchset (together with Nick's DWARF-v5 patchset) which
> > makes it easier to do (depends on PAHOLE_VERSION >= 1201) via Kconfig
>
> I don't think the math works out with 1201 vs 121 (in the future), so
> I'd rather prefer 1.21, if necessary.
>
Aaargh, you are right.
# /opt/pahole/bin/pahole --numeric_version
120
The wish is to have a pahole v1.21.
- Sedat -
> > for example.
> >
> > - Sedat -
> >
> > [1] https://git.kernel.org/pub/scm/devel/pahole/pahole.git/
> > [2] https://github.com/acmel/dwarves/
> >
> > > - Arnaldo
> > >
> > > > My development and testing environment:
> > > >
> > > > 1. Debian/testing AMD64
> > > > 2. Linux v5.11-rc6+ with custom mostly Clang fixes
> > > > 3. Debug-Info: BTF + DWARF-v5 enabled
> > > > 4. Compiler and tools (incl. Clang's Integrated ASsembler IAS):
> > > > LLVM/Clang v12.0.0-rc1 (make LLVM=1 LLVM_IAS=1)
> > > >
> > > > Build and boot on bare metal.
> > > >
> > > > - Sedat -
> > > >
> > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > Signed-off-by: Yonghong Song <yhs@fb.com>
> > > > > ---
> > > > > libbtf.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > > > > 1 file changed, 38 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libbtf.c b/libbtf.c
> > > > > index 9f76283..5843200 100644
> > > > > --- a/libbtf.c
> > > > > +++ b/libbtf.c
> > > > > @@ -373,6 +373,7 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > > > > struct btf *btf = btfe->btf;
> > > > > const struct btf_type *t;
> > > > > uint8_t encoding = 0;
> > > > > + uint16_t byte_sz;
> > > > > int32_t id;
> > > > >
> > > > > if (bt->is_signed) {
> > > > > @@ -384,7 +385,43 @@ int32_t btf_elf__add_base_type(struct btf_elf *btfe, const struct base_type *bt,
> > > > > return -1;
> > > > > }
> > > > >
> > > > > - id = btf__add_int(btf, name, BITS_ROUNDUP_BYTES(bt->bit_size), encoding);
> > > > > + /* dwarf5 may emit DW_ATE_[un]signed_{num} base types where
> > > > > + * {num} is not power of 2 and may exceed 128. Such attributes
> > > > > + * are mostly used to record operation for an actual parameter
> > > > > + * or variable.
> > > > > + * For example,
> > > > > + * DW_AT_location (indexed (0x3c) loclist = 0x00008fb0:
> > > > > + * [0xffffffff82808812, 0xffffffff82808817):
> > > > > + * DW_OP_breg0 RAX+0,
> > > > > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > > + * DW_OP_convert (0x000e97df) "DW_ATE_unsigned_8",
> > > > > + * DW_OP_stack_value,
> > > > > + * DW_OP_piece 0x1,
> > > > > + * DW_OP_breg0 RAX+0,
> > > > > + * DW_OP_convert (0x000e97d5) "DW_ATE_unsigned_64",
> > > > > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > > + * DW_OP_lit8,
> > > > > + * DW_OP_shr,
> > > > > + * DW_OP_convert (0x000e97da) "DW_ATE_unsigned_32",
> > > > > + * DW_OP_convert (0x000e97e4) "DW_ATE_unsigned_24",
> > > > > + * DW_OP_stack_value, DW_OP_piece 0x3
> > > > > + * DW_AT_name ("ebx")
> > > > > + * DW_AT_decl_file ("/linux/arch/x86/events/intel/core.c")
> > > > > + *
> > > > > + * In the above example, at some point, one unsigned_32 value
> > > > > + * is right shifted by 8 and the result is converted to unsigned_32
> > > > > + * and then unsigned_24.
> > > > > + *
> > > > > + * BTF does not need such DW_OP_* information so let us sanitize
> > > > > + * these non-regular int types to avoid libbpf/kernel complaints.
> > > > > + */
> > > > > + byte_sz = BITS_ROUNDUP_BYTES(bt->bit_size);
> > > > > + if (!byte_sz || (byte_sz & (byte_sz - 1))) {
> > > > > + name = "__SANITIZED_FAKE_INT__";
> > > > > + byte_sz = 4;
> > > > > + }
> > > > > +
> > > > > + id = btf__add_int(btf, name, byte_sz, encoding);
> > > > > if (id < 0) {
> > > > > btf_elf__log_err(btfe, BTF_KIND_INT, name, true, "Error emitting BTF type");
> > > > > } else {
> > > > > --
> > > > > 2.24.1
> > > > >
> > >
> > > --
> > >
> > > - Arnaldo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type
[not found] ` <CAMXQf9_Qy5tD05ax1vtETnzM9szLxm95JgpHgT0HzjktixMNNQ@mail.gmail.com>
@ 2021-02-08 21:24 ` Mark Wielaard
0 siblings, 0 replies; 14+ messages in thread
From: Mark Wielaard @ 2021-02-08 21:24 UTC (permalink / raw)
To: David Blaikie, Nick Desaulniers
Cc: Yonghong Song, Arnaldo Carvalho de Melo, dwarves, bpf,
Andrii Nakryiko, Sedat Dilek, Andrii Nakryiko
Hi David,
(Nice to have seen you just recently!)
On Mon, 2021-02-08 at 12:43 -0800, David Blaikie wrote:
> DW_OP_convert was added in DWARFv5 and it can be used for type conversions,
> these are created in the LLVM middle/backend during optimizations, not by
> the frontend. So the middle/backend doesn't have a way to create canonical
> DW_TAG_base_types for frontend language integer types - so it creates
> synthesized ones. So this is intentional/not a particularly quirky thing.
>
> LLVM creates locations incrementally as optimizations are applied - without
> doing any particular canonicalization/reduction later on (maybe it has some
> canonicalization/reduction, but not a very wholistic/aggressive approach
> there) - so it can end up with something that is not the most compact
> representation.
But it seems to end up with a bogus representation (see below in the
original quoted message for an example). The issue isn't really that it
isn't an optimized constant representation (although it is kind of an
inefficient one). It is this DW_OP_convert applied to a
DW_TAG_base_type DIE with a DW_AT_byte_size of zero. Which doesn't
really make any sense to me. It feels like it is asking the DWARF
consumer to do a divide by zero here ("now express this value using
zero bits!"). Could you explain these syntactic "DW_ATE_unsigned_1"
base_type DIEs? What do they represent? Why do they have a zero size?
Should they really have a DW_AT_bit_size and a DW_AT_byte_size of 1?
Thanks,
Mark
> On Mon, Feb 8, 2021 at 11:17 AM Nick Desaulniers <ndesaulniers@google.com>
> wrote:
>
> > On Sun, Feb 7, 2021 at 6:18 AM Mark Wielaard <mark@klomp.org> wrote:
> > >
> > > Hi,
> > >
> > > On Sat, 2021-02-06 at 23:17 -0800, Yonghong Song wrote:
> > > > clang with dwarf5 may generate non-regular int base type,
> > > > i.e., not a signed/unsigned char/short/int/longlong/__int128.
> > > > Such base types are often used to describe
> > > > how an actual parameter or variable is generated. For example,
> > > >
> > > > 0x000015cf: DW_TAG_base_type
> > > > DW_AT_name ("DW_ATE_unsigned_1")
> > > > DW_AT_encoding (DW_ATE_unsigned)
> > > > DW_AT_byte_size (0x00)
> > > >
> > > > 0x00010ed9: DW_TAG_formal_parameter
> > > > DW_AT_location (DW_OP_lit0,
> > > > DW_OP_not,
> > > > DW_OP_convert (0x000015cf)
> >
> > "DW_ATE_unsigned_1",
> > > > DW_OP_convert (0x000015d4)
> >
> > "DW_ATE_unsigned_8",
> > > > DW_OP_stack_value)
> > > > DW_AT_abstract_origin (0x00013984 "branch")
> > > >
> > > > What it does is with a literal "0", did a "not" operation, and the
> >
> > converted to
> > > > one-bit unsigned int and then 8-bit unsigned int.
> > >
> > > Thanks for tracking this down. Do you have any idea why the clang
> > > compiler emits this? You might be right that it is intended to do what
> > > you describe it does (but then it would simply encode an unsigned
> > > constant 1 char in a very inefficient way). But as implemented it
> > > doesn't seem to make any sense. What would DW_OP_convert of an zero
> > > sized base type even mean (if it is intended as a 1 bit sized typed,
> > > then why is there no DW_AT_bit_size)?
> >
> > David,
> > Any thoughts on the above sequence of DW_OP_ entries? This is a part
> > of DWARF I'm unfamiliar with.
> >
> > >
> > > So I do think your patch makes sense. clang clearly is emitting
> > > something bogus. And so some fixup is needed. But maybe we should at
> > > least give a warning about it, otherwise it might never get fixed.
> > >
> > > BTW. If these bogus base types are only emitted as part of a location
> > > expression and not as part of an actual function or variable type
> > > description, then why are we even trying to encode it as a BTF type? It
> > > might be cheaper to just skip/drop it. But maybe the code setup makes
> > > it hard to know whether or not such a (bogus) type is actually
> > > referenced from a function or variable description?
> > >
> > > Cheers,
> > >
> > > Mark
> >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-02-08 21:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-07 7:17 [PATCH dwarves v2] btf_encoder: sanitize non-regular int base type Yonghong Song
2021-02-07 10:32 ` Sedat Dilek
2021-02-08 19:55 ` Arnaldo Carvalho de Melo
2021-02-08 20:46 ` Sedat Dilek
2021-02-08 21:07 ` Andrii Nakryiko
2021-02-08 21:11 ` Sedat Dilek
2021-02-07 14:18 ` Mark Wielaard
2021-02-07 15:15 ` Sedat Dilek
2021-02-07 18:14 ` Yonghong Song
2021-02-08 19:16 ` Nick Desaulniers
[not found] ` <CAMXQf9_Qy5tD05ax1vtETnzM9szLxm95JgpHgT0HzjktixMNNQ@mail.gmail.com>
2021-02-08 21:24 ` Mark Wielaard
2021-02-08 19:22 ` Nick Desaulniers
2021-02-08 19:27 ` Sedat Dilek
2021-02-08 20:13 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.