[clang] __attribute__ bpf_dominating_decl

[urnathan]

Summary

This adds a new attribute requested by BPF developers. Specifically, they want the ability to
(a) #include the machine-generated vmlinux.h header, that describes the structures, enums and typedefs of the running kernel, and
(b) #include regular kernel headers to get at accessor macros and/or [inline] functions

Those kernel headers will also define the structures of the vmlinux.h header, and thus this would normally be expected to die horribly with a compilation error about multiple definitions or conflicting declarations.

Why conflicting declarations? It turns out that the following are also true:
a) the vmlinux definition of structures & enums defines those of the running kernel, which may not have been configured the same way as the compilation under process. So the structure definitions differ.
b) typedefs may be being given conflicting types (due to lack of fidelity in debug info?)

I understand that neither of these mismatches is a problem to BPF developers.

This patch adds a bpf_dominating_decl Decl attribute, applicable to Record, Enum and Typedef Decls. It is only available for bpf targets compiling C.

This attribute does affect language semantics, so breaks the convention (sorry, couldn't fund actual words in a std mandating this) that ignoring attributes does not change the well-formedness of a program.

The new checks in SemaDecl are on the error path, where we're about to give an error, so we do not degrade well-formed program compilation speed.

[aaron.ballman]

clang/docs/LanguageExtensions.rst

3226

This change is unrelated, feel free to land as an NFC with its own commit and revert from here.

3284–3286

This concerns me and I don't think an attribute alone is sufficient. While vendor-specific attributes are free to have required semantics, we generally don't have attributes that ignore fundamental constraints the language has in place, because violating those constraints can lead to all kinds of weird situations. In this case, given that it's only proposed for C and doesn't involve C++ (where you have additional concerns like SFINAE), the damage is somewhat limited, but I am still pretty uncomfortable with this.

One concern is because C's redeclaration rules are not always obvious. e.g., https://godbolt.org/z/817nKacEx and so this has a risk of hiding diagnostics in cases that matter for linkage or other hard-to-detect bugs.

Another concern is with security where the redefinition is incompatible in a way that silently causes layouts to no longer be correct for things that are accessed via pointer manipulations.

Normally I'd suggest we hide this functionality behind a feature flag, but tbh, this feels somewhat like -fpermissive mode in that it's "please ignore what the standard requires".

What are the chances that this functionality will grow to include C++ (or ObjC, etc)? I know we already have some support for BPF in Clang, but this feels like a surprisingly hard step to take. Was there a Clang RFC that included this information somewhere so I could read the thread?

3288–3291

Is vmlinux.h always a system header? If so, are you intending to limit use of this attribute to only system header situations, or is this an attribute that users will be able to sprinkle in their own declarations?

3301–3302

That limitation only applies to standard attributes and not vendor-specific ones, so it's not a huge issue here. For example, calling convention attributes are ones that you often cannot safely ignore without a change in program semantics (due to ABI).

clang/lib/Sema/SemaDecl.cpp

2207

LLVM project coding style is to use a static function here and to name it starting with a lowercase letter.

[yonghong-song]

clang/docs/LanguageExtensions.rst

3284–3286

Let me try to answer some questions here.

Another concern is with security where the redefinition is incompatible in a way that silently causes layouts to no longer be correct for things that are accessed via pointer manipulations.

This is true. This should be made clear in the documentation. With clear documentation, I hope that we don't need additional flag -fpermissive.

What are the chances that this functionality will grow to include C++ (or ObjC, etc)? I know we already have some support for BPF in Clang, but this feels like a surprisingly hard step to take. Was there a Clang RFC that included this information somewhere so I could read the thread?

So far, this functionality is for C only and there is no plan to extend it to C++ like many other bpf/btf related features.

We do have not a clang RFC. The early discussion related to this subject: https://lists.llvm.org/pipermail/cfe-dev/2021-August/068696.html
But the approach has evolved to the current patch. But the motivation should be the same as in the above mailing list.

3288–3291

vmlinux.h is generated from BTF. Here is a good blog to show how vmlinux.h is generated:
https://blog.aquasec.com/vmlinux.h-ebpf-programs

This flag is intended to be used inside vmlinux.h and users are not expected to use them directly.

[urnathan]

clang/docs/LanguageExtensions.rst

3284–3286

Not going to disagree with you Aaron. Adding this functionality to C++ would be terrible, with all the SFINAE complications you mention. they also apply only to types, so the static/extern symbol linkage issue you mention doesn't apply -- and this extension is explicitly blessing terrible ODR violation, even once one gets past the multiple definition issue itself.

I do have a version of this patch that implements it as

`#pragma clang bpf_dominating_decl [push|pop]`

if you think that's preferable

3288–3291

History teaches us that users will use any and all features they can get their hands on, sadly :(
Restricting this to system headers is a good direction.

3301–3302

Thanks for the attribute description -- my mental model of things like ABI tags is that the can be ignored *IF* all compilations ignore them, but that probably doesn't work for things like shared object declspecs (but then one's explicitly stepped outside the std anyway).

[urnathan]

Remove unrelated drive-by cleanup, replacce anon-namespace with static & adjust capitalization

[mizvekov]

clang/lib/Sema/SemaDecl.cpp

2210–2216

Wouldn't Decl->hasAttr<BPFDominatingAttr>() work here?
Might not even be worth a function and could just use that expression instead.

[urnathan]

use hasAttr -- thanks for pointing that out

[mizvekov]

clang/lib/Sema/SemaDecl.cpp

2276

Is there a test case for this one?
It would be interesting to spell out what diagnostic the user would end up seeing here.

18073

Same as before, about a test case for this one.

[aaron.ballman]

clang/docs/LanguageExtensions.rst

3284–3286

Not going to disagree with you Aaron. Adding this functionality to C++ would be terrible, with all the SFINAE complications you mention. they also apply only to types, so the static/extern symbol linkage issue you mention doesn't apply -- and this extension is explicitly blessing terrible ODR violation, even once one gets past the multiple definition issue itself.

I would say that adding this functionality in C++ is a non-starter. I'm not certain I could convince myself if we properly integrate such an extension into C++ in a conforming way that doesn't add immense footguns. To be honest, I'm not entirely convinced for C either.

I do have a version of this patch that implements it as
#pragma clang bpf_dominating_decl [push|pop]
if you think that's preferable

It's not the syntax that bothers me about the feature, it's more about the feature itself.

This is true. This should be made clear in the documentation. With clear documentation, I hope that we don't need additional flag -fpermissive.

Sorry, I was unclear. I wasn't suggesting we need to add support for -fpermissive for this, it was that this functionality reminds me of -fpermissive in terms of how it behaves. It's basically "pretend this is a totally new language that looks similar to C" (or C++). So I'm trying to decide whether we need to add an -fbpf feature flag or some such to enable these sort of odd not-really-how-the-base-language-works attribute extensions or not, or whether this feature is one Clang should expose at all (for example, I think a user explicitly loading a Clang plugin rather than exposing the functionality directly as part of the compiler would alleviate many of my concerns).

3288–3291

+1, if we're going to have this feature at all, it needs to be as bulletproof as we can make it in terms of documentation and diagnostics. Limiting use to only system headers helps reduce the feature surface a bit.

[urnathan]

Added documentation to .td file, requires to be inside system header

[urnathan]

clang/lib/Sema/SemaDecl.cpp

2276

that's the following piece:

#pragma clang attribute push(attribute((bpf_dominating_decl)), \

apply_to = any(record, enum, type_alias))

