| Message ID | 20201121165058.1644182-1-trix@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] MAINTAINERS tag for cleanup robot | expand |
On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com 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. Has anyone actually complained about treewide:? James
On 11/21/20 7:23 PM, Matthew Wilcox wrote: > On Sat, Nov 21, 2020 at 08:50:58AM -0800, trix@redhat.com 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/22/20 6:56 AM, Matthew Wilcox wrote: > On Sun, Nov 22, 2020 at 06:46:46AM -0800, Tom Rix wrote: >> On 11/21/20 7:23 PM, Matthew Wilcox wrote: >>> On Sat, Nov 21, 2020 at 08:50:58AM -0800, trix@redhat.com 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. > Why do we need such a thing? Again, it sounds like more churn. > It's really annoying when I'm working on something important that gets > derailed by pointless churn. Churn also makes it harder to backport > patches to earlier kernels. > A refactoring example on moving to treewide, consistent use of a new api may help. Consider 2efc459d06f1630001e3984854848a5647086232 sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output A new api for printing in the sysfs. How do we use it treewide ? Done manually, it would be a heroic effort requiring high level maintainers pushing and likely only get partially done. If a refactoring programatic fixit is done and validated on a one subsystem, it can run on all the subsystems. The effort is a couple of weeks to write and validate the fixer, hours to run over the tree. It won't be perfect but will be better than doing it manually. Tom
On 11/21/20 9:10 AM, Joe Perches wrote: > On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com 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 Sun, 2020-11-22 at 08:49 -0800, James Bottomley wrote: > We can enforce sysfs_emit going forwards > using tools like checkpatch It's not really possible for checkpatch to find or warn about sysfs uses of sprintf. checkpatch is really just a trivial line-by-line parser and it has no concept of code intent. It just can't warn on every use of the sprintf family. There are just too many perfectly valid uses. > but there's no benefit and a lot of harm to > be done by trying to churn the entire tree Single uses of sprintf for sysfs is not really any problem. But likely there are still several possible overrun sprintf/snprintf paths in sysfs. Some of them are very obscure and unlikely to be found by a robot as the logic for sysfs buf uses can be fairly twisty. But provably correct conversions IMO _should_ be done and IMO churn considerations should generally have less importance.
On Mon, 2020-11-23 at 09:33 +1100, Finn Thain wrote: > On Sun, 22 Nov 2020, Joe Perches wrote: > > But provably correct conversions IMO _should_ be done and IMO churn > > considerations should generally have less importance. [] > Moreover, the patch review workload for skilled humans is being generated > by the automation, which is completely backwards: the machine is supposed > to be helping. Which is why the provably correct matters. coccinelle transforms can be, but are not necessarily, provably correct. The _show transforms done via the sysfs_emit_dev.cocci script are correct as in commit aa838896d87a ("drivers core: Use sysfs_emit and sysfs_emit_at for show(device *...) functions") Worthwhile? A different question, but I think yes as it reduces the overall question space of the existing other sprintf overrun possibilities.
On Sat, 21 Nov 2020, James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com 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.
On Mon, Nov 23, 2020 at 4:52 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Sat, 21 Nov 2020, James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com 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. > Tom, this a problem only kernel janitors encounter; all other developers really do not have that issue. The time spent on creating the patch is much larger than the amount saved if the commit log header line prefix would be derived automatically. I believe Julia Lawall, Arnd Bergmann and Nathan Chancellor as long-term high-frequency janitors do have already scripted approaches to that issue. Maybe they simply need to share these scripts with you and you consolidate them and share with everyone? Also, making clean-up patches cumbersome has a positive side as well; maintainers are not swamped with fully automated patch submissions. There have been some bad experiences with some submitters on that in the past... > > 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. > I agree I do not see the need to introduce something in MAINTAINERS; from my observations maintaining MAINTAINERS, there is sufficient work on adoption and maintenance of the existing entries already without such an yet another additional entry. Some entries are outdated or wrong and the janitor task of cleaning those up is already enough work for involved janitors and enough churn for involved maintainers. So a machine-learned approach as above is probably good enough, but if you think you need more complex rules try to learn them from the data at hand... certainly a nice task to do with machine learning on commit message prefixes. Lukas
On 11/22/20 10:22 AM, Joe Perches wrote: > On Sun, 2020-11-22 at 08:33 -0800, Tom Rix wrote: >> On 11/21/20 9:10 AM, Joe Perches wrote: >>> On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com 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_maintainer.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. > It's not whether the script used is get_maintainer or any other script, > the question is really if the MAINTAINERS file is the appropriate place > to store per-subsystem patch specific prefixes. > > It is. > > Then the question should be how are the forms described and what is the > inheritance priority. My preference would be to have a default of > inherit the parent base and add basename(subsystem dirname). > > Commit history seems to have standardized on using colons as the separator > between the commit prefix and the subject. > > A good mechanism to explore how various subsystems have uses prefixes in > the past might be something like: > > $ git log --no-merges --pretty='%s' -<commit_count> <subsystem_path> | \ > perl -n -e 'print substr($_, 0, rindex($_, ":") + 1) . "\n";' | \ > sort | uniq -c | sort -rn Thanks, I have shamelessly stolen this line and limited the commits to the maintainer. I will post something once the generation of the prefixes is done. Tom
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()
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 <trix@redhat.com> --- Makefile | 7 ++++--- scripts/clang-tools/run-clang-tools.py | 20 +++++++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-)