On Tue, 2022-11-01 at 11:35 -0700, Alexei Starovoitov wrote: > On Tue, Nov 1, 2022 at 9:02 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote: > > > > > > Yes, we discussed this before. This will need to add additional work > > > > in preprocessor. I just made a discussion topic in llvm discourse > > > > > > > > https://discourse.llvm.org/t/add-a-type-checking-macro-is-type-defined-type/66268 > > That would be a great clang feature. > Thanks Yonghong! > > > > > > > > > Let us see whether we can get some upstream agreement or not. > > > > > > > > > > Thanks for starting the conversation! I'll be following along. > > > > > > > > > I think this sort of approach assumes that vmlinux.h is included after > > any uapi headers, and would guard type definitions with > > > > #if type_is_defined(foo) > > struct foo { > > > > }; > > #endif > > > > ...is that right? My concern is that the vmlinux.h definitions have > > the CO-RE attributes. From a BPF perspective, would we like the vmlinux.h > > definitions to dominate over UAPI definitions do you think, or does it > > matter? > > I think it's totally fine to require #include "vmlinux.h" to be last. > The attr(preserve_access_index) is only useful for kernel internal > structs. uapi structs don't need it. > > > > > I was wondering if there might be yet another way to crack this; > > if we did want the vmlinux.h type definitions to be authoritative > > because they have the preserve access index attribute, and because > > bpftool knows all vmlinux types, it could use that info to selectively > > redefine those type names such that we avoid name clashes when later > > including UAPI headers. Something like > > > > #ifdef __VMLINUX_H__ > > //usual vmlinux.h type definitions > > #endif /* __VMLINUX_H__ */ > > > > #ifdef __VMLINUX_ALIAS__ > > if !defined(timespec64) > > #define timespec64 __VMLINUX_ALIAS__timespec64 > > #endif > > // rest of the types define aliases here > > #undef __VMLINUX_ALIAS__ > > #else /* unalias */ > > #if defined(timespec64) > > #undef timespec64 > > #endif > > // rest of types undef aliases here > > #endif /* __VMLINUX_ALIAS__ */ > > > > > > Then the consumer does this: > > > > #define __VMLINUX_ALIAS__ > > #include "vmlinux.h" > > // include uapi headers > > #include "vmlinux.h" > > > > (the latter include of vmlinux.h is needed to undef all the type aliases) > > Sounds like a bunch of complexity for the use case that is not > clear to me. Well, my RFC is not shy of complexity :) What Alan suggests should solve the confilicts described in [1] or any other confilicts of such kind. [1] https://lore.kernel.org/bpf/999da51bdf050f155ba299500061b3eb6e0dcd0d.camel@xxxxxxxxx/ > > > > I tried hacking up bpftool to support this aliasing scheme and while > > it is kind of hacky it does seem to work, aside from some issues with > > IPPROTO_* definitions - for the enumerated IPPROTO_ values linux/in.h does > > this: > > > > enum { > > IPPROTO_IP = 0, /* Dummy protocol for TCP */ > > #define IPPROTO_IP IPPROTO_IP > > IPPROTO_ICMP = 1, /* Internet Control Message Protocol */ > > #define IPPROTO_ICMP IPPROTO_ICMP > > > > > > ...so our enum value definitions for IPPROTO_ values clash with the above > > definitions. These could be individually ifdef-guarded if needed though I think. > > Including vmlinux.h last won't have this enum conflicts, right? > > > I can send the proof-of-concept patch if it would help, I just wanted to > > check in case that might be a workable path too, since it just requires > > changes to bpftool (and changes to in.h). > > I think changing the uapi header like in.h is no-go. > Touching anything in uapi is too much of a risk.
