Skip to content

Add Stats Level config value for use with stats counter collection. #207

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.

Sign up for GitHub

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

Open
zbyrne wants to merge 2 commits into
base: develop
Choose a base branch
from
+108 −1
Open

Add Stats Level config value for use with stats counter collection. #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3cee07c
Add HIPFILE_STATS_LEVEL environment variable.
zbyrne Mar 3, 2026
bec9342
Add StatsLevel to Configuration.
zbyrne Mar 3, 2026
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
13 changes: 12 additions & 1 deletion src/amd_detail/configuration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

using namespace hipFile;

Configuration::Configuration() : m_fastpath(true), m_fallback(true)
Configuration::Configuration() : m_fastpath(true), m_fallback(true), m_statsLevel(0)
{
auto maybe_env_force_compat{Environment::force_compat_mode()};
if (maybe_env_force_compat && maybe_env_force_compat.value()) {
Expand All @@ -24,6 +24,11 @@ Configuration::Configuration() : m_fastpath(true), m_fallback(true)
if (maybe_env_allow_compat && !maybe_env_allow_compat.value()) {
m_fallback = false;
}

auto maybe_stats_level{Environment::stats_level()};
if (maybe_stats_level) {
m_statsLevel = maybe_stats_level.value();
}
}

bool
Expand All @@ -37,3 +42,9 @@ Configuration::fallback() const noexcept
{
return m_fallback;
}

int
Configuration::statsLevel() const noexcept
{
return m_statsLevel;
}
6 changes: 6 additions & 0 deletions src/amd_detail/configuration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,23 @@ class IConfiguration {
/// @brief Checks if the fallback backend is enabled
/// @return true if the fallback backend is enabled, false otherwise
virtual bool fallback() const noexcept = 0;

/// @brief Shows the level of detail for stats collection
/// @return 0 if stats collection disabled, higher levels of detail as value increases
virtual int statsLevel() const noexcept = 0;
Comment on lines +24 to +26
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Adding statsLevel() as a pure virtual method to IConfiguration makes MConfiguration in test/amd_detail/mconfiguration.h abstract and unusable, since that mock does not yet provide a MOCK_METHOD for statsLevel(). While MConfiguration is not currently used in any test .cpp file, it will fail to compile if it ever gets included. The MConfiguration mock should be updated to add MOCK_METHOD(int, statsLevel, (), (const, noexcept, override));.

Copilot uses AI. Check for mistakes.
};

class Configuration : public IConfiguration {
bool m_fastpath;
bool m_fallback;
int m_statsLevel;

public:
Configuration();

bool fastpath() const noexcept override;
bool fallback() const noexcept override;
int statsLevel() const noexcept override;
};

}
27 changes: 27 additions & 0 deletions src/amd_detail/environment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ Environment::get<bool>(const char *key)
return std::nullopt;
}

template <>
std::optional<int>
Environment::get<int>(const char *key)
{
const char *value{Context<Sys>::get()->getenv(key)};
if (value == nullptr)
return std::nullopt;
errno = 0;
char *end;
int x{static_cast<int>(std::strtol(value, &end, 10))};
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The strtol call converts the result with static_cast<int>(...). On platforms where long is larger than int (e.g., 64-bit Linux where long is 64 bits), a value like 2147483648 (INT_MAX + 1) would not trigger ERANGE (since it fits in long) but would silently overflow when cast to int. This means values outside [INT_MIN, INT_MAX] are silently accepted and produce an incorrect result. Consider using std::strtol and checking that the returned long value fits within [INT_MIN, INT_MAX] before the cast, returning nullopt otherwise. Alternatively, use std::strtoimax and clamp to int range.

Copilot uses AI. Check for mistakes.
if (errno != 0)
return std::nullopt;
if (end == nullptr)
return std::nullopt;
Comment on lines +43 to +44
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The check if (end == nullptr) on line 43 is dead code. According to the C standard, when strtol is called with a non-null second argument (&end), it always sets *endptr to a valid pointer — it will never be nullptr. Since end is initialized by strtol and is always set to a non-null pointer, this check can never be true and should be removed to avoid misleading future readers.

Copilot uses AI. Check for mistakes.
if (end == value)
return std::nullopt;
if (*end != '\0')
return std::nullopt;
return x;
}
Comment on lines +31 to +50
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The new Environment::get<int> specialization uses errno and std::strtol but the file only includes <strings.h> and project headers. Neither <cerrno> (for errno) nor <cstdlib> (for std::strtol) is explicitly included. While these may be pulled in transitively on some platforms via <strings.h> or other system headers, relying on transitive inclusion is fragile and non-portable. Other files in the codebase (e.g., src/amd_detail/sys.cpp and test/amd_detail/driver.cpp) explicitly include <cerrno> when they use errno. Both <cerrno> and <cstdlib> should be explicitly included.

Copilot uses AI. Check for mistakes.

