| Message ID | 20230426155357.4158846-1-sdf@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpf: Make bpf_helper_defs.h c++ friendly | expand |
On 4/26/23 8:53 AM, 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: > > $ clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h > > 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; > > v2: > - add clang++ invocation example (Yonghong) > > Cc: Yonghong Song <yhs@meta.com> > Signed-off-by: Peng Wei <pengweiprc@google.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> Acked-by: Yonghong Song <yhs@fb.com>
On Wed, Apr 26, 2023 at 8:54 AM Stanislav Fomichev <sdf@google.com> 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: > > $ clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h > > bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive] Can you use -fpermissive instead? As Yonghong said, C++ is not really supported, so pretending we do will just cause more confusion and issues down the line. BTW, can you elaborate more on v4 vs v6 code reuse (or what was it)? I wonder if there is something that would stay within C domain that could be done? > 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; > > v2: > - add clang++ invocation example (Yonghong) > > Cc: Yonghong Song <yhs@meta.com> > 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('') > > ############################################################################### > -- > 2.40.1.495.gc816e09b53d-goog >
On Wed, Apr 26, 2023 at 6:25 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Apr 26, 2023 at 8:54 AM Stanislav Fomichev <sdf@google.com> 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: > > > > $ clang++ --include linux/types.h ./tools/lib/bpf/bpf_helper_defs.h > > > > bpf_helper_defs.h:57:67: error: invalid conversion from ‘void*’ to ‘void* (*)(void*, const void*)’ [-fpermissive] > > Can you use -fpermissive instead? As Yonghong said, C++ is not really > supported, so pretending we do will just cause more confusion and > issues down the line. I get the same errors with -fpermissive :-( RE unsupported C++: we are not really subscribing to support it here, right? Just making it easier for the folks who want to experiment with it to try it out. Maybe I should stick something like this into the header? #ifdef __cplusplus #warning "C++ is not supported, use at your own risk!" #endif But we should be able to carry this patch internally if you see no value. I wanted to share it in case somebody else is gonna try the same eventually.. > BTW, can you elaborate more on v4 vs v6 code reuse (or what was it)? I > wonder if there is something that would stay within C domain that > could be done? We have a program which handles both v4 and v6 packets. The logic is mostly the same for v4 and v6: parse out the addresses, ports, lookup some maps and do some action on match. Right now, both cases are handled explicitly with a bunch of copy-paste. The idea of this c++ experiment is to leverage templates to write this logic once and parametrize on the address sizes, packet field offsets, etc. I'm pretty certain we can do similar things in C, but it feels like it will be less expressive and a bit more ugly (assuming we'd use a preprocessor to do it). > > 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; > > > > v2: > > - add clang++ invocation example (Yonghong) > > > > Cc: Yonghong Song <yhs@meta.com> > > 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('') > > > > ############################################################################### > > -- > > 2.40.1.495.gc816e09b53d-goog > >
On Thu, Apr 27, 2023 at 9:59 AM Stanislav Fomichev <sdf@google.com> wrote: > > RE unsupported C++: we are not really subscribing to support it here, right? > Just making it easier for the folks who want to experiment with it to > try it out. I think short term experiments should be out of tree. The experiments that lead to a long term goal can certainly be in tree. In that sense if your team is really going to invest into making c++ a supported front-end for bpf than this is a good first step. Such c++ flavor will have restrictions (like no virtual calls and exceptions) and it's ok. The basic things like CO-RE has to work though. In other words if there is a draft patch for llvm to enable existing attrs for c++ then this patch would be fine. This patch alone with no effort made on llvm side should stay out of tree.
On Thu, Apr 27, 2023 at 1:15 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Apr 27, 2023 at 9:59 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > RE unsupported C++: we are not really subscribing to support it here, right? > > Just making it easier for the folks who want to experiment with it to > > try it out. > > I think short term experiments should be out of tree. > The experiments that lead to a long term goal can certainly be in tree. > In that sense if your team is really going to invest into making > c++ a supported front-end for bpf than this is a good first step. > Such c++ flavor will have restrictions (like no virtual calls and > exceptions) and it's ok. The basic things like CO-RE has to work though. > In other words if there is a draft patch for llvm to enable > existing attrs for c++ then this patch would be fine. > This patch alone with no effort made on llvm side should stay out of tree. SG, thanks! I'll relay the message internally.
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('') ###############################################################################