[srihari-uttanur]

Motivation

This PR adds PC Sampling support for rocpd by introducing a self describing schema. The main purpose of the self-describing schema is to enable one schema solution for all GPU architectures. The idea is to store architecture-specific fields as binary blobs in the database, and to decode them on demand using metadata tables that describe the layout of each blob type.

This approach allows rocpd to support new blob types without any schema changes, simply by describing the blob layout in the metadata tables. All the fields which are constant across architectures remain as columns in the main table and architecture depended ones are stored as binary blob.

Technical Details

The current ROCpd schema advances to 3.0.6, while 3.0.5 remains available as a frozen compatibility snapshot.

Two layout metadata tables and one blob-instance table.

rocpd_info_blob_schema: One row per struct version. Records the struct name, which base table holds the blob (source_table), byte order, alignment, total struct size, and version number. Multiple struct versions can coexist in the same database; each gets a unique id (the schema_id used as a foreign key on data rows).

rocpd_info_blob_field: One row per field within a struct. Records the field name, byte offset from the start of the blob, size in bytes, C data type (UINT8, UINT32, etc.), signedness, and an optional description. This is the field dictionary the generic view builder iterates over when decoding columns.

rocpd_blob_event: One row per blob instance, storing raw packed binary data and its schema_id. Each row in rocpd_gpu_pc_sample and its companion blob row share the same (guid, event_id). Both independently reference
rocpd_event.id; there is no blob_event_id column. Join samples and blobs on both guid and event_id to keep records from different recordings separate.

Execution masks are decimal TEXT to preserve unsigned 64-bit values. Use quoted decimal strings or string-bound parameters for equality; ordering follows text semantics.

Disassembly caching (cache_disassembly=True) is intentionally enabled by default for new and existing PC-sampling databases, without feature-flag gating. Analysis commands may persist successfully decoded, hash-verified instructions at normal process exit when the disassembly table exists. Use --no-cache-disassembly or cache_disassembly=False to disable write-back while retaining lazy decoding and access to stored disassembly

Code-object content hashing (capture_content_hash) is enabled only for PC sampling with ROCpd output. CSV/JSON-only captures skip the hash computation, but still read the bytes required for disassembly. During later lazy decoding, code-object bytes must match any recorded content hash; a mismatch reports Decode unavailable. Objects without a recorded hash can still be decoded when their bytes are accessible, but those instructions are not written back to the database.

JIRA ID

JIRA ID : AIPROFSDK-203

Test Plan

Added new integration tests for verifying pc sampling rocpd output.

Test Result

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

[Copilot]

Pull request overview

Adds GPU PC sampling persistence and query support to the rocpd SQLite output so PC sampling records (including arch-specific fields) can be stored efficiently and queried conveniently.

Changes:

• Introduces new rocpd schema objects for PC sampling (rocpd_gpu_pc_sample) and a self-describing binary blob schema (rocpd_info_blob_schema / rocpd_info_blob_field), with schema version bump to 4.
• Extends the rocpd generator to emit PC sampling rows and store arch-specific fields as a SQLite BLOB alongside a schema descriptor.
• Enhances the Python query tool to optionally expose blob fields as computed columns via a temporary view.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/rocpd_views.sql	Adds stable (non-UUID-suffixed) views for new blob schema tables and the PC sampling table.
projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/rocpd_tables.sql	Adds new tables/columns for blob schemas + PC sampling and bumps schema_version to 4.
projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/rocpd_indexes.sql	Adds indexes targeting expected high-volume PC sampling query patterns.
projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/data_views.sql	Adds a convenience view joining PC samples with agent metadata.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-tool/tool.cpp	Wires new PC sampling generators into rocpd output generation.
projects/rocprofiler-sdk/source/lib/python/rocpd/query.py	Adds temp-view expansion to expose blob fields as queryable columns (+ CLI flags).
projects/rocprofiler-sdk/source/lib/output/generateRocpd.hpp	Extends write_rocpd API to accept PC sampling generators.
projects/rocprofiler-sdk/source/lib/output/generateRocpd.cpp	Implements schema registration + insertion of PC sampling rows and blob binding.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

