Skip to content

[NFC][SYCL][XPTI] Fix minor coverity hits #19952

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 3, 2025
Merged

[NFC][SYCL][XPTI] Fix minor coverity hits #19952

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2385ae
Fix coverity hits
uditagarwal97 Sep 2, 2025
371c628
Mark copy and assignment operator as deleted
uditagarwal97 Sep 2, 2025
7627f88
Use try/catch instead
uditagarwal97 Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion sycl/source/detail/device_global_map.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ class DeviceGlobalMap {
DeviceGlobalMap(bool OwnerControlledCleanup)
: MOwnerControlledCleanup{OwnerControlledCleanup} {}

~DeviceGlobalMap() {
DeviceGlobalMap(const DeviceGlobalMap &) = default;
DeviceGlobalMap &operator=(const DeviceGlobalMap &) = default;
Copy link
Contributor Author

@uditagarwal97 uditagarwal97 Sep 2, 2025

Choose a reason for hiding this comment

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

Rule of three


~DeviceGlobalMap() noexcept(false) {
Copy link
Contributor Author

@uditagarwal97 uditagarwal97 Sep 2, 2025

Choose a reason for hiding this comment

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

Coverity is complaining about C++ destructor implicitly marked as noexcept while the destructor body can throw an exception, in DeviceGlobalIt.second->cleanup();.
I've marked the destructor as noexcept(false) to indicate that destructor can throw

Copy link
Contributor

@cperkinsintel cperkinsintel Sep 3, 2025

Choose a reason for hiding this comment

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

I'm not sure this is a good solution. I tried it once and it just lead to more Coverity hits.

If this class is destroyed in any OTHER classes destructor, then the problem recurs. Do you mark that destructor as noexcept(false) too?

I guess many of our impl classes now have try/catch blocks and they stream out errors. But should that occur it'll likely be difficult to trace the problem back to here.

Anyway, I haven't thought about this deeply, and perhaps in this situation you have. Just wanted to check.

Copy link
Contributor Author

@uditagarwal97 uditagarwal97 Sep 3, 2025

Choose a reason for hiding this comment

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

I'm not sure this is a good solution. I tried it once and it just lead to more Coverity hits.

If this class is destroyed in any OTHER classes destructor, then the problem recurs. Do you mark that destructor as noexcept(false) too?

I see. If this solution leads to more Coverity hits, I'd be in favor of using try/catch instead, similar to what you did in #14273

Copy link
Contributor Author

@uditagarwal97 uditagarwal97 Sep 3, 2025

Choose a reason for hiding this comment

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

Fixed in 7627f88

if (!MOwnerControlledCleanup)
for (auto &DeviceGlobalIt : MDeviceGlobals)
DeviceGlobalIt.second->cleanup();
Expand Down
4 changes: 2 additions & 2 deletions sycl/source/detail/device_global_map_entry.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,14 @@ struct DeviceGlobalMapEntry {
// Constructor for only initializing ID and pointer. The other members will
// be initialized later.
DeviceGlobalMapEntry(std::string UniqueId, const void *DeviceGlobalPtr)
: MUniqueId(UniqueId), MDeviceGlobalPtr(DeviceGlobalPtr) {}
: MUniqueId(std::move(UniqueId)), MDeviceGlobalPtr(DeviceGlobalPtr) {}

// Constructor for only initializing ID, type size, and device image scope
// flag. The pointer to the device global will be initialized later.
DeviceGlobalMapEntry(std::string UniqueId, const RTDeviceBinaryImage *Img,
std::uint32_t DeviceGlobalTSize,
bool IsDeviceImageScopeDecorated)
: MUniqueId(UniqueId), MImages{Img},
: MUniqueId(std::move(UniqueId)), MImages{Img},
MImageIdentifiers{reinterpret_cast<uintptr_t>(Img)},
MDeviceGlobalTSize(DeviceGlobalTSize),
MIsDeviceImageScopeDecorated(IsDeviceImageScopeDecorated) {}
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/property_set_io.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ static IntT stringViewToInt(const std::string_view &SV) {
if (SV.empty())
return Result;

bool Negate = std::is_signed_v<IntT> && SV[0] == '-';
const bool Negate = std::is_signed_v<IntT> && SV[0] == '-';

for (size_t I = static_cast<size_t>(Negate); I < SV.size(); ++I) {
const char CurrentC = SV[I];
Expand Down
1 change: 1 addition & 0 deletions xpti/include/xpti/xpti_trace_framework.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1731,6 +1731,7 @@ class tracepoint_t {
m_default_event_type = (uint16_t)xpti::trace_event_type_t::algorithm;
m_default_activity_type = xpti::trace_activity_type_t::active;
m_default_name = "Message"; // Likely never used
m_instID = 0;
Copy link
Contributor Author

@uditagarwal97 uditagarwal97 Sep 2, 2025

Choose a reason for hiding this comment

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

Coverity was complaining about this field be left uninitialized in the constructor.

}
/// The payload data structure that is prepared from code_location(),
/// caller_callee string or kernel name/codepointer based on the opt-in flag.
Expand Down
Loading