typedef unsigned bob;
#pragma clang attribute pop
typedef int bob; // <img src="this is fine dog"/>

18073

#pragma clang attribute push(attribute((bpf_dominating_decl)), \

apply_to = any(record, enum, type_alias))

enum { B };
#pragma clang attribute pop
enum { B,

C }; // OK

int ary2[2 * (C == 1) - 1];

[yonghong-song]

@aaron.ballman do you have any further comments for this patch?

[aaron.ballman]

In D111307#3119610, @yonghong-song wrote:

@aaron.ballman do you have any further comments for this patch?

Thanks for your patience -- this review is a bit of a challenge because the feature makes me deeply uncomfortable. It's so outside of the realm of how C behaves that it feels inappropriate to allow this behavior from an attribute with no other opt in required, but I struggle to articulate a path that would make me less uncomfortable. We require a BPF target AND we require the attribute to be used in system headers, so there is a sort of an opt in and safety rails to this already, but at the same time, this just... isn't C. We're not really extending C in this case, we're defining a wholly new C language. Hmmm, which made me go look to see if this is a conforming extension in C and I'm not certain it is because it seems like it can alter the behavior of a strictly conforming program (changes the meaning of object layouts, e.g.). @urnathan, do you get the same reading from Clause 4 of http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2731.pdf ?

[urnathan]

This doesn't affect cross-TU well-formedness, mismatched type definintions in different TUs remain in UB/NDR territory. This doesn't change the meaning of a well-formed TU, because those never get into the conflicting situations that this affects. It does cause us to accept ill-formed translation units that one would expect a diagnostic for. I'm not sure if I can read clause 4 to say 'thou must issue a diagnostic', 4/1 just seems to say how you can stay in conformance, not what happens in [some] cases of falling outside it.

We allow violations of the constraint at 6.7/3, no more than one declaration of a particular identifier except as:
a) 6.7/2 bullet 1 (typedef redefinition)
b) 6.7.2.3/1 (struct et al defined at most once)

As you say, this is a most un-C-like direction.

[aaron.ballman]

In D111307#3122700, @urnathan wrote:

This doesn't affect cross-TU well-formedness, mismatched type definintions in different TUs remain in UB/NDR territory. This doesn't change the meaning of a well-formed TU, because those never get into the conflicting situations that this affects. It does cause us to accept ill-formed translation units that one would expect a diagnostic for. I'm not sure if I can read clause 4 to say 'thou must issue a diagnostic', 4/1 just seems to say how you can stay in conformance, not what happens in [some] cases of falling outside it.

Hmm, I was thinking about a situation like:

// BPF system header
__attribute__((bpf_dominating_decl)) typedef double foo; // This one now wins

// Original system header
typedef int foo;

// UserCode2.c
extern void func(int a, ...);

int main(void) {
  func(1, 2);
}

// UserCode1.c
#include "header.h"

void func(int a, ...) {
  va_list list;
  va_start(list, a);
  foo val = va_arg(list, foo); // This is now UB due to BPF
  va_end(list);
}

We allow violations of the constraint at 6.7/3, no more than one declaration of a particular identifier except as:
a) 6.7/2 bullet 1 (typedef redefinition)
b) 6.7.2.3/1 (struct et al defined at most once)

As you say, this is a most un-C-like direction.

Yeah, and I'm getting more and more uncomfortable with it. Out of curiosity, how disruptive would rejecting this patch be to your goals? (Note, while I'm considering rejecting this, I'm not doing so now, but if there are other options that could be explored, I would say they'd be worth exploring at this point because that's useful information for this review.)

[urnathan]

In D111307#3124286, @aaron.ballman wrote:

In D111307#3122700, @urnathan wrote:

This doesn't affect cross-TU well-formedness, mismatched type definintions in different TUs remain in UB/NDR territory. This doesn't change the meaning of a well-formed TU, because those never get into the conflicting situations that this affects. It does cause us to accept ill-formed translation units that one would expect a diagnostic for. I'm not sure if I can read clause 4 to say 'thou must issue a diagnostic', 4/1 just seems to say how you can stay in conformance, not what happens in [some] cases of falling outside it.

Hmm, I was thinking about a situation like:

// BPF system header
__attribute__((bpf_dominating_decl)) typedef double foo; // This one now wins

// Original system header
typedef int foo;

