| Message ID | 20230728081144.4124309-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-format for Xen | expand |
On 28.07.2023 10:11, Luca Fancellu wrote: > Add entries to the exclusion list, so that they can be excluded > from the formatter tool. > > TBD: add a field on each entry to understand for what tool is the > exclusion > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > docs/misra/exclude-list.json | 88 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 88 insertions(+) > > diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json > index ca1e2dd678ff..c103c69209c9 100644 > --- a/docs/misra/exclude-list.json > +++ b/docs/misra/exclude-list.json > @@ -1,6 +1,10 @@ > { > "version": "1.0", > "content": [ > + { > + "rel_path": "arch/arm/arm32/lib/assembler.h", > + "comment": "Includes mostly assembly macro and it's meant to be included only in assembly code" > + }, > { > "rel_path": "arch/arm/arm64/cpufeature.c", > "comment": "Imported from Linux, ignore for now" > @@ -13,6 +17,26 @@ > "rel_path": "arch/arm/arm64/lib/find_next_bit.c", > "comment": "Imported from Linux, ignore for now" > }, > + { > + "rel_path": "arch/arm/include/asm/arm32/macros.h", > + "comment": "Includes only assembly macro" > + }, > + { > + "rel_path": "arch/arm/include/asm/arm64/macros.h", > + "comment": "Includes only assembly macro" > + }, > + { > + "rel_path": "arch/arm/include/asm/alternative.h", > + "comment": "Imported from Linux, ignore for now" > + }, > + { > + "rel_path": "arch/arm/include/asm/asm_defns.h", > + "comment": "Includes mostly assembly macro" > + }, > + { > + "rel_path": "arch/arm/include/asm/macros.h", > + "comment": "Includes mostly assembly macro and it's meant to be included only in assembly code" > + }, > { > "rel_path": "arch/x86/acpi/boot.c", > "comment": "Imported from Linux, ignore for now" > @@ -69,6 +93,30 @@ > "rel_path": "arch/x86/cpu/mwait-idle.c", > "comment": "Imported from Linux, ignore for now" > }, > + { > + "rel_path": "arch/x86/include/asm/alternative-asm.h", > + "comment": "Includes mostly assembly macro and it's meant to be included only in assembly code" > + }, > + { > + "rel_path": "arch/x86/include/asm/asm_defns.h", > + "comment": "Includes mostly assembly macro" > + }, > + { > + "rel_path": "arch/x86/include/asm/asm-defns.h", > + "comment": "Includes mostly assembly macro" > + }, > + { > + "rel_path": "arch/x86/include/asm/bug.h", > + "comment": "Includes mostly assembly macro" > + }, Mind me asking why assembly macros wouldn't want maintaining in proper style? > + { > + "rel_path": "arch/x86/include/asm/mpspec.h", > + "comment": "Imported from Linux, also case ranges are not handled by clang-format, ignore for now" > + }, > + { > + "rel_path": "arch/x86/include/asm/spec_ctrl_asm.h", > + "comment": "Includes mostly assembly macro" > + }, > { > "rel_path": "arch/x86/delay.c", > "comment": "Imported from Linux, ignore for now" > @@ -181,6 +229,42 @@ > "rel_path": "drivers/video/font_*", > "comment": "Imported from Linux, ignore for now" > }, > + { > + "rel_path": "include/efi/*.h", > + "comment": "Imported from gnu-efi-3.0k" > + }, > + { > + "rel_path": "include/public/arch-x86/cpufeatureset.h", > + "comment": "This file contains some inputs for the gen-cpuid.py script, leave it out" > + }, > + { > + "rel_path": "include/public/**/**/*.h", > + "comment": "Public headers are quite sensitive to format tools" > + }, > + { > + "rel_path": "include/public/**/*.h", > + "comment": "Public headers are quite sensitive to format tools" > + }, The common meaning of ** that I know is "any level directories", but since you use **/**/ above that can't be it here. Could you clarify what the difference of */ and **/ is here (or maybe in JSON in general)? Jan
Hi Jan, >> + { >> + "rel_path": "arch/x86/include/asm/bug.h", >> + "comment": "Includes mostly assembly macro" >> + }, > > Mind me asking why assembly macros wouldn't want maintaining in proper > style? From what I know (experts on CF correct me if I am wrong) clang-format is meant to format only some languages (C/C++/...) and assembly is not one of them, so what is happening is that most of the time clang-format breaks it, in fact we are formatting only .c and .h, but since we have some headers with assembly macros, I’ve seen some issues that ranges from really ugly formatting to build break. One thing we could do, is to export the headers that contain only assembly stuffs in dedicate headers (<header>_asm.h ?) so that we could easily use a name regex to exclude "*_asm.h” from clang-format? And also these headers could #error if included when __ASSEMBLY__ is not defined? But this requires some agreement on what is the best way I guess, you can know better if it’s feasible or not. >> >> + { >> + "rel_path": "include/public/**/**/*.h", >> + "comment": "Public headers are quite sensitive to format tools" >> + }, >> + { >> + "rel_path": "include/public/**/*.h", >> + "comment": "Public headers are quite sensitive to format tools" >> + }, > > The common meaning of ** that I know is "any level directories", but > since you use **/**/ above that can't be it here. Could you clarify > what the difference of */ and **/ is here (or maybe in JSON in general)? Yes I’ve found that python glob, that we use to solve the wildcard, solves the ** only for one level, maybe we could do something better to solve that, but for now I left it as it is to focus on the clang-format configuration side. Cheers, Luca > > Jan
On 31.07.2023 17:11, Luca Fancellu wrote: >>> + { >>> + "rel_path": "arch/x86/include/asm/bug.h", >>> + "comment": "Includes mostly assembly macro" >>> + }, >> >> Mind me asking why assembly macros wouldn't want maintaining in proper >> style? > > From what I know (experts on CF correct me if I am wrong) clang-format is meant to format only some languages > (C/C++/...) and assembly is not one of them, so what is happening is that most of the time clang-format breaks > it, in fact we are formatting only .c and .h, but since we have some headers with assembly macros, I’ve seen some issues > that ranges from really ugly formatting to build break. > > One thing we could do, is to export the headers that contain only assembly stuffs in dedicate headers (<header>_asm.h ?) > so that we could easily use a name regex to exclude "*_asm.h” from clang-format? And also these headers could #error if > included when __ASSEMBLY__ is not defined? In principle this may be a route to go (naming aside), but first of all I wonder what "assembler macros" are to you: We use both C macros and true assembler macros in assembly code. The former I would hope formatting tools don't have an issue with. Jan
> On 31 Jul 2023, at 16:20, Jan Beulich <jbeulich@suse.com> wrote: > > On 31.07.2023 17:11, Luca Fancellu wrote: >>>> + { >>>> + "rel_path": "arch/x86/include/asm/bug.h", >>>> + "comment": "Includes mostly assembly macro" >>>> + }, >>> >>> Mind me asking why assembly macros wouldn't want maintaining in proper >>> style? >> >> From what I know (experts on CF correct me if I am wrong) clang-format is meant to format only some languages >> (C/C++/...) and assembly is not one of them, so what is happening is that most of the time clang-format breaks >> it, in fact we are formatting only .c and .h, but since we have some headers with assembly macros, I’ve seen some issues >> that ranges from really ugly formatting to build break. >> >> One thing we could do, is to export the headers that contain only assembly stuffs in dedicate headers (<header>_asm.h ?) >> so that we could easily use a name regex to exclude "*_asm.h” from clang-format? And also these headers could #error if >> included when __ASSEMBLY__ is not defined? > > In principle this may be a route to go (naming aside), but first of all > I wonder what "assembler macros" are to you: We use both C macros and > true assembler macros in assembly code. The former I would hope formatting > tools don't have an issue with. Yes, C macros are clearly not an issue, true assembler macros are, like the example below: .macro BUG_FRAME type, line, file_str, second_frame, msg .if \type >= BUGFRAME_NR .error "Invalid BUGFRAME index" .endif .L\@ud: ud2a .pushsection .rodata.str1, "aMS", @progbits, 1 .L\@s1: .asciz "\file_str" .popsection .pushsection .bug_frames.\type, "a", @progbits .p2align 2 .L\@bf: .long (.L\@ud - .L\@bf) + \ ((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH) .long (.L\@s1 - .L\@bf) + \ ((\line & ((1 << BUG_LINE_LO_WIDTH) - 1)) << BUG_DISP_WIDTH) .if \second_frame .pushsection .rodata.str1, "aMS", @progbits, 1 .L\@s2: .asciz "\msg" .popsection .long 0, (.L\@s2 - .L\@bf) .endif .popsection .endm I don’t think CF has knowledge of the token .macro/.endm/.if/[...] and so it formats them in weird ways > > Jan
diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json index ca1e2dd678ff..c103c69209c9 100644 --- a/docs/misra/exclude-list.json +++ b/docs/misra/exclude-list.json @@ -1,6 +1,10 @@ { "version": "1.0", "content": [ + { + "rel_path": "arch/arm/arm32/lib/assembler.h", + "comment": "Includes mostly assembly macro and it's meant to be included only in assembly code" + }, { "rel_path": "arch/arm/arm64/cpufeature.c", "comment": "Imported from Linux, ignore for now" @@ -13,6 +17,26 @@ "rel_path": "arch/arm/arm64/lib/find_next_bit.c", "comment": "Imported from Linux, ignore for now" }, + { + "rel_path": "arch/arm/include/asm/arm32/macros.h", + "comment": "Includes only assembly macro" + }, + { + "rel_path": "arch/arm/include/asm/arm64/macros.h", + "comment": "Includes only assembly macro" + }, + { + "rel_path": "arch/arm/include/asm/alternative.h", + "comment": "Imported from Linux, ignore for now" + }, + { + "rel_path": "arch/arm/include/asm/asm_defns.h", + "comment": "Includes mostly assembly macro" + }, + { + "rel_path": "arch/arm/include/asm/macros.h", + "comment": "Includes mostly assembly macro and it's meant to be included only in assembly code" + }, { "rel_path": "arch/x86/acpi/boot.c", "comment": "Imported from Linux, ignore for now" @@ -69,6 +93,30 @@ "rel_path": "arch/x86/cpu/mwait-idle.c", "comment": "Imported from Linux, ignore for now" }, + { + "rel_path": "arch/x86/include/asm/alternative-asm.h", + "comment": "Includes mostly assembly macro and it's meant to be included only in assembly code" + }, + { + "rel_path": "arch/x86/include/asm/asm_defns.h", + "comment": "Includes mostly assembly macro" + }, + { + "rel_path": "arch/x86/include/asm/asm-defns.h", + "comment": "Includes mostly assembly macro" + }, + { + "rel_path": "arch/x86/include/asm/bug.h", + "comment": "Includes mostly assembly macro" + }, + { + "rel_path": "arch/x86/include/asm/mpspec.h", + "comment": "Imported from Linux, also case ranges are not handled by clang-format, ignore for now" + }, + { + "rel_path": "arch/x86/include/asm/spec_ctrl_asm.h", + "comment": "Includes mostly assembly macro" + }, { "rel_path": "arch/x86/delay.c", "comment": "Imported from Linux, ignore for now" @@ -181,6 +229,42 @@ "rel_path": "drivers/video/font_*", "comment": "Imported from Linux, ignore for now" }, + { + "rel_path": "include/efi/*.h", + "comment": "Imported from gnu-efi-3.0k" + }, + { + "rel_path": "include/public/arch-x86/cpufeatureset.h", + "comment": "This file contains some inputs for the gen-cpuid.py script, leave it out" + }, + { + "rel_path": "include/public/**/**/*.h", + "comment": "Public headers are quite sensitive to format tools" + }, + { + "rel_path": "include/public/**/*.h", + "comment": "Public headers are quite sensitive to format tools" + }, + { + "rel_path": "include/public/*.h", + "comment": "Public headers are quite sensitive to format tools" + }, + { + "rel_path": "include/xen/cper.h", + "comment": "Header does not follow Xen coding style" + }, + { + "rel_path": "include/xen/nodemask.h", + "comment": "Imported from Linux, also case ranges are not handled by clang-format, ignore for now" + }, + { + "rel_path": "include/xen/xen.lds.h", + "comment": "This file contains only macros used inside the linker script" + }, + { + "rel_path": "include/hypercall-defs.c", + "comment": "This file contains only C preprocessing syntax, the other lines are not C and are used to generate the hypercall definition by another script." + }, { "rel_path": "lib/list-sort.c", "comment": "Imported from Linux, ignore for now" @@ -193,6 +277,10 @@ "rel_path": "lib/xxhash*.c", "comment": "Imported from Linux, ignore for now" }, + { + "rel_path": "tools/*", + "comment": "Contains host tools imported from Linux, ignore for now" + }, { "rel_path": "xsm/flask/*", "comment": "Not in scope initially as it generates many violations and it is not enabled in safety configurations"
Add entries to the exclusion list, so that they can be excluded from the formatter tool. TBD: add a field on each entry to understand for what tool is the exclusion Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- docs/misra/exclude-list.json | 88 ++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)