From a260b7ec0885bc9129da64b19b26a71a179c717e Mon Sep 17 00:00:00 2001 From: Alexey Sachkov Date: Tue, 3 Jun 2025 15:44:32 +0200 Subject: [PATCH 1/4] [XPTI] Fix `-Wconversion` warnings `-Wconversion` is a must-have flag for us in according with internal guidelines (see `AddSecurityFlags.cmake`), but we have never built with it + `-Werror` so there are multiple places in `xpti` subproject where we did potentially incorrect implicit conversions. --- xpti/include/xpti/xpti_data_types.h | 38 ++++++++++++++-------- xpti/include/xpti/xpti_trace_framework.h | 2 +- xpti/include/xpti/xpti_trace_framework.hpp | 5 +-- xpti/src/xpti_proxy.cpp | 24 +++++++------- 4 files changed, 40 insertions(+), 29 deletions(-) diff --git a/xpti/include/xpti/xpti_data_types.h b/xpti/include/xpti/xpti_data_types.h index 253171ef31ded..bfc6b1a3e5ba8 100644 --- a/xpti/include/xpti/xpti_data_types.h +++ b/xpti/include/xpti/xpti_data_types.h @@ -105,7 +105,7 @@ inline xpti::uid128_t make_uid128(uint64_t FileID, uint64_t FuncID, int Line, int Col) { xpti::uid128_t UID; UID.p1 = (FileID << 32) | FuncID; - UID.p2 = ((uint64_t)Col << 32) | Line; + UID.p2 = ((uint64_t)Col << 32) | (uint64_t)Line; UID.instance = 0; return UID; } @@ -147,7 +147,15 @@ struct hash_t { /// /// @param value A 64-bit integer for which the bit count is to be calculated. /// @return The number of bits required to represent the input value. - int bit_count(uint64_t value) { return (int)log2(value) + 1; } + unsigned bit_count(uint64_t value) { + // FIXME: using std::log2 is imprecise. There is no guarantee that the + // implementation has actual integer overload for log2 and it is allowed to + // do a static_cast(value) to compute the result. Not every integer + // is represetntable as floating-point, meaning that byte representation of + // value as double may not be the same as for integer, resulting in + // different result. + return static_cast(std::log2(value)) + 1; + } /// @brief Compacts the file ID, function ID, line number, and column number /// into a single 64-bit hash value. @@ -164,7 +172,8 @@ struct hash_t { /// @param col An integer that represents the column number. /// @return A 64-bit hash value that represents the compacted file ID, /// function ID, line number, and column number. - uint64_t compact(uint64_t file_id, uint64_t func_id, int line, int col) { + uint64_t compact(uint64_t file_id, uint64_t func_id, uint64_t line, + uint64_t col) { uint64_t funcB, lineB, colB; // Figure out the bit counts necessary to represent the input values funcB = bit_count(func_id); @@ -177,9 +186,9 @@ struct hash_t { hash <<= funcB; hash = hash | func_id; hash <<= lineB; - hash = hash | (uint64_t)line; + hash = hash | line; hash <<= colB; - hash = hash | (uint64_t)col; + hash = hash | col; #ifdef DEBUG uint64_t fileB = bit_count(file_id); std::cout << "Total bits: " << (fileB + funcB + lineB + colB) << "\n"; @@ -203,7 +212,7 @@ struct hash_t { /// @param line An integer that represents the line number. /// @return A 64-bit hash value that represents the compacted file ID, /// function ID, and line number. - uint64_t compact_short(uint64_t file_id, uint64_t func_id, int line) { + uint64_t compact_short(uint64_t file_id, uint64_t func_id, uint64_t line) { uint64_t funcB, lineB; funcB = bit_count(func_id); lineB = bit_count(line); @@ -215,7 +224,7 @@ struct hash_t { hash <<= funcB; hash = hash | func_id; hash <<= lineB; - hash = hash | (uint64_t)line; + hash = hash | line; #ifdef DEBUG uint64_t fileB = bit_count(file_id); std::cout << "Total bits: " << (fileB + funcB + lineB) << "\n"; @@ -312,7 +321,8 @@ struct uid_t { } // namespace xpti namespace xpti { -constexpr int invalid_id = -1; +template +constexpr T invalid_id = std::numeric_limits::max(); constexpr uint64_t invalid_uid = 0; constexpr uint8_t default_vendor = 0; @@ -428,11 +438,11 @@ struct payload_t { /// Absolute path of the source file; may have to to be unicode string const char *source_file = nullptr; /// Line number information to correlate the trace point - uint32_t line_no = invalid_id; + uint32_t line_no = invalid_id<>; /// For a complex statement, column number may be needed to resolve the /// trace point; currently none of the compiler builtins return a valid /// column no - uint32_t column_no = invalid_id; + uint32_t column_no = invalid_id<>; /// Kernel/lambda/function address const void *code_ptr_va = nullptr; /// Internal bookkeeping slot - do not change. @@ -453,8 +463,8 @@ struct payload_t { code_ptr_va = codeptr; name = nullptr; ///< Invalid name string pointer source_file = nullptr; ///< Invalid source file string pointer - line_no = invalid_id; ///< Invalid line number - column_no = invalid_id; ///< Invalid column number + line_no = invalid_id<>; ///< Invalid line number + column_no = invalid_id<>; ///< Invalid column number if (codeptr) { flags = (uint64_t)payload_flag_t::CodePointerAvailable; } @@ -517,8 +527,8 @@ struct payload_t { /// Capture the rest of the parameters name = kname; source_file = sf; - line_no = line; - column_no = col; + line_no = static_cast(line); + column_no = static_cast(col); if (kname && kname[0] != '\0') { flags = (uint64_t)payload_flag_t::NameAvailable; } diff --git a/xpti/include/xpti/xpti_trace_framework.h b/xpti/include/xpti/xpti_trace_framework.h index f8250a26cfb82..6cc0a48622774 100644 --- a/xpti/include/xpti/xpti_trace_framework.h +++ b/xpti/include/xpti/xpti_trace_framework.h @@ -1018,7 +1018,7 @@ typedef uint16_t (*xpti_register_user_defined_et_t)(const char *, uint8_t); typedef xpti::trace_event_data_t *(*xpti_make_event_t)( const char *, xpti::payload_t *, uint16_t, xpti::trace_activity_type_t, uint64_t *); -typedef const xpti::trace_event_data_t *(*xpti_find_event_t)(int64_t); +typedef const xpti::trace_event_data_t *(*xpti_find_event_t)(uint64_t); typedef const xpti::payload_t *(*xpti_query_payload_t)( xpti::trace_event_data_t *); typedef const xpti::payload_t *(*xpti_query_payload_by_uid_t)(uint64_t uid); diff --git a/xpti/include/xpti/xpti_trace_framework.hpp b/xpti/include/xpti/xpti_trace_framework.hpp index 28c13cc76095c..0509e32efe462 100644 --- a/xpti/include/xpti/xpti_trace_framework.hpp +++ b/xpti/include/xpti/xpti_trace_framework.hpp @@ -961,9 +961,10 @@ class tracepoint_scope_t { MData = const_cast(xptiGetTracepointScopeData()); if (!MData) { if (funcName && fileName) - init(funcName, fileName, line, column); + init(funcName, fileName, static_cast(line), + static_cast(column)); else - init(callerFuncName, nullptr, 0, 0); + init(callerFuncName, nullptr, 0u, 0u); } else { MTraceEvent = MData->event_ref(); } diff --git a/xpti/src/xpti_proxy.cpp b/xpti/src/xpti_proxy.cpp index f40b167939618..5deb4719908fd 100644 --- a/xpti/src/xpti_proxy.cpp +++ b/xpti/src/xpti_proxy.cpp @@ -13,7 +13,7 @@ #include #include -enum functions_t { +enum functions_t : unsigned { XPTI_FRAMEWORK_INITIALIZE, XPTI_FRAMEWORK_FINALIZE, XPTI_INITIALIZE, @@ -69,7 +69,7 @@ enum functions_t { namespace xpti { class ProxyLoader { - std::unordered_map m_function_names = { + std::unordered_map m_function_names = { {XPTI_FRAMEWORK_INITIALIZE, "xptiFrameworkInitialize"}, {XPTI_FRAMEWORK_FINALIZE, "xptiFrameworkFinalize"}, {XPTI_INITIALIZE, "xptiInitialize"}, @@ -150,7 +150,7 @@ class ProxyLoader { inline bool noErrors() { return m_loaded; } - void *functionByIndex(int index) { + void *functionByIndex(unsigned index) { if (index >= XPTI_FRAMEWORK_INITIALIZE && index < XPTI_FW_API_COUNT) { return reinterpret_cast(m_dispatch_table[index]); } @@ -233,7 +233,7 @@ XPTI_EXPORT_API uint16_t xptiRegisterUserDefinedTracePoint( return (*(xpti_register_user_defined_tp_t)f)(tool_name, user_defined_tp); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API uint16_t xptiRegisterUserDefinedEventType( @@ -246,7 +246,7 @@ XPTI_EXPORT_API uint16_t xptiRegisterUserDefinedEventType( user_defined_event); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API xpti::result_t xptiInitialize(const char *stream, uint32_t maj, @@ -278,7 +278,7 @@ XPTI_EXPORT_API uint64_t xptiGetUniversalId() { return (*reinterpret_cast(f))(); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API void xptiSetUniversalId(uint64_t uid) { @@ -329,7 +329,7 @@ XPTI_EXPORT_API uint64_t xptiGetUniqueId() { return (*(xpti_get_unique_id_t)f)(); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API xpti::string_id_t xptiRegisterString(const char *string, @@ -341,7 +341,7 @@ XPTI_EXPORT_API xpti::string_id_t xptiRegisterString(const char *string, return (*(xpti_register_string_t)f)(string, table_string); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API const char *xptiLookupString(xpti::string_id_t id) { @@ -363,7 +363,7 @@ xptiRegisterObject(const char *data, size_t size, uint8_t type) { return (*(xpti_register_object_t)f)(data, size, type); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API xpti::object_data_t xptiLookupObject(xpti::object_id_t id) { @@ -395,7 +395,7 @@ XPTI_EXPORT_API uint8_t xptiRegisterStream(const char *stream_name) { return (*(xpti_register_stream_t)f)(stream_name); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API xpti::result_t xptiUnregisterStream(const char *stream_name) { @@ -689,7 +689,7 @@ XPTI_EXPORT_API uint8_t xptiGetDefaultStreamID() { return (*(xpti_get_default_stream_id_t)f)(); } } - return xpti::invalid_id; + return xpti::invalid_id; } XPTI_EXPORT_API xpti::result_t xptiSetDefaultStreamID(uint8_t stream_id) { @@ -747,4 +747,4 @@ xptiSetDefaultTraceType(xpti::trace_point_type_t trace_type) { } } return xpti::result_t::XPTI_RESULT_FAIL; -} \ No newline at end of file +} From cb535c4167ae368a4ab254859f1830bdf788d527 Mon Sep 17 00:00:00 2001 From: Alexey Sachkov Date: Tue, 3 Jun 2025 16:10:37 +0200 Subject: [PATCH 2/4] Fix clang-format & xptifw build --- xpti/include/xpti/xpti_data_types.h | 4 ++-- xptifw/include/xpti_int64_hash_table.hpp | 4 ++-- xptifw/include/xpti_string_table.hpp | 8 ++++---- xptifw/src/xpti_trace_framework.cpp | 10 +++++----- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/xpti/include/xpti/xpti_data_types.h b/xpti/include/xpti/xpti_data_types.h index bfc6b1a3e5ba8..2e2c378357c18 100644 --- a/xpti/include/xpti/xpti_data_types.h +++ b/xpti/include/xpti/xpti_data_types.h @@ -461,8 +461,8 @@ struct payload_t { // indicates a partial but valid payload. payload_t(const void *codeptr) { code_ptr_va = codeptr; - name = nullptr; ///< Invalid name string pointer - source_file = nullptr; ///< Invalid source file string pointer + name = nullptr; ///< Invalid name string pointer + source_file = nullptr; ///< Invalid source file string pointer line_no = invalid_id<>; ///< Invalid line number column_no = invalid_id<>; ///< Invalid column number if (codeptr) { diff --git a/xptifw/include/xpti_int64_hash_table.hpp b/xptifw/include/xpti_int64_hash_table.hpp index ce1238ad832aa..461c88a2a210a 100644 --- a/xptifw/include/xpti_int64_hash_table.hpp +++ b/xptifw/include/xpti_int64_hash_table.hpp @@ -63,7 +63,7 @@ class Hash64x64Table { #endif return KeyLoc->second; // We found it, so we return the Value } else - return xpti::invalid_id; + return xpti::invalid_id; } // Add a pair to the hash table. If the Key already exists, this @@ -121,7 +121,7 @@ class Hash64x64Table { #endif return ValLoc->second; } else - return xpti::invalid_id; + return xpti::invalid_id; } void printStatistics() { diff --git a/xptifw/include/xpti_string_table.hpp b/xptifw/include/xpti_string_table.hpp index d9fa811c050e5..77b92314d8fe1 100644 --- a/xptifw/include/xpti_string_table.hpp +++ b/xptifw/include/xpti_string_table.hpp @@ -61,7 +61,7 @@ class StringTable { // in the string table is returned through the default argument xpti::string_id_t add(const char *str, const char **ref_str = nullptr) { if (!str) - return xpti::invalid_id; + return xpti::invalid_id; std::string LocalStr = str; return add(LocalStr, ref_str); @@ -69,7 +69,7 @@ class StringTable { xpti::string_id_t add(std::string str, const char **ref_str = nullptr) { if (str.empty()) - return xpti::invalid_id; + return xpti::invalid_id; // Lock-free lookup to see if the string exists in the table; XPTI has // always had this as lock-free, but if instability occurs, we can use a @@ -126,7 +126,7 @@ class StringTable { if (ref_str) *ref_str = nullptr; - return xpti::invalid_id; + return xpti::invalid_id; } } @@ -142,7 +142,7 @@ class StringTable { } // The MMutex will be released here! } - return xpti::invalid_id; + return xpti::invalid_id; } // The reverse query allows one to get the string from the string_id_t that diff --git a/xptifw/src/xpti_trace_framework.cpp b/xptifw/src/xpti_trace_framework.cpp index f0759acdb759d..4945a40146c6c 100644 --- a/xptifw/src/xpti_trace_framework.cpp +++ b/xptifw/src/xpti_trace_framework.cpp @@ -204,9 +204,9 @@ struct TracePointImpl : xpti_payload_t, /// @brief Event data for the trace point. xpti::trace_event_data_t MEvent; /// @brief Cached Function string ID for the trace point. - int32_t MFuncID = xpti::invalid_id; + int32_t MFuncID = xpti::invalid_id<>; /// @brief Cached File string ID for the trace point. - int32_t MFileID = xpti::invalid_id; + int32_t MFileID = xpti::invalid_id<>; /// @brief Iterator for the metadata associated with the trace point. xpti::metadata_t::iterator MCurr; @@ -944,7 +944,7 @@ class Tracepoints { return xpti::result_t::XPTI_RESULT_INVALIDARG; string_id_t KeyID = MStringTableRef.add(Key); - if (KeyID == xpti::invalid_id) { + if (KeyID == xpti::invalid_id) { return xpti::result_t::XPTI_RESULT_INVALIDARG; } @@ -2115,7 +2115,7 @@ class Framework { string_id_t registerString(const char *String, char **TableString) { if (!TableString || !String) - return xpti::invalid_id; + return xpti::invalid_id; *TableString = 0; @@ -2134,7 +2134,7 @@ class Framework { object_id_t registerObject(const char *Object, size_t Size, uint8_t Type) { if (!Object) - return xpti::invalid_id; + return xpti::invalid_id; return MObjectTable.insert(std::string_view(Object, Size), Type); } From 6f04bfd786624c08d935526de7e857a33425c82d Mon Sep 17 00:00:00 2001 From: Alexey Sachkov Date: Tue, 3 Jun 2025 18:25:15 +0200 Subject: [PATCH 3/4] Fix SYCL build --- sycl/source/detail/graph_impl.cpp | 2 +- sycl/source/detail/queue_impl.cpp | 2 +- sycl/source/handler.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/graph_impl.cpp b/sycl/source/detail/graph_impl.cpp index abb2eaea4bd68..81744c8eb67e4 100644 --- a/sycl/source/detail/graph_impl.cpp +++ b/sycl/source/detail/graph_impl.cpp @@ -818,7 +818,7 @@ exec_graph_impl::enqueueNodeDirect(sycl::context Ctx, #ifdef XPTI_ENABLE_INSTRUMENTATION const bool xptiEnabled = xptiTraceEnabled(); - int32_t StreamID = xpti::invalid_id; + int32_t StreamID = xpti::invalid_id<>; xpti_td *CmdTraceEvent = nullptr; uint64_t InstanceID = 0; if (xptiEnabled) { diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 4cc0ba6fa8853..987fb7b182f60 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -591,7 +591,7 @@ void queue_impl::wait(const detail::code_location &CodeLoc) { void *TelemetryEvent = nullptr; uint64_t IId; std::string Name; - int32_t StreamID = xpti::invalid_id; + int32_t StreamID = xpti::invalid_id<>; if (xptiEnabled) { StreamID = xptiRegisterStream(SYCL_STREAM_NAME); TelemetryEvent = instrumentationProlog(CodeLoc, Name, StreamID, IId); diff --git a/sycl/source/handler.cpp b/sycl/source/handler.cpp index 59cbcb5384b8e..1c295fbd90c77 100644 --- a/sycl/source/handler.cpp +++ b/sycl/source/handler.cpp @@ -565,7 +565,7 @@ event handler::finalize() { #endif auto EnqueueKernel = [&]() { #ifdef XPTI_ENABLE_INSTRUMENTATION - int32_t StreamID = xpti::invalid_id; + int32_t StreamID = xpti::invalid_id<>; xpti_td *CmdTraceEvent = nullptr; uint64_t InstanceID = 0; if (xptiEnabled) { From 152f3de613ee29e3bd7a99cd6db65327b713e0b8 Mon Sep 17 00:00:00 2001 From: Alexey Sachkov Date: Wed, 4 Jun 2025 14:50:17 +0200 Subject: [PATCH 4/4] Fix error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] --- xpti/src/xpti_proxy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xpti/src/xpti_proxy.cpp b/xpti/src/xpti_proxy.cpp index 5deb4719908fd..378fe6e8aacb6 100644 --- a/xpti/src/xpti_proxy.cpp +++ b/xpti/src/xpti_proxy.cpp @@ -151,7 +151,7 @@ class ProxyLoader { inline bool noErrors() { return m_loaded; } void *functionByIndex(unsigned index) { - if (index >= XPTI_FRAMEWORK_INITIALIZE && index < XPTI_FW_API_COUNT) { + if (index < XPTI_FW_API_COUNT) { return reinterpret_cast(m_dispatch_table[index]); } return nullptr;