and that's my language lawyer point -- this is already non-conforming (albeit hidden inside headers), so the UB below is just a manifestation of trying to apply the semantics of well-formed C onto this ill-formed C. Unicorns & rainbows! .(... and madness?)

// UserCode2.c
extern void func(int a, ...);

int main(void) {

func(1, 2);

}

// UserCode1.c
#include "header.h"

void func(int a, ...) {

va_list list;
va_start(list, a);
foo val = va_arg(list, foo); // This is now UB due to BPF
va_end(list);

}

> 
> We allow violations of the constraint at 6.7/3, no more than one declaration of a particular identifier except as:
> a) 6.7/2 bullet 1 (typedef redefinition)
> b) 6.7.2.3/1 (struct et al defined at most once)
> 
> As you say, this is a most un-C-like direction.

Yeah, and I'm getting more and more uncomfortable with it. Out of curiosity, how disruptive would rejecting this patch be to your goals? (Note, while I'm considering rejecting this, I'm not doing so now, but if there are other options that could be explored, I would say they'd be worth exploring at this point because that's useful information for this review.)

I can't answer that -- @yonghong-song ?

[aaron.ballman]

In D111307#3124325, @urnathan wrote:

In D111307#3124286, @aaron.ballman wrote:

In D111307#3122700, @urnathan wrote:

This doesn't affect cross-TU well-formedness, mismatched type definintions in different TUs remain in UB/NDR territory. This doesn't change the meaning of a well-formed TU, because those never get into the conflicting situations that this affects. It does cause us to accept ill-formed translation units that one would expect a diagnostic for. I'm not sure if I can read clause 4 to say 'thou must issue a diagnostic', 4/1 just seems to say how you can stay in conformance, not what happens in [some] cases of falling outside it.

Hmm, I was thinking about a situation like:

// BPF system header
__attribute__((bpf_dominating_decl)) typedef double foo; // This one now wins

// Original system header
typedef int foo;

and that's my language lawyer point -- this is already non-conforming (albeit hidden inside headers),

Okay, so we're on the same page that this extension is not a conforming one, but perhaps for different reasons than I was thinking? My logic goes like this:

Implementations are required to diagnose constraint violations (5.1.1.3), but are not required to fail translation because the implementation can then define this space as an extension so long as the extension doesn't change the meaning of a strictly conforming program (Clause 4p6).

So in order to remain conforming, we have to generate at least one diagnostic message in the following full program:

typedef int foo;
__attribute__((bpf_dominating_decl)) typedef double foo;
int main(void) { return 0; }

(otherwise we violate 5.1.1.3), but then we can accept the code. Then, my earlier example becomes an issue because the meaning of the program changes (and worryingly, does so in a way that cannot be detected at compile time). But I suppose it was never a strictly conforming program in the first place because of the duplicate typedefs and the use of attribute, which is your point.

So yeah, I guess it boils down to the diagnostic behavior?

[urnathan]

In D111307#3124357, @aaron.ballman wrote:

Okay, so we're on the same page that this extension is not a conforming one, but perhaps for different reasons than I was thinking? My logic goes like this:

Implementations are required to diagnose constraint violations (5.1.1.3), but are not required to fail translation because the implementation can then define this space as an extension so long as the extension doesn't change the meaning of a strictly conforming program (Clause 4p6).

So in order to remain conforming, we have to generate at least one diagnostic message in the following full program:

thanks for the reference -- I thought it should be in there somewhere. Agreed, for a user tracking down the kind of foot gun this enables, a warning at each violation would be best, but I don't know how often that would occur in usual usage of this feature. Perhaps add an option to inhibit such warnings -- so even on bpf you have to explicity pull a 'I'm gonna step outside the std' lever.

[aaron.ballman]

In D111307#3124388, @urnathan wrote:

thanks for the reference -- I thought it should be in there somewhere. Agreed, for a user tracking down the kind of foot gun this enables, a warning at each violation would be best, but I don't know how often that would occur in usual usage of this feature. Perhaps add an option to inhibit such warnings -- so even on bpf you have to explicity pull a 'I'm gonna step outside the std' lever.

