| Message ID | 20240312013726.1780720-1-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c2a0257c1edf16c6acd2afac7572d7e9043b6577 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpftool: cast pointers for shadow types explicitly. | expand |
On 3/11/24 6:37 PM, Kui-Feng Lee wrote: > According to a report, skeletons fail to assign shadow pointers when being > compiled with C++ programs. Unlike C doing implicit casting for void > pointers, C++ requires an explicit casting. > > To support C++, we do explicit casting for each shadow pointer. > > Cc: yhs@meta.com > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> LGTM. Acked-by: Yonghong Song <yonghong.song@linux.dev>
2024-03-12 05:22 UTC+0000 ~ Yonghong Song <yonghong.song@linux.dev> > > On 3/11/24 6:37 PM, Kui-Feng Lee wrote: >> According to a report, skeletons fail to assign shadow pointers when >> being >> compiled with C++ programs. Unlike C doing implicit casting for void >> pointers, C++ requires an explicit casting. >> >> To support C++, we do explicit casting for each shadow pointer. >> >> Cc: yhs@meta.com >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > > LGTM. > > Acked-by: Yonghong Song <yonghong.song@linux.dev> > Acked-by: Quentin Monnet <quentin@isovalent.com> Thanks!
On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: > > According to a report, skeletons fail to assign shadow pointers when being > compiled with C++ programs. Unlike C doing implicit casting for void > pointers, C++ requires an explicit casting. > > To support C++, we do explicit casting for each shadow pointer. > > Cc: yhs@meta.com > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > --- > tools/bpf/bpftool/gen.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > index 4fa4ade1ce74..dedafea0c127 100644 > --- a/tools/bpf/bpftool/gen.c > +++ b/tools/bpf/bpftool/gen.c > @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) > continue; > codegen("\ > \n\ > - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ Given we have a named struct type for this and we use explicit type names in other parts of generated skeleton code, let's maybe use "struct %s__%s__%s" explicitly here (passing in obj_name, ident, type_name)? No strong preferences, but feels like a consistent approach here would be nice. > \n\ > ", ident); > } > -- > 2.34.1 >
On 3/12/24 15:47, Andrii Nakryiko wrote: > On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: >> >> According to a report, skeletons fail to assign shadow pointers when being >> compiled with C++ programs. Unlike C doing implicit casting for void >> pointers, C++ requires an explicit casting. >> >> To support C++, we do explicit casting for each shadow pointer. >> >> Cc: yhs@meta.com >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >> --- >> tools/bpf/bpftool/gen.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c >> index 4fa4ade1ce74..dedafea0c127 100644 >> --- a/tools/bpf/bpftool/gen.c >> +++ b/tools/bpf/bpftool/gen.c >> @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) >> continue; >> codegen("\ >> \n\ >> - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ >> + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > > Given we have a named struct type for this and we use explicit type > names in other parts of generated skeleton code, let's maybe use > "struct %s__%s__%s" explicitly here (passing in obj_name, ident, > type_name)? I have considered about this solution. But, C++ works differently. It has nested namespaces. That means it should be referred as "XXX_skeleton::OOO_st_ops_map" in C++. Then, we need #if #else #endif directives to provide two separated casting. > > No strong preferences, but feels like a consistent approach here would be nice. > >> \n\ >> ", ident); >> } >> -- >> 2.34.1 >>
On Tue, Mar 12, 2024 at 5:08 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > > > On 3/12/24 15:47, Andrii Nakryiko wrote: > > On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: > >> > >> According to a report, skeletons fail to assign shadow pointers when being > >> compiled with C++ programs. Unlike C doing implicit casting for void > >> pointers, C++ requires an explicit casting. > >> > >> To support C++, we do explicit casting for each shadow pointer. > >> > >> Cc: yhs@meta.com > >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > >> --- > >> tools/bpf/bpftool/gen.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > >> index 4fa4ade1ce74..dedafea0c127 100644 > >> --- a/tools/bpf/bpftool/gen.c > >> +++ b/tools/bpf/bpftool/gen.c > >> @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) > >> continue; > >> codegen("\ > >> \n\ > >> - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > >> + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > > > > Given we have a named struct type for this and we use explicit type > > names in other parts of generated skeleton code, let's maybe use > > "struct %s__%s__%s" explicitly here (passing in obj_name, ident, > > type_name)? > > I have considered about this solution. But, C++ works differently. It > has nested namespaces. That means it should be referred as > "XXX_skeleton::OOO_st_ops_map" in C++. Then, we need #if #else #endif > directives to provide two separated casting. > we cast to (struct <skeleton> *) by name of the skeleton, so it should be fine, I don't see why we'd need to do something C++ specific here > > > > No strong preferences, but feels like a consistent approach here would be nice. > > > >> \n\ > >> ", ident); > >> } > >> -- > >> 2.34.1 > >>
On 3/12/24 17:27, Andrii Nakryiko wrote: > On Tue, Mar 12, 2024 at 5:08 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: >> >> >> >> On 3/12/24 15:47, Andrii Nakryiko wrote: >>> On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: >>>> >>>> According to a report, skeletons fail to assign shadow pointers when being >>>> compiled with C++ programs. Unlike C doing implicit casting for void >>>> pointers, C++ requires an explicit casting. >>>> >>>> To support C++, we do explicit casting for each shadow pointer. >>>> >>>> Cc: yhs@meta.com >>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >>>> --- >>>> tools/bpf/bpftool/gen.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c >>>> index 4fa4ade1ce74..dedafea0c127 100644 >>>> --- a/tools/bpf/bpftool/gen.c >>>> +++ b/tools/bpf/bpftool/gen.c >>>> @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) >>>> continue; >>>> codegen("\ >>>> \n\ >>>> - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ >>>> + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ >>> >>> Given we have a named struct type for this and we use explicit type >>> names in other parts of generated skeleton code, let's maybe use >>> "struct %s__%s__%s" explicitly here (passing in obj_name, ident, >>> type_name)? >> >> I have considered about this solution. But, C++ works differently. It >> has nested namespaces. That means it should be referred as >> "XXX_skeleton::OOO_st_ops_map" in C++. Then, we need #if #else #endif >> directives to provide two separated casting. >> > > we cast to (struct <skeleton> *) by name of the skeleton, so it should > be fine, I don't see why we'd need to do something C++ specific here The skeleton looks like struct struct_ops_module { ...... struct { struct struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed { .... } testmod_zeroed; } struct_ops; }; struct struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed is inside of struct struct_ops_module. In C++, it should be referred as "struct_ops_module::struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed". The other option is moving definitions of these types to the top scope. > >>> >>> No strong preferences, but feels like a consistent approach here would be nice. >>> >>>> \n\ >>>> ", ident); >>>> } >>>> -- >>>> 2.34.1 >>>>
On Tue, Mar 12, 2024 at 5:37 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > > > On 3/12/24 17:27, Andrii Nakryiko wrote: > > On Tue, Mar 12, 2024 at 5:08 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > >> > >> > >> > >> On 3/12/24 15:47, Andrii Nakryiko wrote: > >>> On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: > >>>> > >>>> According to a report, skeletons fail to assign shadow pointers when being > >>>> compiled with C++ programs. Unlike C doing implicit casting for void > >>>> pointers, C++ requires an explicit casting. > >>>> > >>>> To support C++, we do explicit casting for each shadow pointer. > >>>> > >>>> Cc: yhs@meta.com > >>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > >>>> --- > >>>> tools/bpf/bpftool/gen.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > >>>> index 4fa4ade1ce74..dedafea0c127 100644 > >>>> --- a/tools/bpf/bpftool/gen.c > >>>> +++ b/tools/bpf/bpftool/gen.c > >>>> @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) > >>>> continue; > >>>> codegen("\ > >>>> \n\ > >>>> - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > >>>> + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > >>> > >>> Given we have a named struct type for this and we use explicit type > >>> names in other parts of generated skeleton code, let's maybe use > >>> "struct %s__%s__%s" explicitly here (passing in obj_name, ident, > >>> type_name)? > >> > >> I have considered about this solution. But, C++ works differently. It > >> has nested namespaces. That means it should be referred as > >> "XXX_skeleton::OOO_st_ops_map" in C++. Then, we need #if #else #endif > >> directives to provide two separated casting. > >> > > > > we cast to (struct <skeleton> *) by name of the skeleton, so it should > > be fine, I don't see why we'd need to do something C++ specific here > > The skeleton looks like > > struct struct_ops_module { > ...... > struct { > struct > struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed { > .... > } testmod_zeroed; > } struct_ops; > }; > > struct struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed is > inside of struct struct_ops_module. In C++, it should be referred as > "struct_ops_module::struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed". ah, makes sense, thanks for elaborating > > The other option is moving definitions of these types to the top scope. no, it's fine the way you did it in this patch, I'll land it once bpf-next tree is open for new patches, thanks > > > > >>> > >>> No strong preferences, but feels like a consistent approach here would be nice. > >>> > >>>> \n\ > >>>> ", ident); > >>>> } > >>>> -- > >>>> 2.34.1 > >>>>
On Wed, Mar 13, 2024 at 8:51 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Mar 12, 2024 at 5:37 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > > > > > > > On 3/12/24 17:27, Andrii Nakryiko wrote: > > > On Tue, Mar 12, 2024 at 5:08 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > >> > > >> > > >> > > >> On 3/12/24 15:47, Andrii Nakryiko wrote: > > >>> On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: > > >>>> > > >>>> According to a report, skeletons fail to assign shadow pointers when being > > >>>> compiled with C++ programs. Unlike C doing implicit casting for void > > >>>> pointers, C++ requires an explicit casting. > > >>>> > > >>>> To support C++, we do explicit casting for each shadow pointer. > > >>>> > > >>>> Cc: yhs@meta.com > > >>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > > >>>> --- > > >>>> tools/bpf/bpftool/gen.c | 2 +- > > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > > >>>> index 4fa4ade1ce74..dedafea0c127 100644 > > >>>> --- a/tools/bpf/bpftool/gen.c > > >>>> +++ b/tools/bpf/bpftool/gen.c > > >>>> @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) > > >>>> continue; > > >>>> codegen("\ > > >>>> \n\ > > >>>> - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > > >>>> + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ > > >>> > > >>> Given we have a named struct type for this and we use explicit type > > >>> names in other parts of generated skeleton code, let's maybe use > > >>> "struct %s__%s__%s" explicitly here (passing in obj_name, ident, > > >>> type_name)? > > >> > > >> I have considered about this solution. But, C++ works differently. It > > >> has nested namespaces. That means it should be referred as > > >> "XXX_skeleton::OOO_st_ops_map" in C++. Then, we need #if #else #endif > > >> directives to provide two separated casting. > > >> > > > > > > we cast to (struct <skeleton> *) by name of the skeleton, so it should > > > be fine, I don't see why we'd need to do something C++ specific here > > > > The skeleton looks like > > > > struct struct_ops_module { > > ...... > > struct { > > struct > > struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed { > > .... > > } testmod_zeroed; > > } struct_ops; > > }; > > > > struct struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed is > > inside of struct struct_ops_module. In C++, it should be referred as > > "struct_ops_module::struct_ops_module__testmod_zeroed__bpf_testmod_ops___zeroed". > > ah, makes sense, thanks for elaborating > > > > > The other option is moving definitions of these types to the top scope. > > no, it's fine the way you did it in this patch, I'll land it once > bpf-next tree is open for new patches, thanks > so I made the following changes/additions as I was applying your patch: diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c index dedafea0c127..3ce277544c24 100644 --- a/tools/bpf/bpftool/gen.c +++ b/tools/bpf/bpftool/gen.c @@ -1131,7 +1131,8 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) continue; codegen("\ \n\ - obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))\n\ + bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ \n\ ", ident); } diff --git a/tools/testing/selftests/bpf/test_cpp.cpp b/tools/testing/selftests/bpf/test_cpp.cpp index f4936834f76f..dde0bb16e782 100644 --- a/tools/testing/selftests/bpf/test_cpp.cpp +++ b/tools/testing/selftests/bpf/test_cpp.cpp @@ -7,6 +7,7 @@ #include <bpf/bpf.h> #include <bpf/btf.h> #include "test_core_extern.skel.h" +#include "struct_ops_module.skel.h" template <typename T> class Skeleton { @@ -98,6 +99,7 @@ int main(int argc, char *argv[]) { struct btf_dump_opts opts = { }; struct test_core_extern *skel; + struct struct_ops_module *skel2; struct btf *btf; int fd; @@ -118,6 +120,9 @@ int main(int argc, char *argv[]) skel = test_core_extern__open_and_load(); test_core_extern__destroy(skel); + skel2 = struct_ops_module__open_and_load(); + struct_ops_module__destroy(skel2); + fd = bpf_enable_stats(BPF_STATS_RUN_TIME); if (fd < 0) std::cout << "FAILED to enable stats: " << fd << std::endl; test_cpp is a good think to validate that skeletons are compiled with C++ compiler just fine, so I referenced struct_ops-based skeleton there. > > > > > > > >>> > > >>> No strong preferences, but feels like a consistent approach here would be nice. > > >>> > > >>>> \n\ > > >>>> ", ident); > > >>>> } > > >>>> -- > > >>>> 2.34.1 > > >>>>
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Mon, 11 Mar 2024 18:37:26 -0700 you wrote: > According to a report, skeletons fail to assign shadow pointers when being > compiled with C++ programs. Unlike C doing implicit casting for void > pointers, C++ requires an explicit casting. > > To support C++, we do explicit casting for each shadow pointer. > > Cc: yhs@meta.com > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > > [...] Here is the summary with links: - [bpf-next] bpftool: cast pointers for shadow types explicitly. https://git.kernel.org/bpf/bpf-next/c/c2a0257c1edf You are awesome, thank you!
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c index 4fa4ade1ce74..dedafea0c127 100644 --- a/tools/bpf/bpftool/gen.c +++ b/tools/bpf/bpftool/gen.c @@ -1131,7 +1131,7 @@ static void gen_st_ops_shadow_init(struct btf *btf, struct bpf_object *obj) continue; codegen("\ \n\ - obj->struct_ops.%1$s = bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ + obj->struct_ops.%1$s = (typeof(obj->struct_ops.%1$s))bpf_map__initial_value(obj->maps.%1$s, NULL);\n\ \n\ ", ident); }
According to a report, skeletons fail to assign shadow pointers when being compiled with C++ programs. Unlike C doing implicit casting for void pointers, C++ requires an explicit casting. To support C++, we do explicit casting for each shadow pointer. Cc: yhs@meta.com Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> --- tools/bpf/bpftool/gen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)