BPF: adding new clang extension bpf_dominating_decl attribute

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.

Evidence of a significant user community

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!

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.

That said, we do have *one* attribute that I consider to be a
"redefine the language in fundamental ways" feature --
[[clang::overloadable]] allows you to define overload sets in C, which
is a distinctly not-C thing to do because of the name mangling
involved. However, that attribute introduces the C++ semantics into C
whereas the BPF dominating declaration attribute is introducing wholly
novel semantics. So I don't really consider [[clang::overloadable]] as
direct precedent for this.

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic
behavior in the places where there *is* type confusion the compiler
can detect, but the user will at least have an easier time *debugging*
any problems from this.

Which brings up an interesting question -- how does this attribute
interact with debug information?

~Aaron

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

For example, type size, in
https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
we have the following macro:

#define bpf_core_type_size(type) \
        __builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)

So users can get the type size for a particular kernel. Note that the type
might have different sizes for different kernels.

For offsetof issue, the bpf_core_read.h provides the following macro:

#define BPF_CORE_READ(src, a, ...) ({ \
        ___type((src), a, ##__VA_ARGS__) __r; \
        BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__); \
        __r; \
})

which eventually uses the builtin __builtin_preserve_access_index()
so bpfloader can adjust the offsetof properly.

So for relocatable types, user won't use typeof or offsetof.
Otherwise, programs won't be portable even without bpf_dominating_decl
attribute.

That said, we do have *one* attribute that I consider to be a
"redefine the language in fundamental ways" feature --
[[clang::overloadable]] allows you to define overload sets in C, which
is a distinctly not-C thing to do because of the name mangling
involved. However, that attribute introduces the C++ semantics into C
whereas the BPF dominating declaration attribute is introducing wholly
novel semantics. So I don't really consider [[clang::overloadable]] as
direct precedent for this.

>
> A specific need to reside within the Clang tree
> ===============================================
>
> The proposed attribute will be processed by Clang frontend lex and
> sema and it would be
> best to reside within the Clang tree.

Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

behavior in the places where there *is* type confusion the compiler
can detect, but the user will at least have an easier time *debugging*
any problems from this.

Which brings up an interesting question -- how does this attribute
interact with debug information?

I tried a simple example. The debuginfo does not contain *ignored* types.
And this is exactly what we want.

[yhs@devbig309.ftw3 ~/work/tests/llvm/dom] cat t.h
#pragma GCC system_header

#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
                             apply_to = any(record, enum, type_alias))
struct bob {
  int a;
  union {
    int i;
  };
};
#pragma clang attribute pop
struct bob {
  int a;
};
$ cat t.c
#include "t.h"