That would work. I suppose there's really (at least) two ways to go about it: 0) option to explicitly opt into enabling the attribute (and perhaps in a way that's wider than just this review), document that it's a nonconforming language mode, don't bother issuing any diagnostics about these weird things, and maybe even relax the "must be in a system header" safety rails, 1) diagnose conformance issues but give an option to explicitly opt out of those diagnostics messages when in BPF mode. (I'm assuming we're all agreed that the conformance diagnostics here are not pedantic ones but important ones. If we think they're purely pedantic, we could issue them only under -pedantic I suppose, but I personally don't think they're purely pendantic diagnostics.) I lean towards #0, but I'd love to hear thoughts from others.

[yonghong-song]

I am okay with an explicit option to enable this attribute. I remembered in the past we discussed with a possible option -fpermissive to control this. But this may be too generic. Maybe an option like -Xclang -target-feature -Xclang +attr_bpf_dominating_decl? Just a rough idea.

[anakryiko]

In D111307#3124927, @yonghong-song wrote:

I am okay with an explicit option to enable this attribute. I remembered in the past we discussed with a possible option -fpermissive to control this. But this may be too generic. Maybe an option like -Xclang -target-feature -Xclang +attr_bpf_dominating_decl? Just a rough idea.

If we require anyone using vmlinux.h to start adding extra compilation flags, it's going to be a nightmare-ish experience both for core BPF developers explaining all this, as well as end-users. This bpf_dominating_decl attribute is going to be used together with preserve_access_index attribute and BPF CO-RE technology. That attribute explicitly states that BPF program code knows that the actual kernel memory layout might be different and Clang emits additional CO-RE relocation to help BPF loaders (e.g., libbpf) to adjust the final BPF code accordingly.

I understand the concern when some typedef T changes from int to double, as an example. But that's never going to be the case in practice. So compiler diagnostic in this case totally makes sense. But that can't be easily translated to structs, because they are usually accessed and passed by pointer. So in that case the end size of the struct doesn't matter. And then preserve_access_index will take care of relocating field offsets.

So with that said, if we had to have some diagnostics, can we error out or warn only in places where the size of the type (be that a primitive integer type or struct passed by value or through varargs) matters and determines things like stack layout? That would be actually useful as that will warn users that whatever they are trying to do isn't CO-RE-relocatable?

[yonghong-song]

@aaron.ballman Just to make it clear that you might misunderstand the scope of this attribute. It is implemented as a BPF target attribute, so the attribute will not be available for non-BPF targets.

[yonghong-song]

I second Andrii's concern about option as the option will need to be in every user's makefile.
In current vmlinux.h, we have something like below:

#ifndef BPF_NO_PRESERVE_ACCESS_INDEX
#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
#endif

Basically if user does not want to have preserve_access_index attribute (which is a bpf target attribute),
user can add -DBPF_NO_PRESERVE_ACCESS_INDEX to their Makefile.

Similarly, if we have bpf_dominating_decl attribute on by default, in vmlinux.h, we will have

#ifndef BPF_NO_DOMINATING_DECL
#pragma clang attribute push (__attribute__((bpf_dominating_decl)), apply_to = record, typedef, enum)
#endif

this way, we provide a way to opt out bpf_dominating_decl attr but we expect virtually no user will do that.

[yonghong-song]

@aaron.ballman @anakryiko and I added some additional context. Could you take a look and share your comments? Thanks!

[aaron.ballman]

In D111307#3135521, @yonghong-song wrote:

@aaron.ballman @anakryiko and I added some additional context. Could you take a look and share your comments? Thanks!

I can, but I'm in WG14 (C) standards meetings all week this week and on vacation next week, so my availability for code reviews is pretty limited for the next while.

@aaron.ballman Just to make it clear that you might misunderstand the scope of this attribute. It is implemented as a BPF target attribute, so the attribute will not be available for non-BPF targets.

I understood that, but my concern is that this attribute is a nonconforming extension to C and Clang tries to be a conforming compiler. I would like to try to find a path forward so that we meet the conformance requirements for C in a responsible way. Opting into a target doesn't seem sufficient to me (I don't think any other targets currently work this way); usually a feature flag is used to opt into a nonconforming mode we don't diagnose the use of. Another alternative is to diagnose the uses but that could be onerous for users who opt into the target. There may be more alternatives we could explore as well, but I don't have the bandwidth to try to enumerate them at the moment.

[yonghong-song]

@aaron.ballman thanks for the comment. now I understand. The main concern is about outputting diagnostics about nonconforming extension unless an explicit option says not needed.

The following is another suggestion. https://reviews.llvm.org/D69759 implemented a bpf target attribute attribute((preserve_access_index)). Currently it is used for record only. Its purpose to indicate a particular record declaration is subject to bpf-load-time relocation.

How about we extend preserve_access_index to enum and typedef declarations too. For enumerator, we will actually generate a relocation since it is one of goals too. For typedef, there is no relocation generated *so far*.

With preserve_access_index attribute supporting record, enum and typedef, in this patch whenever a bpf_dominating_decl is seen, it also check whether a preserve_access_index attribute is available for that declaration. If it is, there is no need to output a diagnostic since preserve_access_index already says the declaration might be different from real definition and relocation should take care of it. If there is no preserve_access_index attribute attached, issue a diagnostic.

WDYT?

[yonghong-song]

@aaron.ballman Just ping. Did you get time to think about diagnostics issue and my above linking-to-bpf-preserve_access_index-attr idea?

[yonghong-song]

@aaron.ballman just ping, did you get time to look at this patch again?

[aaron.ballman]

In D111307#3182955, @yonghong-song wrote:

@aaron.ballman just ping, did you get time to look at this patch again?

I did, thank you for your patience!

With preserve_access_index attribute supporting record, enum and typedef, in this patch whenever a bpf_dominating_decl is seen, it also check whether a preserve_access_index attribute is available for that declaration. If it is, there is no need to output a diagnostic since preserve_access_index already says the declaration might be different from real definition and relocation should take care of it. If there is no preserve_access_index attribute attached, issue a diagnostic.

I think this could work -- at least, I cannot think of an alternative approach I like better.

That said, I'm still very uncomfortable with this notion, but because this is used within the Linux kernel I think this is a case where I likely need to hold my nose and accept that you have a real need for this. My discomfort is solely around the fact that this is using an attribute to effectively invent a new C-like language because you're ignoring a pretty central tenant of the source language. However, the user has to opt into the target explicitly, the attribute is limited to use within system headers, and will now be diagnosed when used unless used in conjunction with another attribute... so there are plenty of guard rails on this feature. I've added a few more reviewers to see if anyone else has discomfort that rises to the level of "we should not accept this feature" or has other ideas on how to protect the feature.

clang/docs/LanguageExtensions.rst

3284–3285

This isn't clear enough -- it doesn't really say what happens in a situation where a definition has already been seen, just where a subsequent definition is seen. e.g.,

struct S { float f; };
struct __attribute__((bpf_dominating_decl)) S { int a; }; // What happens here?

I think we want this to be crystal clear that the bpf_dominating_decl definition is the one which always wins, and there can only ever be ONE such definition with that attribute. e.g., this is not valid:

struct __attribute__((bpf_dominating_decl)) S { int a; };
struct __attribute__((bpf_dominating_decl)) S { float f; };

(We should also have test cases for all the different permutations of multiple definitions with/without the attribute.)

clang/include/clang/Basic/AttrDocs.td

2007–2011

I think this documentation should also be expanded to cover at least as much information as is in the language extensions document.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3527

I'd rather this be reversed to attribute %0 may only be used within a system header.

clang/lib/Sema/SemaDecl.cpp

16181

18071

[rjmccall]

