* [PATCH v1 00/18] clang-tools support in tools
@ 2023-09-23 5:34 Ian Rogers
2023-09-23 5:34 ` [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_ Ian Rogers
` (17 more replies)
0 siblings, 18 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:34 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
@ 2023-09-23 5:34 ` Ian Rogers
2023-09-25 15:49 ` Nick Desaulniers
2023-09-23 5:34 ` [PATCH v1 02/18] gen_compile_commands: Sort output compile commands by file name Ian Rogers
` (16 subsequent siblings)
17 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:34 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
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 <irogers@google.com>
---
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 02/18] gen_compile_commands: Sort output compile commands by file name
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
2023-09-23 5:34 ` [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_ Ian Rogers
@ 2023-09-23 5:34 ` Ian Rogers
2023-09-25 15:45 ` Nick Desaulniers
2023-09-23 5:35 ` [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments Ian Rogers
` (15 subsequent siblings)
17 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:34 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Make the output more stable and deterministic.
Signed-off-by: Ian Rogers <irogers@google.com>
---
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
2023-09-23 5:34 ` [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_ Ian Rogers
2023-09-23 5:34 ` [PATCH v1 02/18] gen_compile_commands: Sort output compile commands by file name Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-25 15:43 ` Nick Desaulniers
2023-09-23 5:35 ` [PATCH v1 04/18] perf hisi-ptt: Fix potential memory leak Ian Rogers
` (14 subsequent siblings)
17 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Add a -checks argument to allow the checks passed to the clang-tool to
be set on the command line.
Add a pass through -header-filter option.
Signed-off-by: Ian Rogers <irogers@google.com>
---
scripts/clang-tools/run-clang-tools.py | 34 ++++++++++++++++++++------
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
index 3266708a8658..5dfe03852cb4 100755
--- a/scripts/clang-tools/run-clang-tools.py
+++ b/scripts/clang-tools/run-clang-tools.py
@@ -33,6 +33,11 @@ def parse_arguments():
path_help = "Path to the compilation database to parse"
parser.add_argument("path", type=str, help=path_help)
+ checks_help = "Checks to pass to the analysis"
+ parser.add_argument("-checks", type=str, default=None, help=checks_help)
+ header_filter_help = "Pass the -header-filter value to the tool"
+ parser.add_argument("-header-filter", type=str, default=None, help=header_filter_help)
+
return parser.parse_args()
@@ -45,14 +50,29 @@ def init(l, a):
def run_analysis(entry):
# Disable all checks, then re-enable the ones we want
- checks = []
- checks.append("-checks=-*")
- if args.type == "clang-tidy":
- checks.append("linuxkernel-*")
+ global args
+ checks = None
+ if args.checks:
+ checks = args.checks.split(',')
else:
- checks.append("clang-analyzer-*")
- checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
- p = subprocess.run(["clang-tidy", "-p", args.path, ",".join(checks), entry["file"]],
+ checks = ["-*"]
+ if args.type == "clang-tidy":
+ checks.append("linuxkernel-*")
+ else:
+ checks.append("clang-analyzer-*")
+ checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
+ file = entry["file"]
+ if not file.endswith(".c") and not file.endswith(".cpp"):
+ with lock:
+ print(f"Skipping non-C file: '{file}'", file=sys.stderr)
+ return
+ pargs = ["clang-tidy", "-p", args.path]
+ if checks:
+ pargs.append("-checks=" + ",".join(checks))
+ if args.header_filter:
+ pargs.append("-header-filter=" + args.header_filter)
+ pargs.append(file)
+ p = subprocess.run(pargs,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
cwd=entry["directory"])
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 04/18] perf hisi-ptt: Fix potential memory leak
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (2 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 05/18] perf bench uprobe: Fix potential use of memory after free Ian Rogers
` (13 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Fix clang-tidy found potential memory leak and unread value:
```
tools/perf/util/hisi-ptt.c:108:3: warning: Value stored to 'data_offset' is never read [clang-analyzer-deadcode.DeadStores]
data_offset = 0;
^ ~
tools/perf/util/hisi-ptt.c:108:3: note: Value stored to 'data_offset' is never read
data_offset = 0;
^ ~
tools/perf/util/hisi-ptt.c:112:12: warning: Potential leak of memory pointed to by 'data' [clang-analyzer-unix.Malloc]
return -errno;
^
/usr/include/errno.h:38:18: note: expanded from macro 'errno'
^
tools/perf/util/hisi-ptt.c:100:15: note: Memory is allocated
void *data = malloc(size);
^~~~~~~~~~~~
tools/perf/util/hisi-ptt.c:104:6: note: Assuming 'data' is non-null
if (!data)
^~~~~
tools/perf/util/hisi-ptt.c:104:2: note: Taking false branch
if (!data)
^
tools/perf/util/hisi-ptt.c:107:6: note: Assuming the condition is false
if (perf_data__is_pipe(session->data)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/hisi-ptt.c:107:2: note: Taking false branch
if (perf_data__is_pipe(session->data)) {
^
tools/perf/util/hisi-ptt.c:111:7: note: Assuming the condition is true
if (data_offset == -1)
^~~~~~~~~~~~~~~~~
tools/perf/util/hisi-ptt.c:111:3: note: Taking true branch
if (data_offset == -1)
^
tools/perf/util/hisi-ptt.c:112:12: note: Potential leak of memory pointed to by 'data'
return -errno;
^
/usr/include/errno.h:38:18: note: expanded from macro 'errno'
```
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/hisi-ptt.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
index 45b614bb73bf..ea297329c526 100644
--- a/tools/perf/util/hisi-ptt.c
+++ b/tools/perf/util/hisi-ptt.c
@@ -98,18 +98,18 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
int fd = perf_data__fd(session->data);
int size = event->auxtrace.size;
void *data = malloc(size);
- off_t data_offset;
int err;
if (!data)
return -errno;
- if (perf_data__is_pipe(session->data)) {
- data_offset = 0;
- } else {
- data_offset = lseek(fd, 0, SEEK_CUR);
- if (data_offset == -1)
+ if (!perf_data__is_pipe(session->data)) {
+ off_t data_offset = lseek(fd, 0, SEEK_CUR);
+
+ if (data_offset == -1) {
+ free(data);
return -errno;
+ }
}
err = readn(fd, data, size);
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 05/18] perf bench uprobe: Fix potential use of memory after free
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (3 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 04/18] perf hisi-ptt: Fix potential memory leak Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 06/18] perf buildid-cache: Fix use of uninitialized value Ian Rogers
` (12 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Found by clang-tidy:
```
bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc]
bench_uprobe_bpf__destroy(skel);
```
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/bench/uprobe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c
index 914c0817fe8a..5c71fdc419dd 100644
--- a/tools/perf/bench/uprobe.c
+++ b/tools/perf/bench/uprobe.c
@@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench)
return err;
cleanup:
bench_uprobe_bpf__destroy(skel);
+ skel = NULL;
return err;
}
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 06/18] perf buildid-cache: Fix use of uninitialized value
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (4 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 05/18] perf bench uprobe: Fix potential use of memory after free Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 07/18] perf env: Remove unnecessary NULL tests Ian Rogers
` (11 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
The buildid filename is first determined and then from this the
buildid read. If getting the filename fails then the buildid will be
used for a later memcmp uninitialized. Detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-buildid-cache.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index cd381693658b..e2a40f1d9225 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
char filename[PATH_MAX];
struct build_id bid;
- if (dso__build_id_filename(dso, filename, sizeof(filename), false) &&
- filename__read_build_id(filename, &bid) == -1) {
+ if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
+ return true;
+
+ if (filename__read_build_id(filename, &bid) == -1) {
if (errno == ENOENT)
return false;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 07/18] perf env: Remove unnecessary NULL tests
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (5 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 06/18] perf buildid-cache: Fix use of uninitialized value Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 08/18] perf jitdump: Avoid memory leak Ian Rogers
` (10 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
clang-tidy was warning:
```
util/env.c:334:23: warning: Access to field 'nr_pmu_mappings' results in a dereference of a null pointer (loaded from variable 'env') [clang-analyzer-core.NullDereference]
env->nr_pmu_mappings = pmu_num;
```
As functions are called potentially when !env was true. This condition
could never be true as it would produce a segv, so remove the
unnecessary NULL tests and silence clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/env.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index a164164001fb..44140b7f596a 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -457,7 +457,7 @@ const char *perf_env__cpuid(struct perf_env *env)
{
int status;
- if (!env || !env->cpuid) { /* Assume local operation */
+ if (!env->cpuid) { /* Assume local operation */
status = perf_env__read_cpuid(env);
if (status)
return NULL;
@@ -470,7 +470,7 @@ int perf_env__nr_pmu_mappings(struct perf_env *env)
{
int status;
- if (!env || !env->nr_pmu_mappings) { /* Assume local operation */
+ if (!env->nr_pmu_mappings) { /* Assume local operation */
status = perf_env__read_pmu_mappings(env);
if (status)
return 0;
@@ -483,7 +483,7 @@ const char *perf_env__pmu_mappings(struct perf_env *env)
{
int status;
- if (!env || !env->pmu_mappings) { /* Assume local operation */
+ if (!env->pmu_mappings) { /* Assume local operation */
status = perf_env__read_pmu_mappings(env);
if (status)
return NULL;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 08/18] perf jitdump: Avoid memory leak
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (6 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 07/18] perf env: Remove unnecessary NULL tests Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 09/18] perf mem-events: Avoid uninitialized read Ian Rogers
` (9 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
jit_repipe_unwinding_info is called in a loop by jit_process_dump,
avoid leaking unwinding_data by free-ing before overwriting. Error
detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/jitdump.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 6b2b96c16ccd..1f657ef8975f 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -675,6 +675,7 @@ jit_repipe_unwinding_info(struct jit_buf_desc *jd, union jr_entry *jr)
jd->eh_frame_hdr_size = jr->unwinding.eh_frame_hdr_size;
jd->unwinding_size = jr->unwinding.unwinding_size;
jd->unwinding_mapped_size = jr->unwinding.mapped_size;
+ free(jd->unwinding_data);
jd->unwinding_data = unwinding_data;
return 0;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 09/18] perf mem-events: Avoid uninitialized read
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (7 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 08/18] perf jitdump: Avoid memory leak Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 10/18] perf dlfilter: Be defensive against potential NULL dereference Ian Rogers
` (8 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
pmu should be initialized to NULL before perf_pmus__scan loop. Fix and
shrink the scope of pmu at the same time. Issue detected by clang-tidy.
Fixes: 5752c20f3787 ("perf mem: Scan all PMUs instead of just core ones")
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/mem-events.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 39ffe8ceb380..954b235e12e5 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -185,7 +185,6 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
{
int i = *argv_nr, k = 0;
struct perf_mem_event *e;
- struct perf_pmu *pmu;
for (int j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
e = perf_mem_events__ptr(j);
@@ -202,6 +201,8 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
rec_argv[i++] = "-e";
rec_argv[i++] = perf_mem_events__name(j, NULL);
} else {
+ struct perf_pmu *pmu = NULL;
+
if (!e->supported) {
perf_mem_events__print_unsupport_hybrid(e, j);
return -1;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 10/18] perf dlfilter: Be defensive against potential NULL dereference
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (8 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 09/18] perf mem-events: Avoid uninitialized read Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 11/18] perf hists browser: Reorder variables to reduce padding Ian Rogers
` (7 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
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 <irogers@google.com>
---
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 11/18] perf hists browser: Reorder variables to reduce padding
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (9 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 10/18] perf dlfilter: Be defensive against potential NULL dereference Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 12/18] perf hists browser: Avoid potential NULL dereference Ian Rogers
` (6 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Address clang-tidy warning:
```
tools/perf/ui/browsers/hists.c:2416:8: warning: Excessive padding in 'struct popup_action' (8 padding bytes, where 0 is optimal).
Optimal fields order:
time,
thread,
evsel,
fn,
ms,
socket,
rstype,
```
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/ui/browsers/hists.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 70db5a717905..f02ee605bbce 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2416,12 +2416,12 @@ static int switch_data_file(void)
struct popup_action {
unsigned long time;
struct thread *thread;
+ struct evsel *evsel;
+ int (*fn)(struct hist_browser *browser, struct popup_action *act);
struct map_symbol ms;
int socket;
- struct evsel *evsel;
enum rstype rstype;
- int (*fn)(struct hist_browser *browser, struct popup_action *act);
};
static int
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 12/18] perf hists browser: Avoid potential NULL dereference
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (10 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 11/18] perf hists browser: Reorder variables to reduce padding Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 13/18] perf svghelper: Avoid memory leak Ian Rogers
` (5 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
On other code paths browser->he_selection is NULL checked, add a
missing case reported by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/ui/browsers/hists.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f02ee605bbce..f4812b226818 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3302,7 +3302,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
&options[nr_options],
&bi->to.ms,
bi->to.al_addr);
- } else {
+ } else if (browser->he_selection) {
nr_options += add_annotate_opt(browser,
&actions[nr_options],
&options[nr_options],
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 13/18] perf svghelper: Avoid memory leak
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (11 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 12/18] perf hists browser: Avoid potential NULL dereference Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 14/18] perf parse-events: Fix unlikely memory leak when cloning terms Ian Rogers
` (4 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
On success path the sib_core and sib_thr values weren't being
freed. Detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 14/18] perf parse-events: Fix unlikely memory leak when cloning terms
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (12 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 13/18] perf svghelper: Avoid memory leak Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 15/18] tools api: Avoid potential double free Ian Rogers
` (3 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Add missing free on an error path as detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c56e07bd7dd6..23c027cf20ae 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2549,8 +2549,10 @@ int parse_events_term__clone(struct parse_events_term **new,
return new_term(new, &temp, /*str=*/NULL, term->val.num);
str = strdup(term->val.str);
- if (!str)
+ if (!str) {
+ zfree(&temp.config);
return -ENOMEM;
+ }
return new_term(new, &temp, str, /*num=*/0);
}
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 15/18] tools api: Avoid potential double free
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (13 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 14/18] perf parse-events: Fix unlikely memory leak when cloning terms Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 16/18] perf trace-event-info: Avoid passing NULL value to closedir Ian Rogers
` (2 subsequent siblings)
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
io__getline will free the line on error but it doesn't clear the out
argument. This may lead to the line being freed twice, like in
tools/perf/util/srcline.c as detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/lib/api/io.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
index 9fc429d2852d..a77b74c5fb65 100644
--- a/tools/lib/api/io.h
+++ b/tools/lib/api/io.h
@@ -180,6 +180,7 @@ static inline ssize_t io__getline(struct io *io, char **line_out, size_t *line_l
return line_len;
err_out:
free(line);
+ *line_out = NULL;
return -ENOMEM;
}
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 16/18] perf trace-event-info: Avoid passing NULL value to closedir
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (14 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 15/18] tools api: Avoid potential double free Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 17/18] perf header: Fix various error path memory leaks Ian Rogers
2023-09-23 5:35 ` [PATCH v1 18/18] perf bpf_counter: Fix a few " Ian Rogers
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
If opendir failed then closedir was passed NULL which is
erroneous. Caught by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/trace-event-info.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 319ccf09a435..c8755679281e 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -313,7 +313,8 @@ static int record_event_files(struct tracepoint_path *tps)
}
err = 0;
out:
- closedir(dir);
+ if (dir)
+ closedir(dir);
put_tracing_file(path);
return err;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 17/18] perf header: Fix various error path memory leaks
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (15 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 16/18] perf trace-event-info: Avoid passing NULL value to closedir Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 18/18] perf bpf_counter: Fix a few " Ian Rogers
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Memory leaks were detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
| 63 ++++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 25 deletions(-)
--git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d812e1e371a7..41b78e40b22b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2598,8 +2598,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
goto error;
/* include a NULL character at the end */
- if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+ if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
+ free(str);
goto error;
+ }
size += string_size(str);
free(str);
}
@@ -2617,8 +2619,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
goto error;
/* include a NULL character at the end */
- if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+ if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
+ free(str);
goto error;
+ }
size += string_size(str);
free(str);
}
@@ -2681,8 +2685,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
goto error;
/* include a NULL character at the end */
- if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+ if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
+ free(str);
goto error;
+ }
size += string_size(str);
free(str);
}
@@ -2736,10 +2742,9 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
goto error;
n->map = perf_cpu_map__new(str);
+ free(str);
if (!n->map)
goto error;
-
- free(str);
}
ff->ph->env.nr_numa_nodes = nr;
ff->ph->env.numa_nodes = nodes;
@@ -2913,10 +2918,10 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
return -1;
for (i = 0; i < cnt; i++) {
- struct cpu_cache_level c;
+ struct cpu_cache_level *c = &caches[i];
#define _R(v) \
- if (do_read_u32(ff, &c.v))\
+ if (do_read_u32(ff, &c->v)) \
goto out_free_caches; \
_R(level)
@@ -2926,22 +2931,25 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
#undef _R
#define _R(v) \
- c.v = do_read_string(ff); \
- if (!c.v) \
- goto out_free_caches;
+ c->v = do_read_string(ff); \
+ if (!c->v) \
+ goto out_free_caches; \
_R(type)
_R(size)
_R(map)
#undef _R
-
- caches[i] = c;
}
ff->ph->env.caches = caches;
ff->ph->env.caches_cnt = cnt;
return 0;
out_free_caches:
+ for (i = 0; i < cnt; i++) {
+ free(caches[i].type);
+ free(caches[i].size);
+ free(caches[i].map);
+ }
free(caches);
return -1;
}
@@ -3585,18 +3593,16 @@ static int perf_header__adds_write(struct perf_header *header,
struct feat_copier *fc)
{
int nr_sections;
- struct feat_fd ff;
+ struct feat_fd ff = {
+ .fd = fd,
+ .ph = header,
+ };
struct perf_file_section *feat_sec, *p;
int sec_size;
u64 sec_start;
int feat;
int err;
- ff = (struct feat_fd){
- .fd = fd,
- .ph = header,
- };
-
nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
if (!nr_sections)
return 0;
@@ -3623,6 +3629,7 @@ static int perf_header__adds_write(struct perf_header *header,
err = do_write(&ff, feat_sec, sec_size);
if (err < 0)
pr_debug("failed to write feature section\n");
+ free(ff.buf);
free(feat_sec);
return err;
}
@@ -3630,11 +3637,11 @@ static int perf_header__adds_write(struct perf_header *header,
int perf_header__write_pipe(int fd)
{
struct perf_pipe_file_header f_header;
- struct feat_fd ff;
+ struct feat_fd ff = {
+ .fd = fd,
+ };
int err;
- ff = (struct feat_fd){ .fd = fd };
-
f_header = (struct perf_pipe_file_header){
.magic = PERF_MAGIC,
.size = sizeof(f_header),
@@ -3645,7 +3652,7 @@ int perf_header__write_pipe(int fd)
pr_debug("failed to write perf pipe header\n");
return err;
}
-
+ free(ff.buf);
return 0;
}
@@ -3658,11 +3665,12 @@ static int perf_session__do_write_header(struct perf_session *session,
struct perf_file_attr f_attr;
struct perf_header *header = &session->header;
struct evsel *evsel;
- struct feat_fd ff;
+ struct feat_fd ff = {
+ .fd = fd,
+ };
u64 attr_offset;
int err;
- ff = (struct feat_fd){ .fd = fd};
lseek(fd, sizeof(f_header), SEEK_SET);
evlist__for_each_entry(session->evlist, evsel) {
@@ -3670,6 +3678,7 @@ static int perf_session__do_write_header(struct perf_session *session,
err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
if (err < 0) {
pr_debug("failed to write perf header\n");
+ free(ff.buf);
return err;
}
}
@@ -3695,6 +3704,7 @@ static int perf_session__do_write_header(struct perf_session *session,
err = do_write(&ff, &f_attr, sizeof(f_attr));
if (err < 0) {
pr_debug("failed to write perf header attribute\n");
+ free(ff.buf);
return err;
}
}
@@ -3705,8 +3715,10 @@ static int perf_session__do_write_header(struct perf_session *session,
if (at_exit) {
err = perf_header__adds_write(header, evlist, fd, fc);
- if (err < 0)
+ if (err < 0) {
+ free(ff.buf);
return err;
+ }
}
f_header = (struct perf_file_header){
@@ -3728,6 +3740,7 @@ static int perf_session__do_write_header(struct perf_session *session,
lseek(fd, 0, SEEK_SET);
err = do_write(&ff, &f_header, sizeof(f_header));
+ free(ff.buf);
if (err < 0) {
pr_debug("failed to write perf header\n");
return err;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 18/18] perf bpf_counter: Fix a few memory leaks
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
` (16 preceding siblings ...)
2023-09-23 5:35 ` [PATCH v1 17/18] perf header: Fix various error path memory leaks Ian Rogers
@ 2023-09-23 5:35 ` Ian Rogers
17 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-09-23 5:35 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Tom Rix, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
Memory leaks were detected by clang-tidy.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/bpf_counter.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 6732cbbcf9b3..7f9b0e46e008 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -104,7 +104,7 @@ static int bpf_program_profiler_load_one(struct evsel *evsel, u32 prog_id)
struct bpf_prog_profiler_bpf *skel;
struct bpf_counter *counter;
struct bpf_program *prog;
- char *prog_name;
+ char *prog_name = NULL;
int prog_fd;
int err;
@@ -155,10 +155,12 @@ static int bpf_program_profiler_load_one(struct evsel *evsel, u32 prog_id)
assert(skel != NULL);
counter->skel = skel;
list_add(&counter->list, &evsel->bpf_counter_list);
+ free(prog_name);
close(prog_fd);
return 0;
err_out:
bpf_prog_profiler_bpf__destroy(skel);
+ free(prog_name);
free(counter);
close(prog_fd);
return -1;
@@ -180,6 +182,7 @@ static int bpf_program_profiler__load(struct evsel *evsel, struct target *target
(*p != '\0' && *p != ',')) {
pr_err("Failed to parse bpf prog ids %s\n",
target->bpf_str);
+ free(bpf_str_);
return -1;
}
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments
2023-09-23 5:35 ` [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments Ian Rogers
@ 2023-09-25 15:43 ` Nick Desaulniers
2023-10-05 22:38 ` Ian Rogers
0 siblings, 1 reply; 27+ messages in thread
From: Nick Desaulniers @ 2023-09-25 15:43 UTC (permalink / raw)
To: Ian Rogers
Cc: Nathan Chancellor, Tom Rix, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Yicong Yang,
Jonathan Cameron, Yang Jihong, Kan Liang, Ming Wang, Huacai Chen,
Sean Christopherson, K Prateek Nayak, Yanteng Si, Yuan Can,
Ravi Bangoria, James Clark, Paolo Bonzini, llvm, linux-kernel,
linux-perf-users, bpf
On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote:
>
> Add a -checks argument to allow the checks passed to the clang-tool to
> be set on the command line.
>
> Add a pass through -header-filter option.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> scripts/clang-tools/run-clang-tools.py | 34 ++++++++++++++++++++------
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
> index 3266708a8658..5dfe03852cb4 100755
> --- a/scripts/clang-tools/run-clang-tools.py
> +++ b/scripts/clang-tools/run-clang-tools.py
> @@ -33,6 +33,11 @@ def parse_arguments():
> path_help = "Path to the compilation database to parse"
> parser.add_argument("path", type=str, help=path_help)
>
> + checks_help = "Checks to pass to the analysis"
> + parser.add_argument("-checks", type=str, default=None, help=checks_help)
> + header_filter_help = "Pass the -header-filter value to the tool"
> + parser.add_argument("-header-filter", type=str, default=None, help=header_filter_help)
> +
> return parser.parse_args()
>
>
> @@ -45,14 +50,29 @@ def init(l, a):
>
> def run_analysis(entry):
> # Disable all checks, then re-enable the ones we want
> - checks = []
> - checks.append("-checks=-*")
> - if args.type == "clang-tidy":
> - checks.append("linuxkernel-*")
> + global args
> + checks = None
> + if args.checks:
> + checks = args.checks.split(',')
> else:
> - checks.append("clang-analyzer-*")
> - checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
> - p = subprocess.run(["clang-tidy", "-p", args.path, ",".join(checks), entry["file"]],
> + checks = ["-*"]
> + if args.type == "clang-tidy":
> + checks.append("linuxkernel-*")
> + else:
> + checks.append("clang-analyzer-*")
> + checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
> + file = entry["file"]
> + if not file.endswith(".c") and not file.endswith(".cpp"):
> + with lock:
> + print(f"Skipping non-C file: '{file}'", file=sys.stderr)
> + return
> + pargs = ["clang-tidy", "-p", args.path]
> + if checks:
^ can `checks` ever be falsy here? I don't think we need this
conditional check?
> + pargs.append("-checks=" + ",".join(checks))
> + if args.header_filter:
> + pargs.append("-header-filter=" + args.header_filter)
> + pargs.append(file)
> + p = subprocess.run(pargs,
> stdout=subprocess.PIPE,
> stderr=subprocess.STDOUT,
> cwd=entry["directory"])
> --
> 2.42.0.515.g380fc7ccd1-goog
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 02/18] gen_compile_commands: Sort output compile commands by file name
2023-09-23 5:34 ` [PATCH v1 02/18] gen_compile_commands: Sort output compile commands by file name Ian Rogers
@ 2023-09-25 15:45 ` Nick Desaulniers
0 siblings, 0 replies; 27+ messages in thread
From: Nick Desaulniers @ 2023-09-25 15:45 UTC (permalink / raw)
To: Ian Rogers
Cc: Nathan Chancellor, Tom Rix, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Yicong Yang,
Jonathan Cameron, Yang Jihong, Kan Liang, Ming Wang, Huacai Chen,
Sean Christopherson, K Prateek Nayak, Yanteng Si, Yuan Can,
Ravi Bangoria, James Clark, Paolo Bonzini, llvm, linux-kernel,
linux-perf-users, bpf
On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote:
>
> Make the output more stable and deterministic.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_
2023-09-23 5:34 ` [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_ Ian Rogers
@ 2023-09-25 15:49 ` Nick Desaulniers
2023-09-25 16:06 ` Ian Rogers
0 siblings, 1 reply; 27+ messages in thread
From: Nick Desaulniers @ 2023-09-25 15:49 UTC (permalink / raw)
To: Ian Rogers
Cc: Nathan Chancellor, Tom Rix, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Yicong Yang,
Jonathan Cameron, Yang Jihong, Kan Liang, Ming Wang, Huacai Chen,
Sean Christopherson, K Prateek Nayak, Yanteng Si, Yuan Can,
Ravi Bangoria, James Clark, Paolo Bonzini, llvm, linux-kernel,
linux-perf-users, bpf
On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> 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 <ndesaulniers@google.com>
> that changing the regular expression is more robust and works with the
> addition of a new match group.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_
2023-09-25 15:49 ` Nick Desaulniers
@ 2023-09-25 16:06 ` Ian Rogers
2023-09-28 14:23 ` Nicolas Schier
0 siblings, 1 reply; 27+ messages in thread
From: Ian Rogers @ 2023-09-25 16:06 UTC (permalink / raw)
To: Nick Desaulniers, Nicolas Schier, Masahiro Yamada
Cc: Nathan Chancellor, Tom Rix, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Yicong Yang,
Jonathan Cameron, Yang Jihong, Kan Liang, Ming Wang, Huacai Chen,
Sean Christopherson, K Prateek Nayak, Yanteng Si, Yuan Can,
Ravi Bangoria, James Clark, Paolo Bonzini, llvm, linux-kernel,
linux-perf-users, bpf
On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> 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 <ndesaulniers@google.com>
I agree. The savedcmd_ change came from Masahiro in:
https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/
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 <irogers@google.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_
2023-09-25 16:06 ` Ian Rogers
@ 2023-09-28 14:23 ` Nicolas Schier
2023-10-03 14:31 ` Masahiro Yamada
0 siblings, 1 reply; 27+ messages in thread
From: Nicolas Schier @ 2023-09-28 14:23 UTC (permalink / raw)
To: Ian Rogers
Cc: Nick Desaulniers, Masahiro Yamada, Nathan Chancellor, Tom Rix,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote:
> On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> 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 <ndesaulniers@google.com>
>
> I agree. The savedcmd_ change came from Masahiro in:
> https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_
2023-09-28 14:23 ` Nicolas Schier
@ 2023-10-03 14:31 ` Masahiro Yamada
2023-10-05 22:35 ` Ian Rogers
0 siblings, 1 reply; 27+ messages in thread
From: Masahiro Yamada @ 2023-10-03 14:31 UTC (permalink / raw)
To: Nicolas Schier
Cc: Ian Rogers, Nick Desaulniers, Nathan Chancellor, Tom Rix,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
On Thu, Sep 28, 2023 at 11:26 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote:
> > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> 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 <ndesaulniers@google.com>
> >
> > I agree. The savedcmd_ change came from Masahiro in:
> > https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_
2023-10-03 14:31 ` Masahiro Yamada
@ 2023-10-05 22:35 ` Ian Rogers
0 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-10-05 22:35 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nicolas Schier, Nick Desaulniers, Nathan Chancellor, Tom Rix,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Yicong Yang, Jonathan Cameron, Yang Jihong,
Kan Liang, Ming Wang, Huacai Chen, Sean Christopherson,
K Prateek Nayak, Yanteng Si, Yuan Can, Ravi Bangoria,
James Clark, Paolo Bonzini, llvm, linux-kernel, linux-perf-users,
bpf
On Tue, Oct 3, 2023 at 7:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, Sep 28, 2023 at 11:26 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote:
> > > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers
> > > <ndesaulniers@google.com> wrote:
> > > >
> > > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> 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 <ndesaulniers@google.com>
> > >
> > > I agree. The savedcmd_ change came from Masahiro in:
> > > https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/
> > > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments
2023-09-25 15:43 ` Nick Desaulniers
@ 2023-10-05 22:38 ` Ian Rogers
0 siblings, 0 replies; 27+ messages in thread
From: Ian Rogers @ 2023-10-05 22:38 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Nathan Chancellor, Tom Rix, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Yicong Yang,
Jonathan Cameron, Yang Jihong, Kan Liang, Ming Wang, Huacai Chen,
Sean Christopherson, K Prateek Nayak, Yanteng Si, Yuan Can,
Ravi Bangoria, James Clark, Paolo Bonzini, llvm, linux-kernel,
linux-perf-users, bpf
On Mon, Sep 25, 2023 at 8:44 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Add a -checks argument to allow the checks passed to the clang-tool to
> > be set on the command line.
> >
> > Add a pass through -header-filter option.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > scripts/clang-tools/run-clang-tools.py | 34 ++++++++++++++++++++------
> > 1 file changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
> > index 3266708a8658..5dfe03852cb4 100755
> > --- a/scripts/clang-tools/run-clang-tools.py
> > +++ b/scripts/clang-tools/run-clang-tools.py
> > @@ -33,6 +33,11 @@ def parse_arguments():
> > path_help = "Path to the compilation database to parse"
> > parser.add_argument("path", type=str, help=path_help)
> >
> > + checks_help = "Checks to pass to the analysis"
> > + parser.add_argument("-checks", type=str, default=None, help=checks_help)
> > + header_filter_help = "Pass the -header-filter value to the tool"
> > + parser.add_argument("-header-filter", type=str, default=None, help=header_filter_help)
> > +
> > return parser.parse_args()
> >
> >
> > @@ -45,14 +50,29 @@ def init(l, a):
> >
> > def run_analysis(entry):
> > # Disable all checks, then re-enable the ones we want
> > - checks = []
> > - checks.append("-checks=-*")
> > - if args.type == "clang-tidy":
> > - checks.append("linuxkernel-*")
> > + global args
> > + checks = None
> > + if args.checks:
> > + checks = args.checks.split(',')
> > else:
> > - checks.append("clang-analyzer-*")
> > - checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
> > - p = subprocess.run(["clang-tidy", "-p", args.path, ",".join(checks), entry["file"]],
> > + checks = ["-*"]
> > + if args.type == "clang-tidy":
> > + checks.append("linuxkernel-*")
> > + else:
> > + checks.append("clang-analyzer-*")
> > + checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
> > + file = entry["file"]
> > + if not file.endswith(".c") and not file.endswith(".cpp"):
> > + with lock:
> > + print(f"Skipping non-C file: '{file}'", file=sys.stderr)
> > + return
> > + pargs = ["clang-tidy", "-p", args.path]
> > + if checks:
>
> ^ can `checks` ever be falsy here? I don't think we need this
> conditional check?
Agreed, it was for an option that I removed. Will fix in v2.
Thanks,
Ian
> > + pargs.append("-checks=" + ",".join(checks))
> > + if args.header_filter:
> > + pargs.append("-header-filter=" + args.header_filter)
> > + pargs.append(file)
> > + p = subprocess.run(pargs,
> > stdout=subprocess.PIPE,
> > stderr=subprocess.STDOUT,
> > cwd=entry["directory"])
> > --
> > 2.42.0.515.g380fc7ccd1-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-10-05 22:39 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-23 5:34 [PATCH v1 00/18] clang-tools support in tools Ian Rogers
2023-09-23 5:34 ` [PATCH v1 01/18] gen_compile_commands: Allow the line prefix to still be cmd_ Ian Rogers
2023-09-25 15:49 ` Nick Desaulniers
2023-09-25 16:06 ` Ian Rogers
2023-09-28 14:23 ` Nicolas Schier
2023-10-03 14:31 ` Masahiro Yamada
2023-10-05 22:35 ` Ian Rogers
2023-09-23 5:34 ` [PATCH v1 02/18] gen_compile_commands: Sort output compile commands by file name Ian Rogers
2023-09-25 15:45 ` Nick Desaulniers
2023-09-23 5:35 ` [PATCH v1 03/18] run-clang-tools: Add pass through checks and and header-filter arguments Ian Rogers
2023-09-25 15:43 ` Nick Desaulniers
2023-10-05 22:38 ` Ian Rogers
2023-09-23 5:35 ` [PATCH v1 04/18] perf hisi-ptt: Fix potential memory leak Ian Rogers
2023-09-23 5:35 ` [PATCH v1 05/18] perf bench uprobe: Fix potential use of memory after free Ian Rogers
2023-09-23 5:35 ` [PATCH v1 06/18] perf buildid-cache: Fix use of uninitialized value Ian Rogers
2023-09-23 5:35 ` [PATCH v1 07/18] perf env: Remove unnecessary NULL tests Ian Rogers
2023-09-23 5:35 ` [PATCH v1 08/18] perf jitdump: Avoid memory leak Ian Rogers
2023-09-23 5:35 ` [PATCH v1 09/18] perf mem-events: Avoid uninitialized read Ian Rogers
2023-09-23 5:35 ` [PATCH v1 10/18] perf dlfilter: Be defensive against potential NULL dereference Ian Rogers
2023-09-23 5:35 ` [PATCH v1 11/18] perf hists browser: Reorder variables to reduce padding Ian Rogers
2023-09-23 5:35 ` [PATCH v1 12/18] perf hists browser: Avoid potential NULL dereference Ian Rogers
2023-09-23 5:35 ` [PATCH v1 13/18] perf svghelper: Avoid memory leak Ian Rogers
2023-09-23 5:35 ` [PATCH v1 14/18] perf parse-events: Fix unlikely memory leak when cloning terms Ian Rogers
2023-09-23 5:35 ` [PATCH v1 15/18] tools api: Avoid potential double free Ian Rogers
2023-09-23 5:35 ` [PATCH v1 16/18] perf trace-event-info: Avoid passing NULL value to closedir Ian Rogers
2023-09-23 5:35 ` [PATCH v1 17/18] perf header: Fix various error path memory leaks Ian Rogers
2023-09-23 5:35 ` [PATCH v1 18/18] perf bpf_counter: Fix a few " Ian Rogers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).