int foo(struct bob *arg) {
  return arg->a;
}
$ clang -target bpf -O2 -g -c t.c
$ llvm-dwarfdump t.o
t.o: file format elf64-bpf

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000088, format = DWARF32,
version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at
0x0000008c)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer ("clang version 14.0.0
(https://github.com/llvm/llvm-project.git
81703ebc2a7673de5b6cd71f8a6526beb1aee4ae)")
              DW_AT_language (DW_LANG_C99)
              DW_AT_name ("t.c")
              DW_AT_stmt_list (0x00000000)
              DW_AT_comp_dir ("/home/yhs/work/tests/llvm/dom")
              DW_AT_low_pc (0x0000000000000000)
              DW_AT_high_pc (0x0000000000000010)

0x0000002a: DW_TAG_subprogram
                DW_AT_low_pc (0x0000000000000000)
                DW_AT_high_pc (0x0000000000000010)
                DW_AT_frame_base (DW_OP_reg10 W10)
                DW_AT_GNU_all_call_sites (true)
                DW_AT_name ("foo")
                DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/t.c")
                DW_AT_decl_line (3)
                DW_AT_prototyped (true)
                DW_AT_type (0x00000051 "int")
                DW_AT_external (true)

0x00000043: DW_TAG_formal_parameter
                  DW_AT_location (DW_OP_reg1 W1)
                  DW_AT_name ("arg")
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/t.c")
                  DW_AT_decl_line (3)
                  DW_AT_type (0x00000058 "bob *")

0x00000050: NULL

0x00000051: DW_TAG_base_type
                DW_AT_name ("int")
                DW_AT_encoding (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000058: DW_TAG_pointer_type
                DW_AT_type (0x0000005d "bob")

0x0000005d: DW_TAG_structure_type
                DW_AT_name ("bob")
                DW_AT_byte_size (0x08)
                DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                DW_AT_decl_line (5)

0x00000065: DW_TAG_member
                  DW_AT_name ("a")
                  DW_AT_type (0x00000051 "int")
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                  DW_AT_decl_line (6)
                  DW_AT_data_member_location (0x00)

0x00000071: DW_TAG_member
                  DW_AT_type (0x00000079 "bob::union ")
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                  DW_AT_decl_line (7)
                  DW_AT_data_member_location (0x04)
0x00000079: DW_TAG_union_type
                  DW_AT_byte_size (0x04)
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                  DW_AT_decl_line (7)

0x0000007d: DW_TAG_member
                    DW_AT_name ("i")
                    DW_AT_type (0x00000051 "int")
                    DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                    DW_AT_decl_line (8)
                    DW_AT_data_member_location (0x00)

0x00000089: NULL

0x0000008a: NULL

0x0000008b: NULL

The attribute may only be applied to the first definition of an elaborated type (or first declaration of a typedef). Thus debug info and/or any completion semantics are from that instance. Subsequent redefinitions (or redeclarations) are ignored.

does that answer your question?

nathan

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

For example, type size, in
https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
we have the following macro:

#define bpf_core_type_size(type) \
        __builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)

So users can get the type size for a particular kernel. Note that the type
might have different sizes for different kernels.

For offsetof issue, the bpf_core_read.h provides the following macro:

#define BPF_CORE_READ(src, a, ...) ({ \
        ___type((src), a, ##__VA_ARGS__) __r; \
        BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__); \
        __r; \
})

which eventually uses the builtin __builtin_preserve_access_index()
so bpfloader can adjust the offsetof properly.

So for relocatable types, user won't use typeof or offsetof.

Will their use be diagnosed for BPF targets?

Otherwise, programs won't be portable even without bpf_dominating_decl
attribute.

>
> That said, we do have *one* attribute that I consider to be a
> "redefine the language in fundamental ways" feature --
> [[clang::overloadable]] allows you to define overload sets in C, which
> is a distinctly not-C thing to do because of the name mangling
> involved. However, that attribute introduces the C++ semantics into C
> whereas the BPF dominating declaration attribute is introducing wholly
> novel semantics. So I don't really consider [[clang::overloadable]] as
> direct precedent for this.
>
> >
> > A specific need to reside within the Clang tree
> > ===============================================
> >
> > The proposed attribute will be processed by Clang frontend lex and
> > sema and it would be
> > best to reside within the Clang tree.
>
> Would it be plausible/appropriate to instead run the source code
> through a processing tool which emits modified source code with the
> correct definitions instead of hoping this dominating declaration
> works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

I think Clang's architecture as a series of libraries provides
extensive support for building your own tooling to perform these kinds
of code transformations. For example, clang-tools-extra has
clang-change-namespace, clang-include-fixer, clang-reorder-fields, etc
and they all make use of Clang as a library without needing to modify
Clang itself. I think it would be reasonable to explore the idea of
adding such a tool to perform the rewriting for you (it could
potentially even live in-tree) as you would continue to use the
existing Clang code and still benefit from future Clang improvements.

> behavior in the places where there *is* type confusion the compiler
> can detect, but the user will at least have an easier time *debugging*
> any problems from this.
>
> Which brings up an interesting question -- how does this attribute
> interact with debug information?

I tried a simple example. The debuginfo does not contain *ignored* types.
And this is exactly what we want.

[yhs@devbig309.ftw3 ~/work/tests/llvm/dom] cat t.h
#pragma GCC system_header

#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
                             apply_to = any(record, enum, type_alias))
struct bob {
  int a;
  union {
    int i;
  };
};
#pragma clang attribute pop
struct bob {
  int a;
};
$ cat t.c
#include "t.h"