optional<bool>
Environment::allow_compat_mode()
{
Expand All @@ -39,3 +60,9 @@ Environment::force_compat_mode()
{
return Environment::get<bool>(Environment::FORCE_COMPAT_MODE);
}

optional<int>
Environment::stats_level()
{
return Environment::get<int>(Environment::STATS_LEVEL);
}
8 changes: 8 additions & 0 deletions src/amd_detail/environment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ class Environment {
static constexpr const char *const FORCE_COMPAT_MODE{"HIPFILE_FORCE_COMPAT_MODE"};

static std::optional<bool> force_compat_mode();

/// @brief Control how much information is collected to be reported to ais-stats
///
/// 0 indicates no stats should be recorded.
/// >= 1 indicates basic stats will be collected.
static constexpr const char *const STATS_LEVEL{"HIPFILE_STATS_LEVEL"};

static std::optional<int> stats_level();
};

}
27 changes: 27 additions & 0 deletions test/amd_detail/configuration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ struct ConfigurationExpectationBuilder {
StrictMock<MHip> &m_mhip;
std::optional<const char *> m_env_force_compat_mode;
std::optional<const char *> m_env_allow_compat_mode;
std::optional<const char *> m_env_stats_level;
void *m_hip_amd_file_read{reinterpret_cast<void *>(0xDEADBEEF)};
void *m_hip_amd_file_write{reinterpret_cast<void *>(0x0BADF00D)};

Expand All @@ -47,6 +48,12 @@ struct ConfigurationExpectationBuilder {
return *this;
}

ConfigurationExpectationBuilder &env_stats_level(const char *value)
{
m_env_stats_level = value;
return *this;
}

ConfigurationExpectationBuilder &hip_amd_file_read(void *value)
{
m_hip_amd_file_read = value;
Expand Down Expand Up @@ -81,6 +88,14 @@ struct ConfigurationExpectation {
EXPECT_CALL(builder.m_msys, getenv(StrEq(hipFile::Environment::ALLOW_COMPAT_MODE)));
}

if (builder.m_env_stats_level) {
EXPECT_CALL(builder.m_msys, getenv(StrEq(hipFile::Environment::STATS_LEVEL)))
.WillOnce(Return(const_cast<char *>(builder.m_env_stats_level.value())));
}
else {
EXPECT_CALL(builder.m_msys, getenv(StrEq(hipFile::Environment::STATS_LEVEL)));
}

EXPECT_CALL(builder.m_mhip, hipRuntimeGetVersion).Times(2);
EXPECT_CALL(builder.m_mhip, hipGetProcAddress(StrEq("hipAmdFileRead"), _, _, _))
.WillOnce(Return(builder.m_hip_amd_file_read));
Expand Down Expand Up @@ -148,4 +163,16 @@ TEST_F(HipFileConfiguration, FallbackDisabledIfAllowCompatModeEnvironmentVariabl
ASSERT_FALSE(Configuration().fallback());
}

TEST_F(HipFileConfiguration, StatsLevelEnvironmentVariableIsNotSetOrInvalid)
{
ConfigurationExpectationBuilder{msys, mhip}.build();
ASSERT_EQ(0, Configuration().statsLevel());
}

TEST_F(HipFileConfiguration, StatsLevelEnvironmentVariableIsSet)
{
ConfigurationExpectationBuilder{msys, mhip}.env_stats_level("1").build();
ASSERT_EQ(1, Configuration().statsLevel());
}

HIPFILE_WARN_NO_GLOBAL_CTOR_ON
28 changes: 28 additions & 0 deletions test/amd_detail/environment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,32 @@ TEST(HipFileEnvironment, GetBoolReturnsOptionalFalseIfFalse)
ASSERT_EQ(hipFile::Environment::get<bool>(""), make_optional<>(false));
}

TEST(HipFileEnvironment, GetIntReturnsNulloptIfNotSet)
{
StrictMock<MSys> msys;

EXPECT_CALL(msys, getenv).WillOnce(Return(nullptr));
ASSERT_EQ(hipFile::Environment::get<int>(""), nullopt);
}

TEST(HipFileEnvironment, GetIntReturnsNulloptIfValueIsInvalid)
{
StrictMock<MSys> msys;

EXPECT_CALL(msys, getenv).WillOnce(Return(const_cast<char *>("blah")));
ASSERT_EQ(hipFile::Environment::get<int>(""), nullopt);
EXPECT_CALL(msys, getenv).WillOnce(Return(const_cast<char *>("")));
ASSERT_EQ(hipFile::Environment::get<int>(""), nullopt);
EXPECT_CALL(msys, getenv).WillOnce(Return(const_cast<char *>("1abc")));
ASSERT_EQ(hipFile::Environment::get<int>(""), nullopt);
}

TEST(HipFileEnvironment, GetIntReturnsIntIfValueIsInt)
{
StrictMock<MSys> msys;

EXPECT_CALL(msys, getenv).WillOnce(Return(const_cast<char *>("0")));
ASSERT_EQ(hipFile::Environment::get<int>(""), make_optional<>(0));
}

HIPFILE_WARN_NO_GLOBAL_CTOR_ON