diff mbox series

[bpf-next] bpf: Make bpf_helper_defs.h c++ friendly

Message ID 20230425000144.3125269-1-sdf@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] bpf: Make bpf_helper_defs.h c++ friendly | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on s390x with gcc

Commit Message

Stanislav Fomichev April 25, 2023, 12:01 a.m. UTC 
From: Peng Wei <pengweiprc@google.com>

Compiling C++ BPF programs with existing bpf_helper_defs.h is not
possible due to stricter C++ type conversions. C++ complains
about (void *) type conversions:

bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
   57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
      |                                                                   ^~~~~~~~~~
      |                                                                   |
      |                                                                   void*

Extend bpf_doc.py to use proper function type instead of void.

Before:
static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;

After:
static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;

Signed-off-by: Peng Wei <pengweiprc@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 scripts/bpf_doc.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Yonghong Song April 25, 2023, 1:56 a.m. UTC | #1 
On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
> From: Peng Wei <pengweiprc@google.com>
> 
> Compiling C++ BPF programs with existing bpf_helper_defs.h is not
> possible due to stricter C++ type conversions. C++ complains
> about (void *) type conversions:
> 
> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
>     57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>        |                                                                   ^~~~~~~~~~
>        |                                                                   |
>        |                                                                   void*
> 
> Extend bpf_doc.py to use proper function type instead of void.

Could you specify what exactly the compilation command triggering the 
above error?

> 
> Before:
> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> 
> After:
> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
> 
> Signed-off-by: Peng Wei <pengweiprc@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>   scripts/bpf_doc.py | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index eaae2ce78381..fa21137a90e7 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -827,6 +827,9 @@ COMMANDS
>                   print(' *{}{}'.format(' \t' if line else '', line))
>   
>           print(' */')
> +        fptr_type = '%s%s(*)(' % (
> +            self.map_type(proto['ret_type']),
> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
>           print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
>                                         proto['ret_star'], proto['name']), end='')
>           comma = ''
> @@ -845,8 +848,10 @@ COMMANDS
>                   one_arg += '{}'.format(n)
>               comma = ', '
>               print(one_arg, end='')
> +            fptr_type += one_arg
>   
> -        print(') = (void *) %d;' % helper.enum_val)
> +        fptr_type += ')'
> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
>           print('')
>   
>   ###############################################################################
Stanislav Fomichev April 25, 2023, 5:04 p.m. UTC | #2 
On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
> > From: Peng Wei <pengweiprc@google.com>
> >
> > Compiling C++ BPF programs with existing bpf_helper_defs.h is not
> > possible due to stricter C++ type conversions. C++ complains
> > about (void *) type conversions:
> >
> > bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
> >     57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >        |                                                                   ^~~~~~~~~~
> >        |                                                                   |
> >        |                                                                   void*
> >
> > Extend bpf_doc.py to use proper function type instead of void.
>
> Could you specify what exactly the compilation command triggering the
> above error?

The following does it for me:
clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h


> >
> > Before:
> > static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >
> > After:
> > static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
> >
> > Signed-off-by: Peng Wei <pengweiprc@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >   scripts/bpf_doc.py | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> > index eaae2ce78381..fa21137a90e7 100755
> > --- a/scripts/bpf_doc.py
> > +++ b/scripts/bpf_doc.py
> > @@ -827,6 +827,9 @@ COMMANDS
> >                   print(' *{}{}'.format(' \t' if line else '', line))
> >
> >           print(' */')
> > +        fptr_type = '%s%s(*)(' % (
> > +            self.map_type(proto['ret_type']),
> > +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
> >           print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
> >                                         proto['ret_star'], proto['name']), end='')
> >           comma = ''
> > @@ -845,8 +848,10 @@ COMMANDS
> >                   one_arg += '{}'.format(n)
> >               comma = ', '
> >               print(one_arg, end='')
> > +            fptr_type += one_arg
> >
> > -        print(') = (void *) %d;' % helper.enum_val)
> > +        fptr_type += ')'
> > +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
> >           print('')
> >
> >   ###############################################################################
Yonghong Song April 25, 2023, 6:10 p.m. UTC | #3 
On 4/25/23 10:04 AM, Stanislav Fomichev wrote:
> On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
>>> From: Peng Wei <pengweiprc@google.com>
>>>
>>> Compiling C++ BPF programs with existing bpf_helper_defs.h is not

