[SDK] Fix null handles (#474)

* Fix null handle - use .handle=0, not .handle=numeric_limits<>::max() * Update lib.common.hasher * Fix ROCPROFILER_CONTEXT_NONE * Use context operator== * Update CHANGELOG * Updated null handle for scratch memory and changed allocation test so that free ops account for null agent --------- Co-authored-by: Ian Trowbridge <Ian.Trowbridge@amd.com> [ROCm/rocprofiler-sdk commit: 4d6a61f5e5]
2025-07-18 12:05:52 -05:00
parent 483a10f53a
commit 990946e956
12 changed files with 75 additions and 50 deletions
@@ -204,6 +204,8 @@ Full documentation for ROCprofiler-SDK is available at [rocm.docs.amd.com/projec
 - rocprofv3 avail tool renamed from rocprofv3_avail to rocprofv3-avail tool
 - rocprofv3 avail tool has support for command line arguments.
 - rocprofv3 tool now allows for Thread Trace + PC Sampling on the same agent
+- fixed inconsistency for what is a "null" handle in `rocprofiler_*_id_t` structs.
+  - correct answer is `.handle = 0` but some definitions used `UINT64_MAX`

 ### Resolved issues

@@ -38,7 +38,7 @@ ROCPROFILER_EXTERN_C_INIT
 /**
 * The NULL Context handle.
 */
-#define ROCPROFILER_CONTEXT_NONE ROCPROFILER_HANDLE_LITERAL(rocprofiler_context_id_t, UINT64_MAX)
+#define ROCPROFILER_CONTEXT_NONE ROCPROFILER_HANDLE_LITERAL(rocprofiler_context_id_t, 0UL)

 /**
 * @brief Create context.
@@ -28,7 +28,9 @@

 #include <cstddef>
 #include <cstdint>
+#include <optional>
 #include <string>
+#include <string_view>
 #include <type_traits>

 namespace rocprofiler
@@ -53,17 +55,20 @@ struct fnv1a_hasher

    // Hashes a numeric value.
    template <typename Tp, typename std::enable_if_t<std::is_arithmetic<Tp>::value, bool> = true>
-    void update(Tp data);
+    fnv1a_hasher& update(Tp data);
+
+    template <typename Tp>
+    fnv1a_hasher& update(const std::optional<Tp>& data);

    // Using the loop instead of "update(str, strlen(str))" to avoid looping twice
-    void update(const char* str);
+    fnv1a_hasher& update(const char* str);

    // Hashes a byte array.
-    void update(const char* data, size_t size);
+    fnv1a_hasher& update(const char* data, size_t size);

-    void update(std::string_view s) { update(s.data(), s.size()); }
+    fnv1a_hasher& update(std::string_view s) { return update(s.data(), s.size()); }

-    void update(const std::string& s) { update(s.data(), s.size()); }
+    fnv1a_hasher& update(const std::string& s) { return update(s.data(), s.size()); }

    // Usage: uint64_t hashed_value = Hash::combine(33, false, "ABC", 458L, 3u, 'x');
    template <typename Tp, typename... TailT>
@@ -72,7 +77,7 @@ struct fnv1a_hasher
    // fnv1a_hasher.update_all(33, false, "ABC")` is shorthand for calling fnv1a_hasher.update(...)
    // for each value in the same order
    template <typename Tp, typename... TailT>
-    void update_all(Tp&& arg, TailT&&... args);
+    fnv1a_hasher& update_all(Tp&& arg, TailT&&... args);

    uint64_t digest() const { return m_result; }

@@ -84,13 +89,20 @@ private:
 };

 template <typename Tp, typename std::enable_if_t<std::is_arithmetic<Tp>::value, bool>>
-void
+fnv1a_hasher&
 fnv1a_hasher::update(Tp data)
 {
-    update(reinterpret_cast<const char*>(&data), sizeof(data));
+    return update(reinterpret_cast<const char*>(&data), sizeof(data));
 }

-inline void
+template <typename Tp>
+fnv1a_hasher&
+fnv1a_hasher::update(const std::optional<Tp>& data)
+{
+    return (data) ? update(*data) : *this;
+}
+
+inline fnv1a_hasher&
 fnv1a_hasher::update(const char* str)
 {
    constexpr auto max_n = 4096;
@@ -101,9 +113,10 @@ fnv1a_hasher::update(const char* str)
        update(*p);
        if(++n >= max_n) break;  // prevent infinite loop
    }
+    return *this;
 }

-inline void
+inline fnv1a_hasher&
 fnv1a_hasher::update(const char* data, size_t size)
 {
    for(size_t i = 0; i < size; ++i)
@@ -113,23 +126,23 @@ fnv1a_hasher::update(const char* data, size_t size)
        // signed integers.
        m_result *= kFnv1a64Prime;
    }
+    return *this;
 }

 template <typename Tp, typename... TailT>
 uint64_t
 fnv1a_hasher::combine(Tp&& arg, TailT&&... args)
 {
-    auto _hasher = fnv1a_hasher{};
-    _hasher.update_all(std::forward<Tp>(arg), std::forward<TailT>(args)...);
-    return _hasher.digest();
+    return fnv1a_hasher{}.update_all(std::forward<Tp>(arg), std::forward<TailT>(args)...).digest();
 }

 template <typename Tp, typename... TailT>
-void
+fnv1a_hasher&
 fnv1a_hasher::update_all(Tp&& arg, TailT&&... args)
 {
    update(arg);
    if constexpr(sizeof...(TailT) > 0) update_all(std::forward<TailT>(args)...);
+    return *this;
 }
 }  // namespace common
 }  // namespace rocprofiler
@@ -491,9 +491,8 @@ write_otf2(const output_config&                                          cfg,
        {
            // Free functions do not track agent information. Below handles case where
            // null rocprof agent id is passed to generate OTF2
-            constexpr auto null_rocp_agent_id =
-                rocprofiler_agent_id_t{.handle = std::numeric_limits<uint64_t>::max()};
-            const rocprofiler_agent_t* _agent = nullptr;
+            constexpr auto             null_rocp_agent_id = rocprofiler_agent_id_t{.handle = 0};
+            const rocprofiler_agent_t* _agent             = nullptr;
            if(agent != null_rocp_agent_id)
            {
                _agent = _get_agent(agent);
@@ -896,9 +895,8 @@ write_otf2(const output_config&                                          cfg,
            auto _hash = get_hash_id(evt.name);
            // Using max numeric limits results in an out-of-bound runtime error for OTF2
            // and perfetto for agent ids. Setting handle to 0 for free functions.
-            constexpr auto null_rocp_agent_id =
-                rocprofiler_agent_id_t{.handle = std::numeric_limits<uint64_t>::max()};
-            auto handle = agent.handle;
+            constexpr auto null_rocp_agent_id = rocprofiler_agent_id_t{.handle = 0};
+            auto           handle             = agent.handle;
            if(agent == null_rocp_agent_id) handle = 0;

            add_write_string(_hash, evt.name);
@@ -837,8 +837,7 @@ write_perfetto(
        }

        // memory allocation counter track
-        constexpr auto null_rocp_agent_id =
-            rocprofiler_agent_id_t{.handle = std::numeric_limits<uint64_t>::max()};
+        constexpr auto null_rocp_agent_id = rocprofiler_agent_id_t{.handle = 0};
        struct free_memory_information
        {
            rocprofiler_timestamp_t start_timestamp = 0;
@@ -855,9 +854,8 @@ write_perfetto(

        struct agent_and_size
        {
-            rocprofiler_agent_id_t agent_id =
-                rocprofiler_agent_id_t{.handle = std::numeric_limits<uint64_t>::max()};
-            uint64_t size = {0};
+            rocprofiler_agent_id_t agent_id = rocprofiler_agent_id_t{.handle = 0};
+            uint64_t               size     = {0};
        };

        auto mem_alloc_endpoints =
@@ -159,8 +159,7 @@ rocprofiler_configure_buffer_tracing_service(rocprofiler_context_id_t

    if(buffer_id.handle == 0) return ROCPROFILER_STATUS_ERROR_BUFFER_NOT_FOUND;

-    constexpr auto invalid_buffer_id =
-        rocprofiler_buffer_id_t{std::numeric_limits<uint64_t>::max()};
+    constexpr auto invalid_buffer_id = rocprofiler_buffer_id_t{0};

    if(!ctx->buffered_tracer)
    {
@@ -168,7 +167,7 @@ rocprofiler_configure_buffer_tracing_service(rocprofiler_context_id_t
        ctx->buffered_tracer->buffer_data.fill(invalid_buffer_id);
    }

-    if(ctx->buffered_tracer->buffer_data.at(kind).handle != invalid_buffer_id.handle)
+    if(ctx->buffered_tracer->buffer_data.at(kind) != invalid_buffer_id)
        return ROCPROFILER_STATUS_ERROR_SERVICE_ALREADY_CONFIGURED;

    RETURN_STATUS_ON_FAIL(rocprofiler::context::add_domain(ctx->buffered_tracer->domains, kind));
@@ -20,14 +20,15 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.

-#include <rocprofiler-sdk/context.h>
-#include <rocprofiler-sdk/fwd.h>
-
 #include "lib/rocprofiler-sdk/context/context.hpp"
 #include "lib/rocprofiler-sdk/context/domain.hpp"
 #include "lib/rocprofiler-sdk/hsa/hsa.hpp"
 #include "lib/rocprofiler-sdk/registration.hpp"

+#include <rocprofiler-sdk/context.h>
+#include <rocprofiler-sdk/fwd.h>
+#include <rocprofiler-sdk/cxx/operators.hpp>
+
 #include <atomic>
 #include <vector>

@@ -60,7 +61,7 @@ rocprofiler_create_context(rocprofiler_context_id_t* context_id)
 rocprofiler_status_t
 rocprofiler_start_context(rocprofiler_context_id_t context_id)
 {
-    if(context_id.handle == rocprofiler_context_none.handle ||
+    if(context_id == rocprofiler_context_none ||
       !rocprofiler::context::get_registered_context(context_id))
        return ROCPROFILER_STATUS_ERROR_CONTEXT_NOT_FOUND;

@@ -74,7 +75,7 @@ rocprofiler_start_context(rocprofiler_context_id_t context_id)
 rocprofiler_status_t
 rocprofiler_stop_context(rocprofiler_context_id_t context_id)
 {
-    if(context_id.handle == rocprofiler_context_none.handle ||
+    if(context_id == rocprofiler_context_none ||
       !rocprofiler::context::get_registered_context(context_id))
        return ROCPROFILER_STATUS_ERROR_CONTEXT_NOT_FOUND;

@@ -91,7 +92,7 @@ rocprofiler_context_is_active(rocprofiler_context_id_t context_id, int* status)
    *status = 0;

    // return context not found if not registered
-    if(context_id.handle == rocprofiler_context_none.handle ||
+    if(context_id == rocprofiler_context_none ||
       !rocprofiler::context::get_registered_context(context_id))
        return ROCPROFILER_STATUS_ERROR_CONTEXT_NOT_FOUND;

@@ -114,7 +115,7 @@ rocprofiler_context_is_valid(rocprofiler_context_id_t context_id, int* status)
 {
    *status = 0;

-    if(context_id.handle == rocprofiler_context_none.handle ||
+    if(context_id == rocprofiler_context_none ||
       !rocprofiler::context::get_registered_context(context_id))
        return ROCPROFILER_STATUS_ERROR_CONTEXT_NOT_FOUND;

@@ -142,8 +142,7 @@ context_filter(const context::context* ctx)
    return (has_buffered || has_callback);
 }

-constexpr auto null_rocp_agent_id =
-    rocprofiler_agent_id_t{.handle = std::numeric_limits<uint64_t>::max()};
+constexpr auto null_rocp_agent_id = rocprofiler_agent_id_t{.handle = 0};

 struct async_copy_data
 {
@@ -324,8 +324,7 @@ get_next_dispatch()
    return _v;
 }

-constexpr auto null_rocp_agent_id =
-    rocprofiler_agent_id_t{.handle = std::numeric_limits<uint64_t>::max()};
+constexpr auto null_rocp_agent_id = rocprofiler_agent_id_t{.handle = 0};

 struct memory_allocation_data
 {
@@ -83,9 +83,9 @@ destroy_queue(hsa_queue_t* hsa_queue)
 }

 constexpr rocprofiler_agent_t default_agent =
-    rocprofiler_agent_t{.size = sizeof(rocprofiler_agent_t),
-                        .id   = rocprofiler_agent_id_t{std::numeric_limits<uint64_t>::max()},
-                        .type = ROCPROFILER_AGENT_TYPE_NONE,
+    rocprofiler_agent_t{.size                       = sizeof(rocprofiler_agent_t),
+                        .id                         = rocprofiler_agent_id_t{.handle = 0},
+                        .type                       = ROCPROFILER_AGENT_TYPE_NONE,
                        .cpu_cores_count            = 0,
                        .simd_count                 = 0,
                        .mem_banks_count            = 0,
@@ -129,8 +129,8 @@ constexpr rocprofiler_agent_t default_agent =
                        .local_mem_size             = 0,
                        .hive_id                    = 0,
                        .gpu_id                     = 0,
-                        .workgroup_max_dim          = {0, 0, 0},
-                        .grid_max_dim               = {0, 0, 0},
+                        .workgroup_max_dim          = {.x = 0, .y = 0, .z = 0},
+                        .grid_max_dim               = {.x = 0, .y = 0, .z = 0},
                        .mem_banks                  = nullptr,
                        .caches                     = nullptr,
                        .io_links                   = nullptr,
@@ -462,7 +462,7 @@ impl(Args... args)

    [[maybe_unused]] const auto get_agent_id =
        [](const hsa_queue_t* hsa_queue) -> rocprofiler_agent_id_t {
-        rocprofiler_agent_id_t _agent_id{static_cast<uint64_t>(-1)};
+        rocprofiler_agent_id_t _agent_id{.handle = 0};
        bool                   found_agent{false};

        rocprofiler::hsa::get_queue_controller()->iterate_queues(
@@ -59,7 +59,18 @@ def test_memory_allocation(json_data):

    assert len(bf_op_names) == 5

-    allocation_reported_agent_ids = set()
+    # Op values:
+    UNKNOWN_OP = 0
+    HSA_MEMORY_ALLOCATE_OP = 1
+    HSA_AMD_VMEM_HANDLE_CREATE_OP = 2
+    HSA_MEMORY_FREE_OP = 3
+    HSA_AMD_VMEM_HANDLE_RELEASE = 4
+
+    valid_agent_ids = set()
+    for row in data["agents"]:
+        if "id" in row and "handle" in row.id:
+            valid_agent_ids.add(row.id.handle)
+
    # check buffering data
    for node in memory_allocation_data:
        assert "size" in node
@@ -78,7 +89,14 @@ def test_memory_allocation(json_data):
        assert node.allocation_size >= 0
        assert len(node.address) > 0
        assert node.thread_id > 0
-        assert node.agent_id.handle > 0
+
+        op_id = node.operation
+        assert op_id != UNKNOWN_OP
+        if op_id == HSA_MEMORY_ALLOCATE_OP or op_id == HSA_AMD_VMEM_HANDLE_CREATE_OP:
+            assert node.agent_id.handle in valid_agent_ids
+        else:
+            assert node.agent_id.handle == 0  # free ops record agent id as null
+
        assert node.start_timestamp > 0
        assert node.end_timestamp > 0
        assert node.start_timestamp < node.end_timestamp
@@ -89,8 +107,6 @@ def test_memory_allocation(json_data):
            in bf_op_names
        )

-        allocation_reported_agent_ids.add(node["agent_id"]["handle"])
-

 def test_otf2_data(otf2_data, json_data):
    import rocprofiler_sdk.tests.rocprofv3 as rocprofv3