• Add new commits
• Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

[vlaindic]

I focused on reviewing PC sampling related part. Didn't have time to check the whole populating logic. I also ignored changes inside 3.0.1 folder due to shortage of time.

[vlaindic]

To complement PC sampling schema focused review: #4946 (review), I asked AI to review the rest and here are the findings.

---

🔴 CRITICAL (1) + ⚠️ IMPORTANT-CANDIDATE (1)

1. Sentinel 0 may conflict with valid agent node_id=0 (verify before escalating)

dispatch_to_agent_id uses 0 as the "not present" sentinel (dispatch_to_agent_id.at(...) != 0), but agent_node_id comes from get_agent()->node_id (KFD topology node IDs). Whether node_id=0 is reachable in practice depends on the topology, but relying on 0 as a sentinel is fragile. Same pattern applies to dispatch_to_thread_id.

File: generateRocpd.cpp (~lines 1554–1562, ~2295–2310)

Suggestion: Consider using std::optional<uint64_t> instead of a sentinel-valued stable_vector<uint64_t> — initialize each slot to std::nullopt, store real values on dispatch, and use .has_value() for the "present" check. This makes the intent explicit and eliminates the sentinel ambiguity entirely.

---

2. versions.yml 3.0.2 entry has no snapshot directory — schema versioning is broken (CRITICAL)

The versions.yml entry for 3.0.2 points to the top-level mutable files (rocpd_metadata.sql, rocpd_tables.sql, etc.) with no path prefix, while 3.0.1 correctly points to versions/3.0.1/…. There is no versions/3.0.2/ directory in this PR. The versioning system exists precisely so that a frozen snapshot of schema N is available alongside schema N+1; without it, the 3.0.2 "snapshot" is the same mutable live files that keep changing.

File: versions.yml, versions/CMakeLists.txt

Fix: Create versions/3.0.2/ with its own frozen SQL snapshots (equivalent to what versions/3.0.1/ does for that version), add a versions/3.0.2/CMakeLists.txt, and register it with add_subdirectory(3.0.2) in versions/CMakeLists.txt. Update versions.yml to use versions/3.0.2/ paths.

---

⚠️ IMPORTANT (6)

3. exec_mask cast to int64_t loses unsigned semantics

insert_value("exec_mask", static_cast<int64_t>(record.exec_mask)) — if the high bit of the 64-bit exec mask is set (e.g., all 64 lanes active = 0xFFFFFFFFFFFFFFFF), the cast produces -1 in SQLite. The column is BIGINT which SQLite stores as signed. This makes exec_mask values with MSB set appear negative to consumers.

File: generateRocpd.cpp (~line 2399)

Fix: Use insert_value("exec_mask", record.exec_mask) directly — sql_insert_value::value already accepts uint64_t via the variant. Or add a uint64_t overload that stores as-is (SQLite bit pattern is the same; the issue is the explicit cast).

---

4. sqlite3_last_insert_rowid race in register_blob_schema

register_blob_schema() flushes the schema row and then calls sqlite3_last_insert_rowid(db.conn) to get the ID. This is correct in theory (single connection, no concurrent writers), but if the batch-flush path ever inserts rows for other tables after the schema row (e.g., via pending batch reorder), the rowid will be wrong. The code relies on flush_pending_insert_batch being the absolute last SQLite operation before last_insert_rowid. This is fragile.

Fix: After the flush, issue SELECT last_insert_rowid() or SELECT MAX(id) FROM rocpd_info_blob_schema{{uuid}} WHERE name='pc_sample_extdata_v1' to get the schema ID deterministically, rather than relying on last_insert_rowid.

---

5. Missing JIRA ID — description is entirely template placeholders