Just curious, why you want to compile BPF programs with C++?
The patch looks good to me. But it would be great to know
some reasoning since a lot of stuff, e.g., some CORE related
intrinsics, not available for C++.

>>> possible due to stricter C++ type conversions. C++ complains
>>> about (void *) type conversions:
>>>
>>> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
>>>      57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>>>         |                                                                   ^~~~~~~~~~
>>>         |                                                                   |
>>>         |                                                                   void*
>>>
>>> Extend bpf_doc.py to use proper function type instead of void.
>>
>> Could you specify what exactly the compilation command triggering the
>> above error?
> 
> The following does it for me:
> clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h

Thanks. It would be good if you add the above compilation command
in the commit message.

> 
> 
>>>
>>> Before:
>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>>>
>>> After:
>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
>>>
>>> Signed-off-by: Peng Wei <pengweiprc@google.com>
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>> ---
>>>    scripts/bpf_doc.py | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>>> index eaae2ce78381..fa21137a90e7 100755
>>> --- a/scripts/bpf_doc.py
>>> +++ b/scripts/bpf_doc.py
>>> @@ -827,6 +827,9 @@ COMMANDS
>>>                    print(' *{}{}'.format(' \t' if line else '', line))
>>>
>>>            print(' */')
>>> +        fptr_type = '%s%s(*)(' % (
>>> +            self.map_type(proto['ret_type']),
>>> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
>>>            print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
>>>                                          proto['ret_star'], proto['name']), end='')
>>>            comma = ''
>>> @@ -845,8 +848,10 @@ COMMANDS
>>>                    one_arg += '{}'.format(n)
>>>                comma = ', '
>>>                print(one_arg, end='')
>>> +            fptr_type += one_arg
>>>
>>> -        print(') = (void *) %d;' % helper.enum_val)
>>> +        fptr_type += ')'
>>> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
>>>            print('')
>>>
>>>    ###############################################################################
Stanislav Fomichev April 25, 2023, 6:22 p.m. UTC | #4 
On Tue, Apr 25, 2023 at 11:10 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 4/25/23 10:04 AM, Stanislav Fomichev wrote:
> > On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
> >>
> >>
> >>
> >> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
> >>> From: Peng Wei <pengweiprc@google.com>
> >>>
> >>> Compiling C++ BPF programs with existing bpf_helper_defs.h is not
>
> Just curious, why you want to compile BPF programs with C++?
> The patch looks good to me. But it would be great to know
> some reasoning since a lot of stuff, e.g., some CORE related
> intrinsics, not available for C++.

Can you share more? What's not available? Any pointers to the docs maybe?

People here want to try to use c++ to see if templating helps with v4
vs v6 handling.
We have a bunch of copy-paste around this place and would like to see
whether c++ could make it a bit more readable.

> >>> possible due to stricter C++ type conversions. C++ complains
> >>> about (void *) type conversions:
> >>>
> >>> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
> >>>      57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >>>         |                                                                   ^~~~~~~~~~
> >>>         |                                                                   |
> >>>         |                                                                   void*
> >>>
> >>> Extend bpf_doc.py to use proper function type instead of void.
> >>
> >> Could you specify what exactly the compilation command triggering the
> >> above error?
> >
> > The following does it for me:
> > clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h
>
> Thanks. It would be good if you add the above compilation command
> in the commit message.

Sure, will add.

