* bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
@ 2023-07-04 13:30 Lorenz Bauer
2023-07-06 4:50 ` Yonghong Song
0 siblings, 1 reply; 12+ messages in thread
From: Lorenz Bauer @ 2023-07-04 13:30 UTC (permalink / raw)
To: bpf, Yonghong Song, Andrii Nakryiko
Hi,
I think that CO-RE has inconsistent behaviour wrt. BPF_TYPE_ID_LOCAL
and BPF_TYPE_ID_TARGET when dealing with qualifiers (modifiers?) Given
the following C:
enum bpf_type_id_kind {
BPF_TYPE_ID_LOCAL = 0, /* BTF type ID in local program */
BPF_TYPE_ID_TARGET = 1, /* BTF type ID in target kernel */
};
int foo(void) {
return __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_TARGET)
!= __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_LOCAL);
}
That line with __builtin_btf_type_id is just the expansion of
bpf_core_type_id_kernel, etc. clang generates the following BPF:
foo:
18 01 00 00 02 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x2 ll
79 11 00 00 00 00 00 00 r1 = *(u64 *)(r1 + 0x0)
18 02 00 00 04 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x4 ll
79 22 00 00 00 00 00 00 r2 = *(u64 *)(r2 + 0x0)
b7 03 00 00 00 00 00 00 r3 = 0x0
7b 3a f0 ff 00 00 00 00 *(u64 *)(r10 - 0x10) = r3
b7 03 00 00 01 00 00 00 r3 = 0x1
7b 3a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r3
5d 21 02 00 00 00 00 00 if r1 != r2 goto +0x2 <LBB0_2>
79 a1 f0 ff 00 00 00 00 r1 = *(u64 *)(r10 - 0x10)
7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r1
LBB0_2:
79 a0 f8 ff 00 00 00 00 r0 = *(u64 *)(r10 - 0x8)
95 00 00 00 00 00 00 00 exit
Link to godbolt: https://godbolt.org/z/jr63hKz9E (contains version info)
Note that the first two ldimm64 have distinct type IDs. I added some
debug logging to cilium/ebpf and found that the compiler indeed also
emits distinct CO-RE relocations:
foo {InsnOff:0 TypeID:2 AccessStrOff:69 Kind:local_type_id}
foo {InsnOff:2 TypeID:4 AccessStrOff:69 Kind:target_type_id}
It seems that for BPF_TYPE_ID_TARGET the outer const is peeled, while
this doesn't happen for the local variant.
CORERelocation(local_type_id, Const[0], local_id=4) local_type_id=4->4
CORERelocation(target_type_id, Int:"int"[0], local_id=2) target_type_id=2->2
Similar behaviour exists for BPF_TYPE_EXISTS, probably others.
The behaviour goes away if I drop the pointer casting magic:
__builtin_btf_type_id((const int)0, BPF_TYPE_ID_TARGET) !=
__builtin_btf_type_id((const int)0, BPF_TYPE_ID_LOCAL)
Intuitively I'd say that the root cause is that dereferencing the
pointer drops the constness of the type. Why does TARGET behave
differently than LOCAL though?
Cheers
Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-07-04 13:30 bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local Lorenz Bauer
@ 2023-07-06 4:50 ` Yonghong Song
2023-07-06 21:06 ` Andrii Nakryiko
2023-10-31 15:46 ` Lorenz Bauer
0 siblings, 2 replies; 12+ messages in thread
From: Yonghong Song @ 2023-07-06 4:50 UTC (permalink / raw)
To: Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On 7/4/23 6:30 AM, Lorenz Bauer wrote:
> Hi,
>
> I think that CO-RE has inconsistent behaviour wrt. BPF_TYPE_ID_LOCAL
> and BPF_TYPE_ID_TARGET when dealing with qualifiers (modifiers?) Given
> the following C:
>
> enum bpf_type_id_kind {
> BPF_TYPE_ID_LOCAL = 0, /* BTF type ID in local program */
> BPF_TYPE_ID_TARGET = 1, /* BTF type ID in target kernel */
> };
>
> int foo(void) {
> return __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_TARGET)
> != __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_LOCAL);
> }
>
> That line with __builtin_btf_type_id is just the expansion of
> bpf_core_type_id_kernel, etc. clang generates the following BPF:
>
> foo:
> 18 01 00 00 02 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x2 ll
> 79 11 00 00 00 00 00 00 r1 = *(u64 *)(r1 + 0x0)
> 18 02 00 00 04 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x4 ll
> 79 22 00 00 00 00 00 00 r2 = *(u64 *)(r2 + 0x0)
> b7 03 00 00 00 00 00 00 r3 = 0x0
> 7b 3a f0 ff 00 00 00 00 *(u64 *)(r10 - 0x10) = r3
> b7 03 00 00 01 00 00 00 r3 = 0x1
> 7b 3a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r3
> 5d 21 02 00 00 00 00 00 if r1 != r2 goto +0x2 <LBB0_2>
> 79 a1 f0 ff 00 00 00 00 r1 = *(u64 *)(r10 - 0x10)
> 7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r1
> LBB0_2:
> 79 a0 f8 ff 00 00 00 00 r0 = *(u64 *)(r10 - 0x8)
> 95 00 00 00 00 00 00 00 exit
>
> Link to godbolt: https://godbolt.org/z/jr63hKz9E (contains version info)
>
> Note that the first two ldimm64 have distinct type IDs. I added some
> debug logging to cilium/ebpf and found that the compiler indeed also
> emits distinct CO-RE relocations:
>
> foo {InsnOff:0 TypeID:2 AccessStrOff:69 Kind:local_type_id}
> foo {InsnOff:2 TypeID:4 AccessStrOff:69 Kind:target_type_id}
>
> It seems that for BPF_TYPE_ID_TARGET the outer const is peeled, while
> this doesn't happen for the local variant.
>
> CORERelocation(local_type_id, Const[0], local_id=4) local_type_id=4->4
> CORERelocation(target_type_id, Int:"int"[0], local_id=2) target_type_id=2->2
>
> Similar behaviour exists for BPF_TYPE_EXISTS, probably others.
>
> The behaviour goes away if I drop the pointer casting magic:
>
> __builtin_btf_type_id((const int)0, BPF_TYPE_ID_TARGET) !=
> __builtin_btf_type_id((const int)0, BPF_TYPE_ID_LOCAL)
>
> Intuitively I'd say that the root cause is that dereferencing the
> pointer drops the constness of the type. Why does TARGET behave
> differently than LOCAL though?
Thanks for reporting. The difference of type w.r.t. 'const' modifier
is a deliberate choice in llvm:
See
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFPreserveDIType.cpp#L84-L103
if (FlagValue == BPFCoreSharedInfo::BTF_TYPE_ID_LOCAL_RELOC) {
Reloc = BPFCoreSharedInfo::BTF_TYPE_ID_LOCAL;
} else {
Reloc = BPFCoreSharedInfo::BTF_TYPE_ID_REMOTE;
DIType *Ty = cast<DIType>(MD);
while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
unsigned Tag = DTy->getTag();
if (Tag != dwarf::DW_TAG_const_type &&
Tag != dwarf::DW_TAG_volatile_type)
break;
Ty = DTy->getBaseType();
}
if (Ty->getName().empty()) {
if (isa<DISubroutineType>(Ty))
report_fatal_error(
"SubroutineType not supported for BTF_TYPE_ID_REMOTE reloc");
else
report_fatal_error("Empty type name for BTF_TYPE_ID_REMOTE
reloc");
}
MD = Ty;
}
Basically, the BTF_TYPE_ID_REMOTE (the kernel term BPF_TYPE_ID_TARGET)
needs further checking to prevent some invalid cases.
Also for kernel type matching, it would be good to eliminate modifiers
otherwise, there could be many instances of 'const' which makes
kernel matching is more complicated.
But I see your point. Maybe we should preserve the original type
for BTF_TYPE_ID_TARGET as well. Will check what libbpf/kernel
will handle 'const int *' case and get back to this thread later.
>
> Cheers
> Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-07-06 4:50 ` Yonghong Song
@ 2023-07-06 21:06 ` Andrii Nakryiko
2023-07-11 16:20 ` Lorenz Bauer
2023-10-31 15:46 ` Lorenz Bauer
1 sibling, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2023-07-06 21:06 UTC (permalink / raw)
To: Yonghong Song; +Cc: Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On Wed, Jul 5, 2023 at 9:50 PM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 7/4/23 6:30 AM, Lorenz Bauer wrote:
> > Hi,
> >
> > I think that CO-RE has inconsistent behaviour wrt. BPF_TYPE_ID_LOCAL
> > and BPF_TYPE_ID_TARGET when dealing with qualifiers (modifiers?) Given
> > the following C:
> >
> > enum bpf_type_id_kind {
> > BPF_TYPE_ID_LOCAL = 0, /* BTF type ID in local program */
> > BPF_TYPE_ID_TARGET = 1, /* BTF type ID in target kernel */
> > };
> >
> > int foo(void) {
> > return __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_TARGET)
> > != __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_LOCAL);
> > }
> >
> > That line with __builtin_btf_type_id is just the expansion of
> > bpf_core_type_id_kernel, etc. clang generates the following BPF:
> >
> > foo:
> > 18 01 00 00 02 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x2 ll
> > 79 11 00 00 00 00 00 00 r1 = *(u64 *)(r1 + 0x0)
> > 18 02 00 00 04 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x4 ll
> > 79 22 00 00 00 00 00 00 r2 = *(u64 *)(r2 + 0x0)
> > b7 03 00 00 00 00 00 00 r3 = 0x0
> > 7b 3a f0 ff 00 00 00 00 *(u64 *)(r10 - 0x10) = r3
> > b7 03 00 00 01 00 00 00 r3 = 0x1
> > 7b 3a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r3
> > 5d 21 02 00 00 00 00 00 if r1 != r2 goto +0x2 <LBB0_2>
> > 79 a1 f0 ff 00 00 00 00 r1 = *(u64 *)(r10 - 0x10)
> > 7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r1
> > LBB0_2:
> > 79 a0 f8 ff 00 00 00 00 r0 = *(u64 *)(r10 - 0x8)
> > 95 00 00 00 00 00 00 00 exit
> >
> > Link to godbolt: https://godbolt.org/z/jr63hKz9E (contains version info)
> >
> > Note that the first two ldimm64 have distinct type IDs. I added some
> > debug logging to cilium/ebpf and found that the compiler indeed also
> > emits distinct CO-RE relocations:
> >
> > foo {InsnOff:0 TypeID:2 AccessStrOff:69 Kind:local_type_id}
> > foo {InsnOff:2 TypeID:4 AccessStrOff:69 Kind:target_type_id}
> >
> > It seems that for BPF_TYPE_ID_TARGET the outer const is peeled, while
> > this doesn't happen for the local variant.
> >
> > CORERelocation(local_type_id, Const[0], local_id=4) local_type_id=4->4
> > CORERelocation(target_type_id, Int:"int"[0], local_id=2) target_type_id=2->2
> >
> > Similar behaviour exists for BPF_TYPE_EXISTS, probably others.
> >
> > The behaviour goes away if I drop the pointer casting magic:
> >
> > __builtin_btf_type_id((const int)0, BPF_TYPE_ID_TARGET) !=
> > __builtin_btf_type_id((const int)0, BPF_TYPE_ID_LOCAL)
> >
> > Intuitively I'd say that the root cause is that dereferencing the
> > pointer drops the constness of the type. Why does TARGET behave
> > differently than LOCAL though?
>
> Thanks for reporting. The difference of type w.r.t. 'const' modifier
> is a deliberate choice in llvm:
>
> See
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFPreserveDIType.cpp#L84-L103
>
> if (FlagValue == BPFCoreSharedInfo::BTF_TYPE_ID_LOCAL_RELOC) {
> Reloc = BPFCoreSharedInfo::BTF_TYPE_ID_LOCAL;
> } else {
> Reloc = BPFCoreSharedInfo::BTF_TYPE_ID_REMOTE;
> DIType *Ty = cast<DIType>(MD);
> while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
> unsigned Tag = DTy->getTag();
> if (Tag != dwarf::DW_TAG_const_type &&
> Tag != dwarf::DW_TAG_volatile_type)
> break;
> Ty = DTy->getBaseType();
> }
>
> if (Ty->getName().empty()) {
> if (isa<DISubroutineType>(Ty))
> report_fatal_error(
> "SubroutineType not supported for BTF_TYPE_ID_REMOTE reloc");
> else
> report_fatal_error("Empty type name for BTF_TYPE_ID_REMOTE
> reloc");
> }
> MD = Ty;
> }
>
> Basically, the BTF_TYPE_ID_REMOTE (the kernel term BPF_TYPE_ID_TARGET)
> needs further checking to prevent some invalid cases.
> Also for kernel type matching, it would be good to eliminate modifiers
> otherwise, there could be many instances of 'const' which makes
> kernel matching is more complicated.
>
> But I see your point. Maybe we should preserve the original type
> for BTF_TYPE_ID_TARGET as well. Will check what libbpf/kernel
> will handle 'const int *' case and get back to this thread later.
I think it's better the other way around: make BTF_TYPE_ID_LOCAL strip
const/volatile/restrict modifiers. For all other relocations we rely
on having named types, so const/volatile makes no sense and will fail
relocation. It's hard to come up with the situation where recording
const/volatile/restrict in BTF_TYPE_ID_LOCAL would make sense, so I'd
say that it should behave just like all the other relos.
>
> >
> > Cheers
> > Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-07-06 21:06 ` Andrii Nakryiko
@ 2023-07-11 16:20 ` Lorenz Bauer
2023-07-11 16:38 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: Lorenz Bauer @ 2023-07-11 16:20 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: Yonghong Song, bpf, Yonghong Song, Andrii Nakryiko
On Thu, Jul 6, 2023 at 10:07 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> I think it's better the other way around: make BTF_TYPE_ID_LOCAL strip
> const/volatile/restrict modifiers. For all other relocations we rely
> on having named types, so const/volatile makes no sense and will fail
> relocation. It's hard to come up with the situation where recording
> const/volatile/restrict in BTF_TYPE_ID_LOCAL would make sense, so I'd
> say that it should behave just like all the other relos.
Would the relocation then point at the stripped type instead of the
start of the qualifier chain? I found this by running our unit tests
which essentially check that the compiler generated local ID from the
instruction stream matches what the lib generates. I'd like to be able
to keep doing this.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-07-11 16:20 ` Lorenz Bauer
@ 2023-07-11 16:38 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2023-07-11 16:38 UTC (permalink / raw)
To: Lorenz Bauer; +Cc: Yonghong Song, bpf, Yonghong Song, Andrii Nakryiko
On Tue, Jul 11, 2023 at 9:20 AM Lorenz Bauer <lmb@isovalent.com> wrote:
>
> On Thu, Jul 6, 2023 at 10:07 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > I think it's better the other way around: make BTF_TYPE_ID_LOCAL strip
> > const/volatile/restrict modifiers. For all other relocations we rely
> > on having named types, so const/volatile makes no sense and will fail
> > relocation. It's hard to come up with the situation where recording
> > const/volatile/restrict in BTF_TYPE_ID_LOCAL would make sense, so I'd
> > say that it should behave just like all the other relos.
>
> Would the relocation then point at the stripped type instead of the
> start of the qualifier chain? I found this by running our unit tests
yep, I think it makes most sense. Important is to not skip typedef,
it's not really a modifier (and libbpf code base makes it very
explicit, unlike in-kernel handling of typedef as a modifier).
> which essentially check that the compiler generated local ID from the
> instruction stream matches what the lib generates. I'd like to be able
> to keep doing this.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-07-06 4:50 ` Yonghong Song
2023-07-06 21:06 ` Andrii Nakryiko
@ 2023-10-31 15:46 ` Lorenz Bauer
2023-10-31 18:24 ` Andrii Nakryiko
1 sibling, 1 reply; 12+ messages in thread
From: Lorenz Bauer @ 2023-10-31 15:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On Thu, Jul 6, 2023 at 5:50 AM Yonghong Song <yhs@meta.com> wrote:
>
> But I see your point. Maybe we should preserve the original type
> for BTF_TYPE_ID_TARGET as well. Will check what libbpf/kernel
> will handle 'const int *' case and get back to this thread later.
>
> >
> > Cheers
> > Lorenz
Did you get round to fixing this, or did you decide to leave it as is?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-10-31 15:46 ` Lorenz Bauer
@ 2023-10-31 18:24 ` Andrii Nakryiko
2023-11-01 14:17 ` Lorenz Bauer
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2023-10-31 18:24 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Yonghong Song, Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On Tue, Oct 31, 2023 at 8:46 AM Lorenz Bauer <lorenz.bauer@isovalent.com> wrote:
>
> On Thu, Jul 6, 2023 at 5:50 AM Yonghong Song <yhs@meta.com> wrote:
> >
> > But I see your point. Maybe we should preserve the original type
> > for BTF_TYPE_ID_TARGET as well. Will check what libbpf/kernel
> > will handle 'const int *' case and get back to this thread later.
> >
> > >
> > > Cheers
> > > Lorenz
>
> Did you get round to fixing this, or did you decide to leave it as is?
Trying to recall, was there anything to do on the libbpf side, or was
it purely a compiler-side change?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-10-31 18:24 ` Andrii Nakryiko
@ 2023-11-01 14:17 ` Lorenz Bauer
2023-11-01 22:42 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: Lorenz Bauer @ 2023-11-01 14:17 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Yonghong Song, Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On Tue, Oct 31, 2023 at 6:24 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> >
> > Did you get round to fixing this, or did you decide to leave it as is?
>
> Trying to recall, was there anything to do on the libbpf side, or was
> it purely a compiler-side change?
I'm not 100% sure TBH. I'd like clang to behave consistently for
local_id and target_id. I don't know whether that would break libbpf.
Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-11-01 14:17 ` Lorenz Bauer
@ 2023-11-01 22:42 ` Andrii Nakryiko
2023-11-02 0:34 ` Yonghong Song
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 22:42 UTC (permalink / raw)
To: Lorenz Bauer, Eduard Zingerman
Cc: Yonghong Song, Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On Wed, Nov 1, 2023 at 7:18 AM Lorenz Bauer <lorenz.bauer@isovalent.com> wrote:
>
> On Tue, Oct 31, 2023 at 6:24 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >
> > > Did you get round to fixing this, or did you decide to leave it as is?
> >
> > Trying to recall, was there anything to do on the libbpf side, or was
> > it purely a compiler-side change?
>
> I'm not 100% sure TBH. I'd like clang to behave consistently for
> local_id and target_id. I don't know whether that would break libbpf.
>
*checks code* libbpf just passes through whatever ID compiler
generated, so there doesn't seem to be any change to libbpf. Seems
like compiler-only change. cc'ing Eduard as well, if he's curious
enough to check
> Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-11-01 22:42 ` Andrii Nakryiko
@ 2023-11-02 0:34 ` Yonghong Song
2023-11-02 2:16 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: Yonghong Song @ 2023-11-02 0:34 UTC (permalink / raw)
To: Andrii Nakryiko, Lorenz Bauer, Eduard Zingerman
Cc: Yonghong Song, Lorenz Bauer, bpf, Yonghong Song, Andrii Nakryiko
On 11/1/23 3:42 PM, Andrii Nakryiko wrote:
> On Wed, Nov 1, 2023 at 7:18 AM Lorenz Bauer <lorenz.bauer@isovalent.com> wrote:
>> On Tue, Oct 31, 2023 at 6:24 PM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>>>> Did you get round to fixing this, or did you decide to leave it as is?
>>> Trying to recall, was there anything to do on the libbpf side, or was
>>> it purely a compiler-side change?
>> I'm not 100% sure TBH. I'd like clang to behave consistently for
>> local_id and target_id. I don't know whether that would break libbpf.
>>
> *checks code* libbpf just passes through whatever ID compiler
> generated, so there doesn't seem to be any change to libbpf. Seems
> like compiler-only change. cc'ing Eduard as well, if he's curious
> enough to check
Okay, let us try to have a consistent behavior in local/remote type_id
by changing local_id semantics to be the same as target_id.
The corresponding llvm change is similar to
[yhs@devbig309.ftw3 ~/work/llvm-project (ed)]$ git diff
diff --git a/llvm/lib/Target/BPF/BPFPreserveDIType.cpp b/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
index 78e1bf90f1bd..1fbe1207dc6e 100644
--- a/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
+++ b/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
@@ -86,15 +86,17 @@ static bool BPFPreserveDITypeImpl(Function &F) {
Reloc = BTF::BTF_TYPE_ID_LOCAL;
} else {
Reloc = BTF::BTF_TYPE_ID_REMOTE;
- DIType *Ty = cast<DIType>(MD);
- while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
- unsigned Tag = DTy->getTag();
- if (Tag != dwarf::DW_TAG_const_type &&
- Tag != dwarf::DW_TAG_volatile_type)
- break;
- Ty = DTy->getBaseType();
- }
+ }
+ DIType *Ty = cast<DIType>(MD);
+ while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
+ unsigned Tag = DTy->getTag();
+ if (Tag != dwarf::DW_TAG_const_type &&
+ Tag != dwarf::DW_TAG_volatile_type)
+ break;
+ Ty = DTy->getBaseType();
+ }
+ if (Reloc == BTF::BTF_TYPE_ID_REMOTE) {
if (Ty->getName().empty()) {
if (isa<DISubroutineType>(Ty))
report_fatal_error(
@@ -102,8 +104,8 @@ static bool BPFPreserveDITypeImpl(Function &F) {
else
report_fatal_error("Empty type name for BTF_TYPE_ID_REMOTE reloc");
}
- MD = Ty;
}
+ MD = Ty;
BasicBlock *BB = Call->getParent();
IntegerType *VarType = Type::getInt64Ty(BB->getContext());
Either Eduard or Myself will submit a llvm patch to fix this in llvm18.
>
>
>> Lorenz
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-11-02 0:34 ` Yonghong Song
@ 2023-11-02 2:16 ` Andrii Nakryiko
2023-11-03 19:58 ` Yonghong Song
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2023-11-02 2:16 UTC (permalink / raw)
To: Yonghong Song
Cc: Lorenz Bauer, Eduard Zingerman, Yonghong Song, Lorenz Bauer, bpf,
Yonghong Song, Andrii Nakryiko
On Wed, Nov 1, 2023 at 5:34 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 11/1/23 3:42 PM, Andrii Nakryiko wrote:
> > On Wed, Nov 1, 2023 at 7:18 AM Lorenz Bauer <lorenz.bauer@isovalent.com> wrote:
> >> On Tue, Oct 31, 2023 at 6:24 PM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >>>> Did you get round to fixing this, or did you decide to leave it as is?
> >>> Trying to recall, was there anything to do on the libbpf side, or was
> >>> it purely a compiler-side change?
> >> I'm not 100% sure TBH. I'd like clang to behave consistently for
> >> local_id and target_id. I don't know whether that would break libbpf.
> >>
> > *checks code* libbpf just passes through whatever ID compiler
> > generated, so there doesn't seem to be any change to libbpf. Seems
> > like compiler-only change. cc'ing Eduard as well, if he's curious
> > enough to check
>
> Okay, let us try to have a consistent behavior in local/remote type_id
> by changing local_id semantics to be the same as target_id.
>
> The corresponding llvm change is similar to
>
> [yhs@devbig309.ftw3 ~/work/llvm-project (ed)]$ git diff
> diff --git a/llvm/lib/Target/BPF/BPFPreserveDIType.cpp b/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
> index 78e1bf90f1bd..1fbe1207dc6e 100644
> --- a/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
> +++ b/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
> @@ -86,15 +86,17 @@ static bool BPFPreserveDITypeImpl(Function &F) {
> Reloc = BTF::BTF_TYPE_ID_LOCAL;
> } else {
> Reloc = BTF::BTF_TYPE_ID_REMOTE;
> - DIType *Ty = cast<DIType>(MD);
> - while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
> - unsigned Tag = DTy->getTag();
> - if (Tag != dwarf::DW_TAG_const_type &&
> - Tag != dwarf::DW_TAG_volatile_type)
> - break;
> - Ty = DTy->getBaseType();
> - }
> + }
> + DIType *Ty = cast<DIType>(MD);
> + while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
> + unsigned Tag = DTy->getTag();
> + if (Tag != dwarf::DW_TAG_const_type &&
> + Tag != dwarf::DW_TAG_volatile_type)
> + break;
> + Ty = DTy->getBaseType();
> + }
>
> + if (Reloc == BTF::BTF_TYPE_ID_REMOTE) {
> if (Ty->getName().empty()) {
> if (isa<DISubroutineType>(Ty))
> report_fatal_error(
> @@ -102,8 +104,8 @@ static bool BPFPreserveDITypeImpl(Function &F) {
> else
> report_fatal_error("Empty type name for BTF_TYPE_ID_REMOTE reloc");
> }
> - MD = Ty;
> }
> + MD = Ty;
>
> BasicBlock *BB = Call->getParent();
> IntegerType *VarType = Type::getInt64Ty(BB->getContext());
>
> Either Eduard or Myself will submit a llvm patch to fix this in llvm18.
Sounds good, and thank you!
>
> >
> >
> >> Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local
2023-11-02 2:16 ` Andrii Nakryiko
@ 2023-11-03 19:58 ` Yonghong Song
0 siblings, 0 replies; 12+ messages in thread
From: Yonghong Song @ 2023-11-03 19:58 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Lorenz Bauer, Eduard Zingerman, Yonghong Song, Lorenz Bauer, bpf,
Yonghong Song, Andrii Nakryiko
On 11/1/23 7:16 PM, Andrii Nakryiko wrote:
> On Wed, Nov 1, 2023 at 5:34 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 11/1/23 3:42 PM, Andrii Nakryiko wrote:
>>> On Wed, Nov 1, 2023 at 7:18 AM Lorenz Bauer <lorenz.bauer@isovalent.com> wrote:
>>>> On Tue, Oct 31, 2023 at 6:24 PM Andrii Nakryiko
>>>> <andrii.nakryiko@gmail.com> wrote:
>>>>>> Did you get round to fixing this, or did you decide to leave it as is?
>>>>> Trying to recall, was there anything to do on the libbpf side, or was
>>>>> it purely a compiler-side change?
>>>> I'm not 100% sure TBH. I'd like clang to behave consistently for
>>>> local_id and target_id. I don't know whether that would break libbpf.
>>>>
>>> *checks code* libbpf just passes through whatever ID compiler
>>> generated, so there doesn't seem to be any change to libbpf. Seems
>>> like compiler-only change. cc'ing Eduard as well, if he's curious
>>> enough to check
>> Okay, let us try to have a consistent behavior in local/remote type_id
>> by changing local_id semantics to be the same as target_id.
>>
>> The corresponding llvm change is similar to
>>
>> [yhs@devbig309.ftw3 ~/work/llvm-project (ed)]$ git diff
>> diff --git a/llvm/lib/Target/BPF/BPFPreserveDIType.cpp b/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
>> index 78e1bf90f1bd..1fbe1207dc6e 100644
>> --- a/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
>> +++ b/llvm/lib/Target/BPF/BPFPreserveDIType.cpp
>> @@ -86,15 +86,17 @@ static bool BPFPreserveDITypeImpl(Function &F) {
>> Reloc = BTF::BTF_TYPE_ID_LOCAL;
>> } else {
>> Reloc = BTF::BTF_TYPE_ID_REMOTE;
>> - DIType *Ty = cast<DIType>(MD);
>> - while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
>> - unsigned Tag = DTy->getTag();
>> - if (Tag != dwarf::DW_TAG_const_type &&
>> - Tag != dwarf::DW_TAG_volatile_type)
>> - break;
>> - Ty = DTy->getBaseType();
>> - }
>> + }
>> + DIType *Ty = cast<DIType>(MD);
>> + while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
>> + unsigned Tag = DTy->getTag();
>> + if (Tag != dwarf::DW_TAG_const_type &&
>> + Tag != dwarf::DW_TAG_volatile_type)
>> + break;
>> + Ty = DTy->getBaseType();
>> + }
>>
>> + if (Reloc == BTF::BTF_TYPE_ID_REMOTE) {
>> if (Ty->getName().empty()) {
>> if (isa<DISubroutineType>(Ty))
>> report_fatal_error(
>> @@ -102,8 +104,8 @@ static bool BPFPreserveDITypeImpl(Function &F) {
>> else
>> report_fatal_error("Empty type name for BTF_TYPE_ID_REMOTE reloc");
>> }
>> - MD = Ty;
>> }
>> + MD = Ty;
>>
>> BasicBlock *BB = Call->getParent();
>> IntegerType *VarType = Type::getInt64Ty(BB->getContext());
>>
>> Either Eduard or Myself will submit a llvm patch to fix this in llvm18.
The change is merged into upstream llvm-project trunk ('main' branch):
https://github.com/llvm/llvm-project/commit/32e35b21b5971cc939b1de1194145d9b934fcb54
> Sounds good, and thank you!
>
>>>
>>>> Lorenz
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-11-03 19:58 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-04 13:30 bpf_core_type_id_kernel is not consistent with bpf_core_type_id_local Lorenz Bauer
2023-07-06 4:50 ` Yonghong Song
2023-07-06 21:06 ` Andrii Nakryiko
2023-07-11 16:20 ` Lorenz Bauer
2023-07-11 16:38 ` Andrii Nakryiko
2023-10-31 15:46 ` Lorenz Bauer
2023-10-31 18:24 ` Andrii Nakryiko
2023-11-01 14:17 ` Lorenz Bauer
2023-11-01 22:42 ` Andrii Nakryiko
2023-11-02 0:34 ` Yonghong Song
2023-11-02 2:16 ` Andrii Nakryiko
2023-11-03 19:58 ` Yonghong Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox