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

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

[Kui-Feng Lee]

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\
 			\n\
 			", ident);
 	}
-- 
2.34.1

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

[Yonghong Song]

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>

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

[Quentin Monnet]

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!

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

[Andrii Nakryiko]

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
>

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

[Kui-Feng Lee]

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
>>

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

[Andrii Nakryiko]

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
> >>

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

[Kui-Feng Lee]

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
>>>>

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

[Andrii Nakryiko]

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
> >>>>

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

[Andrii Nakryiko]

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
> > >>>>

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html