When dealing with some gross situation where a source generator is generating things that don't really work cleanly with the normal language, I have a strong preference for saying that the source generator should change. In this case, that would mean e.g. generating different type names in the generated headers. Is that really such a non-starter? This is really quite a terrible feature.

[anakryiko]

In D111307#3190832, @rjmccall wrote:

When dealing with some gross situation where a source generator is generating things that don't really work cleanly with the normal language, I have a strong preference for saying that the source generator should change. In this case, that would mean e.g. generating different type names in the generated headers. Is that really such a non-starter? This is really quite a terrible feature.

When our tooling is generating vmlinux.h, which contains all kernel types (include down to the most primitive typedefs like __u32), we don't know what other headers users will be including along the vmlinux.h. So it's impossible to generate this header in a way that won't conflict with types and typedefs defined in kernel headers. I'd love to be able to do this without introducing new attributes/built-ins, but we never figured out any other way to achieve that. Yet the problem of (impossibility of) combining vmlinux.h with other kernel headers is quite acute and painful and was brought up by users multiple times already.

Generating different type names for every single typedef/struct/union/enum/enum value is really horrible hack from end-user usability point of view. BPF program code will be littered with struct task_struct___1, struct pt_regs___1 and alike everywhere, hurting code readability and maintainability significantly, negating most of the appeal and benefits of using auto-generated vmlinux.h header.

Given this new dominating_decl attribute is:

1. BPF-only (guarded by -target bpf at compiler invocation site)
2. and is only allowed to be used with BPF-only __attribute__((preserve_access_index))
3. which is a statement of user's consent to BPF CO-RE compiler emitting CO-RE relocations
4. which are later used by BPF loader library to adjust generated assembly code at loading time to accommodate struct memory layout changes between what's declared in BPF program (types coming from vmlinux.h) versus what is the actual memory layout of the running Linux kernel (which we get from kernel's BTF type information)
5. and so the user expects that all such types are just a "declaration of intent" and in no way is the actual memory layout that should be used...

it seems there are all the guardrails we could have come up with and have them implemented in a reasonable way. There will definitely be very little, if any, surprise on user's side. And we'll definitely make sure to emphasize that user's should assume the exact memory layout of those types coming from vmlinux.h (we do that already in various blog posts, but never hurts to re-emphasize all that again).

But in short, this is a really crucial feature for great end-user usability for BPF and BPF CO-RE. So while I understand from pure C language stand point how dangerous this might seem, in the case of BPF-specific C language extensions and BPF core all this is par for the course by now and will be even more so going forward.

[rjmccall]

It sounds like this work you're doing for your target is in essence a target-specific language extension. Have you gone through our standard procedure for contributing extensions? I don't think being target-specific changes our policies here.

The procedure is basically to start an RFC thread on cfe-dev with some specific information. It's all described here:

https://clang.llvm.org/get_involved.html

This isn't a "no"; it's a "I need to know more about your project". I honestly don't know much about BPF. Please CC me on that thread when you make it.

[yonghong-song]

@rjmccall We actually have a discussion in cfe-dev list. https://lists.llvm.org/pipermail/cfe-dev/2021-August/068696.html which contains the background information for this implementation. This patch has evolved a little further from the discussion (e.g., name change, become target specific, proposed to coordinate with bpf preserve_access_index attribute). But the cfe-dev discussion should give you some information about why we want to do this feature and how it helped the bpf community. Please take a look and let us know if you have any questions. Thanks!

[rjmccall]

I've skimmed that thread, and it doesn't look like there was ever a step where you actually "officially" went through the process of asking Clang to support the extensions you want in order to support BPF / CO-RE. It seems like there's already an attribute in place that causes Clang to emit accesses to fields using a non-static offset. Do you know how long that's been there? Did you go through the process before adding that?

There are a lot of reasons going through the process is important. We need to understand the overall work you're trying to achieve, how large your body of users is, how committed you are to maintaining this, and so on. All of this is laid out in the extensions policy. As an open-source project, we have a lot of people coming to us with research ideas that they think would be valuable to upstream into the compiler. We have this policy in place so that we can decide what we should accept and, potentially, what we should later remove if people don't live up to their commitments.

[yonghong-song]

In D111307#3198862, @rjmccall wrote:

I've skimmed that thread, and it doesn't look like there was ever a step where you actually "officially" went through the process of asking Clang to support the extensions you want in order to support BPF / CO-RE. It seems like there's already an attribute in place that causes Clang to emit accesses to fields using a non-static offset. Do you know how long that's been there? Did you go through the process before adding that?

For the original process_access_index attribute, there is no discussion in llvm-dev and cfe-dev. I just submitted the patch and we just discussed there. Nobody suggested us to initiate a mailing list discussion. Otherwise, we would follow the suggestion to discuss in the mailing list. The feature has landed in H2 2019 and the feature is used by virtually every bpf deverloper or bpf application. The feature will be supported forever. The following are some of posts/blogs mentioning CO-RE:

https://nakryiko.com/posts/bpf-core-reference-guide/
https://www.brendangregg.com/blog/2020-11-04/bpf-co-re-btf-libbpf.html
https://blogs.oracle.com/linux/post/bpf-application-development-and-libbpf 
https://pingcap.com/blog/why-we-switched-from-bcc-to-libbpf-for-linux-bpf-performance-analysis

There are a lot of reasons going through the process is important. We need to understand the overall work you're trying to achieve, how large your body of users is, how committed you are to maintaining this, and so on. All of this is laid out in the extensions policy. As an open-source project, we have a lot of people coming to us with research ideas that they think would be valuable to upstream into the compiler. We have this policy in place so that we can decide what we should accept and, potentially, what we should later remove if people don't live up to their commitments.

For bpf_dominating_decl attribute, I am happy to follow the process. Please let me know what else I should be beyond discussion in cfe-dev mailing list. I searched "clang extension policy" in the internet and didn't get a meaningful result.

[rjmccall]

In D111307#3199088, @yonghong-song wrote:

In D111307#3198862, @rjmccall wrote:

