From ea9063a1da9fcee70e4ff2a8ad25cb6650daa941 Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Mon, 18 May 2026 16:42:03 +0200 Subject: [PATCH 1/9] Replace get_env and set_env from timemory inside rocprofiler-systems --- .../source/lib/common/environment.hpp | 81 ++++++++ .../lib/common/tests/test_environment.cpp | 191 ++++++++++++++++++ .../source/lib/core/common.hpp | 2 - .../source/lib/core/config.cpp | 67 +++--- .../source/lib/core/config.hpp | 1 + .../source/lib/core/dynamic_library.cpp | 6 +- .../source/lib/core/dynamic_library.hpp | 2 +- .../source/lib/core/mpi.hpp | 10 +- .../source/lib/core/perfetto.cpp | 2 +- .../source/lib/core/timemory.hpp | 2 - .../source/lib/core/trace_cache/discovery.cpp | 2 +- .../source/lib/rocprof-sys/library.cpp | 30 +-- .../lib/rocprof-sys/library/causal/data.cpp | 2 +- .../library/components/backtrace.cpp | 2 +- .../library/components/fork_gotcha.cpp | 4 +- .../library/components/mpi_gotcha.cpp | 2 +- .../rocprof-sys/library/components/mpip.hpp | 4 +- .../lib/rocprof-sys/library/kokkosp.cpp | 7 +- .../rocprof-sys/library/rocprofiler-sdk.cpp | 2 +- .../lib/rocprof-sys/library/runtime.cpp | 4 +- .../lib/rocprof-sys/library/tracing.cpp | 9 +- .../source/lib/rocprof-sys/timeout.cpp | 11 +- .../source/python/libpyrocprofsys.cpp | 8 +- 23 files changed, 360 insertions(+), 91 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index e30feef72d9d..6846cc883590 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -13,8 +13,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -134,6 +136,52 @@ get_env_impl(std::string_view env_id, bool _default) } return _default; } +template +inline std::enable_if_t, Tp> +get_env_impl(std::string_view env_id, Tp _default) +{ + if(env_id.empty()) return _default; + char* env_var = ::std::getenv(env_id.data()); + if(env_var) + { + try + { + return static_cast(std::stod(env_var)); + } catch(std::exception& _e) + { + fprintf(stderr, + "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " + "%s to float: %s\n", + env_id.data(), env_var, _e.what()); + } + return _default; + } + return _default; +} + +template +inline std::enable_if_t< + std::is_integral_v && !std::is_same_v && !std::is_same_v, Tp> +get_env_impl(std::string_view env_id, Tp _default) +{ + if(env_id.empty()) return _default; + char* env_var = ::std::getenv(env_id.data()); + if(env_var) + { + try + { + return static_cast(std::stoll(env_var)); + } catch(std::exception& _e) + { + fprintf(stderr, + "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " + "%s to integer: %s\n", + env_id.data(), env_var, _e.what()); + } + return _default; + } + return _default; +} } // namespace template @@ -152,6 +200,39 @@ get_env(std::string_view env_id, Tp&& _default) } } +template +inline auto +get_env(std::string_view env_id) +{ + return get_env(env_id, Tp{}); +} + +template +void +set_env(const std::string& env_var, const Tp& _val, int override) +{ + std::stringstream ss_val; + ss_val << _val; + + setenv(env_var.c_str(), ss_val.str().c_str(), override); +} + +template +inline auto +get_env_choice(std::string_view env_id, Tp _default, std::set _choices) +{ + auto _val = get_env(env_id, _default); + if(_choices.find(_val) == _choices.end()) + { + fprintf(stderr, + "[rocprof-sys][get_env] Environment variable \"%s\" has invalid value " + "\"%s\". Reverting to default.\n", + env_id.data(), get_env(env_id).c_str()); + return _default; + } + return _val; +} + struct ROCPROFSYS_INTERNAL_API env_config { std::string env_name = {}; diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index 22861cd477e3..05ff24b51c04 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -189,6 +189,197 @@ TEST_F(DuplicatedEnvironmentEntriesTest, RocmEventsPreservesDeviceSyntax) "ROCPROFSYS_ROCM_EVENTS=GRBM_COUNT,SQ_WAVES,SQ_INSTS_VALU,TA_TA_BUSY:device=0"); } +// ── get_env ────────────────────────────────────────────────────────────────── + +class GetEnvTest : public ::testing::Test +{ +protected: + void SetUp() override { unsetenv("ROCPROFSYS_TEST_VAR"); } + void TearDown() override { unsetenv("ROCPROFSYS_TEST_VAR"); } +}; + +TEST_F(GetEnvTest, StringReturnsDefaultWhenUnset) +{ + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{ "fallback" }), "fallback"); +} + +TEST_F(GetEnvTest, StringReturnsValueWhenSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "hello", 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{ "fallback" }), "hello"); +} + +TEST_F(GetEnvTest, StringEmptyVarNameReturnsDefault) +{ + EXPECT_EQ(get_env("", std::string{ "fallback" }), "fallback"); +} + +TEST_F(GetEnvTest, IntReturnsDefaultWhenUnset) +{ + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 42), 42); +} + +TEST_F(GetEnvTest, IntReturnsValueWhenSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "7", 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 42), 7); +} + +TEST_F(GetEnvTest, IntReturnsDefaultOnInvalidValue) +{ + setenv("ROCPROFSYS_TEST_VAR", "not_a_number", 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 42), 42); +} + +TEST_F(GetEnvTest, IntHandlesNegativeValue) +{ + setenv("ROCPROFSYS_TEST_VAR", "-5", 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 0), -5); +} + +TEST_F(GetEnvTest, BoolReturnsDefaultWhenUnset) +{ + EXPECT_FALSE(get_env("ROCPROFSYS_TEST_VAR", false)); + EXPECT_TRUE(get_env("ROCPROFSYS_TEST_VAR", true)); +} + +TEST_F(GetEnvTest, BoolTrueVariants) +{ + for(const char* v : { "1", "true", "TRUE", "yes", "on" }) + { + setenv("ROCPROFSYS_TEST_VAR", v, 1); + EXPECT_TRUE(get_env("ROCPROFSYS_TEST_VAR", false)) << "value: " << v; + } +} + +TEST_F(GetEnvTest, BoolFalseVariants) +{ + for(const char* v : { "0", "false", "FALSE", "no", "off" }) + { + setenv("ROCPROFSYS_TEST_VAR", v, 1); + EXPECT_FALSE(get_env("ROCPROFSYS_TEST_VAR", true)) << "value: " << v; + } +} + +TEST_F(GetEnvTest, SizeTReturnsDefaultWhenUnset) +{ + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", size_t{ 100 }), size_t{ 100 }); +} + +TEST_F(GetEnvTest, SizeTReturnsValueWhenSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "512", 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", size_t{ 1 }), size_t{ 512 }); +} + +TEST_F(GetEnvTest, DoubleReturnsDefaultWhenUnset) +{ + EXPECT_DOUBLE_EQ(get_env("ROCPROFSYS_TEST_VAR", -1.0), -1.0); +} + +TEST_F(GetEnvTest, DoubleReturnsValueWhenSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "3.14", 1); + EXPECT_NEAR(get_env("ROCPROFSYS_TEST_VAR", 0.0), 3.14, 1e-9); +} + +TEST_F(GetEnvTest, DoubleHandlesNegativeValue) +{ + setenv("ROCPROFSYS_TEST_VAR", "-2.5", 1); + EXPECT_DOUBLE_EQ(get_env("ROCPROFSYS_TEST_VAR", 0.0), -2.5); +} + +TEST_F(GetEnvTest, OneArgFormReturnsEmptyStringWhenUnset) +{ + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR"), ""); +} + +TEST_F(GetEnvTest, OneArgFormReturnsValueWhenSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "found", 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR"), "found"); +} + +// ── set_env ─────────────────────────────────────────────────────────────────── + +class SetEnvTest : public ::testing::Test +{ +protected: + void SetUp() override { unsetenv("ROCPROFSYS_TEST_VAR"); } + void TearDown() override { unsetenv("ROCPROFSYS_TEST_VAR"); } +}; + +TEST_F(SetEnvTest, SetsStringValue) +{ + set_env("ROCPROFSYS_TEST_VAR", std::string{ "val" }, 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{}), "val"); +} + +TEST_F(SetEnvTest, SetsIntValue) +{ + set_env("ROCPROFSYS_TEST_VAR", 99, 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 0), 99); +} + +TEST_F(SetEnvTest, OverrideZeroDoesNotOverwriteExisting) +{ + setenv("ROCPROFSYS_TEST_VAR", "original", 1); + set_env("ROCPROFSYS_TEST_VAR", std::string{ "new" }, 0); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{}), "original"); +} + +TEST_F(SetEnvTest, OverrideOneOverwritesExisting) +{ + setenv("ROCPROFSYS_TEST_VAR", "original", 1); + set_env("ROCPROFSYS_TEST_VAR", std::string{ "new" }, 1); + EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{}), "new"); +} + +// ── get_env_choice ─────────────────────────────────────────────────────────── + +class GetEnvChoiceTest : public ::testing::Test +{ +protected: + void SetUp() override { unsetenv("ROCPROFSYS_TEST_VAR"); } + void TearDown() override { unsetenv("ROCPROFSYS_TEST_VAR"); } +}; + +TEST_F(GetEnvChoiceTest, ReturnsDefaultWhenUnset) +{ + auto result = get_env_choice("ROCPROFSYS_TEST_VAR", "trace", + { "trace", "sampling", "causal" }); + EXPECT_EQ(result, "trace"); +} + +TEST_F(GetEnvChoiceTest, ReturnsValueWhenValidChoiceSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "sampling", 1); + auto result = get_env_choice("ROCPROFSYS_TEST_VAR", "trace", + { "trace", "sampling", "causal" }); + EXPECT_EQ(result, "sampling"); +} + +TEST_F(GetEnvChoiceTest, ReturnsDefaultWhenInvalidChoiceSet) +{ + setenv("ROCPROFSYS_TEST_VAR", "invalid_mode", 1); + auto result = get_env_choice("ROCPROFSYS_TEST_VAR", "trace", + { "trace", "sampling", "causal" }); + EXPECT_EQ(result, "trace"); +} + +TEST_F(GetEnvChoiceTest, AllValidChoicesAccepted) +{ + const std::set choices = { "inprocess", "system", "all" }; + for(const auto& choice : choices) + { + setenv("ROCPROFSYS_TEST_VAR", choice.c_str(), 1); + EXPECT_EQ( + get_env_choice("ROCPROFSYS_TEST_VAR", "inprocess", choices), + choice) + << "choice: " << choice; + } +} + class AddTorchLibraryPathTest : public ::testing::Test { protected: diff --git a/projects/rocprofiler-systems/source/lib/core/common.hpp b/projects/rocprofiler-systems/source/lib/core/common.hpp index 30deb9b8a3c6..4c5ca816f467 100644 --- a/projects/rocprofiler-systems/source/lib/core/common.hpp +++ b/projects/rocprofiler-systems/source/lib/core/common.hpp @@ -96,8 +96,6 @@ namespace trait = ::tim::trait; // NOLINT namespace cereal = ::tim::cereal; // NOLINT using ::tim::auto_lock_t; // NOLINT -using ::tim::get_env; // NOLINT -using ::tim::set_env; // NOLINT using ::tim::type_mutex; // NOLINT struct construct_on_thread diff --git a/projects/rocprofiler-systems/source/lib/core/config.cpp b/projects/rocprofiler-systems/source/lib/core/config.cpp index 978330d0b851..835ef8043d51 100644 --- a/projects/rocprofiler-systems/source/lib/core/config.cpp +++ b/projects/rocprofiler-systems/source/lib/core/config.cpp @@ -4,6 +4,7 @@ #include "config.hpp" #include "amd_smi.hpp" #include "common/defines.h" +#include "common/environment.hpp" #include "common/static_object.hpp" #include "constraint.hpp" #include "gpu.hpp" @@ -19,8 +20,6 @@ #include #include #include -#include -#include #include #include #include @@ -65,8 +64,8 @@ using settings = tim::settings; namespace { -int verbose_value = tim::get_env("ROCPROFSYS_VERBOSE", 0, false); -bool debug_value = tim::get_env("ROCPROFSYS_DEBUG", false, false); +int verbose_value = rocprofsys::get_env("ROCPROFSYS_VERBOSE", 0); +bool debug_value = rocprofsys::get_env("ROCPROFSYS_DEBUG", false); auto configure_once = std::once_flag{}; TIMEMORY_NOINLINE bool& @@ -236,10 +235,10 @@ configure_settings(bool _init) auto _config = *get_config_impl(); // if using timemory, default to perfetto being off - auto _default_perfetto_v = !tim::get_env("ROCPROFSYS_PROFILE", false, false); + auto _default_perfetto_v = !rocprofsys::get_env("ROCPROFSYS_PROFILE", false); auto _system_backend = - tim::get_env("ROCPROFSYS_PERFETTO_BACKEND_SYSTEM", false, false); + rocprofsys::get_env("ROCPROFSYS_PERFETTO_BACKEND_SYSTEM", false); ROCPROFSYS_CONFIG_SETTING(std::string, "ROCPROFSYS_LOG_LEVEL", "Rocprofiler-systems log level", "info", "debugging", @@ -251,7 +250,7 @@ configure_settings(bool _init) "rocprof-sys-log.txt", "debugging", "advanced"); auto _rocprofsys_debug = _config->get("ROCPROFSYS_DEBUG"); - if(_rocprofsys_debug) tim::set_env("TIMEMORY_DEBUG_SETTINGS", "1", 0); + if(_rocprofsys_debug) rocprofsys::set_env("TIMEMORY_DEBUG_SETTINGS", "1", 0); ROCPROFSYS_CONFIG_SETTING( std::string, "ROCPROFSYS_MODE", @@ -574,12 +573,12 @@ configure_settings(bool _init) "you want to restrict samples to particular threads.", std::string{}, "sampling", "advanced"); - auto _backend = tim::get_env_choice( + auto _backend = rocprofsys::get_env_choice( "ROCPROFSYS_PERFETTO_BACKEND", (_system_backend) ? "system" // if ROCPROFSYS_PERFETTO_BACKEND_SYSTEM is true, // default to system. : "inprocess", // Otherwise, default to inprocess - { "inprocess", "system", "all" }, false); + { "inprocess", "system", "all" }); ROCPROFSYS_CONFIG_SETTING(std::string, "ROCPROFSYS_PERFETTO_BACKEND", "Specify the perfetto backend to activate. Options are: " @@ -1067,7 +1066,7 @@ configure_settings(bool _init) // always initialize timemory because gotcha wrappers are always used auto _cmd = tim::read_command_line(process::get_id()); - auto _cmd_env = tim::get_env("ROCPROFSYS_COMMAND_LINE", ""); + auto _cmd_env = rocprofsys::get_env("ROCPROFSYS_COMMAND_LINE", ""); if(!_cmd_env.empty()) _cmd = tim::delimit(_cmd_env, " "); auto _exe = (_cmd.empty()) ? "exe" : _cmd.front(); get_exe_realpath() = filepath::realpath(_exe, nullptr, false); @@ -1174,8 +1173,8 @@ configure_settings(bool _init) auto _dl_verbose = _config->find("ROCPROFSYS_DL_VERBOSE"); if(_dl_verbose->second->get_config_updated()) - tim::set_env(std::string{ _dl_verbose->first }, _dl_verbose->second->as_string(), - 0); + rocprofsys::set_env(std::string{ _dl_verbose->first }, + _dl_verbose->second->as_string(), 0); if(_config->get_papi_events().empty()) { @@ -1255,7 +1254,7 @@ configure_mode_settings(const std::shared_ptr& _config) if(_config->get("ROCPROFSYS_USE_KOKKOSP")) { - auto _current_kokkosp_lib = tim::get_env("KOKKOS_TOOLS_LIBS"); + auto _current_kokkosp_lib = rocprofsys::get_env("KOKKOS_TOOLS_LIBS"); if(_current_kokkosp_lib.find("librocprof-sys-dl.so") == std::string::npos && _current_kokkosp_lib.find("librocprof-sys.so") == std::string::npos) { @@ -1268,12 +1267,12 @@ configure_mode_settings(const std::shared_ptr& _config) fmt::format(" (forced. Previous value: '{}')", _current_kokkosp_lib); } LOG_WARNING("Setting KOKKOS_TOOLS_LIBS={}{}", "librocprof-sys.so", _message); - tim::set_env("KOKKOS_TOOLS_LIBS", "librocprof-sys.so", _force); + rocprofsys::set_env("KOKKOS_TOOLS_LIBS", "librocprof-sys.so", _force); } } // recycle all subsequent thread ids - threading::recycle_ids() = tim::get_env( + threading::recycle_ids() = rocprofsys::get_env( "ROCPROFSYS_RECYCLE_TIDS", !_config->get("ROCPROFSYS_USE_SAMPLING")); if(!_config->get_enabled()) @@ -1352,13 +1351,12 @@ void configure_signal_handler(const std::shared_ptr& _config) { auto _ignore_dyninst_trampoline = - tim::get_env("ROCPROFSYS_IGNORE_DYNINST_TRAMPOLINE", false); + rocprofsys::get_env("ROCPROFSYS_IGNORE_DYNINST_TRAMPOLINE", false); // this is how dyninst looks up the env variable static auto _dyninst_trampoline_signal = getenv("DYNINST_SIGNAL_TRAMPOLINE_SIGILL") ? SIGILL : SIGTRAP; - static auto root_pid = - get_env("ROCPROFSYS_ROOT_PROCESS", process::get_id(), false); + static auto root_pid = get_env("ROCPROFSYS_ROOT_PROCESS", process::get_id()); if(_config->get_enable_signal_handler()) { tim::signals::disable_signal_detection(); @@ -1645,8 +1643,9 @@ print_settings( std::stringstream _os{}; - bool _print_desc = get_debug() || tim::get_env("ROCPROFSYS_SETTINGS_DESC", false); - bool _md = tim::get_env("ROCPROFSYS_SETTINGS_DESC_MARKDOWN", false); + bool _print_desc = + get_debug() || rocprofsys::get_env("ROCPROFSYS_SETTINGS_DESC", false); + bool _md = rocprofsys::get_env("ROCPROFSYS_SETTINGS_DESC_MARKDOWN", false); constexpr size_t nfields = 3; using str_array_t = std::array; @@ -1815,7 +1814,7 @@ get_mode() { if(!settings_are_configured()) { - auto _mode = tim::get_env_choice( + auto _mode = rocprofsys::get_env_choice( "ROCPROFSYS_MODE", "trace", { "trace", "sampling", "causal", "coverage" }); if(_mode == "sampling") return Mode::Sampling; @@ -1866,19 +1865,19 @@ get_debug_env() { return (settings_are_configured()) ? get_debug() - : tim::get_env("ROCPROFSYS_DEBUG", false, false); + : rocprofsys::get_env("ROCPROFSYS_DEBUG", false); } bool get_debug_init() { - return tim::get_env("ROCPROFSYS_DEBUG_INIT", get_debug_env()); + return rocprofsys::get_env("ROCPROFSYS_DEBUG_INIT", get_debug_env()); } bool get_debug_finalize() { - return tim::get_env("ROCPROFSYS_DEBUG_FINALIZE", false); + return rocprofsys::get_env("ROCPROFSYS_DEBUG_FINALIZE", false); } bool @@ -1891,9 +1890,9 @@ get_debug() bool get_debug_sampling() { - static bool _v = - tim::get_env("ROCPROFSYS_DEBUG_SAMPLING", - (settings_are_configured() ? get_debug() : get_debug_env())); + static bool _v = rocprofsys::get_env( + "ROCPROFSYS_DEBUG_SAMPLING", + (settings_are_configured() ? get_debug() : get_debug_env())); return _v; } @@ -1902,7 +1901,7 @@ get_verbose_env() { return (settings_are_configured()) ? get_verbose() - : tim::get_env("ROCPROFSYS_VERBOSE", 0, false); + : rocprofsys::get_env("ROCPROFSYS_VERBOSE", 0); } int @@ -2500,7 +2499,8 @@ get_debug_tid() { static auto _vlist = parse_numeric_range>( - tim::get_env("ROCPROFSYS_DEBUG_TIDS", ""), "debug tids", 1L); + rocprofsys::get_env("ROCPROFSYS_DEBUG_TIDS", ""), "debug tids", + 1L); static thread_local bool _v = _vlist.empty() || _vlist.count(tim::threading::get_id()) > 0; return _v; @@ -2511,7 +2511,8 @@ get_debug_pid() { static auto _vlist = parse_numeric_range>( - tim::get_env("ROCPROFSYS_DEBUG_PIDS", ""), "debug pids", 1L); + rocprofsys::get_env("ROCPROFSYS_DEBUG_PIDS", ""), "debug pids", + 1L); static bool _v = _vlist.empty() || _vlist.count(tim::process::get_id()) > 0 || _vlist.count(dmp::rank()) > 0; return _v; @@ -2762,7 +2763,7 @@ get_mpi_rank_from_env() for(const auto& env_var : rank_env_var_options) { - const std::string rank_str = get_env(env_var, std::string{}, false); + const std::string rank_str = get_env(env_var, std::string{}); if(rank_str.empty()) continue; try @@ -3077,8 +3078,8 @@ get_causal_mode() { if(!settings_are_configured()) { - auto _mode = tim::get_env_choice("ROCPROFSYS_CAUSAL_MODE", - "function", { "line", "function" }); + auto _mode = rocprofsys::get_env_choice( + "ROCPROFSYS_CAUSAL_MODE", "function", { "line", "function" }); if(_mode == "line") return CausalMode::Line; return CausalMode::Function; } diff --git a/projects/rocprofiler-systems/source/lib/core/config.hpp b/projects/rocprofiler-systems/source/lib/core/config.hpp index 6080df3e0e53..3f1110333ab4 100644 --- a/projects/rocprofiler-systems/source/lib/core/config.hpp +++ b/projects/rocprofiler-systems/source/lib/core/config.hpp @@ -5,6 +5,7 @@ #include "common.hpp" #include "common/defines.h" +#include "common/environment.hpp" #include "state.hpp" #include "timemory.hpp" #include diff --git a/projects/rocprofiler-systems/source/lib/core/dynamic_library.cpp b/projects/rocprofiler-systems/source/lib/core/dynamic_library.cpp index d802d728d36c..bc7d6f75de90 100644 --- a/projects/rocprofiler-systems/source/lib/core/dynamic_library.cpp +++ b/projects/rocprofiler-systems/source/lib/core/dynamic_library.cpp @@ -4,7 +4,7 @@ #include "dynamic_library.hpp" #include "common.hpp" -#include +#include "common/environment.hpp" #include #include #include @@ -60,7 +60,7 @@ find_library_path(const std::string& _name, const std::vector& _env } dynamic_library::dynamic_library(std::string _env, std::string _fname, int _flags, - bool _open, bool _query_env, bool _store) + bool _open, bool _query_env) : envname{ std::move(_env) } , filename{ std::move(_fname) } , flags{ _flags } @@ -70,7 +70,7 @@ dynamic_library::dynamic_library(std::string _env, std::string _fname, int _flag if(_query_env) { - auto _env_val = get_env(envname, std::string{}, _store); + auto _env_val = common::get_env(envname, std::string{}); // if the environment variable is set to an absolute path that exists, // override with value if(!_env_val.empty()) diff --git a/projects/rocprofiler-systems/source/lib/core/dynamic_library.hpp b/projects/rocprofiler-systems/source/lib/core/dynamic_library.hpp index 46cdd11fd8ea..c2ca255518e6 100644 --- a/projects/rocprofiler-systems/source/lib/core/dynamic_library.hpp +++ b/projects/rocprofiler-systems/source/lib/core/dynamic_library.hpp @@ -27,7 +27,7 @@ struct dynamic_library dynamic_library(std::string _env, std::string _fname, int _flags = (RTLD_LAZY | RTLD_GLOBAL), bool _open = true, - bool _query_env = true, bool _store = true); + bool _query_env = true); ~dynamic_library(); diff --git a/projects/rocprofiler-systems/source/lib/core/mpi.hpp b/projects/rocprofiler-systems/source/lib/core/mpi.hpp index 9bc4ec29e23d..cd31be93b36a 100644 --- a/projects/rocprofiler-systems/source/lib/core/mpi.hpp +++ b/projects/rocprofiler-systems/source/lib/core/mpi.hpp @@ -10,7 +10,7 @@ #include -#include +#include "common/environment.hpp" #include #include "logger/debug.hpp" @@ -194,7 +194,7 @@ inline void set_size(std::int32_t, comm_t = comm_world_v); inline bool& use_mpi_thread() { - static bool _instance = tim::get_env("ROCPROFSYS_MPI_THREAD", true); + static bool _instance = rocprofsys::get_env("ROCPROFSYS_MPI_THREAD", true); return _instance; } @@ -204,7 +204,7 @@ inline std::string& use_mpi_thread_type() { static std::string _instance = - tim::get_env("ROCPROFSYS_MPI_THREAD_TYPE", ""); + rocprofsys::get_env("ROCPROFSYS_MPI_THREAD_TYPE", ""); return _instance; } @@ -213,7 +213,7 @@ use_mpi_thread_type() inline bool& fail_on_error() { - static bool _instance = tim::get_env("ROCPROFSYS_MPI_FAIL_ON_ERROR", false); + static bool _instance = rocprofsys::get_env("ROCPROFSYS_MPI_FAIL_ON_ERROR", false); return _instance; } @@ -222,7 +222,7 @@ fail_on_error() inline bool& quiet() { - static bool _instance = tim::get_env("ROCPROFSYS_MPI_QUIET", false); + static bool _instance = rocprofsys::get_env("ROCPROFSYS_MPI_QUIET", false); return _instance; } diff --git a/projects/rocprofiler-systems/source/lib/core/perfetto.cpp b/projects/rocprofiler-systems/source/lib/core/perfetto.cpp index cd9a73e7381f..bcfd3d2ea1c5 100644 --- a/projects/rocprofiler-systems/source/lib/core/perfetto.cpp +++ b/projects/rocprofiler-systems/source/lib/core/perfetto.cpp @@ -277,7 +277,7 @@ post_process(tim::manager* _timemory_manager, bool& _perfetto_output_error, { auto _output_folder = filepath::dirname(_filename); auto _script_path = std::string{ "rocprof-sys-merge-output.sh" }; - auto _script_dir = get_env("ROCPROFSYS_SCRIPT_PATH", std::string{}, false); + auto _script_dir = get_env("ROCPROFSYS_SCRIPT_PATH", std::string{}); if(!_script_dir.empty()) { diff --git a/projects/rocprofiler-systems/source/lib/core/timemory.hpp b/projects/rocprofiler-systems/source/lib/core/timemory.hpp index 13e75d08be1a..b7e27be5df54 100644 --- a/projects/rocprofiler-systems/source/lib/core/timemory.hpp +++ b/projects/rocprofiler-systems/source/lib/core/timemory.hpp @@ -34,6 +34,4 @@ namespace units = ::tim::units; // NOLINT using settings = ::tim::settings; // NOLINT -using ::tim::get_env; // NOLINT -using ::tim::set_env; // NOLINT } // namespace rocprofsys diff --git a/projects/rocprofiler-systems/source/lib/core/trace_cache/discovery.cpp b/projects/rocprofiler-systems/source/lib/core/trace_cache/discovery.cpp index e66f0c0f8fa1..7a778a1348f4 100644 --- a/projects/rocprofiler-systems/source/lib/core/trace_cache/discovery.cpp +++ b/projects/rocprofiler-systems/source/lib/core/trace_cache/discovery.cpp @@ -143,7 +143,7 @@ merge_perfetto_files() auto _filename = config::get_perfetto_output_filename(); auto _output_folder = tim::filepath::dirname(_filename); auto _script_path = std::string{ "rocprof-sys-merge-output.sh" }; - auto _script_dir = get_env("ROCPROFSYS_SCRIPT_PATH", std::string{}, false); + auto _script_dir = get_env("ROCPROFSYS_SCRIPT_PATH", std::string{}); if(!_script_dir.empty()) _script_path = fmt::format("{}/{}", _script_dir, _script_path); diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp index cc21854c50fa..791416d667e2 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp @@ -233,12 +233,12 @@ ensure_finalization(bool _static_init = false) { auto _verbose = get_verbose_env() + ((get_debug_env() || get_debug_init()) ? 16 : 0); - auto _search_paths = fmt::format("{}:{}:{}:{}:{}", - tim::get_env("ROCPROFSYS_PATH", ""), - tim::get_env("PWD"), ".", - tim::get_env("LD_LIBRARY_PATH", ""), - tim::get_env("LIBRARY_PATH", ""), - tim::get_env("PATH", "")); + auto _search_paths = fmt::format( + "{}:{}:{}:{}:{}", rocprofsys::get_env("ROCPROFSYS_PATH", ""), + rocprofsys::get_env("PWD"), ".", + rocprofsys::get_env("LD_LIBRARY_PATH", ""), + rocprofsys::get_env("LIBRARY_PATH", ""), + rocprofsys::get_env("PATH", "")); common::setup_environ(_verbose, _search_paths); } @@ -324,7 +324,7 @@ rocprofsys_set_env_hidden(const char* env_name, const char* env_val) LOG_DEBUG("Setting env: {} = {}", env_name, env_val); } - tim::set_env(env_name, env_val, 0); + rocprofsys::set_env(env_name, env_val, 0); if(_success && get_state() >= State::Init) { @@ -457,7 +457,7 @@ rocprofsys_set_mpi_hidden(bool use, bool attached) if(use && !attached && get_state() == State::PreInit) { - tim::set_env("ROCPROFSYS_USE_PID", "ON", 1); + rocprofsys::set_env("ROCPROFSYS_USE_PID", "ON", 1); } else if(!use) { @@ -571,9 +571,9 @@ rocprofsys_init_library_hidden() extern "C" bool rocprofsys_init_tooling_hidden(void) { - if(get_env("ROCPROFSYS_MONOCHROME", false, false)) tim::log::monochrome() = true; + if(get_env("ROCPROFSYS_MONOCHROME", false)) tim::log::monochrome() = true; - if(!tim::get_env("ROCPROFSYS_INIT_TOOLING", true)) + if(!rocprofsys::get_env("ROCPROFSYS_INIT_TOOLING", true)) { rocprofsys_init_library_hidden(); return false; @@ -866,7 +866,7 @@ rocprofsys_init_hidden(const char* _mode, bool _is_binary_rewrite, const char* _ (_is_binary_rewrite) ? "y" : "n", _argv0); } - tim::set_env("ROCPROFSYS_MODE", _mode, 0); + rocprofsys::set_env("ROCPROFSYS_MODE", _mode, 0); config::is_binary_rewrite() = _is_binary_rewrite; if(_set_mpi_called) @@ -880,7 +880,7 @@ rocprofsys_init_hidden(const char* _mode, bool _is_binary_rewrite, const char* _ extern "C" void rocprofsys_reset_preload_hidden(void) { - tim::set_env("ROCPROFSYS_PRELOAD", "0", 1); + rocprofsys::set_env("ROCPROFSYS_PRELOAD", "0", 1); auto&& _preload_libs = common::get_env("LD_PRELOAD", std::string{}); if(_preload_libs.find("librocprof-sys") != std::string::npos) { @@ -893,7 +893,7 @@ rocprofsys_reset_preload_hidden(void) if(!_modified_preload.empty() && _modified_preload.find(':') == 0) _modified_preload = _modified_preload.substr(1); - tim::set_env("LD_PRELOAD", _modified_preload, 1); + rocprofsys::set_env("LD_PRELOAD", _modified_preload, 1); } } @@ -996,7 +996,7 @@ rocprofsys_finalize_hidden(void) if(dmp::rank() == 0) { config::print_settings( - tim::get_env("ROCPROFSYS_PRINT_ENV", get_debug())); + rocprofsys::get_env("ROCPROFSYS_PRINT_ENV", get_debug())); } } @@ -1279,7 +1279,7 @@ rocprofsys_finalize_hidden(void) } if(_push_count > _pop_count && - !get_env("ROCPROFSYS_CI_SKIP_PUSH_POP_CHECK", false, false)) + !get_env("ROCPROFSYS_CI_SKIP_PUSH_POP_CHECK", false)) { throw std::runtime_error(fmt::format( "rocprofsys_push_trace was called more times than " diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/causal/data.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/causal/data.cpp index 26e6c341a247..761e123c4c6d 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/causal/data.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/causal/data.cpp @@ -617,7 +617,7 @@ perform_experiment_impl(std::shared_ptr> _started) // NOLINT // if launched via rocprof-sys-causal, allow end-to-end runs that do not // start experiments auto _omni_causal_launcher = - get_env("ROCPROFSYS_LAUNCHER", "", false) == + get_env("ROCPROFSYS_LAUNCHER", "") == "rocprof-sys-causal"; if(!(get_causal_end_to_end() && _omni_causal_launcher)) diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/backtrace.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/backtrace.cpp index d7d8e0917aef..2d4d5351b730 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/backtrace.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/backtrace.cpp @@ -112,7 +112,7 @@ backtrace::filter_and_patch(const std::vector& _data) return 1; }; - static bool _keep_suffix = tim::get_env( + static bool _keep_suffix = rocprofsys::get_env( "ROCPROFSYS_SAMPLING_KEEP_DYNINST_SUFFIX", get_debug_sampling()); // in the dyninst binary rewrite runtime, instrumented functions are appended with diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/fork_gotcha.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/fork_gotcha.cpp index f7e46624cc5a..f7a36b292cea 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/fork_gotcha.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/fork_gotcha.cpp @@ -54,8 +54,8 @@ prefork_setup() if(get_state() < State::Active && !config::settings_are_configured()) rocprofsys_init_library_hidden(); - tim::set_env("ROCPROFSYS_PRELOAD", "0", 1); - tim::set_env("ROCPROFSYS_ROOT_PROCESS", process::get_id(), 0); + rocprofsys::set_env("ROCPROFSYS_PRELOAD", "0", 1); + rocprofsys::set_env("ROCPROFSYS_ROOT_PROCESS", process::get_id(), 0); rocprofsys_reset_preload_hidden(); LOG_INFO("fork() called on PID {} (rank: {}), TID {}", process::get_id(), dmp::rank(), threading::get_id()); diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpi_gotcha.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpi_gotcha.cpp index 4f6d6e514e0c..cdfb9253ebaa 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpi_gotcha.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpi_gotcha.cpp @@ -396,7 +396,7 @@ mpi_gotcha::audit(const gotcha_data_t& _data, audit::outgoing, int _retval) { static thread_local int _num_updates = 0; static int _disable_after = - tim::get_env("ROCPROFSYS_MPI_MAX_COMM_UPDATES", 4); + rocprofsys::get_env("ROCPROFSYS_MPI_MAX_COMM_UPDATES", 4); if(_num_updates++ < _disable_after) update(); } } diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpip.hpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpip.hpp index 78cdecc39fbf..bf81bbdbe003 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpip.hpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/components/mpip.hpp @@ -732,7 +732,7 @@ rocprofsys::component::configure_mpip(const std::set& permit, mpip_gotcha_t::get_reject_list() = [reject]() { auto _reject = reject; // check environment - auto reject_list = tim::get_env( + auto reject_list = rocprofsys::get_env( TIMEMORY_SETTINGS_PREFIX "MPIP_REJECT_LIST", ""); // add environment setting for(const auto& itr : tim::delimit(reject_list)) @@ -744,7 +744,7 @@ rocprofsys::component::configure_mpip(const std::set& permit, mpip_gotcha_t::get_permit_list() = [permit]() { auto _permit = permit; // check environment - auto permit_list = tim::get_env( + auto permit_list = rocprofsys::get_env( TIMEMORY_SETTINGS_PREFIX "MPIP_PERMIT_LIST", ""); // add environment setting for(const auto& itr : tim::delimit(permit_list)) diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/kokkosp.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/kokkosp.cpp index fa617a333046..274a67eb9249 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/kokkosp.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/kokkosp.cpp @@ -251,7 +251,7 @@ extern "C" _command_line.append(" ").append(argv[i]); } if(_command_line.length() > 1) _command_line = _command_line.substr(1); - tim::set_env("ROCPROFSYS_COMMAND_LINE", _command_line, 0); + rocprofsys::set_env("ROCPROFSYS_COMMAND_LINE", _command_line, 0); } } @@ -282,7 +282,8 @@ extern "C" (!rocprofsys::config::settings_are_configured() && rocprofsys::get_state() < rocprofsys::State::Active)) { - auto _kokkos_profile_lib = tim::get_env("KOKKOS_TOOLS_LIBS"); + auto _kokkos_profile_lib = + rocprofsys::get_env("KOKKOS_TOOLS_LIBS"); if(_kokkos_profile_lib.find("librocprof-sys.so") != std::string::npos) { auto _maps = tim::procfs::read_maps(tim::process::get_id()); @@ -316,7 +317,7 @@ extern "C" } LOG_DEBUG("Initializing rocprof-sys (standalone)... "); - auto _mode = tim::get_env("ROCPROFSYS_MODE", "trace"); + auto _mode = rocprofsys::get_env("ROCPROFSYS_MODE", "trace"); auto _arg0 = (_initialize_arguments.empty()) ? std::string{ "unknown" } : _initialize_arguments.at(0); diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp index 96c1cd4ee61e..08dd581bc416 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp @@ -3072,7 +3072,7 @@ extern "C" _first = false; } - if(!tim::get_env("ROCPROFSYS_INIT_TOOLING", true)) return nullptr; + if(!rocprofsys::get_env("ROCPROFSYS_INIT_TOOLING", true)) return nullptr; if(!tim::settings::enabled()) return nullptr; if(!sdk_tool_configure(version, runtime_version, id)) return nullptr; diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/runtime.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/runtime.cpp index b7c65fd95776..e7d006f931f3 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/runtime.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/runtime.cpp @@ -14,7 +14,6 @@ #include #include #include -#include #include #include #include @@ -38,8 +37,7 @@ namespace rocprofsys { namespace { -auto root_process_id = - get_env("ROCPROFSYS_ROOT_PROCESS", process::get_id(), false); +auto root_process_id = get_env("ROCPROFSYS_ROOT_PROCESS", process::get_id()); auto& get_sampling_on_child_threads_history(std::int64_t _idx = utility::get_thread_index()) diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/tracing.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/tracing.cpp index 1d23ee8e51bc..28b49b048219 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/library/tracing.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/library/tracing.cpp @@ -39,10 +39,11 @@ get_timemory_hash_aliases(std::int64_t _tid) } } // namespace -bool debug_push = tim::get_env("ROCPROFSYS_DEBUG_PUSH", false) || get_debug_env(); -bool debug_pop = tim::get_env("ROCPROFSYS_DEBUG_POP", false) || get_debug_env(); -bool debug_mark = tim::get_env("ROCPROFSYS_DEBUG_MARK", false) || get_debug_env(); -bool debug_user = tim::get_env("ROCPROFSYS_DEBUG_USER_REGIONS", false) || get_debug_env(); +bool debug_push = rocprofsys::get_env("ROCPROFSYS_DEBUG_PUSH", false) || get_debug_env(); +bool debug_pop = rocprofsys::get_env("ROCPROFSYS_DEBUG_POP", false) || get_debug_env(); +bool debug_mark = rocprofsys::get_env("ROCPROFSYS_DEBUG_MARK", false) || get_debug_env(); +bool debug_user = + rocprofsys::get_env("ROCPROFSYS_DEBUG_USER_REGIONS", false) || get_debug_env(); std::unordered_map& get_perfetto_track_uuids() diff --git a/projects/rocprofiler-systems/source/lib/rocprof-sys/timeout.cpp b/projects/rocprofiler-systems/source/lib/rocprof-sys/timeout.cpp index 58381859db10..1192147ff707 100644 --- a/projects/rocprofiler-systems/source/lib/rocprof-sys/timeout.cpp +++ b/projects/rocprofiler-systems/source/lib/rocprof-sys/timeout.cpp @@ -80,9 +80,8 @@ ensure_ci_timeout_backtrace(double _ci_timeout_seconds, std::uint64_t _ci_timeout_nitr = 0; std::int64_t _ci_timeout_nanosec = (_ci_timeout_seconds - _factor) * units::sec; auto _ci_timeout_total_count = - get_env("ROCPROFSYS_CI_TIMEOUT_COUNT", 1, false); - const auto root_pid = - get_env("ROCPROFSYS_ROOT_PROCESS", process::get_id(), false); + get_env("ROCPROFSYS_CI_TIMEOUT_COUNT", 1); + const auto root_pid = get_env("ROCPROFSYS_ROOT_PROCESS", process::get_id()); while(get_state() < State::Finalized && _ci_timeout_nitr < _ci_timeout_total_count) { @@ -170,14 +169,14 @@ setup() // set, start a thread that will print out the backtrace for each thread // before the timeout is hit (i.e. killed by CTest) so we can potentially // diagnose where the code is stuck - auto _ci = get_env("ROCPROFSYS_CI", false, false); + auto _ci = get_env("ROCPROFSYS_CI", false); if(_ci) { // set by CTest - auto _ci_timeout_default = get_env("ROCPROFSYS_CI_TIMEOUT", -1.0, false); + auto _ci_timeout_default = get_env("ROCPROFSYS_CI_TIMEOUT", -1.0); // allow override by user auto _ci_timeout_seconds = - get_env("ROCPROFSYS_CI_TIMEOUT_OVERRIDE", _ci_timeout_default, false); + get_env("ROCPROFSYS_CI_TIMEOUT_OVERRIDE", _ci_timeout_default); if(_ci_timeout_seconds > 0.0) { diff --git a/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp b/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp index ce9edd3dae55..eb99b08dc75c 100644 --- a/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp +++ b/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp @@ -8,9 +8,9 @@ #include "rocprofiler-systems/categories.h" #include "rocprofiler-systems/user.h" +#include "common/environment.hpp" #include #include -#include #include #include #include @@ -135,7 +135,7 @@ PYBIND11_MODULE(libpyrocprofsys, omni) if(!_cmd_line.empty()) { _cmd_line = _cmd_line.substr(_cmd_line.find_first_not_of(' ')); - tim::set_env("ROCPROFSYS_COMMAND_LINE", _cmd_line, 0); + rocprofsys::set_env("ROCPROFSYS_COMMAND_LINE", _cmd_line, 0); } rocprofsys_init("trace", false, _cmd.c_str()); }, @@ -155,11 +155,11 @@ PYBIND11_MODULE(libpyrocprofsys, omni) pycoverage::generate(omni); pyuser::generate(omni); - auto _python_path = tim::get_env("ROCPROFSYS_PATH", std::string{}, false); + auto _python_path = rocprofsys::get_env("ROCPROFSYS_PATH", std::string{}); auto _libpath = std::string{ "librocprof-sys-dl.so" }; if(!_python_path.empty()) _libpath = TIMEMORY_JOIN("/", _python_path, _libpath); // permit env override if default path fails/is wrong - _libpath = tim::get_env("ROCPROFSYS_DL_LIBRARY", _libpath); + _libpath = rocprofsys::get_env("ROCPROFSYS_DL_LIBRARY", _libpath); // this is necessary when building with -static-libstdc++ // without it, loading librocprof-sys.so within librocprof-sys-dl.so segfaults if(!dlopen(_libpath.c_str(), RTLD_NOW | RTLD_GLOBAL)) From b8dc6656710617246f952cd589368a04006378eb Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Tue, 19 May 2026 09:45:35 +0200 Subject: [PATCH 2/9] refactor environment to be mockable and unit testable --- .../source/lib/common/environment.hpp | 264 +++++++++--------- .../source/lib/common/setup.hpp | 4 +- .../lib/common/tests/test_environment.cpp | 261 +++++++++-------- 3 files changed, 276 insertions(+), 253 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index 6846cc883590..a0bfbcc4087e 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -69,170 +69,146 @@ namespace rocprofsys { inline namespace common { -namespace -{ -inline std::string -get_env_impl(std::string_view env_id, std::string_view _default) +// POSIX environment backend — calls the real ::getenv / ::setenv. +struct posix_env { - if(env_id.empty()) return std::string{ _default }; - char* env_var = ::std::getenv(env_id.data()); - if(env_var) return std::string{ env_var }; - return std::string{ _default }; -} - -inline std::string -get_env_impl(std::string_view env_id, const char* _default) -{ - return get_env_impl(env_id, std::string_view{ _default }); -} - -inline int -get_env_impl(std::string_view env_id, int _default) -{ - if(env_id.empty()) return _default; - char* env_var = ::std::getenv(env_id.data()); - if(env_var) + static int setenv(const char* name, const char* value, int overwrite) { - try - { - return std::stoi(env_var); - } catch(std::exception& _e) - { - fprintf(stderr, - "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " - "%s to integer :: %s. Using default value of %i\n", - env_id.data(), env_var, _e.what(), _default); - } - return _default; + return ::setenv(name, value, overwrite); } - return _default; -} -inline bool -get_env_impl(std::string_view env_id, bool _default) + static char* getenv(const char* name) { return ::getenv(name); } +}; + +// environment — all env-read/write logic parameterised over the backend. +// Use environment (the default) for production code. +// Inject a fake backend in unit tests to avoid touching the real process environment. +template +struct environment { - if(env_id.empty()) return _default; - char* env_var = ::std::getenv(env_id.data()); - if(env_var) +private: + template + static auto get_env_impl(std::string_view env_id, Tp _default) { - if(std::string_view{ env_var }.empty()) - throw std::runtime_error(std::string{ "No boolean value provided for " } + - std::string{ env_id }); - - if(std::string_view{ env_var }.find_first_not_of("0123456789") == - std::string_view::npos) + if constexpr(std::is_same_v, std::string> || + std::is_same_v, std::string_view> || + std::is_same_v, const char*>) { - return static_cast(std::stoi(env_var)); + if(env_id.empty()) return std::string{ _default }; + auto* env_var = EnvType::getenv(env_id.data()); + return env_var ? std::string{ env_var } : std::string{ _default }; } - else + else if constexpr(std::is_same_v) { + if(env_id.empty()) return _default; + auto* env_var = EnvType::getenv(env_id.data()); + if(!env_var) return _default; + + if(std::string_view{ env_var }.empty()) + throw std::runtime_error(std::string{ "No boolean value provided for " } + + std::string{ env_id }); + + if(std::string_view{ env_var }.find_first_not_of("0123456789") == + std::string_view::npos) + return static_cast(std::stoi(env_var)); + for(size_t i = 0; i < strlen(env_var); ++i) env_var[i] = tolower(env_var[i]); for(const auto& itr : { "off", "false", "no", "n", "f", "0" }) if(strcmp(env_var, itr) == 0) return false; + return true; } - return true; - } - return _default; -} -template -inline std::enable_if_t, Tp> -get_env_impl(std::string_view env_id, Tp _default) -{ - if(env_id.empty()) return _default; - char* env_var = ::std::getenv(env_id.data()); - if(env_var) - { - try - { - return static_cast(std::stod(env_var)); - } catch(std::exception& _e) + else if constexpr(std::is_floating_point_v) { - fprintf(stderr, + if(env_id.empty()) return _default; + auto* env_var = EnvType::getenv(env_id.data()); + if(!env_var) return _default; + try + { + return static_cast(std::stod(env_var)); + } catch(const std::exception& e) + { + fprintf( + stderr, "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " "%s to float: %s\n", - env_id.data(), env_var, _e.what()); + env_id.data(), env_var, e.what()); + } + return _default; + } + else // integral: int, long, size_t, etc. + { + if(env_id.empty()) return _default; + auto* env_var = EnvType::getenv(env_id.data()); + if(!env_var) return _default; + try + { + if constexpr(std::is_same_v) + return std::stoi(env_var); + else + return static_cast(std::stoll(env_var)); + } catch(const std::exception& e) + { + fprintf( + stderr, + "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " + "%s to integer: %s\n", + env_id.data(), env_var, e.what()); + } + return _default; } - return _default; } - return _default; -} -template -inline std::enable_if_t< - std::is_integral_v && !std::is_same_v && !std::is_same_v, Tp> -get_env_impl(std::string_view env_id, Tp _default) -{ - if(env_id.empty()) return _default; - char* env_var = ::std::getenv(env_id.data()); - if(env_var) +public: + template + static auto get_env(std::string_view env_id, Tp&& _default) { - try + if constexpr(std::is_enum::value) { - return static_cast(std::stoll(env_var)); - } catch(std::exception& _e) + using Up = std::underlying_type_t; + return static_cast(get_env_impl(env_id, static_cast(_default))); + } + else { - fprintf(stderr, - "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " - "%s to integer: %s\n", - env_id.data(), env_var, _e.what()); + return get_env_impl(env_id, std::forward(_default)); } - return _default; } - return _default; -} -} // namespace -template -inline auto -get_env(std::string_view env_id, Tp&& _default) -{ - if constexpr(std::is_enum::value) + template + static auto get_env(std::string_view env_id) { - using Up = std::underlying_type_t; - // cast to underlying type -> get_env -> cast to enum type - return static_cast(get_env_impl(env_id, static_cast(_default))); + return get_env(env_id, Tp{}); } - else + + template + static void set_env(const std::string& env_var, const Tp& _val, int override) { - return get_env_impl(env_id, std::forward(_default)); + std::stringstream ss_val; + ss_val << _val; + EnvType::setenv(env_var.c_str(), ss_val.str().c_str(), override); } -} -template -inline auto -get_env(std::string_view env_id) -{ - return get_env(env_id, Tp{}); -} - -template -void -set_env(const std::string& env_var, const Tp& _val, int override) -{ - std::stringstream ss_val; - ss_val << _val; - - setenv(env_var.c_str(), ss_val.str().c_str(), override); -} - -template -inline auto -get_env_choice(std::string_view env_id, Tp _default, std::set _choices) -{ - auto _val = get_env(env_id, _default); - if(_choices.find(_val) == _choices.end()) + template + static auto get_env_choice(std::string_view env_id, Tp _default, + std::set _choices) { - fprintf(stderr, + auto _val = get_env(env_id, _default); + if(_choices.find(_val) == _choices.end()) + { + fprintf( + stderr, "[rocprof-sys][get_env] Environment variable \"%s\" has invalid value " "\"%s\". Reverting to default.\n", env_id.data(), get_env(env_id).c_str()); - return _default; + return _default; + } + return _val; } - return _val; -} +}; +// Templated env_config — operator() calls EnvType::setenv so the backend is injectable. +template struct ROCPROFSYS_INTERNAL_API env_config { std::string env_name = {}; @@ -244,10 +220,44 @@ struct ROCPROFSYS_INTERNAL_API env_config if(env_name.empty()) return -1; ROCPROFSYS_ENVIRON_LOG(_verbose, "setenv(\"%s\", \"%s\", %i)\n", env_name.c_str(), env_value.c_str(), override); - return setenv(env_name.c_str(), env_value.c_str(), override); + return EnvType::setenv(env_name.c_str(), env_value.c_str(), override); } }; +// ── Forwarding free functions ──────────────────────────────────────────────── +// These preserve the existing rocprofsys::common::get_env / set_env / get_env_choice +// API so no call sites need to change. They delegate to environment. + +template +inline auto +get_env(std::string_view env_id, Tp&& _default) +{ + return environment<>::get_env(env_id, std::forward(_default)); +} + +template +inline auto +get_env(std::string_view env_id) +{ + return environment<>::get_env(env_id); +} + +template +inline void +set_env(const std::string& env_var, const Tp& _val, int override) +{ + environment<>::set_env(env_var, _val, override); +} + +template +inline auto +get_env_choice(std::string_view env_id, Tp _default, std::set _choices) +{ + return environment<>::get_env_choice(env_id, _default, std::move(_choices)); +} + +// ── Env-vector helpers (operate on std::vector, not the real env) ── + inline void remove_env(std::vector& _environ, std::string_view _env_var, const std::unordered_set& _original_envs) diff --git a/projects/rocprofiler-systems/source/lib/common/setup.hpp b/projects/rocprofiler-systems/source/lib/common/setup.hpp index de45a02242b7..a98f5da950b8 100644 --- a/projects/rocprofiler-systems/source/lib/common/setup.hpp +++ b/projects/rocprofiler-systems/source/lib/common/setup.hpp @@ -66,12 +66,12 @@ namespace rocprofsys { inline namespace common { -inline std::vector +inline std::vector> get_environ(int _verbose, std::string _search_paths = {}, std::string _omnilib = "librocprof-sys.so", std::string _omnilib_dl = "librocprof-sys-dl.so") { - auto _data = std::vector{}; + auto _data = std::vector>{}; auto _omnilib_path = path::get_origin(_omnilib); auto _omnilib_dl_path = path::get_origin(_omnilib_dl); diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index 05ff24b51c04..96dd78328e1e 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -3,10 +3,38 @@ #include "common/environment.hpp" +#include #include +#include using namespace rocprofsys::common; +// ── fake_env ───────────────────────────────────────────────────────────────── +// In-memory environment backend for unit tests. +// Call fake_env::reset() in SetUp()/TearDown() to isolate tests. +struct fake_env +{ + inline static std::unordered_map store; + + static int setenv(const char* name, const char* value, int overwrite) + { + if(!overwrite && store.count(name)) return 0; + store[name] = value; + return 0; + } + + static char* getenv(const char* name) + { + auto it = store.find(name); + return it != store.end() ? it->second.data() : nullptr; + } + + static void reset() { store.clear(); } +}; + +// Convenience alias used throughout the injection tests. +using fake_environment = environment; + class IsPythonInterpreterTest : public ::testing::Test {}; @@ -189,215 +217,200 @@ TEST_F(DuplicatedEnvironmentEntriesTest, RocmEventsPreservesDeviceSyntax) "ROCPROFSYS_ROCM_EVENTS=GRBM_COUNT,SQ_WAVES,SQ_INSTS_VALU,TA_TA_BUSY:device=0"); } -// ── get_env ────────────────────────────────────────────────────────────────── - -class GetEnvTest : public ::testing::Test +class AddTorchLibraryPathTest : public ::testing::Test { protected: - void SetUp() override { unsetenv("ROCPROFSYS_TEST_VAR"); } - void TearDown() override { unsetenv("ROCPROFSYS_TEST_VAR"); } + std::unordered_set updated_envs; }; -TEST_F(GetEnvTest, StringReturnsDefaultWhenUnset) +TEST_F(AddTorchLibraryPathTest, SkipsNonPythonExecutables) { - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{ "fallback" }), "fallback"); + std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; + add_torch_library_path(envp, "/usr/bin/bash", false, updated_envs); + ASSERT_EQ(envp.size(), 1); + EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); } -TEST_F(GetEnvTest, StringReturnsValueWhenSet) +TEST_F(AddTorchLibraryPathTest, HandlesEmptyExecutable) { - setenv("ROCPROFSYS_TEST_VAR", "hello", 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{ "fallback" }), "hello"); + std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; + add_torch_library_path(envp, std::string_view{}, false, updated_envs); + ASSERT_EQ(envp.size(), 1); + EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); } -TEST_F(GetEnvTest, StringEmptyVarNameReturnsDefault) -{ - EXPECT_EQ(get_env("", std::string{ "fallback" }), "fallback"); -} +// ── Dependency-injection tests via fake_env ─────────────────────────────────── +// These tests never touch the real process environment. -TEST_F(GetEnvTest, IntReturnsDefaultWhenUnset) +class FakeEnvGetEnvTest : public ::testing::Test { - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 42), 42); -} +protected: + void SetUp() override { fake_env::reset(); } + void TearDown() override { fake_env::reset(); } +}; -TEST_F(GetEnvTest, IntReturnsValueWhenSet) +TEST_F(FakeEnvGetEnvTest, StringReturnsDefaultWhenUnset) { - setenv("ROCPROFSYS_TEST_VAR", "7", 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 42), 7); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{ "default" }), "default"); } -TEST_F(GetEnvTest, IntReturnsDefaultOnInvalidValue) +TEST_F(FakeEnvGetEnvTest, StringReturnsValueWhenSet) { - setenv("ROCPROFSYS_TEST_VAR", "not_a_number", 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 42), 42); + fake_env::setenv("FOO", "bar", 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{ "default" }), "bar"); } -TEST_F(GetEnvTest, IntHandlesNegativeValue) +TEST_F(FakeEnvGetEnvTest, IntReturnsDefaultWhenUnset) { - setenv("ROCPROFSYS_TEST_VAR", "-5", 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 0), -5); + EXPECT_EQ(fake_environment::get_env("FOO", 42), 42); } -TEST_F(GetEnvTest, BoolReturnsDefaultWhenUnset) +TEST_F(FakeEnvGetEnvTest, IntReturnsValueWhenSet) { - EXPECT_FALSE(get_env("ROCPROFSYS_TEST_VAR", false)); - EXPECT_TRUE(get_env("ROCPROFSYS_TEST_VAR", true)); + fake_env::setenv("FOO", "7", 1); + EXPECT_EQ(fake_environment::get_env("FOO", 42), 7); } -TEST_F(GetEnvTest, BoolTrueVariants) +TEST_F(FakeEnvGetEnvTest, BoolTrueVariants) { - for(const char* v : { "1", "true", "TRUE", "yes", "on" }) + for(const char* v : { "1", "true", "yes", "on" }) { - setenv("ROCPROFSYS_TEST_VAR", v, 1); - EXPECT_TRUE(get_env("ROCPROFSYS_TEST_VAR", false)) << "value: " << v; + fake_env::reset(); + fake_env::setenv("FOO", v, 1); + EXPECT_TRUE(fake_environment::get_env("FOO", false)) << "value: " << v; } } -TEST_F(GetEnvTest, BoolFalseVariants) +TEST_F(FakeEnvGetEnvTest, BoolFalseVariants) { - for(const char* v : { "0", "false", "FALSE", "no", "off" }) + for(const char* v : { "0", "false", "no", "off" }) { - setenv("ROCPROFSYS_TEST_VAR", v, 1); - EXPECT_FALSE(get_env("ROCPROFSYS_TEST_VAR", true)) << "value: " << v; + fake_env::reset(); + fake_env::setenv("FOO", v, 1); + EXPECT_FALSE(fake_environment::get_env("FOO", true)) << "value: " << v; } } -TEST_F(GetEnvTest, SizeTReturnsDefaultWhenUnset) -{ - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", size_t{ 100 }), size_t{ 100 }); -} - -TEST_F(GetEnvTest, SizeTReturnsValueWhenSet) -{ - setenv("ROCPROFSYS_TEST_VAR", "512", 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", size_t{ 1 }), size_t{ 512 }); -} - -TEST_F(GetEnvTest, DoubleReturnsDefaultWhenUnset) +TEST_F(FakeEnvGetEnvTest, DoubleReturnsValueWhenSet) { - EXPECT_DOUBLE_EQ(get_env("ROCPROFSYS_TEST_VAR", -1.0), -1.0); + fake_env::setenv("FOO", "3.14", 1); + EXPECT_NEAR(fake_environment::get_env("FOO", 0.0), 3.14, 1e-9); } -TEST_F(GetEnvTest, DoubleReturnsValueWhenSet) +TEST_F(FakeEnvGetEnvTest, OneArgFormReturnsEmptyWhenUnset) { - setenv("ROCPROFSYS_TEST_VAR", "3.14", 1); - EXPECT_NEAR(get_env("ROCPROFSYS_TEST_VAR", 0.0), 3.14, 1e-9); + EXPECT_EQ(fake_environment::get_env("FOO"), ""); } -TEST_F(GetEnvTest, DoubleHandlesNegativeValue) +TEST_F(FakeEnvGetEnvTest, EmptyVarNameReturnsDefault) { - setenv("ROCPROFSYS_TEST_VAR", "-2.5", 1); - EXPECT_DOUBLE_EQ(get_env("ROCPROFSYS_TEST_VAR", 0.0), -2.5); + EXPECT_EQ(fake_environment::get_env("", std::string{ "fallback" }), "fallback"); } -TEST_F(GetEnvTest, OneArgFormReturnsEmptyStringWhenUnset) +TEST_F(FakeEnvGetEnvTest, DoesNotLeakToRealEnvironment) { - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR"), ""); + fake_env::setenv("ROCPROFSYS_FAKE_TEST_ISOLATION", "injected", 1); + // The real process environment must not have been modified. + EXPECT_EQ(::getenv("ROCPROFSYS_FAKE_TEST_ISOLATION"), nullptr); } -TEST_F(GetEnvTest, OneArgFormReturnsValueWhenSet) -{ - setenv("ROCPROFSYS_TEST_VAR", "found", 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR"), "found"); -} - -// ── set_env ─────────────────────────────────────────────────────────────────── - -class SetEnvTest : public ::testing::Test +class FakeEnvSetEnvTest : public ::testing::Test { protected: - void SetUp() override { unsetenv("ROCPROFSYS_TEST_VAR"); } - void TearDown() override { unsetenv("ROCPROFSYS_TEST_VAR"); } + void SetUp() override { fake_env::reset(); } + void TearDown() override { fake_env::reset(); } }; -TEST_F(SetEnvTest, SetsStringValue) +TEST_F(FakeEnvSetEnvTest, SetsStringValue) { - set_env("ROCPROFSYS_TEST_VAR", std::string{ "val" }, 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{}), "val"); + fake_environment::set_env("FOO", std::string{ "hello" }, 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "hello"); } -TEST_F(SetEnvTest, SetsIntValue) +TEST_F(FakeEnvSetEnvTest, SetsIntValue) { - set_env("ROCPROFSYS_TEST_VAR", 99, 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", 0), 99); + fake_environment::set_env("FOO", 99, 1); + EXPECT_EQ(fake_environment::get_env("FOO", 0), 99); } -TEST_F(SetEnvTest, OverrideZeroDoesNotOverwriteExisting) +TEST_F(FakeEnvSetEnvTest, OverrideZeroDoesNotOverwrite) { - setenv("ROCPROFSYS_TEST_VAR", "original", 1); - set_env("ROCPROFSYS_TEST_VAR", std::string{ "new" }, 0); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{}), "original"); + fake_env::setenv("FOO", "original", 1); + fake_environment::set_env("FOO", std::string{ "new" }, 0); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "original"); } -TEST_F(SetEnvTest, OverrideOneOverwritesExisting) +TEST_F(FakeEnvSetEnvTest, OverrideOneOverwrites) { - setenv("ROCPROFSYS_TEST_VAR", "original", 1); - set_env("ROCPROFSYS_TEST_VAR", std::string{ "new" }, 1); - EXPECT_EQ(get_env("ROCPROFSYS_TEST_VAR", std::string{}), "new"); + fake_env::setenv("FOO", "original", 1); + fake_environment::set_env("FOO", std::string{ "new" }, 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "new"); } -// ── get_env_choice ─────────────────────────────────────────────────────────── - -class GetEnvChoiceTest : public ::testing::Test +class FakeEnvGetEnvChoiceTest : public ::testing::Test { protected: - void SetUp() override { unsetenv("ROCPROFSYS_TEST_VAR"); } - void TearDown() override { unsetenv("ROCPROFSYS_TEST_VAR"); } + void SetUp() override { fake_env::reset(); } + void TearDown() override { fake_env::reset(); } }; -TEST_F(GetEnvChoiceTest, ReturnsDefaultWhenUnset) +TEST_F(FakeEnvGetEnvChoiceTest, ReturnsDefaultWhenUnset) { - auto result = get_env_choice("ROCPROFSYS_TEST_VAR", "trace", - { "trace", "sampling", "causal" }); + auto result = fake_environment::get_env_choice( + "FOO", "trace", { "trace", "sampling", "causal" }); EXPECT_EQ(result, "trace"); } -TEST_F(GetEnvChoiceTest, ReturnsValueWhenValidChoiceSet) +TEST_F(FakeEnvGetEnvChoiceTest, ReturnsValueWhenValidChoiceSet) { - setenv("ROCPROFSYS_TEST_VAR", "sampling", 1); - auto result = get_env_choice("ROCPROFSYS_TEST_VAR", "trace", - { "trace", "sampling", "causal" }); + fake_env::setenv("FOO", "sampling", 1); + auto result = fake_environment::get_env_choice( + "FOO", "trace", { "trace", "sampling", "causal" }); EXPECT_EQ(result, "sampling"); } -TEST_F(GetEnvChoiceTest, ReturnsDefaultWhenInvalidChoiceSet) +TEST_F(FakeEnvGetEnvChoiceTest, ReturnsDefaultWhenInvalidChoiceSet) { - setenv("ROCPROFSYS_TEST_VAR", "invalid_mode", 1); - auto result = get_env_choice("ROCPROFSYS_TEST_VAR", "trace", - { "trace", "sampling", "causal" }); + fake_env::setenv("FOO", "bad_value", 1); + auto result = fake_environment::get_env_choice( + "FOO", "trace", { "trace", "sampling", "causal" }); EXPECT_EQ(result, "trace"); } -TEST_F(GetEnvChoiceTest, AllValidChoicesAccepted) -{ - const std::set choices = { "inprocess", "system", "all" }; - for(const auto& choice : choices) - { - setenv("ROCPROFSYS_TEST_VAR", choice.c_str(), 1); - EXPECT_EQ( - get_env_choice("ROCPROFSYS_TEST_VAR", "inprocess", choices), - choice) - << "choice: " << choice; - } -} - -class AddTorchLibraryPathTest : public ::testing::Test +class FakeEnvConfigTest : public ::testing::Test { protected: - std::unordered_set updated_envs; + void SetUp() override { fake_env::reset(); } + void TearDown() override { fake_env::reset(); } }; -TEST_F(AddTorchLibraryPathTest, SkipsNonPythonExecutables) +TEST_F(FakeEnvConfigTest, OperatorSetsValue) { - std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; - add_torch_library_path(envp, "/usr/bin/bash", false, updated_envs); - ASSERT_EQ(envp.size(), 1); - EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); + env_config cfg; + cfg.env_name = "FOO"; + cfg.env_value = "injected"; + cfg.override = 1; + cfg(); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "injected"); } -TEST_F(AddTorchLibraryPathTest, HandlesEmptyExecutable) +TEST_F(FakeEnvConfigTest, OperatorRespectsOverrideZero) { - std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; - add_torch_library_path(envp, std::string_view{}, false, updated_envs); - ASSERT_EQ(envp.size(), 1); - EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); + fake_env::setenv("FOO", "original", 1); + env_config cfg; + cfg.env_name = "FOO"; + cfg.env_value = "new"; + cfg.override = 0; + cfg(); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "original"); +} + +TEST_F(FakeEnvConfigTest, EmptyNameIsNoop) +{ + env_config cfg; + cfg.env_name = ""; + cfg.env_value = "ignored"; + cfg.override = 1; + EXPECT_EQ(cfg(), -1); + EXPECT_TRUE(fake_env::store.empty()); } From db9ad77e61ff9928122d335b81f2d32b6ea298fb Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Tue, 19 May 2026 10:46:59 +0200 Subject: [PATCH 3/9] fix ci --- .../source/lib/common/defines.h.in | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/defines.h.in b/projects/rocprofiler-systems/source/lib/common/defines.h.in index 1d9c5bec0954..a1c979855a50 100644 --- a/projects/rocprofiler-systems/source/lib/common/defines.h.in +++ b/projects/rocprofiler-systems/source/lib/common/defines.h.in @@ -118,9 +118,15 @@ #define ROCPROFSYS_ATTRIBUTE(...) __attribute__((__VA_ARGS__)) #define ROCPROFSYS_VISIBILITY(MODE) ROCPROFSYS_ATTRIBUTE(visibility(MODE)) -#define ROCPROFSYS_PUBLIC_API ROCPROFSYS_VISIBILITY("default") -#define ROCPROFSYS_HIDDEN_API ROCPROFSYS_VISIBILITY("hidden") -#define ROCPROFSYS_INTERNAL_API ROCPROFSYS_VISIBILITY("internal") +#if !defined(ROCPROFSYS_PUBLIC_API) +# define ROCPROFSYS_PUBLIC_API ROCPROFSYS_VISIBILITY("default") +#endif +#if !defined(ROCPROFSYS_HIDDEN_API) +# define ROCPROFSYS_HIDDEN_API ROCPROFSYS_VISIBILITY("hidden") +#endif +#if !defined(ROCPROFSYS_INTERNAL_API) +# define ROCPROFSYS_INTERNAL_API ROCPROFSYS_VISIBILITY("internal") +#endif #define ROCPROFSYS_INLINE ROCPROFSYS_ATTRIBUTE(always_inline) inline #define ROCPROFSYS_NOINLINE ROCPROFSYS_ATTRIBUTE(noinline) #define ROCPROFSYS_HOT ROCPROFSYS_ATTRIBUTE(hot) From a726483053c167ec8a8ff5ab48c26399a43c0a23 Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Tue, 19 May 2026 12:05:31 +0200 Subject: [PATCH 4/9] fix python extension undefined symbol and clang-format violation libpyrocprofsys.so had an undefined symbol for tim::get_env because: - timemory/mpl/policy.hpp includes mpl/type_traits.hpp which calls get_env via TIMEMORY_REPORT_ENV_QUERY, with only environment/types.hpp visible (explicit specialization declaration, no definition) - previously librocprof-sys.so exported this symbol by pulling it from timemory-cxx-static; after removing tim::get_env usages from the library, the linker dropped the symbol from the export table - fix: include timemory/environment/definition.hpp before mpl/policy.hpp so the inline specialization definition is compiled into libpyrocprofsys.so Also fix clang-format violation in defines.h.in (macro alignment whitespace). Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../source/lib/common/defines.h.in | 16 ++++++++-------- .../source/python/libpyrocprofsys.cpp | 5 +++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/defines.h.in b/projects/rocprofiler-systems/source/lib/common/defines.h.in index a1c979855a50..0c563dbf79bf 100644 --- a/projects/rocprofiler-systems/source/lib/common/defines.h.in +++ b/projects/rocprofiler-systems/source/lib/common/defines.h.in @@ -127,14 +127,14 @@ #if !defined(ROCPROFSYS_INTERNAL_API) # define ROCPROFSYS_INTERNAL_API ROCPROFSYS_VISIBILITY("internal") #endif -#define ROCPROFSYS_INLINE ROCPROFSYS_ATTRIBUTE(always_inline) inline -#define ROCPROFSYS_NOINLINE ROCPROFSYS_ATTRIBUTE(noinline) -#define ROCPROFSYS_HOT ROCPROFSYS_ATTRIBUTE(hot) -#define ROCPROFSYS_COLD ROCPROFSYS_ATTRIBUTE(cold) -#define ROCPROFSYS_CONST ROCPROFSYS_ATTRIBUTE(const) -#define ROCPROFSYS_PURE ROCPROFSYS_ATTRIBUTE(pure) -#define ROCPROFSYS_WEAK ROCPROFSYS_ATTRIBUTE(weak) -#define ROCPROFSYS_PACKED ROCPROFSYS_ATTRIBUTE(__packed__) +#define ROCPROFSYS_INLINE ROCPROFSYS_ATTRIBUTE(always_inline) inline +#define ROCPROFSYS_NOINLINE ROCPROFSYS_ATTRIBUTE(noinline) +#define ROCPROFSYS_HOT ROCPROFSYS_ATTRIBUTE(hot) +#define ROCPROFSYS_COLD ROCPROFSYS_ATTRIBUTE(cold) +#define ROCPROFSYS_CONST ROCPROFSYS_ATTRIBUTE(const) +#define ROCPROFSYS_PURE ROCPROFSYS_ATTRIBUTE(pure) +#define ROCPROFSYS_WEAK ROCPROFSYS_ATTRIBUTE(weak) +#define ROCPROFSYS_PACKED ROCPROFSYS_ATTRIBUTE(__packed__) #define ROCPROFSYS_PACKED_ALIGN(VAL) \ ROCPROFSYS_PACKED ROCPROFSYS_ATTRIBUTE(__aligned__(VAL)) #define ROCPROFSYS_LIKELY(...) __builtin_expect((__VA_ARGS__), 1) diff --git a/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp b/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp index eb99b08dc75c..002314004ec7 100644 --- a/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp +++ b/projects/rocprofiler-systems/source/python/libpyrocprofsys.cpp @@ -11,6 +11,11 @@ #include "common/environment.hpp" #include #include +// Provides inline tim::get_env/ specialization definitions before +// mpl/policy.hpp pulls in mpl/type_traits.hpp, which calls get_env via +// TIMEMORY_REPORT_ENV_QUERY. libpyrocprofsys.so does not link against timemory-cxx-static +// so it cannot rely on librocprof-sys.so exporting these symbols. +#include #include #include #include From ef6cd91ea48268df53c7fa0007cc80353a4e9650 Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Wed, 27 May 2026 10:54:43 +0200 Subject: [PATCH 5/9] increase test coverage --- .../source/lib/common/environment.hpp | 65 +++-- .../lib/common/tests/test_environment.cpp | 241 ++++++++++++++++++ 2 files changed, 288 insertions(+), 18 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index a0bfbcc4087e..5837e879f278 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -95,35 +96,58 @@ struct environment std::is_same_v, std::string_view> || std::is_same_v, const char*>) { - if(env_id.empty()) return std::string{ _default }; + if(env_id.empty()) + { + return std::string{ _default }; + } auto* env_var = EnvType::getenv(env_id.data()); return env_var ? std::string{ env_var } : std::string{ _default }; } else if constexpr(std::is_same_v) { - if(env_id.empty()) return _default; + if(env_id.empty()) + { + return _default; + } auto* env_var = EnvType::getenv(env_id.data()); if(!env_var) return _default; if(std::string_view{ env_var }.empty()) + { throw std::runtime_error(std::string{ "No boolean value provided for " } + std::string{ env_id }); + } if(std::string_view{ env_var }.find_first_not_of("0123456789") == std::string_view::npos) + { return static_cast(std::stoi(env_var)); + } for(size_t i = 0; i < strlen(env_var); ++i) + { env_var[i] = tolower(env_var[i]); + } for(const auto& itr : { "off", "false", "no", "n", "f", "0" }) - if(strcmp(env_var, itr) == 0) return false; + { + if(strcmp(env_var, itr) == 0) + { + return false; + } + } return true; } else if constexpr(std::is_floating_point_v) { - if(env_id.empty()) return _default; + if(env_id.empty()) + { + return _default; + } auto* env_var = EnvType::getenv(env_id.data()); - if(!env_var) return _default; + if(!env_var) + { + return _default; + } try { return static_cast(std::stod(env_var)); @@ -145,9 +169,13 @@ struct environment try { if constexpr(std::is_same_v) + { return std::stoi(env_var); + } else + { return static_cast(std::stoll(env_var)); + } } catch(const std::exception& e) { fprintf( @@ -162,16 +190,17 @@ struct environment public: template - static auto get_env(std::string_view env_id, Tp&& _default) + static auto get_env(std::string_view env_id, Tp&& value_default) { - if constexpr(std::is_enum::value) + if constexpr(std::is_enum_v) { - using Up = std::underlying_type_t; - return static_cast(get_env_impl(env_id, static_cast(_default))); + using up_t = std::underlying_type_t; + return static_cast( + get_env_impl(env_id, static_cast(value_default))); } else { - return get_env_impl(env_id, std::forward(_default)); + return get_env_impl(env_id, std::forward(value_default)); } } @@ -182,28 +211,28 @@ struct environment } template - static void set_env(const std::string& env_var, const Tp& _val, int override) + static void set_env(const std::string& env_var, const Tp& value, int override) { std::stringstream ss_val; - ss_val << _val; + ss_val << value; EnvType::setenv(env_var.c_str(), ss_val.str().c_str(), override); } template - static auto get_env_choice(std::string_view env_id, Tp _default, - std::set _choices) + static auto get_env_choice(std::string_view env_id, Tp value_default, + std::set choices) { - auto _val = get_env(env_id, _default); - if(_choices.find(_val) == _choices.end()) + auto value = get_env(env_id, value_default); + if(choices.find(value) == choices.end()) { fprintf( stderr, "[rocprof-sys][get_env] Environment variable \"%s\" has invalid value " "\"%s\". Reverting to default.\n", env_id.data(), get_env(env_id).c_str()); - return _default; + return value_default; } - return _val; + return value; } }; diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index 96dd78328e1e..02baca104006 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -239,6 +239,79 @@ TEST_F(AddTorchLibraryPathTest, HandlesEmptyExecutable) EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); } +TEST_F(AddTorchLibraryPathTest, PythonExecutableWithoutTorchLeavesEnvUnchanged) +{ + // "/nonexistent/python3" is recognised as a Python interpreter but + // discover_torch_libpath returns "" → early return, env unchanged. + std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; + add_torch_library_path(envp, "/nonexistent/python3", false, updated_envs); + ASSERT_EQ(envp.size(), 1); + EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); +} + +// ── discover_torch_libpath direct tests (L384–L442) ────────────────────────── + +TEST(DiscoverTorchLibpathTest, EmptyBinaryReturnsEmpty) +{ + EXPECT_EQ(discover_torch_libpath(""), ""); +} + +TEST(DiscoverTorchLibpathTest, UnsafeCharInPathReturnsEmpty) +{ + // ';' hits default: return false in is_safe_executable_path + EXPECT_EQ(discover_torch_libpath("/usr/bin/python;injected", true), ""); +} + +TEST(DiscoverTorchLibpathTest, SpaceInPathReturnsEmpty) +{ + EXPECT_EQ(discover_torch_libpath("/path with space/python3", true), ""); +} + +TEST(DiscoverTorchLibpathTest, DollarSignInPathReturnsEmpty) +{ + EXPECT_EQ(discover_torch_libpath("$HOME/python3", true), ""); +} + +TEST(DiscoverTorchLibpathTest, SafePathWithAllowedCharsAndNoTorchReturnsEmpty) +{ + // Path uses all allowed non-alnum chars (/ _ - + .) to exercise every + // switch case in is_safe_executable_path. Shell exits non-zero (not found) + // → popen succeeds, status != 0 → returns empty string. + EXPECT_EQ(discover_torch_libpath("/nonexistent_dir/python-3.11+safe.bin", true), ""); +} + +// ── posix_env + forwarding free functions (L77, L79, L269–L279) ────────────── +// Calling get_env / set_env without the fake_environment:: prefix exercises +// posix_env::getenv / setenv through the real environment. + +TEST(FreeFunctionEnvTest, GetEnvReturnsDefaultForUnsetVar) +{ + // Unset variable → default returned; covers posix_env::getenv + free-func body + EXPECT_EQ(get_env("ROCPROFSYS_FREE_FUNC_TEST_UNSET_99887766", std::string{ "dflt" }), + "dflt"); +} + +TEST(FreeFunctionEnvTest, GetEnvOneArgReturnsEmptyForUnsetVar) +{ + EXPECT_EQ(get_env("ROCPROFSYS_FREE_FUNC_TEST_UNSET_99887766"), ""); +} + +TEST(FreeFunctionEnvTest, SetEnvAndGetViaFreeFunction) +{ + const char* name = "ROCPROFSYS_FREE_FUNC_SETENV_TEST_99887766"; + set_env(std::string{ name }, std::string{ "free_val" }, 1); + EXPECT_EQ(get_env(name, std::string{}), "free_val"); + ::unsetenv(name); +} + +// ── Enum used by the dispatch tests below ──────────────────────────────────── +enum class test_color : int +{ + red = 0, + green = 1, + blue = 2, +}; + // ── Dependency-injection tests via fake_env ─────────────────────────────────── // These tests never touch the real process environment. @@ -377,6 +450,110 @@ TEST_F(FakeEnvGetEnvChoiceTest, ReturnsDefaultWhenInvalidChoiceSet) EXPECT_EQ(result, "trace"); } +// ── Bool branch gaps ────────────────────────────────────────────────────────── + +TEST_F(FakeEnvGetEnvTest, BoolEmptyEnvIdReturnsDefault) +{ + EXPECT_FALSE(fake_environment::get_env("", false)); + EXPECT_TRUE(fake_environment::get_env("", true)); +} + +TEST_F(FakeEnvGetEnvTest, BoolEmptyValueThrows) +{ + fake_env::setenv("FOO", "", 1); + EXPECT_THROW(fake_environment::get_env("FOO", false), std::runtime_error); +} + +TEST_F(FakeEnvGetEnvTest, BoolNumericGreaterThanOneIsTrue) +{ + fake_env::setenv("FOO", "2", 1); + EXPECT_TRUE(fake_environment::get_env("FOO", false)); +} + +TEST_F(FakeEnvGetEnvTest, BoolUpperCaseFalseVariants) +{ + for(const char* v : { "FALSE", "NO", "OFF", "F", "N" }) + { + fake_env::reset(); + fake_env::setenv("FOO", v, 1); + EXPECT_FALSE(fake_environment::get_env("FOO", true)) << "value: " << v; + } +} + +// ── Float branch gaps ───────────────────────────────────────────────────────── + +TEST_F(FakeEnvGetEnvTest, FloatEmptyEnvIdReturnsDefault) +{ + EXPECT_NEAR(fake_environment::get_env("", 1.5), 1.5, 1e-9); +} + +TEST_F(FakeEnvGetEnvTest, FloatInvalidStringReturnsDefault) +{ + fake_env::setenv("FOO", "not_a_float", 1); + EXPECT_NEAR(fake_environment::get_env("FOO", 9.9), 9.9, 1e-9); +} + +TEST_F(FakeEnvGetEnvTest, FloatTypeReturnsValue) +{ + fake_env::setenv("FOO", "1.5", 1); + EXPECT_NEAR(fake_environment::get_env("FOO", 0.0f), 1.5f, 1e-6f); +} + +// ── Integral branch gaps ────────────────────────────────────────────────────── + +TEST_F(FakeEnvGetEnvTest, IntEmptyEnvIdReturnsDefault) +{ + EXPECT_EQ(fake_environment::get_env("", 99), 99); +} + +TEST_F(FakeEnvGetEnvTest, IntInvalidStringReturnsDefault) +{ + fake_env::setenv("FOO", "not_an_int", 1); + EXPECT_EQ(fake_environment::get_env("FOO", 42), 42); +} + +TEST_F(FakeEnvGetEnvTest, LongReturnsValueWhenSet) +{ + fake_env::setenv("FOO", "123456789012", 1); + EXPECT_EQ(fake_environment::get_env("FOO", 0L), 123456789012L); +} + +TEST_F(FakeEnvGetEnvTest, SizeTReturnsValueWhenSet) +{ + fake_env::setenv("FOO", "42", 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::size_t{ 0 }), std::size_t{ 42 }); +} + +// ── to_env_string ───────────────────────────────────────────────────────────── + +TEST(ToEnvStringTest, BoolTrue) { EXPECT_EQ(to_env_string(true), "true"); } + +TEST(ToEnvStringTest, BoolFalse) { EXPECT_EQ(to_env_string(false), "false"); } + +TEST(ToEnvStringTest, Int) { EXPECT_EQ(to_env_string(42), "42"); } + +TEST(ToEnvStringTest, Double) { EXPECT_EQ(to_env_string(3.14), std::to_string(3.14)); } + +TEST(ToEnvStringTest, StringPassthrough) +{ + EXPECT_EQ(to_env_string(std::string{ "hello" }), "hello"); +} + +TEST(ToEnvStringTest, ConstCharPtrPassthrough) +{ + EXPECT_EQ(to_env_string("world"), std::string{ "world" }); +} + +// ── consolidate_env_entries: no-'=' entry ───────────────────────────────────── + +TEST_F(DuplicatedEnvironmentEntriesTest, SkipsEntryWithoutEqualsSign) +{ + std::vector env_vars = { "NO_EQUALS", "PATH=/usr/bin" }; + consolidate_env_entries(env_vars); + ASSERT_EQ(env_vars.size(), 1); + EXPECT_EQ(env_vars[0], "PATH=/usr/bin"); +} + class FakeEnvConfigTest : public ::testing::Test { protected: @@ -405,6 +582,16 @@ TEST_F(FakeEnvConfigTest, OperatorRespectsOverrideZero) EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "original"); } +TEST_F(FakeEnvConfigTest, VerbosePathExecutes) +{ + env_config cfg; + cfg.env_name = "FOO"; + cfg.env_value = "verbose_value"; + cfg.override = 1; + EXPECT_EQ(cfg(true), 0); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "verbose_value"); +} + TEST_F(FakeEnvConfigTest, EmptyNameIsNoop) { env_config cfg; @@ -414,3 +601,57 @@ TEST_F(FakeEnvConfigTest, EmptyNameIsNoop) EXPECT_EQ(cfg(), -1); EXPECT_TRUE(fake_env::store.empty()); } + +// ── Enum dispatch (get_env where is_enum_v) ────────────────────────── + +class FakeEnvEnumTest : public ::testing::Test +{ +protected: + void SetUp() override { fake_env::reset(); } + void TearDown() override { fake_env::reset(); } +}; + +TEST_F(FakeEnvEnumTest, EnumReturnsDefaultWhenUnset) +{ + EXPECT_EQ(fake_environment::get_env("FOO", test_color::green), test_color::green); +} + +TEST_F(FakeEnvEnumTest, EnumReturnsValueWhenSet) +{ + fake_env::setenv("FOO", "2", 1); + EXPECT_EQ(fake_environment::get_env("FOO", test_color::red), test_color::blue); +} + +TEST_F(FakeEnvEnumTest, EnumEmptyEnvIdReturnsDefault) +{ + EXPECT_EQ(fake_environment::get_env("", test_color::green), test_color::green); +} + +// ── get_env_choice with non-string type ─────────────────────────────────────── + +class FakeEnvIntChoiceTest : public ::testing::Test +{ +protected: + void SetUp() override { fake_env::reset(); } + void TearDown() override { fake_env::reset(); } +}; + +TEST_F(FakeEnvIntChoiceTest, ReturnsDefaultWhenUnset) +{ + auto result = fake_environment::get_env_choice("FOO", 1, std::set{ 1, 2, 3 }); + EXPECT_EQ(result, 1); +} + +TEST_F(FakeEnvIntChoiceTest, ReturnsValueWhenValidChoiceSet) +{ + fake_env::setenv("FOO", "3", 1); + auto result = fake_environment::get_env_choice("FOO", 1, std::set{ 1, 2, 3 }); + EXPECT_EQ(result, 3); +} + +TEST_F(FakeEnvIntChoiceTest, ReturnsDefaultWhenInvalidChoiceSet) +{ + fake_env::setenv("FOO", "99", 1); + auto result = fake_environment::get_env_choice("FOO", 1, std::set{ 1, 2, 3 }); + EXPECT_EQ(result, 1); +} From c89cd156ee31c43f20f912d7edfea37700ef778c Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Wed, 27 May 2026 11:38:31 +0200 Subject: [PATCH 6/9] improve readability --- .../source/lib/common/environment.hpp | 260 ++++++++++-------- .../lib/common/tests/test_environment.cpp | 24 +- 2 files changed, 157 insertions(+), 127 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index 5837e879f278..742c4dc373fb 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -4,15 +4,17 @@ #pragma once #include "common/defines.h" -#include - #include "common/join.hpp" + +#include +#include + #include +#include #include #include #include #include -#include #include #include #include @@ -20,10 +22,12 @@ #include #include #include -#include #include #include +#include #include +#include +#include #if !defined(ROCPROFSYS_ENVIRON_LOG_NAME) # if defined(ROCPROFSYS_COMMON_LIBRARY_NAME) @@ -89,102 +93,119 @@ template struct environment { private: + // env_id is always constructed from a null-terminated string at every call site + static const char* fetch_raw_env(std::string_view env_id) + { + if(env_id.empty()) return nullptr; + return EnvType::getenv( + env_id.data()); // NOLINT(bugprone-suspicious-stringview-data-usage) + } + template - static auto get_env_impl(std::string_view env_id, Tp _default) + static std::string get_env_string(std::string_view env_id, const Tp& fallback) { - if constexpr(std::is_same_v, std::string> || - std::is_same_v, std::string_view> || - std::is_same_v, const char*>) + const char* raw = fetch_raw_env(env_id); + return raw ? std::string{ raw } : std::string{ fallback }; + } + + static bool get_env_bool(std::string_view env_id, bool fallback) + { + const char* raw = fetch_raw_env(env_id); + if(!raw) return fallback; + + const std::string_view env_sv{ raw }; + if(env_sv.empty()) { - if(env_id.empty()) - { - return std::string{ _default }; - } - auto* env_var = EnvType::getenv(env_id.data()); - return env_var ? std::string{ env_var } : std::string{ _default }; + throw std::runtime_error( + std::string{ "No boolean value provided for " }.append(env_id)); } - else if constexpr(std::is_same_v) + + if(env_sv.find_first_not_of("0123456789") == std::string_view::npos) { - if(env_id.empty()) - { - return _default; - } - auto* env_var = EnvType::getenv(env_id.data()); - if(!env_var) return _default; + return static_cast(std::stoi(raw)); + } - if(std::string_view{ env_var }.empty()) - { - throw std::runtime_error(std::string{ "No boolean value provided for " } + - std::string{ env_id }); - } + std::string lower{ env_sv }; + std::transform(lower.begin(), lower.end(), lower.begin(), + [](unsigned char chr) { return std::tolower(chr); }); + + constexpr auto false_values = std::array{ + std::string_view{ "off" }, std::string_view{ "false" }, + std::string_view{ "no" }, std::string_view{ "n" }, + std::string_view{ "f" }, std::string_view{ "0" }, + }; + return !std::any_of(false_values.begin(), false_values.end(), + [&lower](std::string_view val) { return lower == val; }); + } - if(std::string_view{ env_var }.find_first_not_of("0123456789") == - std::string_view::npos) - { - return static_cast(std::stoi(env_var)); - } + template + static Tp get_env_float(std::string_view env_id, Tp fallback) + { + const char* raw = fetch_raw_env(env_id); + if(!raw) return fallback; + try + { + return static_cast(std::stod(raw)); + } catch(const std::exception& exc) + { + (void) fprintf( + stderr, + "[rocprof-sys][get_env] Exception thrown converting " + "getenv(\"%s\") = %s to float: %s\n", + env_id.data(), // NOLINT(bugprone-suspicious-stringview-data-usage) + raw, exc.what()); + } + return fallback; + } - for(size_t i = 0; i < strlen(env_var); ++i) + template + static Tp get_env_integral(std::string_view env_id, Tp fallback) + { + const char* raw = fetch_raw_env(env_id); + if(!raw) return fallback; + try + { + if constexpr(std::is_same_v) { - env_var[i] = tolower(env_var[i]); + return std::stoi(raw); } - for(const auto& itr : { "off", "false", "no", "n", "f", "0" }) + else { - if(strcmp(env_var, itr) == 0) - { - return false; - } + return static_cast(std::stoll(raw)); } - return true; + } catch(const std::exception& exc) + { + (void) fprintf( + stderr, + "[rocprof-sys][get_env] Exception thrown converting " + "getenv(\"%s\") = %s to integer: %s\n", + env_id.data(), // NOLINT(bugprone-suspicious-stringview-data-usage) + raw, exc.what()); + } + return fallback; + } + + template + static auto get_env_impl(std::string_view env_id, const Tp& fallback) + { + if constexpr(std::is_same_v, std::string> || + std::is_same_v, std::string_view> || + std::is_same_v, const char*> || + std::is_same_v, char*>) + { + return get_env_string(env_id, fallback); + } + else if constexpr(std::is_same_v) + { + return get_env_bool(env_id, fallback); } else if constexpr(std::is_floating_point_v) { - if(env_id.empty()) - { - return _default; - } - auto* env_var = EnvType::getenv(env_id.data()); - if(!env_var) - { - return _default; - } - try - { - return static_cast(std::stod(env_var)); - } catch(const std::exception& e) - { - fprintf( - stderr, - "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " - "%s to float: %s\n", - env_id.data(), env_var, e.what()); - } - return _default; + return get_env_float(env_id, fallback); } - else // integral: int, long, size_t, etc. + else { - if(env_id.empty()) return _default; - auto* env_var = EnvType::getenv(env_id.data()); - if(!env_var) return _default; - try - { - if constexpr(std::is_same_v) - { - return std::stoi(env_var); - } - else - { - return static_cast(std::stoll(env_var)); - } - } catch(const std::exception& e) - { - fprintf( - stderr, - "[rocprof-sys][get_env] Exception thrown converting getenv(\"%s\") = " - "%s to integer: %s\n", - env_id.data(), env_var, e.what()); - } - return _default; + return get_env_integral(env_id, fallback); } } @@ -240,16 +261,16 @@ struct environment template struct ROCPROFSYS_INTERNAL_API env_config { - std::string env_name = {}; - std::string env_value = {}; - int override = 0; + std::string m_env_name = {}; + std::string m_env_value = {}; + int m_override = 0; auto operator()(bool _verbose = false) const { - if(env_name.empty()) return -1; - ROCPROFSYS_ENVIRON_LOG(_verbose, "setenv(\"%s\", \"%s\", %i)\n", env_name.c_str(), - env_value.c_str(), override); - return EnvType::setenv(env_name.c_str(), env_value.c_str(), override); + if(m_env_name.empty()) return -1; + ROCPROFSYS_ENVIRON_LOG(_verbose, "setenv(\"%s\", \"%s\", %i)\n", + m_env_name.c_str(), m_env_value.c_str(), m_override); + return EnvType::setenv(m_env_name.c_str(), m_env_value.c_str(), m_override); } }; @@ -259,9 +280,9 @@ struct ROCPROFSYS_INTERNAL_API env_config template inline auto -get_env(std::string_view env_id, Tp&& _default) +get_env(std::string_view env_id, Tp&& value_default) { - return environment<>::get_env(env_id, std::forward(_default)); + return environment<>::get_env(env_id, std::forward(value_default)); } template @@ -273,58 +294,65 @@ get_env(std::string_view env_id) template inline void -set_env(const std::string& env_var, const Tp& _val, int override) +set_env(const std::string& env_var, const Tp& value, int override) { - environment<>::set_env(env_var, _val, override); + environment<>::set_env(env_var, value, override); } template inline auto -get_env_choice(std::string_view env_id, Tp _default, std::set _choices) +get_env_choice(std::string_view env_id, Tp value_default, std::set value_choices) { - return environment<>::get_env_choice(env_id, _default, std::move(_choices)); + return environment<>::get_env_choice(env_id, value_default, std::move(value_choices)); } // ── Env-vector helpers (operate on std::vector, not the real env) ── inline void -remove_env(std::vector& _environ, std::string_view _env_var, - const std::unordered_set& _original_envs) +remove_env(std::vector& env_list, std::string_view env_variable, + const std::unordered_set& original_envs) { - auto key = join("", _env_var, "="); + auto key = join("", env_variable, "="); - _environ.erase(std::remove_if(_environ.begin(), _environ.end(), + env_list.erase(std::remove_if(env_list.begin(), env_list.end(), [&key](const std::string& entry) { return std::string_view{ entry }.find(key) == 0; }), - _environ.end()); + env_list.end()); // Restore from original_envs if previously existed - for(const auto& orig : _original_envs) + for(const auto& orig : original_envs) { - if(std::string_view{ orig }.find(key) == 0) _environ.emplace_back(orig); + if(std::string_view{ orig }.find(key) == 0) env_list.emplace_back(orig); } } inline std::string discover_llvm_libdir_for_ompt(bool verbose = false) { - auto strip = [](std::string s) { - if(!s.empty() && s.back() == '/') s.pop_back(); - return s; + auto strip = [](std::string value_to_strip) { + if(!value_to_strip.empty() && value_to_strip.back() == '/') + { + value_to_strip.pop_back(); + } + return value_to_strip; }; // Common ROCm envs const auto rocm_dir = strip(get_env("ROCM_PATH", "/opt/rocm")); const auto rocmv_dir = strip(get_env("ROCmVersion_DIR", "")); + const constexpr auto number_of_candidates = 6; + std::vector candidates; - candidates.reserve(6); + candidates.reserve(number_of_candidates); - auto push_unique = [&](const std::string& p) { - if(p.empty()) return; - if(std::find(candidates.begin(), candidates.end(), p) == candidates.end()) - candidates.emplace_back(p); + auto push_unique = [&](const std::string& candidate) { + if(candidate.empty()) return; + if(std::find(candidates.begin(), candidates.end(), candidate) == candidates.end()) + { + candidates.emplace_back(candidate); + } }; if(!rocmv_dir.empty()) @@ -343,11 +371,11 @@ discover_llvm_libdir_for_ompt(bool verbose = false) }; // Pick the first candidate that contains libomptarget.so - auto it = std::find_if(candidates.begin(), candidates.end(), has_libomptarget); - if(it != candidates.end()) + auto result = std::find_if(candidates.begin(), candidates.end(), has_libomptarget); + if(result != candidates.end()) { - ROCPROFSYS_ENVIRON_LOG(verbose, "Using LLVM libdir: %s\n", it->c_str()); - return *it; + ROCPROFSYS_ENVIRON_LOG(verbose, "Using LLVM libdir: %s\n", result->c_str()); + return *result; } ROCPROFSYS_ENVIRON_LOG(verbose, @@ -646,7 +674,9 @@ consolidate_env_entries(std::vector& envp) std::size_t total_parts_length = 0; for(const auto& part : parts) + { total_parts_length += part.size(); + } result.reserve(result.size() + total_parts_length + (parts.size() - 1)); diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index 02baca104006..f56393740d41 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -564,9 +564,9 @@ class FakeEnvConfigTest : public ::testing::Test TEST_F(FakeEnvConfigTest, OperatorSetsValue) { env_config cfg; - cfg.env_name = "FOO"; - cfg.env_value = "injected"; - cfg.override = 1; + cfg.m_env_name = "FOO"; + cfg.m_env_value = "injected"; + cfg.m_override = 1; cfg(); EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "injected"); } @@ -575,9 +575,9 @@ TEST_F(FakeEnvConfigTest, OperatorRespectsOverrideZero) { fake_env::setenv("FOO", "original", 1); env_config cfg; - cfg.env_name = "FOO"; - cfg.env_value = "new"; - cfg.override = 0; + cfg.m_env_name = "FOO"; + cfg.m_env_value = "new"; + cfg.m_override = 0; cfg(); EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "original"); } @@ -585,9 +585,9 @@ TEST_F(FakeEnvConfigTest, OperatorRespectsOverrideZero) TEST_F(FakeEnvConfigTest, VerbosePathExecutes) { env_config cfg; - cfg.env_name = "FOO"; - cfg.env_value = "verbose_value"; - cfg.override = 1; + cfg.m_env_name = "FOO"; + cfg.m_env_value = "verbose_value"; + cfg.m_override = 1; EXPECT_EQ(cfg(true), 0); EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "verbose_value"); } @@ -595,9 +595,9 @@ TEST_F(FakeEnvConfigTest, VerbosePathExecutes) TEST_F(FakeEnvConfigTest, EmptyNameIsNoop) { env_config cfg; - cfg.env_name = ""; - cfg.env_value = "ignored"; - cfg.override = 1; + cfg.m_env_name = ""; + cfg.m_env_value = "ignored"; + cfg.m_override = 1; EXPECT_EQ(cfg(), -1); EXPECT_TRUE(fake_env::store.empty()); } From 9fcea3286d4a3751ee2cfbfdf4609c0ddc0d8654 Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Mon, 1 Jun 2026 15:15:45 +0200 Subject: [PATCH 7/9] Applied suggestions from code review --- .../source/bin/common/tool_runner.cpp | 5 +- .../source/bin/rocprof-sys-causal/impl.cpp | 3 +- .../bin/rocprof-sys-instrument/CMakeLists.txt | 1 + .../source/lib/common/CMakeLists.txt | 5 + .../source/lib/common/environment.hpp | 103 ++++-------------- .../source/lib/common/setup.hpp | 2 +- .../tests/test_discover_llvm_libdir.cpp | 8 +- .../lib/common/tests/test_environment.cpp | 22 ++-- .../source/lib/core/argparse.cpp | 4 +- .../source/lib/core/argparse.hpp | 2 +- 10 files changed, 52 insertions(+), 103 deletions(-) diff --git a/projects/rocprofiler-systems/source/bin/common/tool_runner.cpp b/projects/rocprofiler-systems/source/bin/common/tool_runner.cpp index 0fe140d7705d..e743acced3d8 100644 --- a/projects/rocprofiler-systems/source/bin/common/tool_runner.cpp +++ b/projects/rocprofiler-systems/source/bin/common/tool_runner.cpp @@ -330,8 +330,7 @@ tool_runner::get_initial_environment() if(!libexec_path.empty()) data.env.set(env::SCRIPT_PATH, libexec_path); update_verbose_from_env(); - const bool verbose = (data.out.verbose > 0); - if(auto llvm_dir = rocprofsys::common::discover_llvm_libdir_for_ompt(verbose); + if(auto llvm_dir = rocprofsys::common::discover_llvm_libdir_for_ompt(); !llvm_dir.empty()) { data.env.set("LD_LIBRARY_PATH", llvm_dir, update_mode::APPEND); @@ -394,7 +393,7 @@ tool_runner::prepare_environment() rocprofsys::argparse::add_ld_library_path(data); } - rocprofsys::argparse::add_torch_library_path(data, data.out.verbose > 0); + rocprofsys::argparse::add_torch_library_path(data); rocprofsys::common::consolidate_env_entries(data.env.current); } diff --git a/projects/rocprofiler-systems/source/bin/rocprof-sys-causal/impl.cpp b/projects/rocprofiler-systems/source/bin/rocprof-sys-causal/impl.cpp index aba73351666b..234ec22cae44 100644 --- a/projects/rocprofiler-systems/source/bin/rocprof-sys-causal/impl.cpp +++ b/projects/rocprofiler-systems/source/bin/rocprof-sys-causal/impl.cpp @@ -159,8 +159,7 @@ get_initial_environment() update_env(_env, "ROCPROFSYS_LAUNCHER", "rocprof-sys-causal"); // Ensure libomptarget.so can be found by the target (OpenMP/HIP apps) - if(auto llvm_dir = - rocprofsys::common::discover_llvm_libdir_for_ompt(get_verbose() > 0); + if(auto llvm_dir = rocprofsys::common::discover_llvm_libdir_for_ompt(); !llvm_dir.empty()) { update_env(_env, "LD_LIBRARY_PATH", llvm_dir, /*append=*/true); diff --git a/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt b/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt index a3e9c8e35093..247eaf164344 100644 --- a/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt +++ b/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt @@ -37,6 +37,7 @@ target_link_libraries( rocprofiler-systems::rocprofiler-systems-compile-options rocprofiler-systems::rocprofiler-systems-compile-definitions rocprofiler-systems::rocprofiler-systems-sanitizer + rocprofiler-systems::rocprofiler-systems-spdlog timemory::timemory-headers timemory::timemory-extensions timemory::timemory-core diff --git a/projects/rocprofiler-systems/source/lib/common/CMakeLists.txt b/projects/rocprofiler-systems/source/lib/common/CMakeLists.txt index ddc3ba38c4c5..4ae070f7a501 100644 --- a/projects/rocprofiler-systems/source/lib/common/CMakeLists.txt +++ b/projects/rocprofiler-systems/source/lib/common/CMakeLists.txt @@ -54,6 +54,11 @@ target_compile_definitions( INTERFACE $ ) +target_link_libraries( + rocprofiler-systems-common-library + INTERFACE $ +) + if(ROCPROFSYS_BUILD_TESTING) add_subdirectory(tests) endif() diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index 742c4dc373fb..bc654d0e6ec2 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -5,8 +5,8 @@ #include "common/defines.h" #include "common/join.hpp" +#include "logger/debug.hpp" -#include #include #include @@ -29,47 +29,6 @@ #include #include -#if !defined(ROCPROFSYS_ENVIRON_LOG_NAME) -# if defined(ROCPROFSYS_COMMON_LIBRARY_NAME) -# define ROCPROFSYS_ENVIRON_LOG_NAME "[" ROCPROFSYS_COMMON_LIBRARY_NAME "]" -# else -# define ROCPROFSYS_ENVIRON_LOG_NAME -# endif -#endif - -#if !defined(ROCPROFSYS_ENVIRON_LOG_START) -# if defined(ROCPROFSYS_COMMON_LIBRARY_LOG_START) -# define ROCPROFSYS_ENVIRON_LOG_START ROCPROFSYS_COMMON_LIBRARY_LOG_START -# elif defined(TIMEMORY_LOG_COLORS_AVAILABLE) -# define ROCPROFSYS_ENVIRON_LOG_START \ - fprintf(stderr, "%s", ::tim::log::color::info()); -# else -# define ROCPROFSYS_ENVIRON_LOG_START -# endif -#endif - -#if !defined(ROCPROFSYS_ENVIRON_LOG_END) -# if defined(ROCPROFSYS_COMMON_LIBRARY_LOG_END) -# define ROCPROFSYS_ENVIRON_LOG_END ROCPROFSYS_COMMON_LIBRARY_LOG_END -# elif defined(TIMEMORY_LOG_COLORS_AVAILABLE) -# define ROCPROFSYS_ENVIRON_LOG_END \ - fprintf(stderr, "%s", ::tim::log::color::end()); -# else -# define ROCPROFSYS_ENVIRON_LOG_END -# endif -#endif - -#define ROCPROFSYS_ENVIRON_LOG(CONDITION, ...) \ - if(CONDITION) \ - { \ - fflush(stderr); \ - ROCPROFSYS_ENVIRON_LOG_START \ - fprintf(stderr, "[rocprof-sys]" ROCPROFSYS_ENVIRON_LOG_NAME "[%i] ", getpid()); \ - fprintf(stderr, __VA_ARGS__); \ - ROCPROFSYS_ENVIRON_LOG_END \ - fflush(stderr); \ - } - namespace rocprofsys { inline namespace common @@ -148,12 +107,9 @@ struct environment return static_cast(std::stod(raw)); } catch(const std::exception& exc) { - (void) fprintf( - stderr, - "[rocprof-sys][get_env] Exception thrown converting " - "getenv(\"%s\") = %s to float: %s\n", - env_id.data(), // NOLINT(bugprone-suspicious-stringview-data-usage) - raw, exc.what()); + LOG_ERROR("[get_env] Exception thrown converting getenv(\"{}\") = {} to " + "float: {}", + env_id, raw, exc.what()); } return fallback; } @@ -175,12 +131,9 @@ struct environment } } catch(const std::exception& exc) { - (void) fprintf( - stderr, - "[rocprof-sys][get_env] Exception thrown converting " - "getenv(\"%s\") = %s to integer: %s\n", - env_id.data(), // NOLINT(bugprone-suspicious-stringview-data-usage) - raw, exc.what()); + LOG_ERROR("[get_env] Exception thrown converting getenv(\"{}\") = {} to " + "integer: {}", + env_id, raw, exc.what()); } return fallback; } @@ -246,11 +199,9 @@ struct environment auto value = get_env(env_id, value_default); if(choices.find(value) == choices.end()) { - fprintf( - stderr, - "[rocprof-sys][get_env] Environment variable \"%s\" has invalid value " - "\"%s\". Reverting to default.\n", - env_id.data(), get_env(env_id).c_str()); + LOG_WARNING("[get_env] Environment variable \"{}\" has invalid value \"{}\". " + "Reverting to default.", + env_id, get_env(env_id)); return value_default; } return value; @@ -265,11 +216,10 @@ struct ROCPROFSYS_INTERNAL_API env_config std::string m_env_value = {}; int m_override = 0; - auto operator()(bool _verbose = false) const + auto operator()() const { if(m_env_name.empty()) return -1; - ROCPROFSYS_ENVIRON_LOG(_verbose, "setenv(\"%s\", \"%s\", %i)\n", - m_env_name.c_str(), m_env_value.c_str(), m_override); + LOG_DEBUG("setenv(\"{}\", \"{}\", {})", m_env_name, m_env_value, m_override); return EnvType::setenv(m_env_name.c_str(), m_env_value.c_str(), m_override); } }; @@ -328,7 +278,7 @@ remove_env(std::vector& env_list, std::string_view env_variable, } inline std::string -discover_llvm_libdir_for_ompt(bool verbose = false) +discover_llvm_libdir_for_ompt() { auto strip = [](std::string value_to_strip) { if(!value_to_strip.empty() && value_to_strip.back() == '/') @@ -374,12 +324,11 @@ discover_llvm_libdir_for_ompt(bool verbose = false) auto result = std::find_if(candidates.begin(), candidates.end(), has_libomptarget); if(result != candidates.end()) { - ROCPROFSYS_ENVIRON_LOG(verbose, "Using LLVM libdir: %s\n", result->c_str()); + LOG_DEBUG("Using LLVM libdir: {}", *result); return *result; } - ROCPROFSYS_ENVIRON_LOG(verbose, - "libomptarget.so not found in candidate LLVM libdirs\n"); + LOG_DEBUG("libomptarget.so not found in candidate LLVM libdirs"); return {}; } @@ -409,7 +358,7 @@ is_python_interpreter(std::string_view executable) } inline std::string -discover_torch_libpath(const std::string& python_binary, bool verbose = false) +discover_torch_libpath(const std::string& python_binary) { if(python_binary.empty()) return {}; @@ -434,9 +383,8 @@ discover_torch_libpath(const std::string& python_binary, bool verbose = false) if(!is_safe_executable_path(python_binary)) { - ROCPROFSYS_ENVIRON_LOG( - verbose, "Unsafe characters detected in Python interpreter path: %s\n", - python_binary.c_str()); + LOG_WARNING("Unsafe characters detected in Python interpreter path: {}", + python_binary); return {}; } @@ -446,7 +394,7 @@ discover_torch_libpath(const std::string& python_binary, bool verbose = false) FILE* pipe = popen(cmd.c_str(), "r"); if(!pipe) { - ROCPROFSYS_ENVIRON_LOG(verbose, "Failed to execute command: %s\n", cmd.c_str()); + LOG_WARNING("Failed to execute command: {}", cmd); return {}; } @@ -463,8 +411,7 @@ discover_torch_libpath(const std::string& python_binary, bool verbose = false) if(status != 0 || result.empty()) { - ROCPROFSYS_ENVIRON_LOG(verbose, "torch not found for Python interpreter: %s\n", - python_binary.c_str()); + LOG_DEBUG("torch not found for Python interpreter: {}", python_binary); return {}; } @@ -480,13 +427,11 @@ discover_torch_libpath(const std::string& python_binary, bool verbose = false) if(!::tim::filepath::direxists(torch_libdir)) { - ROCPROFSYS_ENVIRON_LOG(verbose, "torch lib directory does not exist: %s\n", - torch_libdir.c_str()); + LOG_WARNING("torch lib directory does not exist: {}", torch_libdir); return {}; } - ROCPROFSYS_ENVIRON_LOG(verbose, "Discovered torch library path: %s\n", - torch_libdir.c_str()); + LOG_DEBUG("Discovered torch library path: {}", torch_libdir); return torch_libdir; } @@ -567,12 +512,12 @@ update_env(std::vector& _environ, std::string_view _env_var, Tp&& _ template inline void add_torch_library_path(std::vector& envp, std::string_view executable, - bool verbose, UpdatedEnvsT& updated_envs) + UpdatedEnvsT& updated_envs) { if(executable.empty()) return; if(!is_python_interpreter(executable)) return; - auto torch_libpath = discover_torch_libpath(std::string{ executable }, verbose); + auto torch_libpath = discover_torch_libpath(std::string{ executable }); if(torch_libpath.empty()) return; std::unordered_set seen{ torch_libpath }; diff --git a/projects/rocprofiler-systems/source/lib/common/setup.hpp b/projects/rocprofiler-systems/source/lib/common/setup.hpp index a98f5da950b8..d2c53c7a3b5f 100644 --- a/projects/rocprofiler-systems/source/lib/common/setup.hpp +++ b/projects/rocprofiler-systems/source/lib/common/setup.hpp @@ -101,7 +101,7 @@ setup_environ(int _verbose, const std::string& _search_paths = {}, auto _data = get_environ(_verbose, _search_paths, std::move(_omnilib), std::move(_omnilib_dl)); for(const auto& itr : _data) - itr(_verbose >= 3); + itr(); } } // namespace common } // namespace rocprofsys diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_discover_llvm_libdir.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_discover_llvm_libdir.cpp index bbafecb34efc..087cf850daec 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_discover_llvm_libdir.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_discover_llvm_libdir.cpp @@ -250,7 +250,7 @@ TEST_F(DiscoverLlvmLibdirTest, EmptyRocmVersionDir) EXPECT_EQ(result, llvm_lib); } -TEST_F(DiscoverLlvmLibdirTest, VerboseModeDoesNotCrash) +TEST_F(DiscoverLlvmLibdirTest, FoundLibDoesNotThrow) { std::string rocm_path = m_test_dir + "/rocm"; std::string llvm_lib = rocm_path + "/llvm/lib"; @@ -259,10 +259,10 @@ TEST_F(DiscoverLlvmLibdirTest, VerboseModeDoesNotCrash) set_rocm_path(rocm_path); clear_rocmv_dir(); - EXPECT_NO_THROW({ discover_llvm_libdir_for_ompt(true); }); + EXPECT_NO_THROW({ discover_llvm_libdir_for_ompt(); }); } -TEST_F(DiscoverLlvmLibdirTest, VerboseModeNoLibFound) +TEST_F(DiscoverLlvmLibdirTest, NoLibFoundDoesNotThrow) { std::string rocm_path = m_test_dir + "/rocm"; create_directory(rocm_path); @@ -270,7 +270,7 @@ TEST_F(DiscoverLlvmLibdirTest, VerboseModeNoLibFound) set_rocm_path(rocm_path); clear_rocmv_dir(); - EXPECT_NO_THROW({ discover_llvm_libdir_for_ompt(true); }); + EXPECT_NO_THROW({ discover_llvm_libdir_for_ompt(); }); } TEST_F(DiscoverLlvmLibdirTest, RocmVersionDirLlvmLibPreferredOverLib) diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index f56393740d41..69156ef86e31 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -226,7 +226,7 @@ class AddTorchLibraryPathTest : public ::testing::Test TEST_F(AddTorchLibraryPathTest, SkipsNonPythonExecutables) { std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; - add_torch_library_path(envp, "/usr/bin/bash", false, updated_envs); + add_torch_library_path(envp, "/usr/bin/bash", updated_envs); ASSERT_EQ(envp.size(), 1); EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); } @@ -234,7 +234,7 @@ TEST_F(AddTorchLibraryPathTest, SkipsNonPythonExecutables) TEST_F(AddTorchLibraryPathTest, HandlesEmptyExecutable) { std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; - add_torch_library_path(envp, std::string_view{}, false, updated_envs); + add_torch_library_path(envp, std::string_view{}, updated_envs); ASSERT_EQ(envp.size(), 1); EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); } @@ -244,7 +244,7 @@ TEST_F(AddTorchLibraryPathTest, PythonExecutableWithoutTorchLeavesEnvUnchanged) // "/nonexistent/python3" is recognised as a Python interpreter but // discover_torch_libpath returns "" → early return, env unchanged. std::vector envp = { "LD_LIBRARY_PATH=/usr/lib" }; - add_torch_library_path(envp, "/nonexistent/python3", false, updated_envs); + add_torch_library_path(envp, "/nonexistent/python3", updated_envs); ASSERT_EQ(envp.size(), 1); EXPECT_EQ(envp[0], "LD_LIBRARY_PATH=/usr/lib"); } @@ -259,17 +259,17 @@ TEST(DiscoverTorchLibpathTest, EmptyBinaryReturnsEmpty) TEST(DiscoverTorchLibpathTest, UnsafeCharInPathReturnsEmpty) { // ';' hits default: return false in is_safe_executable_path - EXPECT_EQ(discover_torch_libpath("/usr/bin/python;injected", true), ""); + EXPECT_EQ(discover_torch_libpath("/usr/bin/python;injected"), ""); } TEST(DiscoverTorchLibpathTest, SpaceInPathReturnsEmpty) { - EXPECT_EQ(discover_torch_libpath("/path with space/python3", true), ""); + EXPECT_EQ(discover_torch_libpath("/path with space/python3"), ""); } TEST(DiscoverTorchLibpathTest, DollarSignInPathReturnsEmpty) { - EXPECT_EQ(discover_torch_libpath("$HOME/python3", true), ""); + EXPECT_EQ(discover_torch_libpath("$HOME/python3"), ""); } TEST(DiscoverTorchLibpathTest, SafePathWithAllowedCharsAndNoTorchReturnsEmpty) @@ -277,7 +277,7 @@ TEST(DiscoverTorchLibpathTest, SafePathWithAllowedCharsAndNoTorchReturnsEmpty) // Path uses all allowed non-alnum chars (/ _ - + .) to exercise every // switch case in is_safe_executable_path. Shell exits non-zero (not found) // → popen succeeds, status != 0 → returns empty string. - EXPECT_EQ(discover_torch_libpath("/nonexistent_dir/python-3.11+safe.bin", true), ""); + EXPECT_EQ(discover_torch_libpath("/nonexistent_dir/python-3.11+safe.bin"), ""); } // ── posix_env + forwarding free functions (L77, L79, L269–L279) ────────────── @@ -582,14 +582,14 @@ TEST_F(FakeEnvConfigTest, OperatorRespectsOverrideZero) EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "original"); } -TEST_F(FakeEnvConfigTest, VerbosePathExecutes) +TEST_F(FakeEnvConfigTest, OperatorReturnsZeroOnSuccess) { env_config cfg; cfg.m_env_name = "FOO"; - cfg.m_env_value = "verbose_value"; + cfg.m_env_value = "some_value"; cfg.m_override = 1; - EXPECT_EQ(cfg(true), 0); - EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "verbose_value"); + EXPECT_EQ(cfg(), 0); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "some_value"); } TEST_F(FakeEnvConfigTest, EmptyNameIsNoop) diff --git a/projects/rocprofiler-systems/source/lib/core/argparse.cpp b/projects/rocprofiler-systems/source/lib/core/argparse.cpp index cb411dccce1d..83a76c11291a 100644 --- a/projects/rocprofiler-systems/source/lib/core/argparse.cpp +++ b/projects/rocprofiler-systems/source/lib/core/argparse.cpp @@ -139,11 +139,11 @@ add_ld_library_path(parser_data& _data) } parser_data& -add_torch_library_path(parser_data& _data, bool verbose) +add_torch_library_path(parser_data& _data) { if(_data.out.command.empty()) return _data; rocprofsys::common::add_torch_library_path( - _data.env.current, _data.out.command.front(), verbose, _data.env.updated); + _data.env.current, _data.out.command.front(), _data.env.updated); return _data; } diff --git a/projects/rocprofiler-systems/source/lib/core/argparse.hpp b/projects/rocprofiler-systems/source/lib/core/argparse.hpp index baa4d2b89782..4c2fa5ca4368 100644 --- a/projects/rocprofiler-systems/source/lib/core/argparse.hpp +++ b/projects/rocprofiler-systems/source/lib/core/argparse.hpp @@ -100,7 +100,7 @@ parser_data& add_ld_library_path(parser_data&); parser_data& -add_torch_library_path(parser_data&, bool verbose = false); +add_torch_library_path(parser_data&); parser_data& add_core_arguments(parser_t&, parser_data&); From e54928e23834333d61c751f903dc274a019620d2 Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Mon, 1 Jun 2026 16:29:20 +0200 Subject: [PATCH 8/9] Applied suggestions from code review 2 --- .../source/lib/common/environment.hpp | 36 +++++++------ .../lib/common/tests/test_environment.cpp | 50 +++++++++++++++++++ 2 files changed, 72 insertions(+), 14 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index bc654d0e6ec2..c3d594342518 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -119,22 +120,29 @@ struct environment { const char* raw = fetch_raw_env(env_id); if(!raw) return fallback; - try - { - if constexpr(std::is_same_v) - { - return std::stoi(raw); - } - else - { - return static_cast(std::stoll(raw)); - } - } catch(const std::exception& exc) + + // Trim surrounding whitespace so values such as " 42 " still parse. + constexpr std::string_view whitespace = " \t\n\r\f\v"; + std::string_view token{ raw }; + const auto first = token.find_first_not_of(whitespace); + if(first == std::string_view::npos) { - LOG_ERROR("[get_env] Exception thrown converting getenv(\"{}\") = {} to " - "integer: {}", - env_id, raw, exc.what()); + LOG_ERROR("[get_env] Cannot convert empty getenv(\"{}\") to integer", env_id); + return fallback; } + token = token.substr(first, token.find_last_not_of(whitespace) - first + 1); + + // std::from_chars parses against the exact target type: it rejects a + // leading '-' for unsigned Tp and reports result_out_of_range, so + // negative or overflowing input falls back to the default instead of + // silently wrapping or truncating. Signed Tp still accepts '-'. + Tp value{}; + const auto* end = token.data() + token.size(); + const auto [ptr, ec] = std::from_chars(token.data(), end, value); + if(ec == std::errc{} && ptr == end) return value; + + LOG_ERROR("[get_env] Failed to convert getenv(\"{}\") = \"{}\" to integer", + env_id, raw); return fallback; } diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index 69156ef86e31..f1b65d8225fe 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: MIT #include "common/environment.hpp" +#include #include #include @@ -524,6 +525,55 @@ TEST_F(FakeEnvGetEnvTest, SizeTReturnsValueWhenSet) EXPECT_EQ(fake_environment::get_env("FOO", std::size_t{ 0 }), std::size_t{ 42 }); } +TEST_F(FakeEnvGetEnvTest, UnsignedNegativeInputReturnsDefault) +{ + // "-1" must not wrap to UINT64_MAX for an unsigned target; fall back instead. + fake_env::setenv("FOO", "-1", 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::uint64_t{ 7 }), std::uint64_t{ 7 }); +} + +TEST_F(FakeEnvGetEnvTest, UnsignedOutOfRangeInputReturnsDefault) +{ + // 70000 does not fit in std::uint16_t -> fall back to the default. + fake_env::setenv("FOO", "70000", 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::uint16_t{ 5 }), std::uint16_t{ 5 }); +} + +TEST_F(FakeEnvGetEnvTest, UnsignedLargeValuePastInt64MaxParses) +{ + // Value > INT64_MAX previously threw out_of_range via stoll; it must now + // parse correctly for an unsigned 64-bit target. + constexpr auto big = std::uint64_t{ 18446744073709551610ULL }; // UINT64_MAX - 5 + fake_env::setenv("FOO", "18446744073709551610", 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::uint64_t{ 0 }), big); +} + +TEST_F(FakeEnvGetEnvTest, SignedNegativeInputParses) +{ + // Signed targets must still accept negative values (e.g. ROCPROFSYS_VERBOSE). + fake_env::setenv("FOO", "-3", 1); + EXPECT_EQ(fake_environment::get_env("FOO", 0), -3); +} + +TEST_F(FakeEnvGetEnvTest, SignedOutOfRangeInputReturnsDefault) +{ + // 70000 does not fit in std::int16_t -> fall back to the default. + fake_env::setenv("FOO", "70000", 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::int16_t{ 9 }), std::int16_t{ 9 }); +} + +TEST_F(FakeEnvGetEnvTest, IntegralTrailingGarbageReturnsDefault) +{ + fake_env::setenv("FOO", "42abc", 1); + EXPECT_EQ(fake_environment::get_env("FOO", 1), 1); +} + +TEST_F(FakeEnvGetEnvTest, IntegralSurroundingWhitespaceParses) +{ + fake_env::setenv("FOO", " 42 ", 1); + EXPECT_EQ(fake_environment::get_env("FOO", 0), 42); +} + // ── to_env_string ───────────────────────────────────────────────────────────── TEST(ToEnvStringTest, BoolTrue) { EXPECT_EQ(to_env_string(true), "true"); } From f9e4282c8ca6f5c329215006855c0bea05a43afa Mon Sep 17 00:00:00 2001 From: Milan Radosavljevic Date: Tue, 2 Jun 2026 10:32:49 +0200 Subject: [PATCH 9/9] Add suggestions from code review --- .../bin/rocprof-sys-instrument/CMakeLists.txt | 2 +- .../source/lib/common/environment.hpp | 299 +++++++++++++++--- .../lib/common/tests/test_environment.cpp | 43 +++ 3 files changed, 304 insertions(+), 40 deletions(-) diff --git a/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt b/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt index 247eaf164344..b15e925f8d79 100644 --- a/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt +++ b/projects/rocprofiler-systems/source/bin/rocprof-sys-instrument/CMakeLists.txt @@ -37,7 +37,7 @@ target_link_libraries( rocprofiler-systems::rocprofiler-systems-compile-options rocprofiler-systems::rocprofiler-systems-compile-definitions rocprofiler-systems::rocprofiler-systems-sanitizer - rocprofiler-systems::rocprofiler-systems-spdlog + rocprofiler-systems::rocprofiler-systems-common-library timemory::timemory-headers timemory::timemory-extensions timemory::timemory-core diff --git a/projects/rocprofiler-systems/source/lib/common/environment.hpp b/projects/rocprofiler-systems/source/lib/common/environment.hpp index c3d594342518..e75220833add 100644 --- a/projects/rocprofiler-systems/source/lib/common/environment.hpp +++ b/projects/rocprofiler-systems/source/lib/common/environment.hpp @@ -15,12 +15,11 @@ #include #include #include -#include #include #include #include #include -#include +#include // POSIX ::setenv / ::getenv #include #include #include @@ -35,40 +34,53 @@ namespace rocprofsys inline namespace common { -// POSIX environment backend — calls the real ::getenv / ::setenv. +/// @brief Production environment backend that forwards to the real POSIX +/// ::getenv / ::setenv. Used as the default @p EnvType of @ref environment; +/// unit tests substitute a fake backend to avoid touching the real process +/// environment. struct posix_env { + /// @brief Forwards to ::setenv. + /// @param name Null-terminated variable name. + /// @param value Null-terminated value to store. + /// @param overwrite Non-zero to replace an existing value; zero to keep it. + /// @return 0 on success, -1 on error. static int setenv(const char* name, const char* value, int overwrite) { return ::setenv(name, value, overwrite); } + /// @brief Forwards to ::getenv. + /// @param name Null-terminated variable name. + /// @return Pointer to the value, or nullptr when the variable is unset. static char* getenv(const char* name) { return ::getenv(name); } }; -// environment — all env-read/write logic parameterised over the backend. -// Use environment (the default) for production code. -// Inject a fake backend in unit tests to avoid touching the real process environment. +/// @brief Environment variable read/write facade, parameterised over the backend. +/// +/// All conversion and parsing logic lives here. Use @c environment (the +/// default) in production; inject a fake backend in unit tests. The free functions +/// @ref get_env / @ref set_env / @ref get_env_choice delegate to @c environment<>. +/// +/// @tparam EnvType Backend providing static @c getenv / @c setenv. template struct environment { private: - // env_id is always constructed from a null-terminated string at every call site - static const char* fetch_raw_env(std::string_view env_id) + static const char* fetch_raw_env(const char* env_id) { - if(env_id.empty()) return nullptr; - return EnvType::getenv( - env_id.data()); // NOLINT(bugprone-suspicious-stringview-data-usage) + if(env_id == nullptr || env_id[0] == '\0') return nullptr; + return EnvType::getenv(env_id); } template - static std::string get_env_string(std::string_view env_id, const Tp& fallback) + static std::string get_env_string(const char* env_id, const Tp& fallback) { const char* raw = fetch_raw_env(env_id); return raw ? std::string{ raw } : std::string{ fallback }; } - static bool get_env_bool(std::string_view env_id, bool fallback) + static bool get_env_bool(const char* env_id, bool fallback) { const char* raw = fetch_raw_env(env_id); if(!raw) return fallback; @@ -82,7 +94,15 @@ struct environment if(env_sv.find_first_not_of("0123456789") == std::string_view::npos) { - return static_cast(std::stoi(raw)); + // Parse with from_chars so a very large all-digit value cannot throw + // (std::stoi would throw std::out_of_range). Any non-zero digit string, + // including one that overflows, is truthy. + std::uint64_t numeric{}; + const auto* last = env_sv.data() + env_sv.size(); + const auto [ptr, ec] = std::from_chars(env_sv.data(), last, numeric); + if(ec == std::errc::result_out_of_range) return true; + if(ec == std::errc{} && ptr == last) return numeric != 0; + return true; } std::string lower{ env_sv }; @@ -99,24 +119,48 @@ struct environment } template - static Tp get_env_float(std::string_view env_id, Tp fallback) + static Tp get_env_float(const char* env_id, Tp fallback) { const char* raw = fetch_raw_env(env_id); if(!raw) return fallback; - try - { - return static_cast(std::stod(raw)); - } catch(const std::exception& exc) + + // Trim surrounding whitespace so values such as " 1.5 " still parse. + constexpr std::string_view whitespace = " \t\n\r\f\v"; + std::string_view token{ raw }; + const auto first = token.find_first_not_of(whitespace); + if(first == std::string_view::npos) { - LOG_ERROR("[get_env] Exception thrown converting getenv(\"{}\") = {} to " - "float: {}", - env_id, raw, exc.what()); + LOG_ERROR("[get_env] Cannot convert empty getenv(\"{}\") to float", env_id); + return fallback; } + token = token.substr(first, token.find_last_not_of(whitespace) - first + 1); + +#if defined(__cpp_lib_to_chars) && __cpp_lib_to_chars >= 201611L + // Locale-independent, non-throwing parse mirroring the integral path: a + // trailing-garbage or unparsable value falls back to the default. + Tp value{}; + const auto* end = token.data() + token.size(); + const auto [ptr, ec] = std::from_chars(token.data(), end, value); + if(ec == std::errc{} && ptr == end) return value; +#else + // Fallback for standard libraries without floating-point from_chars + // (libstdc++ < 11). std::stod is locale-sensitive (assumes C/POSIX locale). + try + { + std::size_t pos = 0; + const std::string str{ token }; + const double parsed = std::stod(str, &pos); + if(pos == str.size()) return static_cast(parsed); + } catch(const std::exception&) + {} +#endif + LOG_ERROR("[get_env] Failed to convert getenv(\"{}\") = \"{}\" to float", env_id, + raw); return fallback; } template - static Tp get_env_integral(std::string_view env_id, Tp fallback) + static Tp get_env_integral(const char* env_id, Tp fallback) { const char* raw = fetch_raw_env(env_id); if(!raw) return fallback; @@ -147,7 +191,7 @@ struct environment } template - static auto get_env_impl(std::string_view env_id, const Tp& fallback) + static auto get_env_impl(const char* env_id, const Tp& fallback) { if constexpr(std::is_same_v, std::string> || std::is_same_v, std::string_view> || @@ -171,8 +215,24 @@ struct environment } public: + /// @brief Read environment variable @p env_id, converted to the type of + /// @p value_default. + /// + /// The conversion is selected from @p value_default's type: std::string / + /// const char* return the raw value, bool is parsed permissively + /// (1/true/yes/on are true; 0/false/no/n/f/off are false), floating-point via + /// std::stod, and integral types via std::from_chars so that negative or + /// out-of-range input for the target type yields @p value_default instead of + /// wrapping. Enums are read through their underlying integral type. + /// + /// Primary, allocation-free overload: @p env_id must be a null-terminated C + /// string (literal, const char*, or constexpr const char*). + /// @param env_id Null-terminated variable name. + /// @param value_default Returned when the variable is unset or unparsable. + /// @return The parsed value, or @p value_default. + /// @throws std::runtime_error when a bool is requested but the value is empty. template - static auto get_env(std::string_view env_id, Tp&& value_default) + static auto get_env(const char* env_id, Tp&& value_default) { if constexpr(std::is_enum_v) { @@ -186,37 +246,96 @@ struct environment } } + /// @brief std::string_view / std::string overload of @ref get_env: materialises + /// a null-terminated copy of @p env_id once, then dispatches to the + /// const char* overload. + template + static auto get_env(std::string_view env_id, Tp&& value_default) + { + const std::string name{ env_id }; + return get_env(name.c_str(), std::forward(value_default)); + } + + /// @brief Read environment variable @p env_id as @p Tp, using a + /// value-initialised @c Tp{} as the fallback. + /// @tparam Tp Target type (defaults to std::string). + /// @param env_id Null-terminated variable name. + /// @return The parsed value, or @c Tp{} when the variable is unset. + template + static auto get_env(const char* env_id) + { + return get_env(env_id, Tp{}); + } + + /// @brief std::string_view / std::string overload of the single-argument + /// @ref get_env. template static auto get_env(std::string_view env_id) { return get_env(env_id, Tp{}); } + /// @brief Set environment variable @p env_var to the stringified @p value. + /// + /// @p value is rendered via operator<< before being stored. Primary, + /// allocation-free overload: @p env_var must be null-terminated. + /// @param env_var Null-terminated variable name. + /// @param value Value to store (stringified). + /// @param override Non-zero to replace an existing value; zero to keep it. template - static void set_env(const std::string& env_var, const Tp& value, int override) + static void set_env(const char* env_var, const Tp& value, int override) { std::stringstream ss_val; ss_val << value; - EnvType::setenv(env_var.c_str(), ss_val.str().c_str(), override); + EnvType::setenv(env_var, ss_val.str().c_str(), override); } + /// @brief std::string_view / std::string overload of @ref set_env. template - static auto get_env_choice(std::string_view env_id, Tp value_default, - std::set choices) + static void set_env(std::string_view env_var, const Tp& value, int override) + { + const std::string name{ env_var }; + set_env(name.c_str(), value, override); + } + + /// @brief Read environment variable @p env_id constrained to a set of allowed + /// values. + /// + /// Reads @p env_id as @p Tp; if the result is not in @p choices, logs a warning + /// and returns @p value_default. Primary, allocation-free overload (@p env_id + /// must be null-terminated). + /// @param env_id Null-terminated variable name. + /// @param value_default Returned when unset or not among @p choices. + /// @param choices Set of accepted values. + /// @return The value when valid, otherwise @p value_default. + template + static auto get_env_choice(const char* env_id, Tp value_default, std::set choices) { auto value = get_env(env_id, value_default); if(choices.find(value) == choices.end()) { + const char* raw = fetch_raw_env(env_id); LOG_WARNING("[get_env] Environment variable \"{}\" has invalid value \"{}\". " "Reverting to default.", - env_id, get_env(env_id)); + env_id, raw ? raw : ""); return value_default; } return value; } + + /// @brief std::string_view / std::string overload of @ref get_env_choice. + template + static auto get_env_choice(std::string_view env_id, Tp value_default, + std::set choices) + { + const std::string name{ env_id }; + return get_env_choice(name.c_str(), std::move(value_default), std::move(choices)); + } }; -// Templated env_config — operator() calls EnvType::setenv so the backend is injectable. +/// @brief Deferred @c setenv command: stores a name/value/override triple and +/// applies it when invoked. Templated on the backend so tests can inject a +/// fake. template struct ROCPROFSYS_INTERNAL_API env_config { @@ -224,6 +343,8 @@ struct ROCPROFSYS_INTERNAL_API env_config std::string m_env_value = {}; int m_override = 0; + /// @brief Apply the stored setenv command. + /// @return The backend setenv result, or -1 when @c m_env_name is empty. auto operator()() const { if(m_env_name.empty()) return -1; @@ -233,9 +354,22 @@ struct ROCPROFSYS_INTERNAL_API env_config }; // ── Forwarding free functions ──────────────────────────────────────────────── -// These preserve the existing rocprofsys::common::get_env / set_env / get_env_choice -// API so no call sites need to change. They delegate to environment. +// These are the primary public API; they delegate to environment. + +/// @brief Read environment variable @p env_id as the type of @p value_default. +/// Allocation-free overload for null-terminated names. See +/// @ref environment::get_env for conversion rules. +/// @param env_id Null-terminated variable name. +/// @param value_default Returned when the variable is unset or unparsable. +/// @return The parsed value, or @p value_default. +template +inline auto +get_env(const char* env_id, Tp&& value_default) +{ + return environment<>::get_env(env_id, std::forward(value_default)); +} +/// @brief std::string_view / std::string overload of @ref get_env. template inline auto get_env(std::string_view env_id, Tp&& value_default) @@ -243,6 +377,19 @@ get_env(std::string_view env_id, Tp&& value_default) return environment<>::get_env(env_id, std::forward(value_default)); } +/// @brief Read environment variable @p env_id as @p Tp, falling back to @c Tp{}. +/// @tparam Tp Target type (defaults to std::string). +/// @param env_id Null-terminated variable name. +/// @return The parsed value, or @c Tp{} when unset. +template +inline auto +get_env(const char* env_id) +{ + return environment<>::get_env(env_id); +} + +/// @brief std::string_view / std::string overload of the single-argument +/// @ref get_env. template inline auto get_env(std::string_view env_id) @@ -250,22 +397,57 @@ get_env(std::string_view env_id) return environment<>::get_env(env_id); } +/// @brief Set environment variable @p env_var to the stringified @p value. +/// Allocation-free overload for null-terminated names. +/// @param env_var Null-terminated variable name. +/// @param value Value to store (stringified). +/// @param override Non-zero to replace an existing value; zero to keep it. +template +inline void +set_env(const char* env_var, const Tp& value, int override) +{ + environment<>::set_env(env_var, value, override); +} + +/// @brief std::string_view / std::string overload of @ref set_env. template inline void -set_env(const std::string& env_var, const Tp& value, int override) +set_env(std::string_view env_var, const Tp& value, int override) { environment<>::set_env(env_var, value, override); } +/// @brief Read environment variable @p env_id constrained to @p value_choices, +/// returning @p value_default when unset or not allowed. Allocation-free +/// overload for null-terminated names. +/// @param env_id Null-terminated variable name. +/// @param value_default Returned when unset or not among @p value_choices. +/// @param value_choices Set of accepted values. +/// @return The value when valid, otherwise @p value_default. +template +inline auto +get_env_choice(const char* env_id, Tp value_default, std::set value_choices) +{ + return environment<>::get_env_choice(env_id, std::move(value_default), + std::move(value_choices)); +} + +/// @brief std::string_view / std::string overload of @ref get_env_choice. template inline auto get_env_choice(std::string_view env_id, Tp value_default, std::set value_choices) { - return environment<>::get_env_choice(env_id, value_default, std::move(value_choices)); + return environment<>::get_env_choice(env_id, std::move(value_default), + std::move(value_choices)); } // ── Env-vector helpers (operate on std::vector, not the real env) ── +/// @brief Remove all "KEY=VALUE" entries for @p env_variable from @p env_list, +/// then restore any entries for that key found in @p original_envs. +/// @param env_list Environment vector to modify in place. +/// @param env_variable Variable name (without '=') to remove. +/// @param original_envs Baseline entries used to restore a pre-existing value. inline void remove_env(std::vector& env_list, std::string_view env_variable, const std::unordered_set& original_envs) @@ -285,6 +467,11 @@ remove_env(std::vector& env_list, std::string_view env_variable, } } +/// @brief Locate the ROCm LLVM library directory that contains libomptarget.so. +/// +/// Probes candidates derived from @c ROCM_PATH and @c ROCmVersion_DIR plus the +/// standard /opt/rocm locations, returning the first that contains the library. +/// @return The matching libdir, or an empty string when none is found. inline std::string discover_llvm_libdir_for_ompt() { @@ -340,6 +527,9 @@ discover_llvm_libdir_for_ompt() return {}; } +/// @brief Test whether @p executable names a Python interpreter. +/// @param executable Path or basename to inspect. +/// @return true for "python", "python3", or "python3."; false otherwise. inline bool is_python_interpreter(std::string_view executable) { @@ -365,6 +555,13 @@ is_python_interpreter(std::string_view executable) [](unsigned char c) { return std::isdigit(c); }); } +/// @brief Discover the PyTorch library directory for a given Python interpreter. +/// +/// Runs @p python_binary to query torch's install path (the path is validated for +/// safe characters first to avoid shell injection) and appends "/lib". +/// @param python_binary Path to the Python interpreter. +/// @return The torch lib directory, or an empty string when torch is unavailable, +/// the path is unsafe, or the directory does not exist. inline std::string discover_torch_libpath(const std::string& python_binary) { @@ -443,14 +640,19 @@ discover_torch_libpath(const std::string& python_binary) return torch_libdir; } +/// @brief How @ref update_env combines a new value with an existing entry. enum class update_mode : std::uint8_t { - REPLACE = 0, - PREPEND, - APPEND, - WEAK, + REPLACE = 0, ///< Overwrite the value and drop duplicate entries. + PREPEND, ///< Insert the new value before the existing one. + APPEND, ///< Insert the new value after the existing one. + WEAK, ///< Update only when the current entry matches the original env. }; +/// @brief Render @p val as an environment-variable string. +/// bool becomes "true"/"false"; arithmetic types use std::to_string; +/// strings pass through. +/// @return The string representation of @p val. template inline std::string to_env_string(Tp&& val) @@ -468,6 +670,17 @@ to_env_string(Tp&& val) return std::to_string(val); } +/// @brief Insert or update an "KEY=VALUE" entry in an environment vector. +/// +/// Adds a new entry when @p _env_var is absent; otherwise combines values per +/// @p _mode (see @ref update_mode). Records @p _env_var in @p _updated_envs. +/// @param _environ Environment vector to modify in place. +/// @param _env_var Variable name (without '='). +/// @param _env_val New value (stringified via @ref to_env_string). +/// @param _mode Combination strategy. +/// @param _join_delim Delimiter used when prepending/appending. +/// @param _updated_envs Set receiving the names touched by this call. +/// @param _original_envs Baseline entries consulted by @ref update_mode::WEAK. template inline void update_env(std::vector& _environ, std::string_view _env_var, Tp&& _env_val, @@ -517,6 +730,14 @@ update_env(std::vector& _environ, std::string_view _env_var, Tp&& _ } } +/// @brief Prepend the PyTorch library directory to @c LD_LIBRARY_PATH in @p envp. +/// +/// No-op unless @p executable is a Python interpreter with torch installed. Merges +/// the discovered torch lib dir with existing @c LD_LIBRARY_PATH entries +/// (deduplicated) and records the touched name in @p updated_envs. +/// @param envp Environment vector to modify in place. +/// @param executable Interpreter path used to locate torch. +/// @param updated_envs Set receiving the names touched by this call. template inline void add_torch_library_path(std::vector& envp, std::string_view executable, diff --git a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp index f1b65d8225fe..b81bbaaf2471 100644 --- a/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp +++ b/projects/rocprofiler-systems/source/lib/common/tests/test_environment.cpp @@ -334,6 +334,21 @@ TEST_F(FakeEnvGetEnvTest, StringReturnsValueWhenSet) EXPECT_EQ(fake_environment::get_env("FOO", std::string{ "default" }), "bar"); } +TEST_F(FakeEnvGetEnvTest, StringViewEnvIdResolvesViaShim) +{ + // A non-null-terminated string_view name must still resolve correctly. + fake_env::setenv("FOO", "bar", 1); + const std::string_view name = std::string_view{ "FOOBAR" }.substr(0, 3); + EXPECT_EQ(fake_environment::get_env(name, std::string{ "default" }), "bar"); +} + +TEST_F(FakeEnvGetEnvTest, StdStringEnvIdResolvesViaShim) +{ + fake_env::setenv("FOO", "7", 1); + const std::string name{ "FOO" }; + EXPECT_EQ(fake_environment::get_env(name, 42), 7); +} + TEST_F(FakeEnvGetEnvTest, IntReturnsDefaultWhenUnset) { EXPECT_EQ(fake_environment::get_env("FOO", 42), 42); @@ -421,6 +436,14 @@ TEST_F(FakeEnvSetEnvTest, OverrideOneOverwrites) EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "new"); } +TEST_F(FakeEnvSetEnvTest, StringViewEnvVarResolvesViaShim) +{ + // A non-null-terminated string_view name must be materialised before setenv. + const std::string_view name = std::string_view{ "FOOBAR" }.substr(0, 3); + fake_environment::set_env(name, std::string{ "baz" }, 1); + EXPECT_EQ(fake_environment::get_env("FOO", std::string{}), "baz"); +} + class FakeEnvGetEnvChoiceTest : public ::testing::Test { protected: @@ -471,6 +494,14 @@ TEST_F(FakeEnvGetEnvTest, BoolNumericGreaterThanOneIsTrue) EXPECT_TRUE(fake_environment::get_env("FOO", false)); } +TEST_F(FakeEnvGetEnvTest, BoolNumericOverflowIsTrueAndDoesNotThrow) +{ + // An all-digit value past UINT64_MAX must not throw (std::stoi would) and + // is still truthy. + fake_env::setenv("FOO", "99999999999999999999999", 1); + EXPECT_NO_THROW({ EXPECT_TRUE(fake_environment::get_env("FOO", false)); }); +} + TEST_F(FakeEnvGetEnvTest, BoolUpperCaseFalseVariants) { for(const char* v : { "FALSE", "NO", "OFF", "F", "N" }) @@ -500,6 +531,18 @@ TEST_F(FakeEnvGetEnvTest, FloatTypeReturnsValue) EXPECT_NEAR(fake_environment::get_env("FOO", 0.0f), 1.5f, 1e-6f); } +TEST_F(FakeEnvGetEnvTest, FloatSurroundingWhitespaceParses) +{ + fake_env::setenv("FOO", " 2.5 ", 1); + EXPECT_NEAR(fake_environment::get_env("FOO", 0.0), 2.5, 1e-9); +} + +TEST_F(FakeEnvGetEnvTest, FloatTrailingGarbageReturnsDefault) +{ + fake_env::setenv("FOO", "1.5abc", 1); + EXPECT_NEAR(fake_environment::get_env("FOO", 9.9), 9.9, 1e-9); +} + // ── Integral branch gaps ────────────────────────────────────────────────────── TEST_F(FakeEnvGetEnvTest, IntEmptyEnvIdReturnsDefault)