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