> >
> >
> >>>
> >>> Before:
> >>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >>>
> >>> After:
> >>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
> >>>
> >>> Signed-off-by: Peng Wei <pengweiprc@google.com>
> >>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>> ---
> >>>    scripts/bpf_doc.py | 7 ++++++-
> >>>    1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> >>> index eaae2ce78381..fa21137a90e7 100755
> >>> --- a/scripts/bpf_doc.py
> >>> +++ b/scripts/bpf_doc.py
> >>> @@ -827,6 +827,9 @@ COMMANDS
> >>>                    print(' *{}{}'.format(' \t' if line else '', line))
> >>>
> >>>            print(' */')
> >>> +        fptr_type = '%s%s(*)(' % (
> >>> +            self.map_type(proto['ret_type']),
> >>> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
> >>>            print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
> >>>                                          proto['ret_star'], proto['name']), end='')
> >>>            comma = ''
> >>> @@ -845,8 +848,10 @@ COMMANDS
> >>>                    one_arg += '{}'.format(n)
> >>>                comma = ', '
> >>>                print(one_arg, end='')
> >>> +            fptr_type += one_arg
> >>>
> >>> -        print(') = (void *) %d;' % helper.enum_val)
> >>> +        fptr_type += ')'
> >>> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
> >>>            print('')
> >>>
> >>>    ###############################################################################
Yonghong Song April 25, 2023, 6:29 p.m. UTC | #5 
On 4/25/23 11:22 AM, Stanislav Fomichev wrote:
> On Tue, Apr 25, 2023 at 11:10 AM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 4/25/23 10:04 AM, Stanislav Fomichev wrote:
>>> On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
>>>>> From: Peng Wei <pengweiprc@google.com>
>>>>>
>>>>> Compiling C++ BPF programs with existing bpf_helper_defs.h is not
>>
>> Just curious, why you want to compile BPF programs with C++?
>> The patch looks good to me. But it would be great to know
>> some reasoning since a lot of stuff, e.g., some CORE related
>> intrinsics, not available for C++.
> 
> Can you share more? What's not available? Any pointers to the docs maybe?

Sorry, it is an attribute, instead of instrinsics.

The attribute preserve_access_index/btf_type_tag/btf_decl_tag are all C 
only.

In llvm-project/clang/include/clang/Basic/Attr.td:

def BPFPreserveAccessIndex : InheritableAttr,
                              TargetSpecificAttr<TargetBPF>  {
   let Spellings = [Clang<"preserve_access_index">];
   let Subjects = SubjectList<[Record], ErrorDiag>;
   let Documentation = [BPFPreserveAccessIndexDocs];
   let LangOpts = [COnly];
}

def BTFDeclTag : InheritableAttr {
   let Spellings = [Clang<"btf_decl_tag">];
   let Args = [StringArgument<"BTFDeclTag">];
   let Subjects = SubjectList<[Var, Function, Record, Field, TypedefName],
                              ErrorDiag>;
   let Documentation = [BTFDeclTagDocs];
   let LangOpts = [COnly];
}

def BTFTypeTag : TypeAttr {
   let Spellings = [Clang<"btf_type_tag">];
   let Args = [StringArgument<"BTFTypeTag">];
   let Documentation = [BTFTypeTagDocs];
   let LangOpts = [COnly];
}