int foo(struct bob *arg) {
  return arg->a;
}
$ clang -target bpf -O2 -g -c t.c
$ llvm-dwarfdump t.o
t.o: file format elf64-bpf

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000088, format = DWARF32,
version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at
0x0000008c)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer ("clang version 14.0.0
(https://github.com/llvm/llvm-project.git
81703ebc2a7673de5b6cd71f8a6526beb1aee4ae)")
              DW_AT_language (DW_LANG_C99)
              DW_AT_name ("t.c")
              DW_AT_stmt_list (0x00000000)
              DW_AT_comp_dir ("/home/yhs/work/tests/llvm/dom")
              DW_AT_low_pc (0x0000000000000000)
              DW_AT_high_pc (0x0000000000000010)

0x0000002a: DW_TAG_subprogram
                DW_AT_low_pc (0x0000000000000000)
                DW_AT_high_pc (0x0000000000000010)
                DW_AT_frame_base (DW_OP_reg10 W10)
                DW_AT_GNU_all_call_sites (true)
                DW_AT_name ("foo")
                DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/t.c")
                DW_AT_decl_line (3)
                DW_AT_prototyped (true)
                DW_AT_type (0x00000051 "int")
                DW_AT_external (true)

0x00000043: DW_TAG_formal_parameter
                  DW_AT_location (DW_OP_reg1 W1)
                  DW_AT_name ("arg")
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/t.c")
                  DW_AT_decl_line (3)
                  DW_AT_type (0x00000058 "bob *")

0x00000050: NULL

0x00000051: DW_TAG_base_type
                DW_AT_name ("int")
                DW_AT_encoding (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000058: DW_TAG_pointer_type
                DW_AT_type (0x0000005d "bob")

0x0000005d: DW_TAG_structure_type
                DW_AT_name ("bob")
                DW_AT_byte_size (0x08)
                DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                DW_AT_decl_line (5)

0x00000065: DW_TAG_member
                  DW_AT_name ("a")
                  DW_AT_type (0x00000051 "int")
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                  DW_AT_decl_line (6)
                  DW_AT_data_member_location (0x00)

0x00000071: DW_TAG_member
                  DW_AT_type (0x00000079 "bob::union ")
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                  DW_AT_decl_line (7)
                  DW_AT_data_member_location (0x04)
0x00000079: DW_TAG_union_type
                  DW_AT_byte_size (0x04)
                  DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                  DW_AT_decl_line (7)

0x0000007d: DW_TAG_member
                    DW_AT_name ("i")
                    DW_AT_type (0x00000051 "int")
                    DW_AT_decl_file ("/home/yhs/work/tests/llvm/dom/./t.h")
                    DW_AT_decl_line (8)
                    DW_AT_data_member_location (0x00)

0x00000089: NULL

0x0000008a: NULL

0x0000008b: NULL

Fantastic, thank you for verifying!

~Aaron

> Which brings up an interesting question -- how does this attribute
> interact with debug information?

The attribute may only be applied to the first definition of an
elaborated type (or first declaration of a typedef). Thus debug info
and/or any completion semantics are from that instance. Subsequent
redefinitions (or redeclarations) are ignored.

does that answer your question?

Yup, thank you!

~Aaron

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

For example, type size, in
https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
we have the following macro:

#define bpf_core_type_size(type)
__builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)

So users can get the type size for a particular kernel. Note that the type
might have different sizes for different kernels.

For offsetof issue, the bpf_core_read.h provides the following macro:

#define BPF_CORE_READ(src, a, …) ({
___type((src), a, ##VA_ARGS) __r;
BPF_CORE_READ_INTO(&__r, (src), a, ##VA_ARGS);
__r;
})

which eventually uses the builtin __builtin_preserve_access_index()
so bpfloader can adjust the offsetof properly.

So for relocatable types, user won’t use typeof or offsetof.

Will their use be diagnosed for BPF targets?

Otherwise, programs won’t be portable even without bpf_dominating_decl
attribute.

That said, we do have one attribute that I consider to be a
“redefine the language in fundamental ways” feature –
[[clang::overloadable]] allows you to define overload sets in C, which
is a distinctly not-C thing to do because of the name mangling
involved. However, that attribute introduces the C++ semantics into C
whereas the BPF dominating declaration attribute is introducing wholly
novel semantics. So I don’t really consider [[clang::overloadable]] as
direct precedent for this.

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

I think Clang’s architecture as a series of libraries provides
extensive support for building your own tooling to perform these kinds
of code transformations. For example, clang-tools-extra has
clang-change-namespace, clang-include-fixer, clang-reorder-fields, etc
and they all make use of Clang as a library without needing to modify
Clang itself. I think it would be reasonable to explore the idea of
adding such a tool to perform the rewriting for you (it could
potentially even live in-tree) as you would continue to use the
existing Clang code and still benefit from future Clang improvements.

The general problem, that it is laborious to solve the problem in a tool, and furthermore such a solution would require duplicating lots of existing clang functionality creating a maintenance burden, and that it is thus preferable to upstream the solution, which burdens other users to some extent, recalls the discussion about upstreaming Swift’s APINotes:
https://lists.llvm.org/pipermail/cfe-dev/2020-October/066944.html

Then and now, I think the best solution in these situations is to encourage/support patches which introduce new handles in ASTConsumer to customize behavior during parsing (rather than always afterward, via HandleTranslationUnit). This would keep consumer details from leaking upstream, and upstream details from leaking into downstream consumers, lessening maintenance burdens for everyone.

In this case: suppose that wherever clang emits an error, that were changed to a call to an ASTConsumer virtual method which by default emits the error.

I think the tool would then be fairly trivial: instead of issuing a type redefinition error, your tool simply deletes or omit the declaration that caused it.

Would this solve the problem, or am I missing something?

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

> For example, type size, in
> https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
> we have the following macro:
>
> #define bpf_core_type_size(type) \
> __builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)
>
> So users can get the type size for a particular kernel. Note that the type
> might have different sizes for different kernels.
>
> For offsetof issue, the bpf_core_read.h provides the following macro:
>
> #define BPF_CORE_READ(src, a, ...) ({ \
> ___type((src), a, ##__VA_ARGS__) __r; \
> BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__); \
> __r; \
> })
>
> which eventually uses the builtin __builtin_preserve_access_index()
> so bpfloader can adjust the offsetof properly.
>
> So for relocatable types, user won't use typeof or offsetof.