The PR body contains only the PR template with all sections (## Motivation, ## Technical Details, ## JIRA ID, ## Test Plan, ## Test Result) left blank. No JIRA ID is present, and the description provides zero information about the change: no goal, no design rationale, no test results.

Fix: Fill in the PR description with at minimum: (a) JIRA ID — use AIPROFSDK-45, (b) a 2–3 sentence summary of the change and its design decisions (especially the blob schema approach), (c) test plan and results.

---

6. No tests for new functionality

This PR adds a new rocpd output path (PC sampling → SQLite), a new schema version, a Python blob-view system, and substantial C++ insertion logic. No test files are added or modified. The existing rocpd integration tests should be extended to verify PC sampling rows are emitted and readable.

Risk: High — new GPU data path landing without any automated verification.

Fix: Extend the existing rocpd pytest suite (likely under projects/rocprofiler-sdk/tests/) to add a test that runs with PC sampling enabled and verifies: (a) rocpd_gpu_pc_sample is populated, (b) rocpd_info_blob_schema/rocpd_info_blob_field are populated, (c) the Python setup_blob_views / _decoded view works correctly.

---

7. register_blob_schema and insert_pc_sampling_data lambdas lack inline documentation

Both lambdas are non-trivial (150+ lines combined), implement a multi-step protocol (flush, last_insert_rowid, field loop, struct cast), and have subtle ordering requirements. Neither has a comment explaining the protocol or the "why flush before rowid" constraint.

Fix: Add a // Design note: comment block at the top of register_blob_schema explaining: (1) why it must flush before calling last_insert_rowid, (2) that the schema is shared by host-trap and stochastic generators. Add a similar comment to insert_pc_sampling_data explaining the dispatch_to_* lookup semantics and the NULL-on-miss design.

---

8. Copyright headers missing from all new files in versions/3.0.1/

All 7 new files under projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.1/ (CMakeLists.txt, data_views.sql, rocpd_indexes.sql, rocpd_metadata.sql, rocpd_tables.sql, rocpd_views.sql, summary_views.sql) carry no copyright header. These are newly added files in this PR.

Fix: Add a copyright header to each new file: -- Copyright (c) 2026 Advanced Micro Devices, Inc. All rights reserved. (SQL files) and the equivalent CMake comment form for CMakeLists.txt. Year must be 2026.

---

Risk / Impact Assessment

Overall: High

• Agent node_id=0 sentinel may silently drop valid data in PC sample rows (verify whether KFD exposes node_id=0 in practice).
• Broken schema versioning (no versions/3.0.2/ snapshot) means schema upgrade tooling cannot safely migrate forward from 3.0.1.
• No tests for a new GPU data path increases regression risk.
• The exec_mask signed-cast is a correctness issue for full-wavefront masks.

[vedithal-amd]

Is the rocpd schema version 3.0.2 only honored when user specifies pc sampling OR is it the default? Ideally it should be default. Could you please clarify this @srihari-uttanur

[srihari-uttanur]

Is the rocpd schema version 3.0.2 only honored when user specifies pc sampling OR is it the default? Ideally it should be default.

@vedithal-amd 3.0.2 is the default version after this PR lands and its not tied to pc sampling.

[yhuiYH]

Looks like your added tests ran on ubuntu CI run just fine, but failed on RHEL and SLES. Odd, probably needs a small fix:

The following tests FAILED: 967 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_verified_decode_is_written_back (Failed) 968 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_rebuilt_code_object_is_rejected (Failed) 969 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_remapped_code_object_must_match_recorded_hash[True] (Failed) 970 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_remapped_code_object_must_match_recorded_hash[False] (Failed) 971 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_unverified_decode_is_not_written_back (Failed) 972 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_memory_code_object_size_mismatch_is_rejected (Failed) 973 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_writeback_skips_stored_rows_without_an_index (Failed) 974 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_deleted_input_is_not_recreated (Failed) 975 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_merged_database_caches_every_recording (Failed) 976 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_merged_recordings_keep_their_blob_layouts_and_parents (Failed) 977 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_merge_skips_views_bound_to_one_recording (Failed) 978 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_pc_csv_identifies_recordings (Failed) 980 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_time_window_on_pc_only_database[125-175] (Failed) 981 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_time_window_on_pc_only_database[25%-75%] (Failed) 982 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_time_window_without_timed_records_reports_error (Failed) 983 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_query_rewrite_keeps_qualified_columns_and_ctes (Failed) 984 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_query_rewrite_handles_quoting_qualifiers_comments_and_literals (Failed) 985 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_per_architecture_layouts_share_one_decoded_view (Failed) 986 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_shared_event_blobs_do_not_multiply_rows (Failed) 987 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_failed_isa_setup_publishes_no_partial_view[False] (Failed) 988 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_failed_isa_setup_publishes_no_partial_view[True] (Failed) 989 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_pc_csv_skips_decoded_view_without_isa_columns (Failed) 990 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_csv_and_query_launchers_accept_decode_options (Failed) 991 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_open_without_decoding_does_not_scan_samples (Failed) 992 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_each_pc_is_decoded_once[False] (Failed) 993 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_each_pc_is_decoded_once[True] (Failed) 994 - tests.integration.validate.rocpd-pc-sampling-postprocess.test_decoder_cache_is_bounded_by_bytes (Failed)

[vlaindic]

Thanks for addressing my previous comments. I mostly focused my attention to the PC sampling related part. Overall, I believe the design is solid, so I'm approving the PC sampling part. I asked a few questions for clarification and maybe some future work.

I eyeball the rocpd implementation and would say it is reasonable. I'm also providing AI agent's review comments that I believe are worth considering.

PR #4946 Review — Introduce PC sampling support for rocpd

Critical

None.

Important

1. Missing workflow comment block in generateRocpd.cpp

This PR adds a new ~1075-line subsystem (schema registration → batched/pooled inserts → flush/destruction) with no workflow comment block near write_rocpd() or the top of the file.

Suggested action: Add a comment block documenting:

• Initialization — schema registration and pool setup
• Usage — batching/flush ordering, FK-parent recursion
• Destruction — final flush, pool lifetime

2. Stale copyright header in importer.py

source/lib/python/rocpd/importer.py still carries a 2023 header despite a ~953-line substantial rewrite in this PR (lazy ISA-decode views, disassembly caching/write-back, code-object path remapping).

Suggested action: Update copyright header in importer.py from 2023 to 2026.

3. cache_disassembly defaults to True — mutates input DB by default

importer.py sets cache_disassembly=True by default, so rocpd writes back decoded disassembly into the input database at process exit unless --no-cache-disassembly is passed. This is a real default-on behavioral/runtime change that isn't called out in the PR description.

Suggested action: Call out this default explicitly in the PR description, and confirm whether mutating input DBs by default is intended (vs. requiring an explicit opt-in flag).

4. SQL schema gaps

• New code_object_id columns lack a foreign-key constraint.
• BIGINT/INTEGER type inconsistency across related columns.
• No join view added to data_views.sql for the new PC-sampling tables.
• Ambiguous comment on the dedup key in rocpd_disassembly_data.

Suggested action: Add the FK constraint (or document why it's intentionally omitted), reconcile the BIGINT/INTEGER usage, add the missing join view, and clarify the dedup-key comment.

5. PR description misdescribes the foreign-key mechanism

The description states domain tables reference rocpd_blob_event via a blob_event_id column — this column does not exist. The actual linkage is a shared event_id, present independently on both rocpd_gpu_pc_sample and rocpd_blob_event, each FK'ing to rocpd_event.id.

Suggested action: Correct the PR description to describe the actual event_id linkage.

6. PR description omits subtle cases

The following non-obvious behaviors are only documented in the .rst docs, not called out in the PR description:

• The exec_mask TEXT-vs-numeric comparison gotcha for masks above 2^63.
• The content-hash gating behavior (capture_content_hash).
• The schema version freeze at 3.0.5 → 3.0.6.

Suggested action: Add a "Subtle cases" section to the PR description covering these.

Suggestions

7. query.py — stale header and thin docstrings on tokenizer helpers

Header still reads 2025 (not bumped) despite +389 lines adding a hand-rolled SQL tokenizer/rewriter. update_query_for_blob_views is well-documented, but _sql_tokens/_cte_names lack docstrings covering their edge-case contracts (quoted identifiers, CTEs, nested parens).

Suggested action: Bump header to 2026; add docstrings to _sql_tokens/_cte_names.

8. Undocumented view: rocpd_gpu_pc_sample_blob_decoded

Referenced once in using-rocpd-output-format.rst but never defined in the docs or the README view table.

Suggested action: Add a one-line definition where first mentioned, or add it to the README view table.

9. Minor stale headers (low priority)

metadata.{cpp,hpp}, output_config.{cpp,hpp}, disassembly.hpp still carry 2023–2025 headers. Diffs here are small, so this falls under the minor-edit exemption — not blocking, but worth bumping to 2026 since it's low-cost.

10. Missing unit test for tokenizer edge cases

No dedicated unit test for query.py's tokenizer on quoted identifiers or nested CTEs — only indirect coverage via normal usage.

Suggested action: Extend pc-sampling-rocpd/validate.py with cases covering quoted identifiers and nested CTEs, rather than adding a new test file.

11. (Nice to have) No guard against _field_pairs silently drifting from the decoded view

test_rocpd_vs_json_all_fields asserts every column _field_pairs() maps for a given method is present in rocpd_gpu_pc_sample_decoded — a strong guard against the view silently dropping a field. There's no corresponding guard in the other direction: nothing fails if the decoded view gains a new field that isn't added to _field_pairs(), so a newly-added decoded column could ship with zero JSON-oracle coverage and the test suite would stay green.

Suggested action (optional): Add a lightweight check that diffs rocpd_gpu_pc_sample_decoded's column set against the keys covered by _field_pairs() (excluding the deliberately-excluded instruction/instruction_comment columns), so new decoded columns are forced to either get JSON-oracle coverage or an explicit documented exclusion.

Verified clean (no action needed)

• Thread-safety (code_object_content_hashes locking, serial blob-pool writer).
• build_pc_sample_blob byte-offset/endianness packing.
• Content-hash early-return (capture_content_hash) — correctly scoped to skip only the hash digest math, not the required file read.
• correlation_id/stack_id split — consistent across all 8 create_event() call sites.
• rocpd_indexes.sql unchanged — not a regression; every index in that file is a commented-out template, consistent with project-wide convention.
• Schema version bump 3.0.5 → 3.0.6 correctly registered in ROCPD_COMPAT_TESTED_SCHEMAS; back-compat test enforces it.
• No non-ASCII / zero-width / homoglyph characters found in the PR description or docs text.

Risk / Impact Assessment

Medium-High. Substantial new feature (PC sampling support in rocpd) touching schema, C++ core, and Python import/query paths. Test coverage is strong for the back-compat/schema-gating dimension, but gaps remain on SQL referential integrity and tokenizer edge cases. The highest-impact single item is #3 — cache_disassembly defaulting to True changes runtime behavior on existing databases by default, with an opt-out flag but no feature-flag gating.

resolved all the comments

[vlaindic]

Thanks for addressing my previous comments. I mostly focused my attention to the PC sampling related part. Overall, I believe the design is solid, so I'm approving the PC sampling part. I asked a few questions for clarification and maybe some future work.

I eyeball the rocpd implementation and would say it is reasonable. I'm also providing AI agent's review comments that I believe are worth considering.

PR #4946 Review — Introduce PC sampling support for rocpd

Critical

None.

Important

1. Missing workflow comment block in generateRocpd.cpp

This PR adds a new ~1075-line subsystem (schema registration → batched/pooled inserts → flush/destruction) with no workflow comment block near write_rocpd() or the top of the file.

Suggested action: Add a comment block documenting:

• Initialization — schema registration and pool setup
• Usage — batching/flush ordering, FK-parent recursion
• Destruction — final flush, pool lifetime

2. Stale copyright header in importer.py

source/lib/python/rocpd/importer.py still carries a 2023 header despite a ~953-line substantial rewrite in this PR (lazy ISA-decode views, disassembly caching/write-back, code-object path remapping).

Suggested action: Update copyright header in importer.py from 2023 to 2026.

3. cache_disassembly defaults to True — mutates input DB by default

importer.py sets cache_disassembly=True by default, so rocpd writes back decoded disassembly into the input database at process exit unless --no-cache-disassembly is passed. This is a real default-on behavioral/runtime change that isn't called out in the PR description.

Suggested action: Call out this default explicitly in the PR description, and confirm whether mutating input DBs by default is intended (vs. requiring an explicit opt-in flag).

4. SQL schema gaps

• New code_object_id columns lack a foreign-key constraint.
• BIGINT/INTEGER type inconsistency across related columns.
• No join view added to data_views.sql for the new PC-sampling tables.
• Ambiguous comment on the dedup key in rocpd_disassembly_data.

Suggested action: Add the FK constraint (or document why it's intentionally omitted), reconcile the BIGINT/INTEGER usage, add the missing join view, and clarify the dedup-key comment.

5. PR description misdescribes the foreign-key mechanism

The description states domain tables reference rocpd_blob_event via a blob_event_id column — this column does not exist. The actual linkage is a shared event_id, present independently on both rocpd_gpu_pc_sample and rocpd_blob_event, each FK'ing to rocpd_event.id.

Suggested action: Correct the PR description to describe the actual event_id linkage.

6. PR description omits subtle cases

The following non-obvious behaviors are only documented in the .rst docs, not called out in the PR description:

• The exec_mask TEXT-vs-numeric comparison gotcha for masks above 2^63.
• The content-hash gating behavior (capture_content_hash).
• The schema version freeze at 3.0.5 → 3.0.6.

Suggested action: Add a "Subtle cases" section to the PR description covering these.

Suggestions

7. query.py — stale header and thin docstrings on tokenizer helpers

Header still reads 2025 (not bumped) despite +389 lines adding a hand-rolled SQL tokenizer/rewriter. update_query_for_blob_views is well-documented, but _sql_tokens/_cte_names lack docstrings covering their edge-case contracts (quoted identifiers, CTEs, nested parens).

Suggested action: Bump header to 2026; add docstrings to _sql_tokens/_cte_names.

8. Undocumented view: rocpd_gpu_pc_sample_blob_decoded

Referenced once in using-rocpd-output-format.rst but never defined in the docs or the README view table.

Suggested action: Add a one-line definition where first mentioned, or add it to the README view table.

9. Minor stale headers (low priority)

metadata.{cpp,hpp}, output_config.{cpp,hpp}, disassembly.hpp still carry 2023–2025 headers. Diffs here are small, so this falls under the minor-edit exemption — not blocking, but worth bumping to 2026 since it's low-cost.

10. Missing unit test for tokenizer edge cases

No dedicated unit test for query.py's tokenizer on quoted identifiers or nested CTEs — only indirect coverage via normal usage.

Suggested action: Extend pc-sampling-rocpd/validate.py with cases covering quoted identifiers and nested CTEs, rather than adding a new test file.

11. (Nice to have) No guard against _field_pairs silently drifting from the decoded view

test_rocpd_vs_json_all_fields asserts every column _field_pairs() maps for a given method is present in rocpd_gpu_pc_sample_decoded — a strong guard against the view silently dropping a field. There's no corresponding guard in the other direction: nothing fails if the decoded view gains a new field that isn't added to _field_pairs(), so a newly-added decoded column could ship with zero JSON-oracle coverage and the test suite would stay green.

Suggested action (optional): Add a lightweight check that diffs rocpd_gpu_pc_sample_decoded's column set against the keys covered by _field_pairs() (excluding the deliberately-excluded instruction/instruction_comment columns), so new decoded columns are forced to either get JSON-oracle coverage or an explicit documented exclusion.

Verified clean (no action needed)

• Thread-safety (code_object_content_hashes locking, serial blob-pool writer).
• build_pc_sample_blob byte-offset/endianness packing.
• Content-hash early-return (capture_content_hash) — correctly scoped to skip only the hash digest math, not the required file read.
• correlation_id/stack_id split — consistent across all 8 create_event() call sites.
• rocpd_indexes.sql unchanged — not a regression; every index in that file is a commented-out template, consistent with project-wide convention.
• Schema version bump 3.0.5 → 3.0.6 correctly registered in ROCPD_COMPAT_TESTED_SCHEMAS; back-compat test enforces it.
• No non-ASCII / zero-width / homoglyph characters found in the PR description or docs text.

Risk / Impact Assessment

Medium-High. Substantial new feature (PC sampling support in rocpd) touching schema, C++ core, and Python import/query paths. Test coverage is strong for the back-compat/schema-gating dimension, but gaps remain on SQL referential integrity and tokenizer edge cases. The highest-impact single item is #3 — cache_disassembly defaulting to True changes runtime behavior on existing databases by default, with an opt-out flag but no feature-flag gating.

PR #4946 — feat(rocprofiler-sdk): Introduce PC sampling support for rocpd

Re-reviewed the latest changes with AI agents to verify all prior review comments were addressed. LGTM.

Asked a couple more questions in the inline comments worth considering before merge.

PR #4946 — Review 5586630448: Status of your 15 inline comments

#	File	Topic	Status	Notes
1	.rst	Exec-mask TEXT vs INTEGER storage — performance follow-up	✅ Addressed in docs, no GitHub reply	New "Execution-mask operations" section explicitly states TEXT storage costs more bytes + conversion overhead; recommends measuring real workload.
2	.rst	Schema naming convention — document it	✅ Addressed in docs, no GitHub reply	New text: schema names are "internal diagnostic labels, not a versioned architecture or capability API" — consumers must discover fields via rocpd_info_blob_field.
3	.rst	(misc doc wording)	✅ Addressed, resolved	—
4	.rst	memory://-only code objects not copied/snapshotted	✅ Addressed in docs now, no GitHub reply	New text: "Pure memory:// objects have no file to copy in the current implementation; preserving their loaded bytes requires a separate capture-time snapshot feature." Directly answers your question.
5	.rst	Are code-object files copied by default when moving a recording?	✅ Addressed in docs now, no GitHub reply	New text: "ROCpd capture and database merge/package operations do not automatically bundle the referenced binaries." Directly answers it — not automatic.
6	generateRocpd.cpp	Formatting nit	✅ Addressed (verified via clang-format diff)	—
7	generateRocpd.cpp	(misc)	✅ Addressed, resolved	—
8	generateRocpd.cpp	Formatting nit	✅ Addressed (verified via clang-format diff)	—
9	generateRocpd.cpp	Formatting nit	✅ Addressed (verified via clang-format diff)	—
10	generateRocpd.cpp	Formatting nit	✅ Addressed (verified via clang-format diff)	—
11	generateRocpd.cpp	GFX9-only fields leaking into GFX12/1250 output	✅ Addressed — now if constexpr(Arch == pc_sample_arch::gfx9) template guards	Verified lines ~1240-1400.
12	generateRocpd.cpp	Same, arb_state_issue_*/arb_state_stall_matrix	✅ Addressed — same template refactor	—
13	generateRocpd.cpp	Runtime branching → should be compile-time	✅ Addressed — template <pc_sample_arch Arch, typename SampleT> + if constexpr	—
14	generateRocpd.cpp	Missing agent/code-object attribution edge case — document fallback behavior	✅ Addressed in docs, no GitHub reply	New "Code-object agent attribution" section: "ROCpd does not invent a code-object or agent mapping... agent_id remains NULL."
15	generateRocpd.cpp	Reuse tool.cpp's agent-lookup pattern instead of reimplementing	✅ Addressed — matches your suggested pattern exactly	Verified get_code_object_agent() + get_agent() lookup at ~3230-3300.

Bottom line

All 15 threads are substantively resolved in code or docs as of the current HEAD (deb9096f717, "Address more comments"). Nothing is open or unanswered. Threads 1, 2, 4, 5, and 14 were fixed in the latest commit but never got a GitHub reply or "Resolve conversation" click.

[yhuiYH]

Please resolve merge conflicts too.