Add kernel.org's .clang-format for editor-agnostic C formatting hints

[bleggett]

clang-format is an editor-agnostic means of describing C formatting prefs for code editors.

Since upstream kernel uses this to define an explicit contract for C formatting, and our
BPF code follows the kernel style, this just brings in a copy of the kernel .clang-format dotfile.

This will get picked up by any editor that supports it (VSCode w/ C extension, emacs with lsp, vims, etc).

This will declare a specific contract for contributors around C style, as well as help this project explicitly declare any deviations from that style.

This is a straight copy of the kernel.org file, with a kernel-specific macro allowlist removed.

[qmonnet]

Thanks for this!

I know we've discussed using clang-format before, but we never really reached a consensus. Sadly, Slack history is not persistent and these discussions are lost.

For my part, I'd be happy to defer formatting to clang-format, but last time I checked it would format some elements in a strange way, even with the kernel's config. This is maybe just a matter of fine-tuning the correct options, but I haven't spent time on that.

kernel uses this to define an explicit contract for C formatting, and our BPF code follows the kernel style

Out of curiosity, do you have a reference for this? My understanding was that the config file was here to help, but the style is not enforced and I don't think it's even considered as an absolute reference, more like a best-effort config? If anything, checkpatch.pl is the one that runs in BPF or Netdev CIs.

This will get picked up by any editor that supports it (VSCode w/ C extension, emacs with lsp, vims, etc).

What does it do? Does it format the new code accordingly, or does it reformat the entire file on save? The former sounds good, the latter not that much - the current state of Cilium's code formatting is too far from what clang-format does and that would mess up with commits?

This will declare a specific contract for contributors around C style, as well as help this project explicitly declare any deviations from that style.

Yes, this intent is good.

This is a straight copy of the kernel.org file, with a kernel-specific macro allowlist removed.

So a few more questions:

• Did you run it on the current sources and did you find anything weird-looking? On a quick look, I note at least the following:

  • When declaring a function, it always moves the return type to the same line as the function name, when we usually have it on a separate line if the type is long (which is often the case given that we've got plenty of static __always_inline ...).
  • It always remove spaces between # and preprocessor directives (# ifdef), but these spaces are sometimes used to visually “indent” nested directives.
  • It struggles with code indent for blocks that are under #ifdefs, I've spotted several locations where it would bring it all the way to the left instead of keeping the indent from the current context (Note: I realised afterwards that I used clang-format from LLVM 10, this might have improved since? TBC).
  • Likely more things to say, but I haven't done an exhaustive pass.

• Related question: Is there a way to turn off clang-format locally? I know some linters support this. I understand you're not proposing to enforce the style in CI checks yet, but this will inevitably come up at some point if we start using clang-format on a regular basis.

• When this time comes and we finally consider it for CI, we'd need to sort out how it plays with the current checkpatch validation (how to solve conflicts between the two, and so on).

• Tetragon uses clang-format, it might be worth looking at the amendments they did to the kernel config file to see if there's anything relevant to Cilium, as well.

TL;DR: Happy with the idea of using clang-format for Cilium, but I've got concerns on how exactly we'd use it, and with some elements that are not formatted correctly.

Cc @aditighag @christarazi who were involved in previous discussions, if I remember correctly.

[bleggett]

For my part, I'd be happy to defer formatting to clang-format, but last time I checked it would format some elements in a strange way, even with the kernel's config. This is maybe just a matter of fine-tuning the correct options, but I haven't spent time on that.

kernel uses this to define an explicit contract for C formatting, and our BPF code follows the kernel style

Out of curiosity, do you have a reference for this? My understanding was that the config file was here to help, but the style is not enforced and I don't think it's even considered as an absolute reference, more like a best-effort config? If anything, checkpatch.pl is the one that runs in BPF or Netdev CIs.

checkpatch is the actual enforced contract (for the kernel and here) yes - but it helps to have a declarative contract that editors understand so you don't have to wait until you run up against checkpatch.

So I'm not proposing we replace checkpatch CI checks with clang-format (at least not until/unless kernel tree does) - better to have both. One is to make local dev easier, the other is a CI gate.

This will get picked up by any editor that supports it (VSCode w/ C extension, emacs with lsp, vims, etc).

What does it do? Does it format the new code accordingly, or does it reformat the entire file on save? The former sounds good, the latter not that much - the current state of Cilium's code formatting is too far from what clang-format does and that would mess up with commits?

It works like editorconfig and configures editor formatting rules. Whenever your editor is told to format C code in this repo or subfolders, if it is configured to use the clang-format tool it will try to do so according to the rules in .clang-format. How and where you choose to apply formatting is a local thing, and for all the editors I know of, auto-reformatting is not the default behavior.

It would be opt-in/only apply if a manual reformat was triggered. I find it helpful since I already use LSP-based extensions and largely don't want to spend time on manual formatting. Someone would have to manually reformat every C file in the repo for this to apply globally, and that's certainly not the intent.

So a few more questions:

• Did you run it on the current sources and did you find anything weird-looking?

Not on all files, but yeah I did notice some stylistic differences. Figured I'd raise this PR and see what people thought about it.

• If there's something that breaks checkpatch, that is something to fix - and anyway checkpatch will catch those corner cases which can be fixed manually, so it's exactly as safe as the existing flow. And if people have non-clang-format based formatting tools/extensions, they can keep using those and ignore this.

• If there's something that doesn't break checkpatch but is a stylistic change from current convention, we should either make .clang-format respect it via an explicit rule, or accept the default - either option is useful.

On a quick look, I note at least the following:

• When declaring a function, it always moves the return type to the same line as the function name, when we usually have it on a separate line if the type is long (which is often the case given that we've got plenty of static __always_inline ...).
• It always remove spaces between # and preprocessor directives (# ifdef), but these spaces are sometimes used to visually “indent” nested directives.
• It struggles with code indent for blocks that are under #ifdefs, I've spotted several locations where it would bring it all the way to the left instead of keeping the indent from the current context (Note: I realised afterwards that I used clang-format from LLVM 10, this might have improved since? TBC).
• Likely more things to say, but I haven't done an exhaustive pass.

TY, I'll look into these and see if they can be explicitly configured.

• Related question: Is there a way to turn off clang-format locally? I know some linters support this. I understand you're not proposing to enforce the style in CI checks yet, but this will inevitably come up at some point if we start using clang-format on a regular basis.

Turn off your editor's clang-format extension (or extension option) - or just locally delete .clang-format if you don't want it to use these rules. Again - pretty much the same as editorconfig.

This is purely opt-in locally for everybody, except for people that might have already been using a plugin that makes use of clang-format and was using some other set of defaults, and didn't realize it.

• When this time comes and we finally consider it for CI, we'd need to sort out how it plays with the current checkpatch validation (how to solve conflicts between the two, and so on).

Yeah as mentioned above - don't wanna do that until kernel tree does the same. In the meantime this is purely a local dev aid to help avoid roundtrips against checkpatch failures.

• Tetragon uses clang-format, it might be worth looking at the amendments they did to the kernel config file to see if there's anything relevant to Cilium, as well.

Oh nice I missed that, will take a look.

TL;DR: Happy with the idea of using clang-format for Cilium, but I've got concerns on how exactly we'd use it, and with some elements that are not formatted correctly.

Thanks for the feedback! I figured that would be the case and it'd be worth discussing. I'll see if I can address those concerns.

[qmonnet]

checkpatch is the actual enforced contract (for the kernel and here) yes - but it helps to have a declarative contract that editors understand so you don't have to wait until you run up against checkpatch.

So I'm not proposing we replace checkpatch CI checks with clang-format (at least not until/unless kernel tree does) - better to have both. One is to make local dev easier, the other is a CI gate.

OK, agreed

It works like editorconfig and configures editor formatting rules. Whenever your editor is told to format C code in this repo or subfolders, if it is configured to use the clang-format tool it will try to do so according to the rules in .clang-format. How and where you choose to apply formatting is a local thing, and for all the editors I know of, auto-reformatting is not the default behavior.

Which is good, OK thanks for clarifying

It would be opt-in/only apply if a manual reformat was triggered. I find it helpful since I already use LSP-based extensions and largely don't want to spend time on manual formatting. Someone would have to manually reformat every C file in the repo for this to apply globally, and that's certainly not the intent.

👍

[...] I did notice some stylistic differences. Figured I'd raise this PR and see what people thought about it.

• If there's something that breaks checkpatch, that is something to fix - and anyway checkpatch will catch those corner cases which can be fixed manually, so it's exactly as safe as the existing flow. And if people have non-clang-format based formatting tools/extensions, they can keep using those and ignore this.

• If there's something that doesn't break checkpatch but is a stylistic change from current convention, we should either make .clang-format respect it via an explicit rule, or accept the default - either option is useful.

Agreed. I guess whether we prefer to keep the current style or adopt clang-format's is something that we need to figure out on a case-by-case basis. The important thing for me at the moment is that by introducing the tool, it does not kinda force us to adopt all its suggestions. There are a few locations, especially when doing complex macro declarations, where elements don't follow the usual formatting rules, and we likely want to keep it that way.

TY, I'll look into these and see if they can be explicitly configured.

Yes please, this would be great!

Turn off your editor's clang-format extension (or extension option) - or just locally delete .clang-format if you don't want it to use these rules. Again - pretty much the same as editorconfig.

This is purely opt-in locally for everybody, except for people that might have already been using a plugin that makes use of clang-format and was using some other set of defaults, and didn't realize it.

OK. I meant more about turning it off for a portion of code if we ever adopt the tool in CI, and can't encode the style we want with clang-format options. I see it should be doable if needed: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code:

int formatted_code;
// clang-format off
    void    unformatted_code  ;
// clang-format on
void formatted_code_again;

Thanks for the feedback! I figured that would be the case and it'd be worth discussing. I'll see if I can address those concerns.

Yes, we've discussed it before and although we never got a consensus, I think it would be a good thing overall to agree on a style definition and get the .clang-format file merged.

Please do take a look at the Tetragon changes, and at whether it seems doable to address the style changes mentioned above, if you have a moment. Would be great to have something as close to the existing style as possible. Then it will be good to go as far as I'm concerned, although I'll try to mention it at the next community meeting (Wednesday), for awareness and to make sure nobody has major concerns about it (I don't expect there will be).

[bimmlerd]

I agree with Quentins comments; we should discuss this in the community meeting. In any case I think this deserves a more experienced pair of eyes from contributing, I'll rerequest review.

EDIT: heh - I guess given @qmonnet's review GitHub won't add another person. 🤷 let's see what the community meeting says.

[qmonnet]

Various comments and observations. I experimented a little, not something exhaustive.

(Additions or updates to the values we got from the kernel's file, if any, should likely go into a separate commit.)

[qmonnet]

We could use this:

Suggested change

	IndentPPDirectives: None
	IndentPPDirectives: AfterHash

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentppdirectives

Although this indents directives everywhere, including where we're not currently using them. Meaning that if, for example, we remove one level of nested #ifdefs, we have to update the indent for inner directives.

If using the above we'd also need to set PPIndentWidth, because it seems to default to IndentWidth but we don't want that:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#ppindentwidth

[bleggett]

Various comments and observations. I experimented a little, not something exhaustive.

(Additions or updates to the values we got from the kernel's file, if any, should likely go into a separate commit.)

My fedora defaults to clang-format 15 - some of the options you added are in 16 only.

I'm inclined to track (current version - 2) just to keep things simple - that seems to track with Ubuntu (and presumably other) distros as well.

[maintainer-s-little-helper]

Commit c5c90cc8be87287d61faa1051405dd91d4433091 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[maintainer-s-little-helper]

Commit c5c90cc8be87287d61faa1051405dd91d4433091 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[maintainer-s-little-helper]

Commit c5c90cc8be87287d61faa1051405dd91d4433091 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[qmonnet]

My fedora defaults to clang-format 15 - some of the options you added are in 16 only.

I'm inclined to track (current version - 2) just to keep things simple - that seems to track with Ubuntu (and presumably other) distros as well.

Makes sense. I didn't pay too much attention to the required version when looking at these options, so indeed it makes sense to leave them aside if they require something too recent.

Thanks!

[qmonnet]

Note: If we accept the file, this PR will need a related entry in CODEOWNERS (assigning reviews to @cilium/contributing, most likely).

I'm not sure why the bot fails to detect your sign-off in the commit description, I can see it. Let's see how it goes on the next PR update.

[bleggett]

I've bumped the CODEOWNERS as requested, and rolled in most of the changes requested - feel like it's in a good spot and we can tweak further once it's in place if everyone is good with this as a starting point.

[qmonnet]

I've bumped the CODEOWNERS as requested, and rolled in most of the changes requested - feel like it's in a good spot and we can tweak further once it's in place if everyone is good with this as a starting point.

Thanks!

Regarding IndentPPDirectives, I think I'd be in favour of the AfterHash value. We could update it later, but I'm thinking we might as well avoid losing the existing indent everywhere we touch the code, so might be worth updating in this PR? Depending on what you think of it, too.

Other than that, agreed, it's in a good shape and we can tweak further later if necessary.

One last thing before we go ahead: Could you please split the commit in two, one with the initial config file copied from the kernel (but without the macro allow-list, so just like in your first version), and then a second one that brings the changes we've agreed on? This way it will be easier to track down the changes we've added for Cilium. I'd also like to have the kernel commit (at which you picked the file) referenced in the description of the commit adding the initial file, so we can track changes on kernel's .clang-format if we ever need to.

[bleggett]

Regarding IndentPPDirectives, I think I'd be in favour of the AfterHash value. We could update it later, but I'm thinking we might as well avoid losing the existing indent everywhere we touch the code, so might be worth updating in this PR? Depending on what you think of it, too.

👍

One last thing before we go ahead: Could you please split the commit in two, one with the initial config file copied from the kernel (but without the macro allow-list, so just like in your first version), and then a second one that brings the changes we've agreed on? This way it will be easier to track down the changes we've added for Cilium. I'd also like to have the kernel commit (at which you picked the file) referenced in the description of the commit adding the initial file, so we can track changes on kernel's .clang-format if we ever need to.

Ah good call, that all makes sense, will do.

[bleggett]

Regarding IndentPPDirectives, I think I'd be in favour of the AfterHash value. We could update it later, but I'm thinking we might as well avoid losing the existing indent everywhere we touch the code, so might be worth updating in this PR? Depending on what you think of it, too.

👍

Hmm actually trying this on e.g. bpf_lxc.c it actually creates some odd-looking results with current code (e.g. line 73-76), so maybe we should leave it off for now?

[bleggett]

@qmonnet Commits re-organized as you asked, please let me know about IndentPPDirectives: AfterHash, it seems to mangle existing formatting a bit too much, but if you want it I will add it.

[qmonnet]

Looks all good, thanks a lot!

please let me know about IndentPPDirectives: AfterHash

OK let's leave it out for now, or let's see if others chime in. Hard to tell what option would be the best here anyway.

The checkpatch workflow reports that your commit subject is too long on your first commit, I think it's because you're just missing the empty line between the subject and the description (the link).

Note to other reviewers: This PR was discussed at the community meeting last Wednesday (2023-05-24) and the consensus was to add .clang-format, nobody raised any particular objections.

[bleggett]

The checkpatch workflow reports that your commit subject is too long on your first commit, I think it's because you're just missing the empty line between the subject and the description (the link).

Ah, missed that, fixed, ty!