Will their use be diagnosed for BPF targets?

Sorry for replying late. We had some discussions internally about what
is the best way to move forward.

For the question whether the use of typeof or offsetof is diagnosed
for BPF targets,
yes, we do some diagnose at bpf loader file. The compilation will be successful
with *provided* types, but during bpf loading, the bpf loader will
actually check
the host vmlinux BTF type. If the type or the field does not exist in
host vmlinux
BTF, the bpf loader will issue an error. Otherwise, bpf loader will
adjust properly
based on host vmlinux BTF.

> Otherwise, programs won't be portable even without bpf_dominating_decl
> attribute.
>
> >
> > That said, we do have *one* attribute that I consider to be a
> > "redefine the language in fundamental ways" feature --
> > [[clang::overloadable]] allows you to define overload sets in C, which
> > is a distinctly not-C thing to do because of the name mangling
> > involved. However, that attribute introduces the C++ semantics into C
> > whereas the BPF dominating declaration attribute is introducing wholly
> > novel semantics. So I don't really consider [[clang::overloadable]] as
> > direct precedent for this.
> >
> > >
> > > A specific need to reside within the Clang tree
> > > ===============================================
> > >
> > > The proposed attribute will be processed by Clang frontend lex and
> > > sema and it would be
> > > best to reside within the Clang tree.
> >
> > Would it be plausible/appropriate to instead run the source code
> > through a processing tool which emits modified source code with the
> > correct definitions instead of hoping this dominating declaration
> > works out? In this case, I think the user will get better diagnostic
>
> Theoretically it is possible. We need to have a preprocessor, parse
> the program, including all
> include files, do exactly the https://reviews.llvm.org/D111307 has
> done to ignore
> those duplicated relocatable types and generate a .i file and feed into clang.
> But this duplicates a lot of current clang code and the tool itself cannot
> automatically benefit from future clang improvements. So I think in-tree
> support is the best option, least maintenance burden.

I think Clang's architecture as a series of libraries provides
extensive support for building your own tooling to perform these kinds
of code transformations. For example, clang-tools-extra has
clang-change-namespace, clang-include-fixer, clang-reorder-fields, etc
and they all make use of Clang as a library without needing to modify
Clang itself. I think it would be reasonable to explore the idea of
adding such a tool to perform the rewriting for you (it could
potentially even live in-tree) as you would continue to use the
existing Clang code and still benefit from future Clang improvements.