> 
> People here want to try to use c++ to see if templating helps with v4
> vs v6 handling.
> We have a bunch of copy-paste around this place and would like to see
> whether c++ could make it a bit more readable.
> 
>>>>> possible due to stricter C++ type conversions. C++ complains
>>>>> about (void *) type conversions:
>>>>>
>>>>> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
>>>>>       57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>>>>>          |                                                                   ^~~~~~~~~~
>>>>>          |                                                                   |
>>>>>          |                                                                   void*
>>>>>
>>>>> Extend bpf_doc.py to use proper function type instead of void.
>>>>
>>>> Could you specify what exactly the compilation command triggering the
>>>> above error?
>>>
>>> The following does it for me:
>>> clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h
>>
>> Thanks. It would be good if you add the above compilation command
>> in the commit message.
> 
> Sure, will add.
> 
>>>
>>>
>>>>>
>>>>> Before:
>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>>>>>
>>>>> After:
>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
>>>>>
>>>>> Signed-off-by: Peng Wei <pengweiprc@google.com>
>>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>>>> ---
>>>>>     scripts/bpf_doc.py | 7 ++++++-
>>>>>     1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>>>>> index eaae2ce78381..fa21137a90e7 100755
>>>>> --- a/scripts/bpf_doc.py
>>>>> +++ b/scripts/bpf_doc.py
>>>>> @@ -827,6 +827,9 @@ COMMANDS
>>>>>                     print(' *{}{}'.format(' \t' if line else '', line))
>>>>>
>>>>>             print(' */')
>>>>> +        fptr_type = '%s%s(*)(' % (
>>>>> +            self.map_type(proto['ret_type']),
>>>>> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
>>>>>             print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
>>>>>                                           proto['ret_star'], proto['name']), end='')
>>>>>             comma = ''
>>>>> @@ -845,8 +848,10 @@ COMMANDS
>>>>>                     one_arg += '{}'.format(n)
>>>>>                 comma = ', '
>>>>>                 print(one_arg, end='')
>>>>> +            fptr_type += one_arg
>>>>>
>>>>> -        print(') = (void *) %d;' % helper.enum_val)
>>>>> +        fptr_type += ')'
>>>>> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
>>>>>             print('')
>>>>>
>>>>>     ###############################################################################
Stanislav Fomichev April 25, 2023, 6:35 p.m. UTC | #6 
On Tue, Apr 25, 2023 at 11:29 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 4/25/23 11:22 AM, Stanislav Fomichev wrote:
> > On Tue, Apr 25, 2023 at 11:10 AM Yonghong Song <yhs@meta.com> wrote:
> >>
> >>
> >>
> >> On 4/25/23 10:04 AM, Stanislav Fomichev wrote:
> >>> On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
> >>>>> From: Peng Wei <pengweiprc@google.com>
> >>>>>
> >>>>> Compiling C++ BPF programs with existing bpf_helper_defs.h is not
> >>
> >> Just curious, why you want to compile BPF programs with C++?
> >> The patch looks good to me. But it would be great to know
> >> some reasoning since a lot of stuff, e.g., some CORE related
> >> intrinsics, not available for C++.
> >
> > Can you share more? What's not available? Any pointers to the docs maybe?
>
> Sorry, it is an attribute, instead of instrinsics.
>
> The attribute preserve_access_index/btf_type_tag/btf_decl_tag are all C
> only.

