On Tue, Feb 28, 2023 at 10:08 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Tue, 2023-02-28 at 03:55 +0100, KP Singh wrote: > > On Mon, Feb 27, 2023 at 9:48 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > > > > On Mon, 2023-02-27 at 11:31 -0800, Andrii Nakryiko wrote: > > > [...] > > > > > > I'd start with understanding what BTF and DWARF differences are > > > > > > causing the issue before trying to come up with the fix. For that we > > > > > > don't even need config or repro steps, it should be enough to share > > > > > > vmlinux with BTF and DWARF, and start from there. > > > > > > > > > > > > > > > > Yes, I suspect that there is some kind of unanticipated > > > > > anomaly for some DWARF encoding for some kind of objects, > > > > > just need to find the root for the diverging type hierarchies. > > > > > > > > > > > But I'm sure Eduard is on top of this already (especially that he can > > > > > > repro the issue now). > > > > > > > > > > I'm working on it, nothing to report yet, but I'm working on it. > > > > > > > > > > > > > Thanks, please keep us posted! > > > > > > It is interesting how everything is interconnected. The patch for > > > pahole below happens to help. I prepared it last week while working on > > > new DWARF encoding scheme for btf_type_tag. > > > > > > I still need to track down which "unspecified_type" entries caused the > > > issue in this particular case. Will post an update tomorrow. > > > > > > Meanwhile, Matt, KP, could you please verify the patch on your side? > > > It is for the "next" branch of pahole. > > > > > > --- > > > > > > From 09fac63ca08e25aea499f827283b07cc87a7daab Mon Sep 17 00:00:00 2001 > > > From: Eduard Zingerman <eddyz87@xxxxxxxxx> > > > Date: Tue, 21 Feb 2023 19:23:00 +0200 > > > Subject: [PATCH] dwarf_loader: Fix for BTF id drift caused by adding > > > unspecified types > > > > > > Recent changes to handle unspecified types (see [1]) cause BTF ID drift. > > > > > > Specifically, the intent of commits [2], [3] and [4] is to render > > > references to unspecified types as void type. > > > However, as a consequence: > > > - in `die__process_unit()` call to `cu__add_tag()` allocates `small_id` > > > for unspecified type tags and adds these tags to `cu->types_table`; > > > - `btf_encoder__encode_tag()` skips generation of BTF entries for > > > `DW_TAG_unspecified_type` tags. > > > > > > Such logic causes ID drift if unspecified type is not the last type > > > processed for compilation unit. `small_id` of each type following > > > unspecified type in the `cu->types_table` would have its BTF id off by -1. > > > Thus renders references established on recode phase invalid. > > > > > > This commit reverts `unspecified_type` id/tag tracking, instead: > > > - `small_id` for unspecified type tags is set to 0, thus reference to > > > unspecified type tag would render BTF id of a `void` on recode phase; > > > - unspecified type tags are not added to `cu->types_table`. > > > > > > [1] https://lore.kernel.org/all/Y0R7uu3s%2FimnvPzM@xxxxxxxxxx/ > > > [2] bcc648a10cbc ("btf_encoder: Encode DW_TAG_unspecified_type returning routines as void") > > > [3] cffe5e1f75e1 ("core: Record if a CU has a DW_TAG_unspecified_type") > > > [4] 75e0fe28bb02 ("core: Add DW_TAG_unspecified_type to tag__is_tag_type() set") > > > > > > Fixes: bcc648a10cbc ("btf_encoder: Encode DW_TAG_unspecified_type returning routines as void") > > > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > > > > Tested-by: KP Singh <kpsingh@xxxxxxxxxx> > > Reported-by: Matt Bobrowski <mattbobrowski@xxxxxxxxxx> > > > > Thank you so much Eduard, this worked: > > > > * No duplicate BTF ID warnings > > * No 15 minute BTF ID generation > > * Matt's reproducer loads successfully. > > > > I had a sneaky suspicion that it was these unspecified types, which is > > why my hacky patch which got unspecified types out of the way got > > things to *mostly* work. > > Hi KP, > > Thanks a lot for testing! > > I found the root cause for the bug (took me longer than I would like > to admit...). Using the patch below the reproducer from Matt works as > expected and warnings are gone. > > Still, I think that my patch from yesterday is a more general > approach, as it correctly handles unspecified types that occur in > non-tail position, so I'll post that one. I agree, please send it to dwarves@ as a proper patch. Thank you for digging into this and fixing it quickly! > > Thanks, > Eduard > > --- > > From daa53248e8a5087edbceaffe1fad51f9eb06e922 Mon Sep 17 00:00:00 2001 > From: Eduard Zingerman <eddyz87@xxxxxxxxx> > Date: Tue, 28 Feb 2023 19:44:22 +0200 > Subject: [PATCH] btf_encoder: reset encoder->unspecified_type for each CU > > The field `encoder->unspecified_type` is set but not reset by function > `btf_encoder__encode_cu()` when processed `cu` has unspecified type. > The following sequence of events might occur when BTF encoding is > requested: > - CU with unspecified type is processed: > - unspecified type id is 42 > - encoder->unspecified_type is set to 42 > - CU without unspecified type is processed next using the same > `encoder` object: > - some `struct foo` has id 42 in this CU > - the references to `struct foo` are set 0 by function > `btf_encoder__tag_type()`. > > This commit sets `encoder->unspecified_type` to 0 when CU does not > have unspecified type. > > This issue was reported in thread [1]. > See also [2]. > [1] https://lore.kernel.org/bpf/Y%2FP1yxAuV6Wj3A0K@xxxxxxxxxx/ > [2] https://lore.kernel.org/all/Y0R7uu3s%2FimnvPzM@xxxxxxxxxx/ > > Fixes: 52b25808e44a ("btf_encoder: Store type_id_off, unspecified type in encoder") > Reported-by: Matt Bobrowski <mattbobrowski@xxxxxxxxxx> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > --- > btf_encoder.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/btf_encoder.c b/btf_encoder.c > index da776f4..24f4c65 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -1748,6 +1748,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co > encoder->type_id_off = btf__type_cnt(encoder->btf) - 1; > if (encoder->cu->unspecified_type.tag) > encoder->unspecified_type = encoder->cu->unspecified_type.type; > + else > + encoder->unspecified_type = 0; > > if (!encoder->has_index_type) { > /* cu__find_base_type_by_name() takes "type_id_t *id" */ > -- > 2.39.1