We internally discussed the extra tool approach. We intend not go to
this route right now as this yet another tool adds complexity to build process
and probably will discourage people from using it. David Rector has
another idea about detecting and removing duplicated types during parsing
stage (before semantic analysis). We would like to explore that first.

Thanks!

Yonghong

[...]

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

For example, type size, in
https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
we have the following macro:

#define bpf_core_type_size(type) \
       __builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)

So users can get the type size for a particular kernel. Note that the type
might have different sizes for different kernels.

For offsetof issue, the bpf_core_read.h provides the following macro:

#define BPF_CORE_READ(src, a, ...) ({ \
       ___type((src), a, ##__VA_ARGS__) __r; \
       BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__); \
       __r; \
})

which eventually uses the builtin __builtin_preserve_access_index()
so bpfloader can adjust the offsetof properly.

So for relocatable types, user won't use typeof or offsetof.

Will their use be diagnosed for BPF targets?

Otherwise, programs won't be portable even without bpf_dominating_decl
attribute.

That said, we do have *one* attribute that I consider to be a
"redefine the language in fundamental ways" feature --
[[clang::overloadable]] allows you to define overload sets in C, which
is a distinctly not-C thing to do because of the name mangling
involved. However, that attribute introduces the C++ semantics into C
whereas the BPF dominating declaration attribute is introducing wholly
novel semantics. So I don't really consider [[clang::overloadable]] as
direct precedent for this.

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

I think Clang's architecture as a series of libraries provides
extensive support for building your own tooling to perform these kinds
of code transformations. For example, clang-tools-extra has
clang-change-namespace, clang-include-fixer, clang-reorder-fields, etc
and they all make use of Clang as a library without needing to modify
Clang itself. I think it would be reasonable to explore the idea of
adding such a tool to perform the rewriting for you (it could
potentially even live in-tree) as you would continue to use the
existing Clang code and still benefit from future Clang improvements.

The general problem, that it is laborious to solve the problem in a tool, and furthermore such a solution would require duplicating lots of existing clang functionality creating a maintenance burden, and that it is thus preferable to upstream the solution, which burdens other users to some extent, recalls the discussion about upstreaming Swift’s APINotes:
https://lists.llvm.org/pipermail/cfe-dev/2020-October/066944.html

Then and now, I think the best solution in these situations is to encourage/support patches which introduce new handles in ASTConsumer to customize behavior during parsing (rather than always afterward, via HandleTranslationUnit). This would keep consumer details from leaking upstream, and upstream details from leaking into downstream consumers, lessening maintenance burdens for everyone.

In this case: suppose that wherever clang emits an error, that were changed to a call to an ASTConsumer virtual method which by default emits the error.

I think the tool would then be fairly trivial: instead of issuing a type redefinition error, your tool simply deletes or omit the declaration that caused it.

Would this solve the problem, or am I missing something?

David, thanks a lot for your suggestion. Currently, as in patch
https://reviews.llvm.org/D111307, the detecting and handling
of redefinition are all in the semantic stage. The redefinition
handling is actually not in HandleTranslationUnit(), but rather
in semantic analysis or parsed constructs. So regarding new
ASTConsumer handlers, do you propose to abstract
existing D111307 handling into a virtual handler, populated with bpf
target? But this is still in the semantic stage.

Maybe you could elaborate your possible solution a little more? I
probably missed some of your points.

Thanks!

behavior in the places where there *is* type confusion the compiler
can detect, but the user will at least have an easier time *debugging*
any problems from this.

[...]

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

For example, type size, in
https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
we have the following macro:

#define bpf_core_type_size(type)
__builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)

So users can get the type size for a particular kernel. Note that the type
might have different sizes for different kernels.

For offsetof issue, the bpf_core_read.h provides the following macro:

#define BPF_CORE_READ(src, a, …) ({
___type((src), a, ##VA_ARGS) __r;
BPF_CORE_READ_INTO(&__r, (src), a, ##VA_ARGS);
__r;
})

which eventually uses the builtin __builtin_preserve_access_index()
so bpfloader can adjust the offsetof properly.

So for relocatable types, user won’t use typeof or offsetof.

Will their use be diagnosed for BPF targets?

Otherwise, programs won’t be portable even without bpf_dominating_decl
attribute.

