Re: [PATCH bpf-next] bpftool: cast pointers for shadow types explicitly.

[Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>]

On Wed, Mar 13, 2024 at 8:51 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Mar 12, 2024 at 5:37 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
> >
> >
> >
> > On 3/12/24 17:27, Andrii Nakryiko wrote:
> > > On Tue, Mar 12, 2024 at 5:08 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
> > >>
> > >>
> > >>
> > >> On 3/12/24 15:47, Andrii Nakryiko wrote:
> > >>> On Mon, Mar 11, 2024 at 6:38 PM Kui-Feng Lee <thinker.li@xxxxxxxxx> 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@xxxxxxxx
> > >>>> Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> > >>>> ---
> > >>>>    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
> > >>>>