[cfe-dev] BPF: adding new clang extension bpf_dominating_decl attribute

[Y Song via cfe-dev]

On Thu, Jan 27, 2022 at 12:33 PM Y Song <ys114321 at gmail.com> wrote:
>
> On Thu, Jan 6, 2022 at 4:47 AM Aaron Ballman <aaron at aaronballman.com> wrote:
> >
> > On Wed, Jan 5, 2022 at 3:31 PM Y Song <ys114321 at gmail.com> wrote:
> > >
> > > On Mon, Jan 3, 2022 at 12:52 PM Aaron Ballman <aaron at aaronballman.com> wrote:
> > > >
> > > > On Mon, Dec 20, 2021 at 7:06 PM Y Song <ys114321 at gmail.com> wrote:
> > > > >
> > > > > This is a request to add a clang extention, more specificly,
> > > > > a clang attribute named bpf_dominating_decl. This clang
> > > > > extention is intended to be used for bpf target only. Below
> > > > > I will explain in detail about this proposed attribute, why
> > > > > bpf community needs this, how it will be used and other
> > > > > aspects as described in https://clang.llvm.org/get_involved.html.
> > > >
> > > > Thank you for this RFC!
> > >
> > > You are welcome!
> > >
> > > >
> > > > > Evidence of a significant user community
> > > > > ========================================
> > > > >
> > > > > We are proposing a new clang attribute bpf_dominating_decl which
> > > > > was implemented in [1]. The feature has also been discussed in
> > > > > cfe-dev mailing list ([2]). It intended to solve the
> > > > > following use case:
> > > > >   - A tool generated vmlinux.h is used for CO-RE (compile once,
> > > > >     run everywhere) use cases.
> > > > >   - vmlinux.h contains all kernel data structures for a particular config,
> > > > >     see [3] and [4] about how it is generated and why it is important.
> > > > >   - but vmlinux.h may have type conflicts with other headers
> > > > >     user intends to use.
> > > > >
> > > > > Macros are such an example. Currently CO-RE relocation cannot
> > > > > handle macros and macros may be defined in some header files accessible
> > > > > to the user. If those header files have type conflict with vmlinux.h,
> > > > > users are forced to copy macro definitions. The same for some simple
> > > > > static inline functions defined in header files. This issue has been
> > > > > discussed before and that is why we proposed this issue. And just last
> > > > > week, it is discussed/complained again ([5]) for not able to use
> > > > > some non-kernel types with a header file which has some type conflicts
> > > > > with vmlinux.h.
> > > > >
> > > > > If it is accepted, the attribute will be used inside the vmlinux.h and
> > > > > it will be used by virtually all bpf developers and it will make bpf devlopers
> > > > > more productive by not copying macros, static inline functions or
> > > > > non-kernel types.
> > > >
> > > > I'm uncomfortable with this attribute. Typically, attributes extend
> > > > rather than redefine the language. e.g., you might add attributes for
> > > > better performance or diagnostic characteristics, but you typically
> > > > should not use an attribute to redefine the basic premises of the
> > > > language.
> > > >
> > > > In this particular case, the attribute is used to tell the compiler to
> > > > ignore type redefinition errors and instead pick a "dominating"
> > > > declaration for the type. While C isn't as type sensitive as C++ is,
> > > > it still has _Generic, __typeof__, and other tricks that can expose
> > > > type system shenanigans like this in surprising ways. Given that type
> > > > size information is critical for many things in C (memcpy, memcmp,
> > > > pointer arithmetic with offsetof, etc), I'm uncomfortable with the
> > > > security aspects of the likely type confusion stemming from this being
> > > > so novel in C.
> > >
> > > To limit the potential impact. As RFC suggested, we can limit the
> > > impact only for CO-RE relocatable types. bpf developers are already
> > > aware and know how to use properly builtin's for CO-RE relocatable
> > > types and the types we are targeting are also CO-RE relocatable types.
> >
> > Limiting this to just the target and just for specific types will
> > certainly help, but doesn't really eliminate the fact that this
> > attribute is definitely not very C-like in what it does. As mentioned
> > on the code review, we have to do some interesting work to ensure we
> > emit the correct diagnostics for conformance to C (or, alternatively,
> > document that this target is not a C target, but that leads right back
> > to my argument that this is making a new language rather than
> > extending an existing one).

Hi, Aaron,

For bpf_dominating_decl attribute, our previous use cases target both
user header files
(like /usr/include/...) and linux kernel internal header files. The
reason bpf_dominating_decl
attribute is needed because some popular linux kernel internal headers
changed quite often
so those types in vmlinux.h are often not the same as the ones in host
kernel internal files.

To address your above concern where we really need to diagnose the
potential mis-matched
types, we are proposing a different attribute with tentative name below:
    __attribute__((bpf_accept_identical_del))

The attribute intends to permit accepting the *identical* types. For
example, currently
  struct t { int a; };
  struct t { int a; };
They are identical and the compiler will issue a redefinition warning
for the second definition.
For the following code,
  struct t { int a; } __attribute__((bpf_accept_identical_decl));
  struct t { int a; };
we intend the compiler will ignore the same 'struct t' types.
This should mostly work for /usr/include/... files as they are quite
stable, meaning the same
as the kernel version of uapi files in vmlinux.h.

The same is for enum types.
   enum { A, B } __attribute__((bpf_accept_identical_decl));
   enum { A. B };
The second 'enum { A, B};' will be ignored since it is identical to the first.

It looks like the compiler can already accept duplicated typedef
declarations. The following
code can compile successfully.
  typedef unsigned __u;
  typedef unsigned __u;

With __attribute__((bpf_accept_identical_decl)), we can avoid tricky
diagnosis issues
as the redefined type is identical to the previous definitions.

What do you think about the new
__attribute__((bpf_accept_identical_decl)) attribute?
Do you think in-clang-tree support is possible or not?
If you are okay with this proposal, I can do some prototyping (yes,
some guidance here
is welcomed) and send a RFC later.

Thanks,

Yonghong