That said, we do have one attribute that I consider to be a
“redefine the language in fundamental ways” feature –
[[clang::overloadable]] allows you to define overload sets in C, which
is a distinctly not-C thing to do because of the name mangling
involved. However, that attribute introduces the C++ semantics into C
whereas the BPF dominating declaration attribute is introducing wholly
novel semantics. So I don’t really consider [[clang::overloadable]] as
direct precedent for this.

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

I think Clang’s architecture as a series of libraries provides
extensive support for building your own tooling to perform these kinds
of code transformations. For example, clang-tools-extra has
clang-change-namespace, clang-include-fixer, clang-reorder-fields, etc
and they all make use of Clang as a library without needing to modify
Clang itself. I think it would be reasonable to explore the idea of
adding such a tool to perform the rewriting for you (it could
potentially even live in-tree) as you would continue to use the
existing Clang code and still benefit from future Clang improvements.

The general problem, that it is laborious to solve the problem in a tool, and furthermore such a solution would require duplicating lots of existing clang functionality creating a maintenance burden, and that it is thus preferable to upstream the solution, which burdens other users to some extent, recalls the discussion about upstreaming Swift’s APINotes:
https://lists.llvm.org/pipermail/cfe-dev/2020-October/066944.html

Then and now, I think the best solution in these situations is to encourage/support patches which introduce new handles in ASTConsumer to customize behavior during parsing (rather than always afterward, via HandleTranslationUnit). This would keep consumer details from leaking upstream, and upstream details from leaking into downstream consumers, lessening maintenance burdens for everyone.

In this case: suppose that wherever clang emits an error, that were changed to a call to an ASTConsumer virtual method which by default emits the error.

I think the tool would then be fairly trivial: instead of issuing a type redefinition error, your tool simply deletes or omit the declaration that caused it.

Would this solve the problem, or am I missing something?

David, thanks a lot for your suggestion. Currently, as in patch
https://reviews.llvm.org/D111307, the detecting and handling
of redefinition are all in the semantic stage. The redefinition
handling is actually not in HandleTranslationUnit(), but rather
in semantic analysis or parsed constructs. So regarding new
ASTConsumer handlers, do you propose to abstract
existing D111307 handling into a virtual handler, populated with bpf
target? But this is still in the semantic stage.

Maybe you could elaborate your possible solution a little more? I
probably missed some of your points.

Well this is not a fully formed solution, and I may be missing some of the particulars, and I sense you have the go-ahead for your attribute-based solution if you just want to be done with it. But if you want to explore this here is the specific part of the discussion my suggestion was responding to:

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

[Aaron] Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

If your problem could be solved with a tool, but the problem is that ASTConsumer does not provide a sufficient interface to customize behavior during the semantic stage, I say just add that interface.

I.e. add a new virtual function in ASTConsumer, call it HandleSemaDiagnostic let’s say, and call it via Consumer.HandleSemaDiagnostic(…) sometime during, presumably, Sema::Diag(…), to give the Consumer a chance to alter the diagnostic — in your case by somehow marking that it should be ignored — before it can be emitted.

I think it might be simple, because the error which is causing all the trouble is one from which clang can and does recover; e.g. you get both errors here:

int *foo = nullptr;
int foo = 3; // Error: redefinition
int bar = foo + bar; // Error: type of foo is int*

So all you need to do is give your tool a way to reach into Sema and prevent that first error from being emitted, and the resulting AST should be exactly as you want it, I think.

Is it more difficult than a few lines somehow marking that the diagnostic should be ignored? Perhaps — others with deeper experience handling diagnostics might want to chime in here.

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

For example, type size, in
https://github.com/torvalds/linux/blob/master/tools/lib/bpf/bpf_core_read.h
we have the following macro:

#define bpf_core_type_size(type) \
      __builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)

So users can get the type size for a particular kernel. Note that the type
might have different sizes for different kernels.

For offsetof issue, the bpf_core_read.h provides the following macro:

#define BPF_CORE_READ(src, a, ...) ({ \
      ___type((src), a, ##__VA_ARGS__) __r; \
      BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__); \
      __r; \
})

which eventually uses the builtin __builtin_preserve_access_index()
so bpfloader can adjust the offsetof properly.

So for relocatable types, user won't use typeof or offsetof.

Will their use be diagnosed for BPF targets?