I've skimmed that thread, and it doesn't look like there was ever a step where you actually "officially" went through the process of asking Clang to support the extensions you want in order to support BPF / CO-RE. It seems like there's already an attribute in place that causes Clang to emit accesses to fields using a non-static offset. Do you know how long that's been there? Did you go through the process before adding that?

For the original process_access_index attribute, there is no discussion in llvm-dev and cfe-dev. I just submitted the patch and we just discussed there. Nobody suggested us to initiate a mailing list discussion. Otherwise, we would follow the suggestion to discuss in the mailing list. The feature has landed in H2 2019 and the feature is used by virtually every bpf deverloper or bpf application. The feature will be supported forever. The following are some of posts/blogs mentioning CO-RE:

https://nakryiko.com/posts/bpf-core-reference-guide/
https://www.brendangregg.com/blog/2020-11-04/bpf-co-re-btf-libbpf.html
https://blogs.oracle.com/linux/post/bpf-application-development-and-libbpf 
https://pingcap.com/blog/why-we-switched-from-bcc-to-libbpf-for-linux-bpf-performance-analysis

There are a lot of reasons going through the process is important. We need to understand the overall work you're trying to achieve, how large your body of users is, how committed you are to maintaining this, and so on. All of this is laid out in the extensions policy. As an open-source project, we have a lot of people coming to us with research ideas that they think would be valuable to upstream into the compiler. We have this policy in place so that we can decide what we should accept and, potentially, what we should later remove if people don't live up to their commitments.

For bpf_dominating_decl attribute, I am happy to follow the process. Please let me know what else I should be beyond discussion in cfe-dev mailing list. I searched "clang extension policy" in the internet and didn't get a meaningful result.

I linked to it above. It's at the bottom of this web page: https://clang.llvm.org/get_involved.html. Just try to cover the points listed there; some of them, like standards bodies, don't apply.

I agree that Clang is highly unlikely to remove its existing support for BPF CO-RE, but please go through this exercise as if all of the support for CO-RE were under review, not just this specific feature. (You can mention that some of this support is already present.) For example, you should talk about the need to have a stable ABI despite the kernel not having one; why users need to include headers with substantially different views of the same types; and any other technical challenges that your environment faces that you think compiler support could help with.

I want to emphasize that this isn't an adversarial process. It sounds like your project really does deserve support in the compiler. A lot of the goal here is for generalist compiler developers, whether reviewing your patches or just doing arbitrary other work on the compiler in ways that touch your code, to have a resource we can turn to to understand enough about your project to both (1) have a good idea of how to resolve questions that come up and (2) know who to reach out to if we need clarification.

[yonghong-song]

@rjmccall Thanks for link and detailed explanation. I will write clang extension request, based on the requirements in the link, for both already-merged CO-RE feature and newer bpf_dominating_del attribute feature and posted to cfe-dev mailing list in the next few days.

[rjmccall]

I appreciate it, thanks.

clang/docs/LanguageExtensions.rst

_foo:
           movl    %gs:(%eax), %eax
           ret
 
 You can also use the GCC compatibility macros ``__seg_fs`` and ``__seg_gs`` for
 the same purpose. The preprocessor symbols ``__SEG_FS`` and ``__SEG_GS``
 indicate their support.
 
 PowerPC Language Extensions
 ---------------------------
 
 Set the Floating Point Rounding Mode
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 PowerPC64/PowerPC64le supports the builtin function ``__builtin_setrnd`` to set
 the floating point rounding mode. This function will use the least significant
 two bits of integer argument to set the floating point rounding mode.
 
 .. code-block:: c++

 
 **Example of Use**:
 
 .. code-block:: c
 
   int a = 1;
   __builtin_dcbf (&a);
 
+
+BPF Language Extensions
+-----------------------
+
+Extensions provided for BPF targets.
+
+Dominating Type Declaration
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Clang provides a ``bpf_dominating_decl`` attribute that can be used to
+mark structure, union and enumeration definitions, and typedef
+declarations as dominating declarations. A subsequent (re)definition
+will be ignored, rather than emit a diagnostic. The two definitions
+do not need to be compatible.
+
+The contents of bpf's ``vmlinux.h`` machine-generated header file can
+be decorated with this attribute, and then subsequent kernel headers
+that also define the same types or typedefs can be included without
+diagnostic.
+
+.. code-block:: c
+
+  #pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                               apply_to = any(record, enum, type_alias))
+  #include <vmlinux.h>
+  #pragma clang attribute pop
+  #include <linux/socket.h>
+
+This attribute breaks the expectation that an attribute does not
+change the well-formedness of source code. This attribute explicitly
+permits multiple definitions of the same structure, union or
+enumeration, or conflicting typedef declarations.
+
 Extensions for Static Analysis
 ==============================
 
 Clang supports additional attributes that are useful for documenting program
 invariants and rules for static analysis tools, such as the `Clang Static
 Analyzer <https://clang-analyzer.llvm.org/>`_. These attributes are documented
 in the analyzer's `list of source-level annotations
 <https://clang-analyzer.llvm.org/annotations.html>`_.

clang/include/clang/Basic/Attr.td

 
 def AMDGPUNumVGPR : InheritableAttr {
   let Spellings = [Clang<"amdgpu_num_vgpr", 0>];
   let Args = [UnsignedArgument<"NumVGPR">];
   let Documentation = [AMDGPUNumSGPRNumVGPRDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "kernel functions">;
 }
 
+def BPFDominating : Attr, TargetSpecificAttr<TargetBPF> {
+  let Spellings = [Clang<"bpf_dominating_decl">];
+  let Subjects = SubjectList<[Record, TypedefName, Enum], ErrorDiag>;
+  let Documentation = [BPFDominatingDeclDocs];
+  let LangOpts = [COnly];
+}
+
 def BPFPreserveAccessIndex : InheritableAttr,
                              TargetSpecificAttr<TargetBPF>  {
   let Spellings = [Clang<"preserve_access_index">];
   let Subjects = SubjectList<[Record], ErrorDiag>;
   let Documentation = [BPFPreserveAccessIndexDocs];
   let LangOpts = [COnly];
 }
 

clang/include/clang/Basic/AttrDocs.td

- If the CPU is not M-class, the return instruction is changed to one of the
 
   Unfortunately the compiler is unable to make this guarantee for an "UNDEF"
   handler, where the offset from "lr" to the preferred return address depends on
   the execution state of the code which generated the exception. In this case
   a sequence equivalent to "movs pc, lr" will be used.
   }];
 }
 
