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

[Kui-Feng Lee <sinquersw@gmail.com>]

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