Otherwise, programs won't be portable even without bpf_dominating_decl
attribute.

That said, we do have *one* attribute that I consider to be a
"redefine the language in fundamental ways" feature --
[[clang::overloadable]] allows you to define overload sets in C, which
is a distinctly not-C thing to do because of the name mangling
involved. However, that attribute introduces the C++ semantics into C
whereas the BPF dominating declaration attribute is introducing wholly
novel semantics. So I don't really consider [[clang::overloadable]] as
direct precedent for this.

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

I think Clang's architecture as a series of libraries provides
extensive support for building your own tooling to perform these kinds
of code transformations. For example, clang-tools-extra has
clang-change-namespace, clang-include-fixer, clang-reorder-fields, etc
and they all make use of Clang as a library without needing to modify
Clang itself. I think it would be reasonable to explore the idea of
adding such a tool to perform the rewriting for you (it could
potentially even live in-tree) as you would continue to use the
existing Clang code and still benefit from future Clang improvements.

The general problem, that it is laborious to solve the problem in a tool, and furthermore such a solution would require duplicating lots of existing clang functionality creating a maintenance burden, and that it is thus preferable to upstream the solution, which burdens other users to some extent, recalls the discussion about upstreaming Swift’s APINotes:
https://lists.llvm.org/pipermail/cfe-dev/2020-October/066944.html

Then and now, I think the best solution in these situations is to encourage/support patches which introduce new handles in ASTConsumer to customize behavior during parsing (rather than always afterward, via HandleTranslationUnit). This would keep consumer details from leaking upstream, and upstream details from leaking into downstream consumers, lessening maintenance burdens for everyone.

In this case: suppose that wherever clang emits an error, that were changed to a call to an ASTConsumer virtual method which by default emits the error.

I think the tool would then be fairly trivial: instead of issuing a type redefinition error, your tool simply deletes or omit the declaration that caused it.

Would this solve the problem, or am I missing something?

David, thanks a lot for your suggestion. Currently, as in patch
https://reviews.llvm.org/D111307, the detecting and handling
of redefinition are all in the semantic stage. The redefinition
handling is actually not in HandleTranslationUnit(), but rather
in semantic analysis or parsed constructs. So regarding new
ASTConsumer handlers, do you propose to abstract
existing D111307 handling into a virtual handler, populated with bpf
target? But this is still in the semantic stage.

Maybe you could elaborate your possible solution a little more? I
probably missed some of your points.

Well this is not a fully formed solution, and I may be missing some of the particulars, and I sense you have the go-ahead for your attribute-based solution if you just want to be done with it. But if you want to explore this here is the specific part of the discussion my suggestion was responding to:

A specific need to reside within the Clang tree

The proposed attribute will be processed by Clang frontend lex and
sema and it would be
best to reside within the Clang tree.

[Aaron] Would it be plausible/appropriate to instead run the source code
through a processing tool which emits modified source code with the
correct definitions instead of hoping this dominating declaration
works out? In this case, I think the user will get better diagnostic

Theoretically it is possible. We need to have a preprocessor, parse
the program, including all
include files, do exactly the https://reviews.llvm.org/D111307 has
done to ignore
those duplicated relocatable types and generate a .i file and feed into clang.
But this duplicates a lot of current clang code and the tool itself cannot
automatically benefit from future clang improvements. So I think in-tree
support is the best option, least maintenance burden.

If your problem could be solved with a tool, but the problem is that ASTConsumer does not provide a sufficient interface to customize behavior during the semantic stage, I say just add that interface.

Thanks for the detailed explanation here and below. I can see your
points now. Extra hook with ASTConsumer can help to handle
redefinition in the tool. Our internal discussion inclined to avoid
the tool. Let me go back to discuss what we could do without an extra
tool.

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

Hi, Aaron, to address your concerns about diagnostic issue about same type name with different type contents, we have proposed a new attribute bpf_accept_identical_decl, targeting to record and enum types. Could you take a look and see whether this is an acceptable attribute to be implemented inside llvm-project repo? Thanks!

Note: You are unlikely to get a response from Aaron here. His issues with the T&C of Discourse has left him unable to sign up, so you may wish to forward the RFC via email to him directly.

Thanks @erichkeane for heads-up. I will forward the email to Aaron.