Interesting, thanks! I don't think we use btf_type_tag/btf_decl_tag in
the program we want to try c++, but losing preserve_access_index might
be unfortunate :-( But we'll see..
Btw, any reason these are explicitly opted out from c++? Doesn't seem
like there is anything c-specific in them?
The c++ we are talking about here is mostly "c with classes +
templates"; no polymorphism / inheritance.

> In llvm-project/clang/include/clang/Basic/Attr.td:
>
> def BPFPreserveAccessIndex : InheritableAttr,
>                               TargetSpecificAttr<TargetBPF>  {
>    let Spellings = [Clang<"preserve_access_index">];
>    let Subjects = SubjectList<[Record], ErrorDiag>;
>    let Documentation = [BPFPreserveAccessIndexDocs];
>    let LangOpts = [COnly];
> }
>
> def BTFDeclTag : InheritableAttr {
>    let Spellings = [Clang<"btf_decl_tag">];
>    let Args = [StringArgument<"BTFDeclTag">];
>    let Subjects = SubjectList<[Var, Function, Record, Field, TypedefName],
>                               ErrorDiag>;
>    let Documentation = [BTFDeclTagDocs];
>    let LangOpts = [COnly];
> }
>
> def BTFTypeTag : TypeAttr {
>    let Spellings = [Clang<"btf_type_tag">];
>    let Args = [StringArgument<"BTFTypeTag">];
>    let Documentation = [BTFTypeTagDocs];
>    let LangOpts = [COnly];
> }
>
>
>
> >
> > People here want to try to use c++ to see if templating helps with v4
> > vs v6 handling.
> > We have a bunch of copy-paste around this place and would like to see
> > whether c++ could make it a bit more readable.
> >
> >>>>> possible due to stricter C++ type conversions. C++ complains
> >>>>> about (void *) type conversions:
> >>>>>
> >>>>> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
> >>>>>       57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >>>>>          |                                                                   ^~~~~~~~~~
> >>>>>          |                                                                   |
> >>>>>          |                                                                   void*
> >>>>>
> >>>>> Extend bpf_doc.py to use proper function type instead of void.
> >>>>
> >>>> Could you specify what exactly the compilation command triggering the
> >>>> above error?
> >>>
> >>> The following does it for me:
> >>> clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h
> >>
> >> Thanks. It would be good if you add the above compilation command
> >> in the commit message.
> >
> > Sure, will add.
> >
> >>>
> >>>
> >>>>>
> >>>>> Before:
> >>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >>>>>
> >>>>> After:
> >>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
> >>>>>
> >>>>> Signed-off-by: Peng Wei <pengweiprc@google.com>
> >>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>>>> ---
> >>>>>     scripts/bpf_doc.py | 7 ++++++-
> >>>>>     1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> >>>>> index eaae2ce78381..fa21137a90e7 100755
> >>>>> --- a/scripts/bpf_doc.py
> >>>>> +++ b/scripts/bpf_doc.py
> >>>>> @@ -827,6 +827,9 @@ COMMANDS
> >>>>>                     print(' *{}{}'.format(' \t' if line else '', line))
> >>>>>
> >>>>>             print(' */')
> >>>>> +        fptr_type = '%s%s(*)(' % (
> >>>>> +            self.map_type(proto['ret_type']),
> >>>>> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
> >>>>>             print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
> >>>>>                                           proto['ret_star'], proto['name']), end='')
> >>>>>             comma = ''
> >>>>> @@ -845,8 +848,10 @@ COMMANDS
> >>>>>                     one_arg += '{}'.format(n)
> >>>>>                 comma = ', '
> >>>>>                 print(one_arg, end='')
> >>>>> +            fptr_type += one_arg
> >>>>>
> >>>>> -        print(') = (void *) %d;' % helper.enum_val)
> >>>>> +        fptr_type += ')'
> >>>>> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
> >>>>>             print('')
> >>>>>
> >>>>>     ###############################################################################
Yonghong Song April 25, 2023, 7:43 p.m. UTC | #7 
On 4/25/23 11:35 AM, Stanislav Fomichev wrote:
> On Tue, Apr 25, 2023 at 11:29 AM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 4/25/23 11:22 AM, Stanislav Fomichev wrote:
>>> On Tue, Apr 25, 2023 at 11:10 AM Yonghong Song <yhs@meta.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/25/23 10:04 AM, Stanislav Fomichev wrote:
>>>>> On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
>>>>>>> From: Peng Wei <pengweiprc@google.com>
>>>>>>>
>>>>>>> Compiling C++ BPF programs with existing bpf_helper_defs.h is not
>>>>
>>>> Just curious, why you want to compile BPF programs with C++?
>>>> The patch looks good to me. But it would be great to know
>>>> some reasoning since a lot of stuff, e.g., some CORE related
>>>> intrinsics, not available for C++.
>>>
>>> Can you share more? What's not available? Any pointers to the docs maybe?
>>
>> Sorry, it is an attribute, instead of instrinsics.
>>
>> The attribute preserve_access_index/btf_type_tag/btf_decl_tag are all C
>> only.
> 
> Interesting, thanks! I don't think we use btf_type_tag/btf_decl_tag in
> the program we want to try c++, but losing preserve_access_index might
> be unfortunate :-( But we'll see..
> Btw, any reason these are explicitly opted out from c++? Doesn't seem
> like there is anything c-specific in them?

Initial use case is C only. If we say to support C++, we will
need to add attribute processing codes in various other places
(member functions, templates, other c++ constructs, etc.)
to convert these attributes to proper debuginfo. There are no use
cases for this, so we didn't do it in the first place.

> The c++ we are talking about here is mostly "c with classes +
> templates"; no polymorphism / inheritance.
> 
>> In llvm-project/clang/include/clang/Basic/Attr.td:
>>
>> def BPFPreserveAccessIndex : InheritableAttr,
>>                                TargetSpecificAttr<TargetBPF>  {
>>     let Spellings = [Clang<"preserve_access_index">];
>>     let Subjects = SubjectList<[Record], ErrorDiag>;
>>     let Documentation = [BPFPreserveAccessIndexDocs];
>>     let LangOpts = [COnly];
>> }
>>
>> def BTFDeclTag : InheritableAttr {
>>     let Spellings = [Clang<"btf_decl_tag">];
>>     let Args = [StringArgument<"BTFDeclTag">];
>>     let Subjects = SubjectList<[Var, Function, Record, Field, TypedefName],
>>                                ErrorDiag>;
>>     let Documentation = [BTFDeclTagDocs];
>>     let LangOpts = [COnly];
>> }
>>
>> def BTFTypeTag : TypeAttr {
>>     let Spellings = [Clang<"btf_type_tag">];
>>     let Args = [StringArgument<"BTFTypeTag">];
>>     let Documentation = [BTFTypeTagDocs];
>>     let LangOpts = [COnly];
>> }
>>
>>
>>
>>>
>>> People here want to try to use c++ to see if templating helps with v4
>>> vs v6 handling.
>>> We have a bunch of copy-paste around this place and would like to see
>>> whether c++ could make it a bit more readable.
>>>
>>>>>>> possible due to stricter C++ type conversions. C++ complains
>>>>>>> about (void *) type conversions:
>>>>>>>
>>>>>>> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
>>>>>>>        57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>>>>>>>           |                                                                   ^~~~~~~~~~
>>>>>>>           |                                                                   |
>>>>>>>           |                                                                   void*
>>>>>>>
>>>>>>> Extend bpf_doc.py to use proper function type instead of void.
>>>>>>
>>>>>> Could you specify what exactly the compilation command triggering the
>>>>>> above error?
>>>>>
>>>>> The following does it for me:
>>>>> clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h
>>>>
>>>> Thanks. It would be good if you add the above compilation command
>>>> in the commit message.
>>>
>>> Sure, will add.
>>>
>>>>>
>>>>>
>>>>>>>
>>>>>>> Before:
>>>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>>>>>>>
>>>>>>> After:
>>>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
>>>>>>>
>>>>>>> Signed-off-by: Peng Wei <pengweiprc@google.com>
>>>>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>>>>>> ---
>>>>>>>      scripts/bpf_doc.py | 7 ++++++-
>>>>>>>      1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>>>>>>> index eaae2ce78381..fa21137a90e7 100755
>>>>>>> --- a/scripts/bpf_doc.py
>>>>>>> +++ b/scripts/bpf_doc.py
>>>>>>> @@ -827,6 +827,9 @@ COMMANDS
>>>>>>>                      print(' *{}{}'.format(' \t' if line else '', line))
>>>>>>>
>>>>>>>              print(' */')
>>>>>>> +        fptr_type = '%s%s(*)(' % (
>>>>>>> +            self.map_type(proto['ret_type']),
>>>>>>> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
>>>>>>>              print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
>>>>>>>                                            proto['ret_star'], proto['name']), end='')
>>>>>>>              comma = ''
>>>>>>> @@ -845,8 +848,10 @@ COMMANDS
>>>>>>>                      one_arg += '{}'.format(n)
>>>>>>>                  comma = ', '
>>>>>>>                  print(one_arg, end='')
>>>>>>> +            fptr_type += one_arg
>>>>>>>
>>>>>>> -        print(') = (void *) %d;' % helper.enum_val)
>>>>>>> +        fptr_type += ')'
>>>>>>> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
>>>>>>>              print('')
>>>>>>>
>>>>>>>      ###############################################################################
Stanislav Fomichev April 26, 2023, 3:52 p.m. UTC | #8 
On Tue, Apr 25, 2023 at 12:43 PM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 4/25/23 11:35 AM, Stanislav Fomichev wrote:
> > On Tue, Apr 25, 2023 at 11:29 AM Yonghong Song <yhs@meta.com> wrote:
> >>
> >>
> >>
> >> On 4/25/23 11:22 AM, Stanislav Fomichev wrote:
> >>> On Tue, Apr 25, 2023 at 11:10 AM Yonghong Song <yhs@meta.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/25/23 10:04 AM, Stanislav Fomichev wrote:
> >>>>> On Mon, Apr 24, 2023 at 6:56 PM Yonghong Song <yhs@meta.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 4/24/23 5:01 PM, Stanislav Fomichev wrote:
> >>>>>>> From: Peng Wei <pengweiprc@google.com>
> >>>>>>>
> >>>>>>> Compiling C++ BPF programs with existing bpf_helper_defs.h is not
> >>>>
> >>>> Just curious, why you want to compile BPF programs with C++?
> >>>> The patch looks good to me. But it would be great to know
> >>>> some reasoning since a lot of stuff, e.g., some CORE related
> >>>> intrinsics, not available for C++.
> >>>
> >>> Can you share more? What's not available? Any pointers to the docs maybe?
> >>
> >> Sorry, it is an attribute, instead of instrinsics.
> >>
> >> The attribute preserve_access_index/btf_type_tag/btf_decl_tag are all C
> >> only.
> >
> > Interesting, thanks! I don't think we use btf_type_tag/btf_decl_tag in
> > the program we want to try c++, but losing preserve_access_index might
> > be unfortunate :-( But we'll see..
> > Btw, any reason these are explicitly opted out from c++? Doesn't seem
> > like there is anything c-specific in them?
>
> Initial use case is C only. If we say to support C++, we will
> need to add attribute processing codes in various other places
> (member functions, templates, other c++ constructs, etc.)
> to convert these attributes to proper debuginfo. There are no use
> cases for this, so we didn't do it in the first place.

I see. In this case, let me respin the patch as is with the clang++
command to reproduce.
We'll experiment with our program internally to see whether these are
the showstoppers.

> > The c++ we are talking about here is mostly "c with classes +
> > templates"; no polymorphism / inheritance.
> >
> >> In llvm-project/clang/include/clang/Basic/Attr.td:
> >>
> >> def BPFPreserveAccessIndex : InheritableAttr,
> >>                                TargetSpecificAttr<TargetBPF>  {
> >>     let Spellings = [Clang<"preserve_access_index">];
> >>     let Subjects = SubjectList<[Record], ErrorDiag>;
> >>     let Documentation = [BPFPreserveAccessIndexDocs];
> >>     let LangOpts = [COnly];
> >> }
> >>
> >> def BTFDeclTag : InheritableAttr {
> >>     let Spellings = [Clang<"btf_decl_tag">];
> >>     let Args = [StringArgument<"BTFDeclTag">];
> >>     let Subjects = SubjectList<[Var, Function, Record, Field, TypedefName],
> >>                                ErrorDiag>;
> >>     let Documentation = [BTFDeclTagDocs];
> >>     let LangOpts = [COnly];
> >> }
> >>
> >> def BTFTypeTag : TypeAttr {
> >>     let Spellings = [Clang<"btf_type_tag">];
> >>     let Args = [StringArgument<"BTFTypeTag">];
> >>     let Documentation = [BTFTypeTagDocs];
> >>     let LangOpts = [COnly];
> >> }
> >>
> >>
> >>
> >>>
> >>> People here want to try to use c++ to see if templating helps with v4
> >>> vs v6 handling.
> >>> We have a bunch of copy-paste around this place and would like to see
> >>> whether c++ could make it a bit more readable.
> >>>
> >>>>>>> possible due to stricter C++ type conversions. C++ complains
> >>>>>>> about (void *) type conversions:
> >>>>>>>
> >>>>>>> bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive]
> >>>>>>>        57 | static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >>>>>>>           |                                                                   ^~~~~~~~~~
> >>>>>>>           |                                                                   |
> >>>>>>>           |                                                                   void*
> >>>>>>>
> >>>>>>> Extend bpf_doc.py to use proper function type instead of void.
> >>>>>>
> >>>>>> Could you specify what exactly the compilation command triggering the
> >>>>>> above error?
> >>>>>
> >>>>> The following does it for me:
> >>>>> clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h
> >>>>
> >>>> Thanks. It would be good if you add the above compilation command
> >>>> in the commit message.
> >>>
> >>> Sure, will add.
> >>>
> >>>>>
> >>>>>
> >>>>>>>
> >>>>>>> Before:
> >>>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
> >>>>>>>
> >>>>>>> After:
> >>>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *(*)(void *map, const void *key)) 1;
> >>>>>>>
> >>>>>>> Signed-off-by: Peng Wei <pengweiprc@google.com>
> >>>>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>>>>>> ---
> >>>>>>>      scripts/bpf_doc.py | 7 ++++++-
> >>>>>>>      1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> >>>>>>> index eaae2ce78381..fa21137a90e7 100755
> >>>>>>> --- a/scripts/bpf_doc.py
> >>>>>>> +++ b/scripts/bpf_doc.py
> >>>>>>> @@ -827,6 +827,9 @@ COMMANDS
> >>>>>>>                      print(' *{}{}'.format(' \t' if line else '', line))
> >>>>>>>
> >>>>>>>              print(' */')
> >>>>>>> +        fptr_type = '%s%s(*)(' % (
> >>>>>>> +            self.map_type(proto['ret_type']),
> >>>>>>> +            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
> >>>>>>>              print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
> >>>>>>>                                            proto['ret_star'], proto['name']), end='')
> >>>>>>>              comma = ''
> >>>>>>> @@ -845,8 +848,10 @@ COMMANDS
> >>>>>>>                      one_arg += '{}'.format(n)
> >>>>>>>                  comma = ', '
> >>>>>>>                  print(one_arg, end='')
> >>>>>>> +            fptr_type += one_arg
> >>>>>>>
> >>>>>>> -        print(') = (void *) %d;' % helper.enum_val)
> >>>>>>> +        fptr_type += ')'
> >>>>>>> +        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
> >>>>>>>              print('')
> >>>>>>>
> >>>>>>>      ###############################################################################
diff mbox series

Patch

diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index eaae2ce78381..fa21137a90e7 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -827,6 +827,9 @@  COMMANDS
                 print(' *{}{}'.format(' \t' if line else '', line))
 
         print(' */')
+        fptr_type = '%s%s(*)(' % (
+            self.map_type(proto['ret_type']),
+            ((' ' + proto['ret_star']) if proto['ret_star'] else ''))
         print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
                                       proto['ret_star'], proto['name']), end='')
         comma = ''
@@ -845,8 +848,10 @@  COMMANDS
                 one_arg += '{}'.format(n)
             comma = ', '
             print(one_arg, end='')
+            fptr_type += one_arg
 
-        print(') = (void *) %d;' % helper.enum_val)
+        fptr_type += ')'
+        print(') = (%s) %d;' % (fptr_type, helper.enum_val))
         print('')
 
 ###############################################################################