Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pfm parsing for perf stat codes per instruction #33

Merged
merged 5 commits into from
Dec 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ bin
__pycache__
dist
phlop.egg-info/
scope_timer.txt

*scope_timer.txt
tpp
32 changes: 26 additions & 6 deletions inc/phlop/timing/threaded_scope_timer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ struct ScopeTimerMan
}
_headers.clear();
thread_storage.clear();
thread_reports.clear();
active = false;
}

Expand Down Expand Up @@ -132,10 +133,16 @@ struct ScopeTimerMan
std::unique_lock<std::mutex> lk(work_);
thread_storage.emplace_back(std::move(pt.reports), std::move(pt.traces));
}
void move(std::shared_ptr<RunTimerReport>& report)
{
std::unique_lock<std::mutex> lk(work_);
thread_reports.emplace_back(std::move(report));
}
Comment on lines +136 to +140
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider passing report by value to clarify ownership transfer

In the void move(std::shared_ptr<RunTimerReport>& report) method, report is moved into thread_reports, which can leave the original shared pointer in a moved-from state. To make ownership transfer explicit and safer, consider passing report by value.

Apply this diff to update the method signature and usage:

-    void move(std::shared_ptr<RunTimerReport>& report)
+    void move(std::shared_ptr<RunTimerReport> report)
     {
         std::unique_lock<std::mutex> lk(work_);
         thread_reports.emplace_back(std::move(report));
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void move(std::shared_ptr<RunTimerReport>& report)
{
std::unique_lock<std::mutex> lk(work_);
thread_reports.emplace_back(std::move(report));
}
void move(std::shared_ptr<RunTimerReport> report)
{
std::unique_lock<std::mutex> lk(work_);
thread_reports.emplace_back(std::move(report));
}


std::mutex work_;
std::vector<std::pair<std::vector<RunTimerReport*>, std::vector<RunTimerReportSnapshot*>>>
thread_storage;
std::vector<std::shared_ptr<RunTimerReport>> thread_reports; // keep alive
};


Expand All @@ -159,9 +166,11 @@ struct RunTimerReportSnapshot
std::vector<RunTimerReportSnapshot*> childs;
};


struct RunTimerReport
{
std::string_view k, f;
std::string const k; // key
std::string const f; // function
std::uint32_t l = 0;

RunTimerReport(std::string_view const& _k, std::string_view const& _f, std::uint32_t const& _l)
Expand All @@ -175,13 +184,20 @@ struct RunTimerReport

~RunTimerReport() {}


auto operator()(std::size_t i) { return snapshots[i].get(); }
auto size() { return snapshots.size(); }

std::vector<std::shared_ptr<RunTimerReportSnapshot>> snapshots; // emplace back breaks pointers!
};


struct ThreadLifeWatcher
{
~ThreadLifeWatcher() { ScopeTimerMan::INSTANCE().move(report); }

std::shared_ptr<RunTimerReport> report;
};


