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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 28 Jul 2023, 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.

Are these the only 2 points to discuss?

I think as a next step it would be worth listing all the code style
decision points we need to make as a group and then go over them during
one of the next calls.


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

Thanks for the amazing work Luca. I did as described above and generate
a 9MB diff patch :-)

I scrolled through it and most of it makes sense but a few things look
weird. Copy/pasting them here individually not to add a 9MB diff in
attachment.

> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index db5085e15d..398dea92e6 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -52,7 +52,7 @@ acpi_map_gic_cpu_interface(struct 
> acpi_madt_generic_interrupt *processor)
>  {
>      int i;
>      int rc;
> -    u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK;
> +    u64 mpidr    = processor->arm_mpidr & MPIDR_HWID_MASK;
>      bool enabled = processor->flags & ACPI_MADT_ENABLED;
>  
>      if ( mpidr == MPIDR_INVALID )

Do we need the = alignment?


It is causing other weird looking changes:

>    rsdp = (struct acpi_table_rsdp *)base_ptr;
>     /* Replace xsdt_physical_address */
>     rsdp->xsdt_physical_address = tbl_add[TBL_XSDT].start;
>-    checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, rsdp), table_size);
>+    checksum       = acpi_tb_checksum(ACPI_CAST_PTR(u8, rsdp), table_size);
>     rsdp->checksum = rsdp->checksum - checksum;
 
[...]

>  #ifdef CONFIG_MULTIBOOT
> -int __init xsm_multiboot_policy_init(
> -    unsigned long *module_map, const multiboot_info_t *mbi,
> -    void **policy_buffer, size_t *policy_size)
> +int __init xsm_multiboot_policy_init(unsigned long *module_map,
> +                                     const multiboot_info_t *mbi,
> +                                     void **policy_buffer, size_t 
> *policy_size)
>  {
>      int i;
>      module_t *mod = (module_t *)__va(mbi->mods_addr);
> -    int rc = 0;
> +    int rc        = 0;
>      u32 *_policy_start;
>      unsigned long _policy_len;
 
Can we take the = alignment out? Without it, most other things look
good, at least at first glance.

Also this looks a bit unnecessary, but not a blocker for me:

> @@ -908,8 +895,7 @@ static int __init efi_check_dt_boot(const 
> EFI_LOADED_IMAGE *loaded_image)
>  }
>  
>  static void __init efi_arch_cpu(void)
> -{   
> -}       
> +{}
>      

I think we should:
- identify "weird" changes like the above
- discuss them as a group, like we do with MISRA, and decide how to
  address them
- once everyone is happy with the new format, come up with a plan on how
  to merge a 9MB patch