Allow the clang-tools scripts to work with builds in tools such as
tools/perf and tools/lib/perf. An example use looks like:
```
$ cd tools/perf
$ make CC=clang CXX=clang++
$ ../../scripts/clang-tools/gen_compile_commands.py
$ ../../scripts/clang-tools/run-clang-tools.py clang-tidy compile_commands.json -checks=-*,readability-named-parameter
Skipping non-C file: 'tools/perf/bench/mem-memcpy-x86-64-asm.S'
Skipping non-C file: 'tools/perf/bench/mem-memset-x86-64-asm.S'
Skipping non-C file: 'tools/perf/arch/x86/tests/regs_load.S'
8 warnings generated.
Suppressed 8 warnings (8 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
4 warnings generated.
Suppressed 4 warnings (4 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
4 warnings generated.
Suppressed 4 warnings (4 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
3 warnings generated.
tools/perf/util/parse-events-flex.c:546:27: warning: all parameters should be named in a function [readability-named-parameter]
void *yyalloc ( yy_size_t , yyscan_t yyscanner );
^
/*size*/
...
```
Fix a number of the more serious low-hanging issues in perf found by
clang-tidy.
This support isn't complete, in particular it doesn't support output
directories properly and so fails for tools/lib/bpf, tools/bpf/bpftool
and if an output directory is used.
Ian Rogers (18):
gen_compile_commands: Allow the line prefix to still be cmd_
gen_compile_commands: Sort output compile commands by file name
run-clang-tools: Add pass through checks and and header-filter
arguments
perf hisi-ptt: Fix potential memory leak
perf bench uprobe: Fix potential use of memory after free
perf buildid-cache: Fix use of uninitialized value
perf env: Remove unnecessary NULL tests
perf jitdump: Avoid memory leak
perf mem-events: Avoid uninitialized read
perf dlfilter: Be defensive against potential NULL dereference
perf hists browser: Reorder variables to reduce padding
perf hists browser: Avoid potential NULL dereference
perf svghelper: Avoid memory leak
perf parse-events: Fix unlikely memory leak when cloning terms
tools api: Avoid potential double free
perf trace-event-info: Avoid passing NULL value to closedir
perf header: Fix various error path memory leaks
perf bpf_counter: Fix a few memory leaks
scripts/clang-tools/gen_compile_commands.py | 8 +--
scripts/clang-tools/run-clang-tools.py | 34 ++++++++---
tools/lib/api/io.h | 1 +
tools/perf/bench/uprobe.c | 1 +
tools/perf/builtin-buildid-cache.c | 6 +-
tools/perf/builtin-lock.c | 1 +
tools/perf/ui/browsers/hists.c | 6 +-
tools/perf/util/bpf_counter.c | 5 +-
tools/perf/util/dlfilter.c | 4 +-
tools/perf/util/env.c | 6 +-
tools/perf/util/header.c | 63 +++++++++++++--------
tools/perf/util/hisi-ptt.c | 12 ++--
tools/perf/util/jitdump.c | 1 +
tools/perf/util/mem-events.c | 3 +-
tools/perf/util/parse-events.c | 4 +-
tools/perf/util/svghelper.c | 5 +-
tools/perf/util/trace-event-info.c | 3 +-
17 files changed, 106 insertions(+), 57 deletions(-)
--
2.42.0.515.g380fc7ccd1-goog
In the unlikely case of having a symbol without a mapping, avoid a
NULL dereference that clang-tidy warns about.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/dlfilter.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
index 1dbf27822ee2..5e54832137a9 100644
--- a/tools/perf/util/dlfilter.c
+++ b/tools/perf/util/dlfilter.c
@@ -52,8 +52,10 @@ static void al_to_d_al(struct addr_location *al, struct perf_dlfilter_al *d_al)
d_al->sym_end = sym->end;
if (al->addr < sym->end)
d_al->symoff = al->addr - sym->start;
- else
+ else if (al->map)
d_al->symoff = al->addr - map__start(al->map) - sym->start;
+ else
+ d_al->symoff = 0;
d_al->sym_binding = sym->binding;
} else {
d_al->sym = NULL;
--
2.42.0.515.g380fc7ccd1-goog
Builds in tools still use the cmd_ prefix in .cmd files, so don't
require the saved part. Name the groups in the line pattern match so
that changing the regular expression is more robust and works with the
addition of a new match group.
Signed-off-by: Ian Rogers <[email protected]>
---
scripts/clang-tools/gen_compile_commands.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
index a84cc5737c2c..b43f9149893c 100755
--- a/scripts/clang-tools/gen_compile_commands.py
+++ b/scripts/clang-tools/gen_compile_commands.py
@@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json'
_DEFAULT_LOG_LEVEL = 'WARNING'
_FILENAME_PATTERN = r'^\..*\.cmd$'
-_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)'
+_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)'
_VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']
# The tools/ directory adopts a different build system, and produces .cmd
# files in a different format. Do not support it.
@@ -213,8 +213,8 @@ def main():
result = line_matcher.match(f.readline())
if result:
try:
- entry = process_line(directory, result.group(1),
- result.group(2))
+ entry = process_line(directory, result.group('command_prefix'),
+ result.group('file_path'))
compile_commands.append(entry)
except ValueError as err:
logging.info('Could not add line from %s: %s',
--
2.42.0.515.g380fc7ccd1-goog
On success path the sib_core and sib_thr values weren't being
freed. Detected by clang-tidy.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-lock.c | 1 +
tools/perf/util/svghelper.c | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index d4b22313e5fc..1b40b00c9563 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2463,6 +2463,7 @@ static int parse_call_stack(const struct option *opt __maybe_unused, const char
entry = malloc(sizeof(*entry) + strlen(tok) + 1);
if (entry == NULL) {
pr_err("Memory allocation failure\n");
+ free(s);
return -1;
}
diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index 0e4dc31c6c9c..1892e9b6aa7f 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
int i, nr_cpus;
struct topology t;
char *sib_core, *sib_thr;
+ int ret = -1;
nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
@@ -799,11 +800,11 @@ int svg_build_topology_map(struct perf_env *env)
scan_core_topology(topology_map, &t, nr_cpus);
- return 0;
+ ret = 0;
exit:
zfree(&t.sib_core);
zfree(&t.sib_thr);
- return -1;
+ return ret;
}
--
2.42.0.515.g380fc7ccd1-goog
Make the output more stable and deterministic.
Signed-off-by: Ian Rogers <[email protected]>
---
scripts/clang-tools/gen_compile_commands.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
index b43f9149893c..180952fb91c1 100755
--- a/scripts/clang-tools/gen_compile_commands.py
+++ b/scripts/clang-tools/gen_compile_commands.py
@@ -221,7 +221,7 @@ def main():
cmdfile, err)
with open(output, 'wt') as f:
- json.dump(compile_commands, f, indent=2, sort_keys=True)
+ json.dump(sorted(compile_commands, key=lambda x: x["file"]), f, indent=2, sort_keys=True)
if __name__ == '__main__':
--
2.42.0.515.g380fc7ccd1-goog
On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <[email protected]> wrote:
>
> Make the output more stable and deterministic.
>
> Signed-off-by: Ian Rogers <[email protected]>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
> scripts/clang-tools/gen_compile_commands.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
> index b43f9149893c..180952fb91c1 100755
> --- a/scripts/clang-tools/gen_compile_commands.py
> +++ b/scripts/clang-tools/gen_compile_commands.py
> @@ -221,7 +221,7 @@ def main():
> cmdfile, err)
>
> with open(output, 'wt') as f:
> - json.dump(compile_commands, f, indent=2, sort_keys=True)
> + json.dump(sorted(compile_commands, key=lambda x: x["file"]), f, indent=2, sort_keys=True)
>
>
> if __name__ == '__main__':
> --
> 2.42.0.515.g380fc7ccd1-goog
>
--
Thanks,
~Nick Desaulniers
On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <[email protected]> wrote:
> >
> > Builds in tools still use the cmd_ prefix in .cmd files, so don't
> > require the saved part. Name the groups in the line pattern match so
>
> Is that something that can be changed in the tools/ Makefiles?
>
> I'm fine with this change, just curious where the difference comes
> from precisely.
> Reviewed-by: Nick Desaulniers <[email protected]>
I agree. The savedcmd_ change came from Masahiro in:
https://lore.kernel.org/lkml/[email protected]/
I was reluctant to change the build logic in tools/ because of the
potential to break things. Maybe Masahiro/Nicolas know of issues?
Thanks,
Ian
>
> > that changing the regular expression is more robust and works with the
> > addition of a new match group.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > scripts/clang-tools/gen_compile_commands.py | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
> > index a84cc5737c2c..b43f9149893c 100755
> > --- a/scripts/clang-tools/gen_compile_commands.py
> > +++ b/scripts/clang-tools/gen_compile_commands.py
> > @@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json'
> > _DEFAULT_LOG_LEVEL = 'WARNING'
> >
> > _FILENAME_PATTERN = r'^\..*\.cmd$'
> > -_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)'
> > +_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)'
> > _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']
> > # The tools/ directory adopts a different build system, and produces .cmd
> > # files in a different format. Do not support it.
> > @@ -213,8 +213,8 @@ def main():
> > result = line_matcher.match(f.readline())
> > if result:
> > try:
> > - entry = process_line(directory, result.group(1),
> > - result.group(2))
> > + entry = process_line(directory, result.group('command_prefix'),
> > + result.group('file_path'))
> > compile_commands.append(entry)
> > except ValueError as err:
> > logging.info('Could not add line from %s: %s',
> > --
> > 2.42.0.515.g380fc7ccd1-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <[email protected]> wrote:
>
> Builds in tools still use the cmd_ prefix in .cmd files, so don't
> require the saved part. Name the groups in the line pattern match so
Is that something that can be changed in the tools/ Makefiles?
I'm fine with this change, just curious where the difference comes
from precisely.
Reviewed-by: Nick Desaulniers <[email protected]>
> that changing the regular expression is more robust and works with the
> addition of a new match group.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> scripts/clang-tools/gen_compile_commands.py | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
> index a84cc5737c2c..b43f9149893c 100755
> --- a/scripts/clang-tools/gen_compile_commands.py
> +++ b/scripts/clang-tools/gen_compile_commands.py
> @@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json'
> _DEFAULT_LOG_LEVEL = 'WARNING'
>
> _FILENAME_PATTERN = r'^\..*\.cmd$'
> -_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)'
> +_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)'
> _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']
> # The tools/ directory adopts a different build system, and produces .cmd
> # files in a different format. Do not support it.
> @@ -213,8 +213,8 @@ def main():
> result = line_matcher.match(f.readline())
> if result:
> try:
> - entry = process_line(directory, result.group(1),
> - result.group(2))
> + entry = process_line(directory, result.group('command_prefix'),
> + result.group('file_path'))
> compile_commands.append(entry)
> except ValueError as err:
> logging.info('Could not add line from %s: %s',
> --
> 2.42.0.515.g380fc7ccd1-goog
>
--
Thanks,
~Nick Desaulniers
On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote:
> On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <[email protected]> wrote:
> > >
> > > Builds in tools still use the cmd_ prefix in .cmd files, so don't
> > > require the saved part. Name the groups in the line pattern match so
> >
> > Is that something that can be changed in the tools/ Makefiles?
> >
> > I'm fine with this change, just curious where the difference comes
> > from precisely.
> > Reviewed-by: Nick Desaulniers <[email protected]>
>
> I agree. The savedcmd_ change came from Masahiro in:
> https://lore.kernel.org/lkml/[email protected]/
> I was reluctant to change the build logic in tools/ because of the
> potential to break things. Maybe Masahiro/Nicolas know of issues?
I haven't seen any issues related to the introduction of savedcmd_; and
roughly searching through tools/ I cannot find a rule that matches the
pattern Masahiro described in commit 92215e7a801d ("kbuild: rename
cmd_$@ to savedcmd_$@ in *.cmd files", 2022-12-29). For consistency,
I'd like to see the build rules in tools/ re-use the ones from scripts/
but as of now I don't see any necessity to introduce savedcmd in
tools/, yet.
Kind regards,
Nicolas
On Thu, Sep 28, 2023 at 11:26 PM Nicolas Schier <[email protected]> wrote:
>
> On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote:
> > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
> > <[email protected]> wrote:
> > >
> > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <[email protected]> wrote:
> > > >
> > > > Builds in tools still use the cmd_ prefix in .cmd files, so don't
> > > > require the saved part. Name the groups in the line pattern match so
> > >
> > > Is that something that can be changed in the tools/ Makefiles?
> > >
> > > I'm fine with this change, just curious where the difference comes
> > > from precisely.
> > > Reviewed-by: Nick Desaulniers <[email protected]>
> >
> > I agree. The savedcmd_ change came from Masahiro in:
> > https://lore.kernel.org/lkml/[email protected]/
> > I was reluctant to change the build logic in tools/ because of the
> > potential to break things. Maybe Masahiro/Nicolas know of issues?
>
> I haven't seen any issues related to the introduction of savedcmd_; and
> roughly searching through tools/ I cannot find a rule that matches the
> pattern Masahiro described in commit 92215e7a801d ("kbuild: rename
> cmd_$@ to savedcmd_$@ in *.cmd files", 2022-12-29). For consistency,
> I'd like to see the build rules in tools/ re-use the ones from scripts/
> but as of now I don't see any necessity to introduce savedcmd in
> tools/, yet.
>
> Kind regards,
> Nicolas
tools/build/Build.include mimics scripts/Kbuild.include
That should be changed in the same way.
--
Best Regards
Masahiro Yamada
On Tue, Oct 3, 2023 at 7:31 AM Masahiro Yamada <[email protected]> wrote:
>
> On Thu, Sep 28, 2023 at 11:26 PM Nicolas Schier <[email protected]> wrote:
> >
> > On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote:
> > > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
> > > <[email protected]> wrote:
> > > >
> > > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <[email protected]> wrote:
> > > > >
> > > > > Builds in tools still use the cmd_ prefix in .cmd files, so don't
> > > > > require the saved part. Name the groups in the line pattern match so
> > > >
> > > > Is that something that can be changed in the tools/ Makefiles?
> > > >
> > > > I'm fine with this change, just curious where the difference comes
> > > > from precisely.
> > > > Reviewed-by: Nick Desaulniers <[email protected]>
> > >
> > > I agree. The savedcmd_ change came from Masahiro in:
> > > https://lore.kernel.org/lkml/[email protected]/
> > > I was reluctant to change the build logic in tools/ because of the
> > > potential to break things. Maybe Masahiro/Nicolas know of issues?
> >
> > I haven't seen any issues related to the introduction of savedcmd_; and
> > roughly searching through tools/ I cannot find a rule that matches the
> > pattern Masahiro described in commit 92215e7a801d ("kbuild: rename
> > cmd_$@ to savedcmd_$@ in *.cmd files", 2022-12-29). For consistency,
> > I'd like to see the build rules in tools/ re-use the ones from scripts/
> > but as of now I don't see any necessity to introduce savedcmd in
> > tools/, yet.
> >
> > Kind regards,
> > Nicolas
>
>
> tools/build/Build.include mimics scripts/Kbuild.include
>
> That should be changed in the same way.
Thanks Masahiro,
I support that as a goal, I'm not sure I want to embrace it here. For
example, in tools/build/Build.include there is:
```
# Echo command
# Short version is used, if $(quiet) equals `quiet_', otherwise full one.
echo-cmd = $(if $($(quiet)cmd_$(1)),\
echo ' $(call escsq,$($(quiet)cmd_$(1)))';)
```
This is relevant given the use of cmd_ and not savedcmd_. In
scripts/Kbuild.include there is no equivalent, nor is there in
scripts. So do I dig into the history of echo-cmd unfork that, and
then go to the next thing? I find a lot of things like
tools/selftests/bpf won't build for me. When I debug a failure is it
because of a change or a pre-existing issue? Given the scope of the
Build.include unification problem, and this 4 line change, I hope we
can move forward with this and keep unification as a separate problem
(I totally support solving that problem).
Thanks,
Ian
> --
> Best Regards
> Masahiro Yamada