+def BPFDominatingDeclDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+Clang provides a ``__attribute__((bpf_dominating_decl)`` attribute for
+the BPF target. The attribute can be used to mark structure, union and
+enumeration definitions, and typedef declarations as dominating
+declarations. A subsequent (re)definition will be ignored, rather than
+emit a diagnostic. The two definitions do not need to be compatible.
+}];
+}
+
 def BPFPreserveAccessIndexDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Clang supports the ``__attribute__((preserve_access_index))``
 attribute for the BPF target. This attribute may be attached to a
 struct or union declaration, where if -g is specified, it enables
 preserving struct or union member access debuginfo indices of this
 struct or union, similar to clang ``__builtin_preserve_access_index()``.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_cannot_use_trivial_abi : ExtWarn<
   "'trivial_abi' cannot be applied to %0">, InGroup<IgnoredAttributes>;
 def note_cannot_use_trivial_abi_reason : Note<
   "'trivial_abi' is disallowed on %0 because %select{"
   "its copy constructors and move constructors are all deleted|"
   "it is polymorphic|"
   "it has a base of a non-trivial class type|it has a virtual base|"
   "it has a __weak field|it has a field of a non-trivial class type}1">;
 
+def err_attribute_not_system_header : Error<
+  "attribute %0 cannot appear outside of system headers">;
+
 // Availability attribute
 def warn_availability_unknown_platform : Warning<
   "unknown platform %0 in availability macro">, InGroup<Availability>;
 def warn_availability_version_ordering : Warning<
   "feature cannot be %select{introduced|deprecated|obsoleted}0 in %1 version "
   "%2 before it was %select{introduced|deprecated|obsoleted}3 in version %4; "
   "attribute ignored">, InGroup<Availability>;
 def warn_mismatched_availability: Warning<

clang/lib/Sema/SemaDecl.cpp

NamedDecl *Sema::LazilyCreateBuiltin(IdentifierInfo *II, unsigned ID,
   // entirely, but we're not there yet.
   DeclContext *SavedContext = CurContext;
   CurContext = New->getDeclContext();
   PushOnScopeChains(New, TUScope);
   CurContext = SavedContext;
   return New;
 }
 
 /// Typedef declarations don't have linkage, but they still denote the same
 /// entity if their types are the same.
 /// FIXME: This is notionally doing the same thing as ASTReaderDecl's
 /// isSameEntity.
 static void filterNonConflictingPreviousTypedefDecls(Sema &S,
                                                      TypedefNameDecl *Decl,
                                                      LookupResult &Previous) {
   // This is only interesting when modules are enabled.
   if (!S.getLangOpts().Modules && !S.getLangOpts().ModulesLocalVisibility)
     return;
 
   // Empty sets are uninteresting.
   if (Previous.empty())
     return;
 
   LookupResult::Filter Filter = Previous.makeFilter();
   while (Filter.hasNext()) {
     NamedDecl *Old = Filter.next();
if (NewType->isVariablyModifiedType()) {
     New->setInvalidDecl();
     return true;
   }
 
   if (OldType != NewType &&
       !OldType->isDependentType() &&
       !NewType->isDependentType() &&
       !Context.hasSameType(OldType, NewType)) {
+    if (Old->hasAttr<BPFDominatingAttr>()) {
+      // Silently ignore, we like playing with fire.
+      New->setInvalidDecl();
+      return true;
+    }
     int Kind = isa<TypeAliasDecl>(Old) ? 1 : 0;
     Diag(New->getLocation(), diag::err_redefinition_different_typedef)
       << Kind << NewType << OldType;
     if (Old->getLocation().isValid())
       notePreviousDefinition(Old, New->getLocation());
     New->setInvalidDecl();
     return true;
   }
if (TagDecl *PrevTagDecl = dyn_cast<TagDecl>(PrevDecl)) {
                   return Def;
                 } else {
                   SkipBody->ShouldSkip = true;
                   SkipBody->Previous = Def;
                   makeMergedDefinitionVisible(Hidden);
                   // Carry on and handle it like a normal definition. We'll
                   // skip starting the definitiion later.
                 }
+              } else if (SkipBody && Def->hasAttr<BPFDominatingAttr>()) {
+                // Don't even care about checking -- just ignore the new def
+                SkipBody->ShouldSkip = true;
+                SkipBody->Previous = Def;
               } else if (!IsExplicitSpecializationAfterInstantiation) {
                 // A redeclaration in function prototype scope in C isn't
                 // visible elsewhere, so merely issue a warning.
                 if (!getLangOpts().CPlusPlus && S->containedInPrototypeScope())
                   Diag(NameLoc, diag::warn_redefinition_in_param_list) << Name;
                 else
                   Diag(NameLoc, diag::err_redefinition) << Name;
                 notePreviousDefinition(Def,
if (!TheEnumDecl->isScoped() && isa<ValueDecl>(PrevDecl)) {
       CheckShadow(New, PrevDecl, R);
     }
 
     // When in C++, we may get a TagDecl with the same name; in this case the
     // enum constant will 'hide' the tag.
     assert((getLangOpts().CPlusPlus || !isa<TagDecl>(PrevDecl)) &&
            "Received TagDecl when not in C++!");
     if (!isa<TagDecl>(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S)) {
-      if (isa<EnumConstantDecl>(PrevDecl))
-        Diag(IdLoc, diag::err_redefinition_of_enumerator) << Id;
-      else
-        Diag(IdLoc, diag::err_redefinition) << Id;
+      auto Msg = diag::err_redefinition;
+      if (isa<EnumConstantDecl>(PrevDecl)) {
+        if (cast<EnumDecl>(PrevDecl->getDeclContext())
+                ->hasAttr<BPFDominatingAttr>()) {
+          // We can't completely bail here, as that would disturb the [default]
+          // value of a following enumerator
+          New->setInvalidDecl();
+          return New;
+        }
+        Msg = diag::err_redefinition_of_enumerator;
+      }
+      Diag(IdLoc, Msg) << Id;
       notePreviousDefinition(PrevDecl, IdLoc);
       return nullptr;
     }
   }
 
   // Process attributes.
   ProcessDeclAttributeList(S, New, Attrs);
   AddPragmaAttributes(S, New);

clang/lib/Sema/SemaDeclAttr.cpp

static void handleAVRSignalAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   }
 
   if (!AL.checkExactlyNumArgs(S, 0))
     return;
 
   handleSimpleAttribute<AVRSignalAttr>(S, D, AL);
 }
 
