A difficult part of automating commits is composing the subsystem
preamble in the commit log. For the ongoing effort of a fixer producing
one or two fixes a release the use of 'treewide:' does not seem appropriate.
It would be better if the normal prefix was used. Unfortunately normal is
not consistent across the tree.
So I am looking for comments for adding a new tag to the MAINTAINERS file
D: Commit subsystem prefix
ex/ for FPGA DFL DRIVERS
D: fpga: dfl:
Continuing with cleaning up clang's -Wextra-semi-stmt
A significant number of warnings are caused by function like macros with
a trailing semicolon. For example.
#define FOO(a) a++; <-- extra, unneeded semicolon
void bar() {
int v = 0;
FOO(a);
}
Clang will warn at the FOO(a); expansion location. Instead of removing
the semicolon there, the fixer removes semicolon from the macro
definition. After the fixer, the code will be:
#define FOO(a) a++
void bar() {
int v = 0;
FOO(a);
}
The fixer review is
https://reviews.llvm.org/D91789
A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives.
The false positives are caused by macros passed to other macros and by
some macro expansions that did not have an extra semicolon.
This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt
warnings in linux-next.
An update to [RFC] clang tooling cleanup
This change adds the clang-tidy-fix as a top level target and
uses it to do the cleaning. The next iteration will do a loop of
cleaners. This will mean breaking clang-tidy-fix out into its own
processing function 'run_fixers'.
Makefile: Add toplevel target clang-tidy-fix to makefile
Calls clang-tidy with -fix option for a set of checkers that
programatically fixes the kernel source in place, treewide.
Signed-off-by: Tom Rix <[email protected]>
---
Makefile | 7 ++++---
scripts/clang-tools/run-clang-tools.py | 20 +++++++++++++++++---
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/Makefile b/Makefile
index 47a8add4dd28..57756dbb767b 100644
--- a/Makefile
+++ b/Makefile
@@ -1567,20 +1567,21 @@ help:
echo ''
@echo 'Static analysers:'
@echo ' checkstack - Generate a list of stack hogs'
@echo ' versioncheck - Sanity check on version.h usage'
@echo ' includecheck - Check for duplicate included header files'
@echo ' export_report - List the usages of all exported symbols'
@echo ' headerdep - Detect inclusion cycles in headers'
@echo ' coccicheck - Check with Coccinelle'
@echo ' clang-analyzer - Check with clang static analyzer'
@echo ' clang-tidy - Check with clang-tidy'
+ @echo ' clang-tidy-fix - Check and fix with clang-tidy'
@echo ''
@echo 'Tools:'
@echo ' nsdeps - Generate missing symbol namespace dependencies'
@echo ''
@echo 'Kernel selftest:'
@echo ' kselftest - Build and run kernel selftest'
@echo ' Build, install, and boot kernel before'
@echo ' running kselftest on it'
@echo ' Run as root for full coverage'
@echo ' kselftest-all - Build kernel selftest'
@@ -1842,30 +1843,30 @@ nsdeps: modules
quiet_cmd_gen_compile_commands = GEN $@
cmd_gen_compile_commands = $(PYTHON3) $< -a $(AR) -o $@ $(filter-out $<, $(real-prereqs))
$(extmod-prefix)compile_commands.json: scripts/clang-tools/gen_compile_commands.py \
$(if $(KBUILD_EXTMOD),,$(KBUILD_VMLINUX_OBJS) $(KBUILD_VMLINUX_LIBS)) \
$(if $(CONFIG_MODULES), $(MODORDER)) FORCE
$(call if_changed,gen_compile_commands)
targets += $(extmod-prefix)compile_commands.json
-PHONY += clang-tidy clang-analyzer
+PHONY += clang-tidy-fix clang-tidy clang-analyzer
ifdef CONFIG_CC_IS_CLANG
quiet_cmd_clang_tools = CHECK $<
cmd_clang_tools = $(PYTHON3) $(srctree)/scripts/clang-tools/run-clang-tools.py $@ $<
-clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
+clang-tidy-fix clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
$(call cmd,clang_tools)
else
-clang-tidy clang-analyzer:
+clang-tidy-fix clang-tidy clang-analyzer:
@echo "$@ requires CC=clang" >&2
@false
endif
# Scripts to check various things for consistency
# ---------------------------------------------------------------------------
PHONY += includecheck versioncheck coccicheck export_report
includecheck:
diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
index fa7655c7cec0..c177ca822c56 100755
--- a/scripts/clang-tools/run-clang-tools.py
+++ b/scripts/clang-tools/run-clang-tools.py
@@ -22,43 +22,57 @@ def parse_arguments():
Returns:
args: Dict of parsed args
Has keys: [path, type]
"""
usage = """Run clang-tidy or the clang static-analyzer on a
compilation database."""
parser = argparse.ArgumentParser(description=usage)
type_help = "Type of analysis to be performed"
parser.add_argument("type",
- choices=["clang-tidy", "clang-analyzer"],
+ choices=["clang-tidy-fix", "clang-tidy", "clang-analyzer"],
help=type_help)
path_help = "Path to the compilation database to parse"
parser.add_argument("path", type=str, help=path_help)
return parser.parse_args()
def init(l, a):
global lock
global args
lock = l
args = a
def run_analysis(entry):
# Disable all checks, then re-enable the ones we want
checks = "-checks=-*,"
- if args.type == "clang-tidy":
+ fix = ""
+ header_filter = ""
+ if args.type == "clang-tidy-fix":
+ checks += "linuxkernel-macro-trailing-semi"
+ #
+ # Fix this
+ # #define M(a) a++; <-- clang-tidy fixes the problem here
+ # int f() {
+ # int v = 0;
+ # M(v); <-- clang reports problem here
+ # return v;
+ # }
+ fix += "-fix"
+ header_filter += "-header-filter=.*"
+ elif args.type == "clang-tidy":
checks += "linuxkernel-*"
else:
checks += "clang-analyzer-*"
- p = subprocess.run(["clang-tidy", "-p", args.path, checks, entry["file"]],
+ p = subprocess.run(["clang-tidy", "-p", args.path, checks, header_filter, fix, entry["file"]],
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
cwd=entry["directory"])
with lock:
sys.stderr.buffer.write(p.stdout)
def main():
args = parse_arguments()
--
2.18.4
On Sat, 2020-11-21 at 09:18 -0800, James Bottomley wrote:
> On Sat, 2020-11-21 at 08:50 -0800, [email protected] wrote:
> > A difficult part of automating commits is composing the subsystem
> > preamble in the commit log. For the ongoing effort of a fixer
> > producing one or two fixes a release the use of 'treewide:' does
> > not seem appropriate.
> >
> > It would be better if the normal prefix was used. Unfortunately
> > normal is not consistent across the tree.
> >
> > D: Commit subsystem prefix
> >
> > ex/ for FPGA DFL DRIVERS
> >
> > D: fpga: dfl:
>
> I've got to bet this is going to cause more issues than it solves.
> SCSI uses scsi: <driver>: for drivers but not every driver has a
> MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer
> things, but we're not consistent. Block uses blk-<something>: for all
> of it's stuff but almost no <somtehing>s have a MAINTAINERS entry. So
> the next thing you're going to cause is an explosion of suggested
> MAINTAINERs entries.
As well as some changes require simultaneous changes across
multiple subsystems.
> Has anyone actually complained about treewide:?
It depends on what you mean by treewide:
If a treewide: patch is applied by some "higher level" maintainer,
then generally, no.
If the treewide patch is also cc'd to many individual maintainers,
then yes, many many times.
Mostly because patches cause what is in their view churn or that
changes are not specific to their subsystem grounds.
The treewide patch is sometimes dropped, sometimes broken up and
generally not completely applied.
What would be useful in many cases like this is for a pre and post
application of the treewide patch to be compiled and the object
code verified for lack of any logic change.
Unfortunately, gcc does not guarantee deterministic compilation so
this isn't feasible with at least gcc. Does clang guarantee this?
I'm not sure it's possible:
https://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html
On 11/21/20 7:23 PM, Matthew Wilcox wrote:
> On Sat, Nov 21, 2020 at 08:50:58AM -0800, [email protected] wrote:
>> The fixer review is
>> https://reviews.llvm.org/D91789
>>
>> A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives.
>> The false positives are caused by macros passed to other macros and by
>> some macro expansions that did not have an extra semicolon.
>>
>> This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt
>> warnings in linux-next.
> Are any of them not false-positives? It's all very well to enable
> stricter warnings, but if they don't fix any bugs, they're just churn.
>
While enabling additional warnings may be a side effect of this effort
the primary goal is to set up a cleaning robot. After that a refactoring robot.
Tom
On 11/21/20 9:10 AM, Joe Perches wrote:
> On Sat, 2020-11-21 at 08:50 -0800, [email protected] wrote:
>> A difficult part of automating commits is composing the subsystem
>> preamble in the commit log. For the ongoing effort of a fixer producing
>> one or two fixes a release the use of 'treewide:' does not seem appropriate.
>>
>> It would be better if the normal prefix was used. Unfortunately normal is
>> not consistent across the tree.
>>
>> So I am looking for comments for adding a new tag to the MAINTAINERS file
>>
>> D: Commit subsystem prefix
>>
>> ex/ for FPGA DFL DRIVERS
>>
>> D: fpga: dfl:
> I'm all for it. Good luck with the effort. It's not completely trivial.
>
> From a decade ago:
>
> https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
>
> (and that thread started with extra semicolon patches too)
Reading the history, how about this.
get_mataintainer.pl outputs a single prefix, if multiple files have the same prefix it works, if they don't its an error.
Another script 'commit_one_file.sh' does the call to get_mainainter.pl to get the prefix and be called by run-clang-tools.py to get the fixer specific message.
Defer minimizing the commits by combining similar subsystems till later.
In a steady state case, this should be uncommon.
>
>> Continuing with cleaning up clang's -Wextra-semi-stmt
>> diff --git a/Makefile b/Makefile
> []
>> @@ -1567,20 +1567,21 @@ help:
>> echo ''
>> @echo 'Static analysers:'
>> @echo ' checkstack - Generate a list of stack hogs'
>> @echo ' versioncheck - Sanity check on version.h usage'
>> @echo ' includecheck - Check for duplicate included header files'
>> @echo ' export_report - List the usages of all exported symbols'
>> @echo ' headerdep - Detect inclusion cycles in headers'
>> @echo ' coccicheck - Check with Coccinelle'
>> @echo ' clang-analyzer - Check with clang static analyzer'
>> @echo ' clang-tidy - Check with clang-tidy'
>> + @echo ' clang-tidy-fix - Check and fix with clang-tidy'
> A pity the ordering of the code below isn't the same as the above.
Taken care thanks!
Tom
On Sat, 21 Nov 2020, James Bottomley <[email protected]> wrote:
> On Sat, 2020-11-21 at 08:50 -0800, [email protected] wrote:
>> A difficult part of automating commits is composing the subsystem
>> preamble in the commit log. For the ongoing effort of a fixer
>> producing
>> one or two fixes a release the use of 'treewide:' does not seem
>> appropriate.
>>
>> It would be better if the normal prefix was used. Unfortunately
>> normal is
>> not consistent across the tree.
>>
>>
>> D: Commit subsystem prefix
>>
>> ex/ for FPGA DFL DRIVERS
>>
>> D: fpga: dfl:
>>
>
> I've got to bet this is going to cause more issues than it solves.
Agreed.
> SCSI uses scsi: <driver>: for drivers but not every driver has a
> MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer
> things, but we're not consistent. Block uses blk-<something>: for all
> of it's stuff but almost no <somtehing>s have a MAINTAINERS entry. So
> the next thing you're going to cause is an explosion of suggested
> MAINTAINERs entries.
On the one hand, adoption of new MAINTAINERS entries has been really
slow. Look at B, C, or P, for instance. On the other hand, if this were
to get adopted, you'll potentially get conflicting prefixes for patches
touching multiple files. Then what?
I'm guessing a script looking at git log could come up with better
suggestions for prefixes via popularity contest than manually maintained
MAINTAINERS entries. It might not always get it right, but then human
outsiders aren't going to always get it right either.
Now you'll only need Someone(tm) to write the script. ;)
Something quick like this:
git log --since={1year} --pretty=format:%s -- <FILES> |\
grep -v "^\(Merge\|Revert\)" |\
sed 's/:[^:]*$//' |\
sort | uniq -c | sort -rn | head -5
already gives me results that really aren't worse than some of the
prefixes invented by drive-by contributors.
> Has anyone actually complained about treewide:?
As Joe said, I'd feel silly applying patches to drivers with that
prefix. If it gets applied by someone else higher up, literally
treewide, then no complaints.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center