struct scope_timer
Expand Down Expand Up @@ -264,7 +280,9 @@ struct BinaryTimerFile
template<typename Trace>
void recurse_traces_for_keys(Trace const& c)
{
std::string s{c->self->k};
assert(c);
assert(c->self);
Comment on lines +283 to +284
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Replace assert with explicit null checks for safety in release builds

Using assert to check for null pointers means these checks are disabled in release builds where NDEBUG is defined. This may result in null pointer dereferences. Consider replacing assert with explicit null checks to ensure safety in all build configurations.

Apply this diff to handle null pointers explicitly:

-    assert(c);
-    assert(c->self);
+    if (!c || !c->self) {
+        // Handle the error appropriately, e.g., log the error or throw an exception
+        throw std::runtime_error("Null pointer encountered in recurse_traces_for_keys");
+    }

Committable suggestion skipped: line range outside the PR's diff.

auto const& s = c->self->k;
if (!key_ids.count(s))
{
auto [it, b] = key_ids.emplace(s, key_ids.size());
Expand Down Expand Up @@ -359,11 +377,13 @@ namespace detail
#endif

#define PHLOP_SCOPE_TIMER(key) \
static phlop::threaded::RunTimerReport PHLOP_STR_CAT(ridx_, __LINE__){key, __FILE__, \
__LINE__}; \
static thread_local auto PHLOP_STR_CAT(ridx_, __LINE__) \
= std::make_shared<phlop::threaded::RunTimerReport>(key, __FILE__, __LINE__); \
static thread_local phlop::threaded::ThreadLifeWatcher PHLOP_STR_CAT(_watcher_, __LINE__){ \
PHLOP_STR_CAT(ridx_, __LINE__)}; \
phlop::threaded::scope_timer PHLOP_STR_CAT(_scope_timer_, \
__LINE__){PHLOP_STR_CAT(ridx_, __LINE__)}; \
phlop::threaded::ScopeTimerMan::local().report_stack_ptr = &PHLOP_STR_CAT(ridx_, __LINE__);
__LINE__){*PHLOP_STR_CAT(ridx_, __LINE__)}; \
phlop::threaded::ScopeTimerMan::local().report_stack_ptr = PHLOP_STR_CAT(ridx_, __LINE__).get();


#endif /*_PHLOP_TIMING_THREADED_SCOPE_TIMER_HPP_*/
29 changes: 29 additions & 0 deletions mkn.pfm.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#! clean build test run -p scope_timer,threaded_scope_timer -Oa "-fPIC -std=c++20" -W 9

# run script first: ./sh/setup_pfm.sh

name: phlop.pfm
parent: base

profile:
- name: base
inc: inc

- name: pfm
inc: tpp/pfm/include

- name: pfm_lib
parent: pfm
mode: static
inc: tpp/pfm/lib, 0
src: tpp/pfm/lib

- name: pfm_events
self: pfm_lib
main: tpp/pfm/examples/check_events.c
out: check_events

- name: pfm_info
self: pfm_lib
main: tpp/pfm/examples/showevtinfo.c
out: showevtinfo
Comment on lines +12 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing directories and version constraint needed

The verification reveals two issues:

  • None of the referenced directories (tpp/pfm/include, tpp/pfm/lib, tpp/pfm/examples) exist in the repository
  • The setup script clones libpfm4 without specifying a version, which could lead to build reproducibility issues

Recommendations:

  • Run the setup script first to create the required directories
  • Add a specific git tag or commit hash in the clone command to ensure consistent builds
🔗 Analysis chain

Verify path existence and add version constraints

The configuration references several paths under tpp/pfm/ but lacks version constraints for the libpfm4 dependency.

Consider adding version constraints for libpfm4 to ensure build reproducibility.


Let me check the setup script to understand how libpfm is being handled.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify referenced paths and check libpfm version information

# Check if referenced directories exist
echo "Checking referenced directories..."
for dir in "tpp/pfm/include" "tpp/pfm/lib" "tpp/pfm/examples"; do
  if [ -d "$dir" ]; then
    echo "$dir exists"
  else
    echo "$dir not found"
  fi
done

# Check for version information in setup script
echo -e "\nChecking for version constraints..."
rg -l "libpfm" ./sh/setup_pfm.sh

Length of output: 681


Script:

#!/bin/bash
# Check the content of setup script to understand libpfm handling
cat ./sh/setup_pfm.sh

Length of output: 275

2 changes: 2 additions & 0 deletions phlop/app/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
phlop.app.cmake
phlop.app.test_cases
phlop.app.git
phlop.app.nvidia
phlop.app.pfm
phlop.app.perf"""

print(available_modules)
5 changes: 5 additions & 0 deletions phlop/app/nvidia/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#
#
#
#
#
12 changes: 12 additions & 0 deletions phlop/app/nvidia/__main__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#
#
#
#
#


available_modules = """Available:
phlop.app.nvidia.csan
phlop.app.nvidia.ncu"""

print(available_modules)
63 changes: 63 additions & 0 deletions phlop/app/nvidia/csan.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# compute sanitizer frontend

# https://docs.nvidia.com/compute-sanitizer/ComputeSanitizer/index.html

## samples
# compute-sanitizer --tool memcheck [sanitizer_options] app_name [app_options]
# compute-sanitizer --tool racecheck [sanitizer_options] app_name [app_options]
#
#
#


from phlop.dict import ValDict
from phlop.proc import run

metrics = [
"all",
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum", # read
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum", # wrte
]
Comment on lines +16 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove inappropriate metrics list

The metrics list appears to be copied from ncu.py but is not relevant for the Compute Sanitizer, which focuses on memory checking and race detection rather than performance metrics.

-metrics = [
-    "all",
-    "l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum",  # read
-    "l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum",  # wrte
-]



def build_command(cli_args):
cmd_parts = [
"compute-sanitizer",
f"--tool {cli_args.tool}",
cli_args.extra if cli_args.extra else "",
" ".join(cli_args.remaining) if cli_args.remaining else "",
]
return " ".join(filter(None, cmd_parts))


def exec(cli_args):
return run(build_command(cli_args), check=True, cwd=cli_args.dir)


def cli_args_parser(description="compute-sanitizer tool"):
import argparse

_help = ValDict(
dir="working directory",
quiet="Redirect output to /dev/null",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="Sanitizer tool to use (memcheck, racecheck, initcheck, synccheck)",
extra="forward string to csan command",
)

parser = argparse.ArgumentParser(
description=description, formatter_class=argparse.ArgumentDefaultsHelpFormatter
)
parser.add_argument("remaining", nargs=argparse.REMAINDER)
parser.add_argument("-d", "--dir", default=".", help=_help.dir)
parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
parser.add_argument("-t", "--tool", default="memcheck", help=_help.tool)
parser.add_argument("--logging", type=int, default=1, help=_help.logging)
parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)

return parser


def verify_cli_args(cli_args):
return cli_args
Comment on lines +62 to +63
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Implement proper argument verification

The verify_cli_args function should validate the tool type against supported sanitizer tools.

 def verify_cli_args(cli_args):
+    """Verify command line arguments.
+    
+    Args:
+        cli_args: Parsed command line arguments
+    
+    Returns:
+        Verified arguments
+    
+    Raises:
+        ValueError: If tool type is not supported
+    """
+    supported_tools = ['memcheck', 'racecheck', 'initcheck', 'synccheck']
+    if cli_args.tool and cli_args.tool not in supported_tools:
+        raise ValueError(f"Unsupported tool type. Must be one of: {supported_tools}")
     return cli_args
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def verify_cli_args(cli_args):
return cli_args
def verify_cli_args(cli_args):
"""Verify command line arguments.
Args:
cli_args: Parsed command line arguments
Returns:
Verified arguments
Raises:
ValueError: If tool type is not supported
"""
supported_tools = ['memcheck', 'racecheck', 'initcheck', 'synccheck']
if cli_args.tool and cli_args.tool not in supported_tools:
raise ValueError(f"Unsupported tool type. Must be one of: {supported_tools}")
return cli_args

57 changes: 57 additions & 0 deletions phlop/app/nvidia/ncu.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Nsight Compute CLI

# https://docs.nvidia.com/nsight-compute/ProfilingGuide/index.html

## samples
# ncu --help
# ncu --metrics all
# ncu --metrics l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum
# ncu --target-processes all -o <report-name> mpirun [mpi arguments] <app> [app arguments]
#


from phlop.dict import ValDict
from phlop.proc import run

metrics = [
"all",
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum", # read
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum", # wrte
]
Comment on lines +16 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance metrics validation and documentation

The metrics list is currently limited and lacks validation in the code. Consider:

  1. Adding more commonly used metrics
  2. Adding validation against this list
  3. Documenting each metric's purpose

Apply this diff to improve metrics handling:

+from typing import List
+
+class NCUMetric:
+    def __init__(self, name: str, description: str):
+        self.name = name
+        self.description = description
+
 metrics = [
-    "all",
-    "l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum",  # read
-    "l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum",  # wrte
+    NCUMetric("all", "All available metrics"),
+    NCUMetric(
+        "l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum",
+        "Number of shared memory bank conflicts on read operations"
+    ),
+    NCUMetric(
+        "l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum",
+        "Number of shared memory bank conflicts on write operations"
+    ),
 ]
+
+def validate_metrics(metric_names: List[str]) -> None:
+    """Validate that all provided metrics are supported."""
+    valid_metrics = {m.name for m in metrics}
+    invalid_metrics = set(metric_names) - valid_metrics
+    if invalid_metrics:
+        raise ValueError(f"Unsupported metrics: {invalid_metrics}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
metrics = [
"all",
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum", # read
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum", # wrte
]
from typing import List
class NCUMetric:
def __init__(self, name: str, description: str):
self.name = name
self.description = description
metrics = [
NCUMetric("all", "All available metrics"),
NCUMetric(
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_ld.sum",
"Number of shared memory bank conflicts on read operations"
),
NCUMetric(
"l1tex__data_bank_conflicts_pipe_lsu_mem_shared_op_st.sum",
"Number of shared memory bank conflicts on write operations"
),
]
def validate_metrics(metric_names: List[str]) -> None:
"""Validate that all provided metrics are supported."""
valid_metrics = {m.name for m in metrics}
invalid_metrics = set(metric_names) - valid_metrics
if invalid_metrics:
raise ValueError(f"Unsupported metrics: {invalid_metrics}")



def build_command(cli_args):
return f"ncu {cli_args.remaining}"


def exec(cli_args):
return run(build_command(cli_args), check=True)

Comment on lines +23 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance command building and execution robustness

The current implementation lacks:

  1. Validation of cli_args.remaining
  2. Error handling for command execution
  3. Support for metrics from the defined list

Consider this improved implementation:

 def build_command(cli_args):
-    return f"ncu {cli_args.remaining}"
+    if not cli_args.remaining:
+        raise ValueError("No command specified to profile")
+    
+    cmd_parts = ["ncu"]
+    if cli_args.outfile:
+        cmd_parts.extend(["-o", cli_args.outfile])
+    if cli_args.extra:
+        cmd_parts.append(cli_args.extra)
+    cmd_parts.extend(cli_args.remaining)
+    return " ".join(cmd_parts)

 def exec(cli_args):
-    return run(build_command(cli_args), check=True)
+    try:
+        cmd = build_command(cli_args)
+        return run(cmd, check=True)
+    except Exception as e:
+        raise RuntimeError(f"Failed to execute ncu command: {e}") from e

Committable suggestion skipped: line range outside the PR's diff.


def cli_args_parser(description="ncu tool"):
import argparse

_help = ValDict(
dir="working directory",
quiet="Redirect output to /dev/null",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="",
extra="forward string to csan command",
)
Comment on lines +34 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve help text definitions

The help texts need improvement and some are undefined.

Apply this diff to fix the help texts:

     _help = ValDict(
         dir="working directory",
         quiet="Redirect output to /dev/null",
         logging="0=off, 1=on non zero exit code, 2=always",
         outfile="path for saved file if active",
-        tool="",
-        extra="forward string to csan command",
+        tool="ncu tool type (e.g., profile, stats)",
+        extra="additional arguments to pass to ncu command",
+        infiles="input files to process",
     )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
_help = ValDict(
dir="working directory",
quiet="Redirect output to /dev/null",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="",
extra="forward string to csan command",
)
_help = ValDict(
dir="working directory",
quiet="Redirect output to /dev/null",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="ncu tool type (e.g., profile, stats)",
extra="additional arguments to pass to ncu command",
infiles="input files to process",
)


parser = argparse.ArgumentParser(
description=description, formatter_class=argparse.ArgumentDefaultsHelpFormatter
)
parser.add_argument("remaining", nargs=argparse.REMAINDER)
parser.add_argument("-d", "--dir", default=".", help=_help.dir)
parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
parser.add_argument("--logging", type=int, default=1, help=_help.logging)
parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)

return parser

Comment on lines +31 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve CLI argument parser implementation

The current implementation has several issues:

  1. Incomplete help texts
  2. Potentially irrelevant arguments
  3. Missing metrics-specific arguments

Apply this diff to improve the implementation:

 def cli_args_parser(description="ncu tool"):
+    """Create argument parser for ncu tool.
+    
+    Args:
+        description: Parser description
+        
+    Returns:
+        ArgumentParser instance
+    """
     import argparse
 
     _help = ValDict(
         dir="working directory",
         quiet="Redirect output to /dev/null",
         logging="0=off, 1=on non zero exit code, 2=always",
-        outfile="path for saved file if active",
-        tool="",
-        extra="forward string to csan command",
+        outfile="path for saving ncu report (must end with .ncu-rep)",
+        extra="additional arguments to pass to ncu command (e.g., --metrics all)",
     )
 
     parser = argparse.ArgumentParser(
         description=description, formatter_class=argparse.ArgumentDefaultsHelpFormatter
     )
     parser.add_argument("remaining", nargs=argparse.REMAINDER)
     parser.add_argument("-d", "--dir", default=".", help=_help.dir)
     parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
-    parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
     parser.add_argument("--logging", type=int, default=1, help=_help.logging)
     parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
+    
+    # Add metrics-specific arguments group
+    metrics_group = parser.add_argument_group('Metrics')
+    metrics_group.add_argument(
+        "--list-metrics",
+        action="store_true",
+        help="List available metrics and exit"
+    )
 
     return parser
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def cli_args_parser(description="ncu tool"):
import argparse
_help = ValDict(
dir="working directory",
quiet="Redirect output to /dev/null",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="",
extra="forward string to csan command",
)
parser = argparse.ArgumentParser(
description=description, formatter_class=argparse.ArgumentDefaultsHelpFormatter
)
parser.add_argument("remaining", nargs=argparse.REMAINDER)
parser.add_argument("-d", "--dir", default=".", help=_help.dir)
parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
parser.add_argument("--logging", type=int, default=1, help=_help.logging)
parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
return parser
def cli_args_parser(description="ncu tool"):
"""Create argument parser for ncu tool.
Args:
description: Parser description
Returns:
ArgumentParser instance
"""
import argparse
_help = ValDict(
dir="working directory",
quiet="Redirect output to /dev/null",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saving ncu report (must end with .ncu-rep)",
extra="additional arguments to pass to ncu command (e.g., --metrics all)",
)
parser = argparse.ArgumentParser(
description=description, formatter_class=argparse.ArgumentDefaultsHelpFormatter
)
parser.add_argument("remaining", nargs=argparse.REMAINDER)
parser.add_argument("-d", "--dir", default=".", help=_help.dir)
parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
parser.add_argument("--logging", type=int, default=1, help=_help.logging)
parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
# Add metrics-specific arguments group
metrics_group = parser.add_argument_group('Metrics')
metrics_group.add_argument(
"--list-metrics",
action="store_true",
help="List available metrics and exit"
)
return parser


def verify_cli_args(cli_args):
return cli_args
Comment on lines +56 to +57
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement proper CLI arguments verification

The current verification function doesn't perform any checks.

Consider this improved implementation:

 def verify_cli_args(cli_args):
-    return cli_args
+    if not cli_args.remaining:
+        raise ValueError("No command specified to profile")
+    
+    if cli_args.outfile and not cli_args.outfile.endswith(('.ncu-rep', '.nsight-cuprof-report')):
+        raise ValueError("Output file must have .ncu-rep or .nsight-cuprof-report extension")
+    
+    return cli_args
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def verify_cli_args(cli_args):
return cli_args
def verify_cli_args(cli_args):
if not cli_args.remaining:
raise ValueError("No command specified to profile")
if cli_args.outfile and not cli_args.outfile.endswith(('.ncu-rep', '.nsight-cuprof-report')):
raise ValueError("Output file must have .ncu-rep or .nsight-cuprof-report extension")
return cli_args

6 changes: 3 additions & 3 deletions phlop/app/perf.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,10 @@ def cli_args_parser(description="Perf tool"):
quiet="Redirect output to /dev/null",
cores="Parallism core/thread count",
infiles="infiles",
print_only="Print only, no execution",
regex="Filter out non-matching execution strings",
logging="0=off, 1=on non zero exit code, 2=always",
outfile="path for saved file if active",
tool="stat/record/etc",
extra="forward string to perf command",
)

parser = argparse.ArgumentParser(
Expand All @@ -129,10 +128,11 @@ def cli_args_parser(description="Perf tool"):
"-p", "--print_only", action="store_true", default=False, help=_help.print_only
)
parser.add_argument("-i", "--infiles", default=None, help=_help.infiles)
parser.add_argument("-r", "--regex", default=None, help=_help.regex)
parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
parser.add_argument("--logging", type=int, default=1, help=_help.logging)
parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)

Comment on lines +134 to +135
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Extra argument needs input validation but shell injection is properly handled

The review comment's security concerns are partially valid, but the shell command execution is actually safe:

  1. The extra parameter is used in perf command construction through string formatting (as seen in stat_cmd and record_cmd functions)
  2. Command execution is handled safely through phlop/proc.py which uses subprocess.run with proper parameter handling
  3. The suggestion to move the argument up for better organization is valid

However, input validation is still recommended to ensure only valid perf options are passed. Here's a revised suggestion:

     parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
     parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
+    parser.add_argument("-e", "--extra", type=str, default="",
+                        help=_help.extra,
+                        # Validate perf options
+                        choices=["", "-A", "-g", "--per-thread", "--per-core"])
     parser.add_argument("--logging", type=int, default=1, help=_help.logging)
-
-    parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
🔗 Analysis chain

Security and validation concerns with the --extra argument.

Forwarding raw strings to shell commands without validation could pose security risks. Additionally, consider:

  1. Moving this argument up with the other related arguments for better code organization
  2. Adding input validation to prevent command injection

Let's check if the extra options are used in shell commands:

Consider applying this diff for better organization and basic validation:

     parser.add_argument("-o", "--outfile", default=None, help=_help.outfile)
     parser.add_argument("-t", "--tool", default="stat", help=_help.tool)
+    parser.add_argument("-e", "--extra", type=str, default="",
+                        help=_help.extra,
+                        # Basic validation to prevent command injection
+                        choices=["-A", "-g", "--per-thread"])
     parser.add_argument("--logging", type=int, default=1, help=_help.logging)

-    parser.add_argument("-e", "--extra", type=str, default="", help=_help.extra)
-
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for shell=True usage with the extra parameter
rg -A 5 "shell=True.*extra" 

Length of output: 29


Script:

#!/bin/bash
# Let's search more broadly for usage of the 'extra' parameter
rg -A 5 '\bextra\b' phlop/app/perf.py

# Also check for subprocess calls that might use the extra parameter
ast-grep --pattern 'subprocess.run($$$)'

Length of output: 490


Script:

#!/bin/bash
# Let's check how the extra parameter is used in command construction
rg -B 10 -A 10 'cmd.*=.*extra' phlop/app/perf.py

# Also check the verify_cli_args function to see if there's any validation
ast-grep --pattern 'def verify_cli_args($_) {
  $$$
}'

Length of output: 107


Script:

#!/bin/bash
# Let's check the entire perf.py file to understand the command construction
rg -B 5 -A 5 'subprocess|cmd.*=' phlop/app/perf.py

# Also check for any imports of subprocess or command execution related modules
rg '^from.*import|^import.*' phlop/app/perf.py

Length of output: 998


Script:

#!/bin/bash
# Let's check how the extra parameter flows through the code
rg -B 5 -A 10 'def main' phlop/app/perf.py

# Also check the proc.py module since it handles command execution
rg -l 'run.*=' phlop/proc.py && cat phlop/proc.py

Length of output: 2030

return parser


Expand Down
5 changes: 5 additions & 0 deletions phlop/app/pfm/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#
#
#
#
#
12 changes: 12 additions & 0 deletions phlop/app/pfm/__main__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#
#
#
#
#


available_modules = """Available:
phlop.app.pfm.check_events
phlop.app.pfm.showevtinfo"""

print(available_modules)
48 changes: 48 additions & 0 deletions phlop/app/pfm/check_events.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#
#
#
#
#


import logging
from pathlib import Path

from phlop.os import pushd
from phlop.proc import run
from phlop.string import decode_bytes

FILE_DIR = Path(__file__).resolve().parent

logger = logging.getLogger(__name__)
check_events_start = "Total events:"


def parse_check_events_output(lines):
return lines[-1].split(":")[1].strip().replace("0x", "r")

Comment on lines +21 to +23
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add type hints and docstring.

The function lacks proper documentation and type hints. It also assumes the input will always have at least one line and the expected format.

-def parse_check_events_output(lines):
+def parse_check_events_output(lines: list[str]) -> str:
+    """Parse the output from check_events command.
+
+    Args:
+        lines: List of output lines from check_events command
+
+    Returns:
+        Parsed event code in perf stat compatible format
+
+    Raises:
+        IndexError: If lines list is empty
+        ValueError: If output format is unexpected
+    """
+    if not lines:
+        raise ValueError("Empty output from check_events")
+    try:
         return lines[-1].split(":")[1].strip().replace("0x", "r")
+    except (IndexError, KeyError) as e:
+        raise ValueError(f"Unexpected output format: {lines[-1]}") from e
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def parse_check_events_output(lines):
return lines[-1].split(":")[1].strip().replace("0x", "r")
def parse_check_events_output(lines: list[str]) -> str:
"""Parse the output from check_events command.
Args:
lines: List of output lines from check_events command
Returns:
Parsed event code in perf stat compatible format
Raises:
IndexError: If lines list is empty
ValueError: If output format is unexpected
"""
if not lines:
raise ValueError("Empty output from check_events")
try:
return lines[-1].split(":")[1].strip().replace("0x", "r")
except (IndexError, KeyError) as e:
raise ValueError(f"Unexpected output format: {lines[-1]}") from e


def run_check_events(code):
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(
run(f"./tpp/pfm/examples/check_events {code}").stdout
).splitlines()
Comment on lines +25 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve path handling and add error handling.

The function uses a hardcoded path and lacks proper error handling for command execution failures.

-def run_check_events(code):
+def run_check_events(code: str) -> list[str]:
+    """Run check_events command with the given code.
+
+    Args:
+        code: Event code to check
+
+    Returns:
+        List of output lines from the command
+
+    Raises:
+        FileNotFoundError: If check_events executable is not found
+        subprocess.CalledProcessError: If command execution fails
+    """
+    check_events_path = FILE_DIR.parent.parent.parent / "tpp" / "pfm" / "examples" / "check_events"
+    if not check_events_path.exists():
+        raise FileNotFoundError(f"check_events not found at {check_events_path}")
+
     with pushd(FILE_DIR.parent.parent.parent):
         return decode_bytes(
-            run(f"./tpp/pfm/examples/check_events {code}").stdout
+            run(f"{check_events_path} {code}", check=True).stdout
         ).splitlines()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def run_check_events(code):
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(
run(f"./tpp/pfm/examples/check_events {code}").stdout
).splitlines()
def run_check_events(code: str) -> list[str]:
"""Run check_events command with the given code.
Args:
code: Event code to check
Returns:
List of output lines from the command
Raises:
FileNotFoundError: If check_events executable is not found
subprocess.CalledProcessError: If command execution fails
"""
check_events_path = FILE_DIR.parent.parent.parent / "tpp" / "pfm" / "examples" / "check_events"
if not check_events_path.exists():
raise FileNotFoundError(f"check_events not found at {check_events_path}")
with pushd(FILE_DIR.parent.parent.parent):
return decode_bytes(
run(f"{check_events_path} {code}", check=True).stdout
).splitlines()



def get_evt_perf_code(code):
return parse_check_events_output(run_check_events(code))
Comment on lines +32 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add docstring and improve error handling.

The function should have proper documentation and propagate errors from its dependencies.

-def get_evt_perf_code(code):
+def get_evt_perf_code(code: str) -> str:
+    """Get performance event code in perf stat compatible format.
+
+    Args:
+        code: Raw event code
+
+    Returns:
+        Event code formatted for perf stat
+
+    Raises:
+        ValueError: If event code is invalid or empty
+    """
+    if not code:
+        raise ValueError("Event code cannot be empty")
     return parse_check_events_output(run_check_events(code))

Committable suggestion skipped: line range outside the PR's diff.



if __name__ == "__main__":
from phlop.app.pfm.showevtinfo import get_evt_info

key, code = "[MULT_FLOPS]", ""
for info in get_evt_info():
if key in info.umask:
code = f"{info.name}:{info.umask[key].code}"
break

assert code != ""

# print("get_evt_perf_code", get_evt_perf_code(code))
print(run(f"perf stat -e {get_evt_perf_code(code)} sleep 5"))
Loading
Loading