+static void handleBPFDominatingAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  // Restrict dominating decl to system header files, because, danger!
+  if (!S.Context.getSourceManager().isInSystemHeader(D->getLocation())) {
+    S.Diag(AL.getLoc(), diag::err_attribute_not_system_header) << AL;
+    return;
+  }
+  D->addAttr(::new (S.Context) BPFDominatingAttr(S.Context, AL));
+}
+
 static void handleBPFPreserveAIRecord(Sema &S, RecordDecl *RD) {
   // Add preserve_access_index attribute to all fields and inner records.
   for (auto D : RD->decls()) {
     if (D->hasAttr<BPFPreserveAccessIndexAttr>())
       continue;
 
     D->addAttr(BPFPreserveAccessIndexAttr::CreateImplicit(S.Context));
     if (auto *Rec = dyn_cast<RecordDecl>(D))
case ParsedAttr::AT_AMDGPUNumSGPR:
     handleAMDGPUNumSGPRAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AMDGPUNumVGPR:
     handleAMDGPUNumVGPRAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AVRSignal:
     handleAVRSignalAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_BPFDominating:
+    handleBPFDominatingAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_BPFPreserveAccessIndex:
     handleBPFPreserveAccessIndexAttr(S, D, AL);
     break;
   case ParsedAttr::AT_BTFDeclTag:
     handleBTFDeclTagAttr(S, D, AL);
     break;
   case ParsedAttr::AT_WebAssemblyExportName:
     handleWebAssemblyExportNameAttr(S, D, AL);

clang/test/Misc/bpf-tantdyalf.c

+// RUN: %clang_cc1 -triple=bpfel -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple=i686 -verify -fsyntax-only %s
+
+// BPF experimental hack
+// These Are Not The Duplicates You Are Looking For
+
+#if !INCLUDE
+#define INCLUDE 1
+#include __FILE__
+#else
+
+#if __bpf__
+
+// expected-error@+1{{outside of system headers}}
+#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                             apply_to = any(record, enum, type_alias))
+struct bad_bob { // expected-note{{this declaration}}
+  int a;
+};
+#pragma clang attribute pop
+
+#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;
+  };
+};
+void f() {
+  struct x {
+    int i;
+  };
+}
+#pragma clang attribute pop
+struct bob {
+  int a;
+};
+
+#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                             apply_to = any(record, enum, type_alias))
+struct bill {
+  int a;
+};
+#pragma clang attribute pop
+struct bill {
+  // <img src="this is fine dog"/>
+  long b;
+  float x;
+};
+
+#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                             apply_to = any(record, enum, type_alias))
+typedef unsigned bob;
+#pragma clang attribute pop
+typedef int bob; // <img src="this is fine dog"/>
+
+// bob should be unsigned
+char ary1[(2 * ((bob)-1 > 0)) - 1];
+
+#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                             apply_to = any(record, enum, type_alias))
+enum X { A };
+#pragma clang attribute pop
+enum X { A };
+
+#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                             apply_to = any(record, enum, type_alias))
+enum { B };
+#pragma clang attribute pop
+enum { B,
+       C }; // OK
+
+int ary2[2 * (C == 1) - 1];
+
+enum { A }; // also OK
+
+#else
+
+// expected-warning@+1{{unknown attribute}}
+#pragma clang attribute push(__attribute__((bpf_dominating_decl)), \
+                             apply_to = any(record, enum, type_alias))
+// expected-note@+1{{when applied to}}
+struct bob { // expected-note{{previous definition}}
+  int a;
+};
+#pragma clang attribute pop
+struct bob { // expected-error{{redefinition}}
+  int a;
+};
+
+#endif
+#endif

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: AlwaysInline (SubjectMatchRule_function)
 // CHECK-NEXT: Annotate ()
 // CHECK-NEXT: AnyX86NoCfCheck (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: ArcWeakrefUnavailable (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ArmBuiltinAlias (SubjectMatchRule_function)
 // CHECK-NEXT: AssumeAligned (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: Assumption (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Availability ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
+// CHECK-NEXT: BPFDominating (SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_enum)
 // CHECK-NEXT: BPFPreserveAccessIndex (SubjectMatchRule_record)
 // CHECK-NEXT: BTFDeclTag (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record, SubjectMatchRule_field, SubjectMatchRule_type_alias)
 // CHECK-NEXT: BuiltinAlias (SubjectMatchRule_function)
 // CHECK-NEXT: CFAuditedTransfer (SubjectMatchRule_function)
 // CHECK-NEXT: CFConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: CFICanonicalJumpTable (SubjectMatchRule_function)
 // CHECK-NEXT: CFUnknownTransfer (SubjectMatchRule_function)
 // CHECK-NEXT: CPUDispatch (SubjectMatchRule_function)