Re: [RFC PATCH 0/5] clang-format for Xen

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Fri, Jul 28, 2023 at 09:11:39AM +0100, Luca Fancellu wrote:
> ## Introduction 
> ################################################################
> 
> In this serie, I would like to get feedbacks on the output generated by the
> configuration of clang-format, unfortunately we can't use only clang-format, 
> but
> we need to call it using a wrapper, because we need the information of what
> files need to be excluded from the tool.
> 
> Another reason is that clang-format has some limitation when formatting asm()
> instruction and most of the time it format them in a very ugly way or it 
> breaks
> the code for example removing spaces that were there for a reason (I don't 
> think
> it's a tool to format asm), so in the wrapper script we protect all asm()
> invocation or macros where there are asm() invocation with in-code comments 
> that
> stops clang-format to act on that section:
> 
> /* clang-format off */section/* clang-format on */
> 
> I've read the past threads about the brave people who dared to try to 
> introduce
> clang-format for the xen codebase, some of them from 5 years ago, two points
> were clear: 1) goto label needs to be indented and 2) do-while loops have the
> braket in the same line.
> While point 1) was quite a blocker, it seemd to me that point 2) was less
> controversial to be changed in the Xen codestyle, so the current wrapper 
> script
> handles only the point 1 (which is easy), the point 2 can be more tricky to
> handle.
> 
> ## The clang-format configuration 
> ##############################################
> 
> In my clang-format configuration I've taken inspiration from EPAM's work, then
> from the configuration in Linux and finally from the clang-format manual, to 
> try
> to produce a comprehensive configuration.
> 
> Every configuration parameter has on top a comment with the description and
> when it was supported, finally I've added also a [not specified] if that
> behavior is not clearly specified in the Xen coding style, I've done that so
> we could discuss about adding more specification in our CODING_STYLE.
> Every comment can be stripped out in the final release of the file, but I 
> think
> that now they are useful for the discussion.
> 
> The minimum clang-format version for the file is 15, my ubuntu 22.04 comes 
> with
> it, we can reason if it's too high, or if we could also use the latest version
> maybe shipped inside a docker image.
15 sounds ok \methinks. In practice we'll just stick it in GitLab and let
it check commits, so we'll be safe even in the case where every developer
has a slightly different version of the tool. I wouldn't try to make this
(hard) task harder by trying to retrofit it in an older version.

It might be worth stitching a flag in the python script to scream if the
clang-format version doesn't match. Or it may do a mess of the tree.

> 
> For every [not specified] behavior, I've tried to guess it from the codebase,
> I've seen that also in that case it's not easy as there is (sometimes) low
> consistency between modules, so we can discuss on every configurable.
> 
> Worth to mention, the public header are all excluded from the format tool,
> because formatting them breaks the build on X86, because there are scripts for
> auto-generation that don't handle the formatted headers, I didn't investigate
> on it, maybe it can be added as technical debt.
Sounds reasonable to me.

> 
> So I've tried building arm32, arm64 and x86_64 with the formatted output and
> they build, I've used Yocto for that.
> 
> ## How to try it? 
> ##############################################################
> 
> So how to generate everything? Just invoke the codestyle.py script without
> parameter and it will format every .c and .h file in the hypervisor codebase.
> 
> ./xen/scripts/codestyle.py
> 
> Optionally you can also pass one or more relative path from the folder you are
> invoking the script and it will format only them.
Worth mentioning that we need a strategy for checking the consistency as
well. The typical fashion is to invoke clang-format as:

  clang-format -Werrror --dry-run

Seeing that there's an external script we probably want some plan for
checking things in CI as well.

> 
> ## What I expect from this RFC 
> #################################################
> 
> I expect feedback on the output, some agreement on what configuration to use,
> and I expect to find possible blocker before working seriously on this serie,
> because if there are outstanding blockers on the adoption of the tool and we
> can't reach an agreement, I won't spend further time on it.
> 
> I understand that the release is coming soon and some people are on holiday in
> this period, so worst case I will ping again after the release.
> 
> Luca Fancellu (5):
>   [WIP]misra: add entries to the excluded list
>   [WIP]cppcheck: rework exclusion_file_list.py code
>   [WIP]xen/scripts: add codestyle.py script
>   x86/HVM: protect mm_type_tbl format from clang-format
>   xen: Add clang-format configuration
> 
>  docs/misra/exclude-list.json                  |  88 +++
>  xen/.clang-format                             | 693 ++++++++++++++++++
>  xen/arch/x86/hvm/mtrr.c                       |   2 +
>  xen/scripts/codestyle.py                      | 261 +++++++
>  xen/scripts/xen_analysis/cppcheck_analysis.py |   6 +-
>  .../xen_analysis/exclusion_file_list.py       |  31 +-
>  6 files changed, 1063 insertions(+), 18 deletions(-)
>  create mode 100644 xen/.clang-format
>  create mode 100755 xen/scripts/codestyle.py
> 
> -- 
> 2.34.1
> 
>