[cfe-dev] support __attribute__((weak)) for non-primitive types

[Andrii Nakryiko via cfe-dev]

On 8/30/21 9:14 AM, James Y Knight wrote:
> In my opinion, this sounds like a hack to work around a problem that 
> might be better solved outside the compiler. Or, at the very least, I 
> think some more justification as to why this is the best solution 
> would be helpful.
>
> Restating the problem:
> You have a tool which generates a "vmlinux.h" header file based on the 
> currently-running kernel (e.g. by parsing 
> /sys/kernel/btf/vmlinux). This allows users to avoid needing a copy 
> of the kernel header files, which may not be readily available. 
> However, the program cannot extract #defines -- only types -- because 
> defines aren't recorded in the kernel BTF debug info. So, despite 
> having an easy way to generate vmlinux.h, users do still want to 
> include the actual kernel headers sometimes, in order to access these 
> constants. You want to allow that.


Correct.


> Your proposed solution is:
> Modify the compiler so that it can ignore the duplicate struct 
> definitions, in order to allow users to include both the kernel 
> headers and the generated vmlinux.h header, even though they don't 
> cooperate with each-other.
>
> But -- is that really necessary? It seems a bit odd to invent a 
> compiler feature to work around non-cooperating headers. Even more so 
> when all the headers are part of the same project.


We knew about this limitation from day 1 (and couldn't do much about it 
with the language features we have at our disposal), and it is brought 
up pretty regularly by various users, so it's a pretty annoying problem 
which we haven't found *any* way to solve satisfactory, except with 
extending Clang to help with it.


>
> Here's some ideas of possible alternatives. You may well have already 
> thought of these and rejected them, but if so, it would be nice to 
> hear why.
>
> 1) Modify the original headers to have appropriate #ifndefs, in order 
> to avoid conflicting.

Kernel community won't allow adding random #ifndefs into kernel headers 
(neither UAPI nor internal, don't even know which one would get more 
opposition), for the sake of BPF user applications. It's also unclear 
what #ifndefs you mean. Is the proposal to have, for each kernel type, a 
corresponding #define, e.g., #define __task_struct__defined for struct 
task_struct, and add #ifndef/#endif block for each such type? If yes, I 
don't see how that will fly with kernel community (and also consider 
that typical kernel configuration defines >80000 types).


>
> 2) Tell users that they can either include either the autogenerated 
> "vmlinux.h", OR set the pragma and include normal kernel headers. But 
> not both. AFAICT, the vmlinux.h file is not really version-independent 
> -- except that it uses the "preserve_access_index" pragma in order to 
> allow field accesses to be adjusted at load-time to match the actual 
> struct layout. Doesn't putting the same pragma around the normal 
> kernel headers work, too?

That's what we are telling users right now. And that's what users are 
complaining about. vmlinux.h is sufficiently version-independent with 
BPF CO-RE, unless some fields are removed/renamed, or you need the very 
latest new field added in more recent kernel than the one you used to 
generate vmlinux.h.


>
> 3) Automatically extract #defines from kernel headers (e.g. using -dD 
> to print all macro-expansions) to create a new header with those 
> values. (Or defines and inline functions. Or everything but struct 
> definitions. Or something along those lines). Have people include 
> that, instead.
>
> ...Although, IIUC, you want use the kernel-internal headers e.g. 
> "include/linux/socket.h", not the stable "uapi" headers, e.g. 
> "include/uapi/linux/socket.h". But can't those defines change 
> arbitrarily? Doesn't that ruin the version-independence aspect? So, 
> maybe you want:

Yes, users often need to work with types defined in kernel-internal 
headers. You are right that internal #defines are "unstable", so users 
are usually interested in stable #defines in UAPI headers. The problem 
is that vmlinux.h is all or nothing approach, currently. If you use 
vmlinux.h (because you need internal types, not just UAPI ones), you 
can't include UAPI headers (for those #defines) anymore due to type 
conflicts. That's exactly what Yonghong is trying to solve here.

>
> 4) Enhance BPF/CO:RE to support defines as 
> compile-time-unknown/runtime-constants [at least some useful subset of 
> them]. Then they too can be fixed up at BPF load-time, instead of 
> hoping that the values never change.

#defines are not relocatable at runtime by any means, because they are 
just arbitrary text substituted by compiler. Once Clang compiled BPF 
program into object file, there is nothing you can do about #defines 
used. I don't see any way for BPF CO-RE to do "relocatable" #defines.


>
> Though, the example you give is "TCP" (IPPROTO_TCP?) which /is/ in the 
> uapi. Maybe you do need only the stable uapi defines? In which case,
>
> 5) Create a set of modified uapi headers for use with bpf, which omits 
> struct definitions, and instead has #include vmlinux.h at the top 
> instead. Distribute with libbpf-dev, perhaps?

That's not really a solution, rather a maintenance nightmare. It's never 
a good idea to maintain a separate copy of something that's developed by 
thousands of developers, and have hope to keep up with all the changes. 
If that's the only possible solution, then going back to status quo 
(making this user's problem and telling them no to use vmlinux.h at all) 
is a less painful way.


>
>
>
> On Mon, Aug 23, 2021 at 7:17 PM Y Song via cfe-dev 
> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>
>     Hi,
>
>     This is a BPF use case. We would like to check whether
>     __attribute__((weak)) support for non-primitive types would be
>     possible in clang or not. For example, if we have two types like
>        struct t { int a; } __attribute__((weak));
>        struct t { int a; };
>
>        typedef unsigned __u32;
>        typedef unsigned __u32 __attribute__((weak));
>     the compilation should not fail. It should just ignore weak
>     definitions.
>
>     In BPF, to support CO-RE (compile once and run everywhere), we
>     generate a *generic* vmlinux.h
>     (https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html
>     <https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html>)
>     which is included in the bpf program. The bpf loader (libbpf) will
>     adjust field offsets properly based on host record layout so the same
>     program can run on different kernels which may have different record
>     layouts.
>
>     But vmlinux.h does not support macros (and simple static inline helper
>     functions in kernel header files). Currently, users need to define
>     these macros and static inline functions explicitly in their bpf
>     program. What we are thinking is to permit users to include kernel
>     header files containing these macros/static inline functions. But this
>     may introduce duplicated types as these types have been defined in
>     vmlinux.h.
>
>     For example, we may have:
>         socket.h:
>             struct socket { ... };
>             #define TCP    6
>         vmlinux.h:
>             struct socket { ... };
>         bpf.c:
>             #include <socket.h>
>             #include <vmlinux.h>
>             ... TCP ...
>
>     In this case, struct "socket" is defined twice. If we can have
>     something like
>         bpf.c:
>           #pragma clang attribute push (__attribute__((weak)),
>     apply_to = record)
>           #include <socket.h>
>           #pragma clang attribute pop
>           #include <vmlinux.h>
>             ... TCP ...
>
>
>     Then bpf program can get header file macro definitions without copying
>     explicitly.
>
>     We need support for "apply_to = typedef" in the above pragma, so we
>     can deal with typedef types as well.
>
>     Another issue is related to what if two types are not compatible.
>     For example,
>         struct t { int a; } __attribute__((weak));
>         struct t { int a; int b; };
>     I think we could have a flag to control whether we allow incompatible
>     weak type or not.
>
>     It would be good if I can get some suggestions/directions on whether
>     such a feature is possible or not for clang frontend.
>
>     Thanks,
>
>     Yonghong
>     _______________________________________________
>     cfe-dev mailing list
>     cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>     <https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210830/9edc726c/attachment-0001.html>