Re: [PATCH v1 36/58] perf export-to-sqlite: Port export-to-sqlite to use python module

sashiko-bot
2026-04-20T00:33:44Z

Sashiko AI review found 8 potential issue(s):
- [Critical] AttributeError crash on `sample.ip`, `sample.time`, and `sample.cpu` access.
- [High] Silent data loss of branch destinations, periods, and weights.
- [High] Missing SQL views breaking downstream tools and documentation.
- [High] Regression in support for power management events, calls, and context switches.
- [Medium] `evsel` event names are improperly formatted with 'evsel()' prefix.
- [Medium] `getattr` fallback behavior results in `NULL` database values for `dso` and `symbol`.
- [Low] Database file removed before connection is closed during error handling.
- [Low] Time-of-Check to Time-of-Use (TOCTOU) race condition when checking for existing database.
--

commit f1bba9aa8219f55def547da60006157b32630634
Author: Ian Rogers <irogers@google.com>

perf export-to-sqlite: Port export-to-sqlite to use python module

This commit ports the export-to-sqlite script to use the modern perf Python
module API and the standard library sqlite3. It implements lazy population
of lookup tables and adds callchain support.

> diff --git a/tools/perf/python/export-to-sqlite.py b/tools/perf/python/export-to-sqlite.py
> new file mode 100755
> index 0000000000000..ec39c1b1e04b3
> --- /dev/null
> +++ b/tools/perf/python/export-to-sqlite.py
> @@ -0,0 +1,351 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +"""
[ ... ]
> + $ sqlite3 pt_example
> + sqlite> .header on
> + sqlite> select * from samples_view where id < 10;

Is this example still valid? The script no longer contains CREATE VIEW
statements for samples_view, which might break downstream tools like
exported-sql-viewer.py that rely on these views to function.

Also, this script appears to drop support for tracking call and return
elapsed times, context switches, and power management events like ptwrite
or cbr. Are these features planned for a future follow-up?

[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Callback for processing events."""
> + thread_id = self.get_thread_id(sample.sample_pid, sample.sample_tid)
[ ... ]
> + dso_id = self.get_dso_id(
> + getattr(sample, 'dso', "Unknown_dso"),
> + getattr(sample, 'dso_long_name', "Unknown_dso_long"),
> + getattr(sample, 'dso_bid', "")
> + )

Does this fallback logic work as expected? If pyrf_sample_event__get_dso
returns None instead of raising an AttributeError, getattr will return
None rather than the Unknown_dso string. This could lead to inserting
NULL into the database instead of the expected textual fallback.

[ ... ]
> + # Insert sample
> + self.con.execute("""
> + INSERT INTO samples VALUES (
> + NULL, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?
> + )
> + """, (
> + self.get_event_id(str(sample.evsel)), 0, thread_id, comm_id,

Will str(sample.evsel) record the event name accurately? The string
representation of an evsel object typically returns a formatted string
like evsel(cycles). Should this use sample.evsel.name instead?

> + dso_id, symbol_id, getattr(sample, 'sym_offset', 0),
> + sample.ip, sample.time, sample.cpu,

Can accessing sample.ip, sample.time, and sample.cpu raise an AttributeError
here? The perf Python API exposes these properties with a sample_ prefix,
such as sample_ip, sample_time, and sample_cpu.

> + 0, 0, 0, 0, # to_dso, to_symbol, to_sym_offset, to_ip
> + getattr(sample, 'period', 0),
> + getattr(sample, 'weight', 0),
> + getattr(sample, 'transaction_', 0),
> + getattr(sample, 'data_src', 0),
> + 0, # branch_type

Does this logic intentionally drop branch destination fields and branch_type
by hardcoding them to 0? This might break hardware branch tracing use-cases
like Intel PT.

Additionally, will getattr(sample, 'period', 0) and getattr(sample,
'weight', 0) successfully retrieve the period and weight? The underlying
C-extension attributes are named sample_period and sample_weight, which
means getattr might silently fall back to 0 and lose this data.

[ ... ]
> +if __name__ == "__main__":
> + ap = argparse.ArgumentParser(
> + description="Export perf data to a sqlite3 database")
> + ap.add_argument("-i", "--input", default="perf.data",
> + help="Input file name")
> + ap.add_argument("-o", "--output", default="perf.db",
> + help="Output database name")
> + args = ap.parse_args()
> +
> + if os.path.exists(args.output):
> + print(f"Error: {args.output} already exists")
> + sys.exit(1)
> +
> + exporter = DatabaseExporter(args.output)

Is there a race condition here between checking if the file exists and
creating the database connection? If a symbolic link is created at the
output path right after the check, it might be followed during
initialization.

[ ... ]
> + except Exception as e:
> + print(f"Error processing events: {e}")
> + if os.path.exists(args.output):
> + os.remove(args.output)
> + finally:
> + exporter.close()

Can removing the database file before calling exporter.close() cause issues?
Deleting the main SQLite database file while the connection is still active
could lead to permissions errors on some operating systems or leave behind
dangling auxiliary files like rollback journals.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260419235911.2186050-1-irogers@google.com?part=36