[rocprofiler-sdk] users/mcao/fix_rocprofv_counter_yaml

[mcao59]

To be closed. See new PR: #5551

Motivation

Fix SIGSEGV crash in rocprofv3 when invalid counter YAML files are provided via the -E flag. Invalid counter definitions should be handled gracefully with warnings rather than aborting the program.

Technical Details

YAML Validation Framework:

Added comprehensive validation logic in metrics.cpp and dimensions.cpp according to the following validation design philosophy:

• Reject / warn
  • parse failure
  • missing/wrong top-level counters node
  • missing name
  • missing/wrong definitions
  • empty or invalid architectures
  • missing both event and expression
  • event without block
  • block without event
  • bad field types
  • duplicate counter definitions that are being ignored
• Ignore silently
  • unknown optional fields
  • extra keys not used by current implementation
• Keep non-fatal
  • one bad counter should not abort the entire file or tool run

Crash Fix:

Changed ROCP_DFATAL to ROCP_WARNING in helpers.cpp:47 when AQL profile queries fail. This allows exceptions to propagate properly for error handling in RelWithDebInfo builds instead of aborting the program.

Test Coverage:

Added 11 tests for extra counters. Extra Counters Test Summary:

• Tests with both Execute + Validate:
  • (existing) rocprofv3-test-counter-collection-pmc1-extra-counters - Happy path: basic extra counter loading and collection
  • rocprofv3-test-yaml-validation-mixed-valid-invalid - Graceful degradation: valid counters work despite invalid ones in same YAML
  • rocprofv3-test-yaml-validation-duplicate-counters - Duplicate handling: duplicate counter definitions don't corrupt output
• Execute-only tests (smoke tests - verify no crashes):
  • rocprofv3-test-yaml-validation-invalid-block - Invalid block name
  • rocprofv3-test-yaml-validation-missing-name - Counter missing name field
  • rocprofv3-test-yaml-validation-wrong-type - Field has wrong type
  • rocprofv3-test-yaml-validation-malformed - Malformed YAML syntax
  • rocprofv3-test-yaml-validation-no-block-event - Counter with neither block/event nor expression
  • rocprofv3-test-yaml-validation-missing-top-key - Missing rocprofiler-sdk top-level key
  • rocprofv3-test-yaml-validation-empty-architectures - Empty architectures list
  • rocprofv3-test-yaml-validation-expression-plus-event - Counter with both expression and event
  • rocprofv3-test-yaml-validation-block-without-event - Block specified without event number

JIRA ID

AIPROFSDK-22

Test Plan

Test Result

All YAML validation tests pass without crashes. Invalid counter definitions now produce appropriate warning messages and gracefully skip the invalid counters instead of aborting the program.

Submission Checklist

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

[bwelton]

Invalid counter definitions should be handled gracefully with warnings rather than aborting the program.

Why do we want to do this instead of aborting? Aborting signals there is a problem right away and handling gracefully here seems like it could cause someone to execute rocprofiler, run it for 2 days, then come back and see no counters and wonder why.

This allows exceptions to propagate properly for error handling in RelWithDebInfo builds instead of aborting the program.

We want these explicitly to fail very loudly.

[mcao59]

Invalid counter definitions should be handled gracefully with warnings rather than aborting the program.

Why do we want to do this instead of aborting? Aborting signals there is a problem right away and handling gracefully here seems like it could cause someone to execute rocprofiler, run it for 2 days, then come back and see no counters and wonder why.

This allows exceptions to propagate properly for error handling in RelWithDebInfo builds instead of aborting the program.

We want these explicitly to fail very loudly.

I see your point that we should fail loudly for serious invalid input. My concern is that the current abort path can fail with misleading/internal error messages, which makes the real problem harder to diagnose.
Perhaps this should be case by case:

• malformed YAML / missing required fields / unusable definitions should fail early with a clear error,
• while less severe cases like duplicate definitions could be warning-and-ignore.

[MythreyaK]

Related: #4039

[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.

[github-actions]

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.