8000 [SYCL][XPTI] Fix static analysis tool warnings by alexbatashev · Pull Request #5040 · intel/llvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SYCL][XPTI] Fix static analysis tool warnings #5040

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
Dec 15, 2021
Merged

[SYCL][XPTI] Fix static analysis tool warnings #5040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
155322f
[SYCL][XPTI] Fix static analysis tool warnings
Nov 29, 2021
24163e0
add comment
Dec 10, 2021
f0c2de7
clang-format
Dec 10, 2021
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
8000
Diff view
12 changes: 12 additions & 0 deletions sycl/source/detail/program_manager/program_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ RetT *waitUntilBuilt(KernelProgramCache &Cache,
/// locked version. Accepts reference to locked version of cache.
/// \tparam BuildFT type of function which will build the entity if it is not in
/// cache. Accepts nothing. Return pointer to built entity.
///
/// \return a pointer to cached build result, return value must not be nullptr.
template <typename RetT, typename ExceptionT, typename KeyT, typename AcquireFT,
typename GetCacheFT, typename BuildFT>
KernelProgramCache::BuildResult<RetT> *
Expand Down Expand Up @@ -528,6 +530,8 @@ RT::PiProgram ProgramManager::getBuiltPIProgram(
std::make_pair(std::make_pair(std::move(SpecConsts), KSId),
std::make_pair(PiDevice, CompileOpts + LinkOpts)),
AcquireF, GetF, BuildF);
// getOrBuild is not supposed to return nullptr
assert(BuildResult != nullptr && "Invalid build result");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have asserts always on? Wouldn't it be safe to have exception throw here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CI we always build and test with assertions. Honestly, I don’t see a code path, that returns nullptr. And I assume assert makes more sense here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexbatashev If BuildResult is guaranteed to not be nullptr, can we document it somewhere that this is the case, hence the use of assert()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tovinkere I can add a comment to getOrBuild
Generally, I think this is a perfect case for C++ contracts, but they have not been standardized yet, unfortunately.

return BuildResult->Ptr.load();
}

Expand Down Expand Up @@ -593,6 +597,8 @@ ProgramManager::getOrCreateKernel(OSModuleHandle M,

auto BuildResult = getOrBuild<PiKernelT, invalid_object_error>(
Cache, KernelName, AcquireF, GetF, BuildF);
// getOrBuild is not supposed to return nullptr
assert(BuildResult != nullptr && "Invalid build result");
auto ret_val = std::make_tuple(BuildResult->Ptr.load(),
&(BuildResult->MBuildResultMutex), Program);
Cache.saveKernel(key, ret_val);
Expand Down Expand Up @@ -1793,6 +1799,8 @@ device_image_plain ProgramManager::build(const device_image_plain &DeviceImage,
std::make_pair(std::make_pair(std::move(SpecConsts), (size_t)ImgPtr),
std::make_pair(PiDevice, CompileOpts + LinkOpts)),
AcquireF, GetF, BuildF);
// getOrBuild is not supposed to return nullptr
assert(BuildResult != nullptr && "Invalid build result");

RT::PiProgram ResProgram = BuildResult->Ptr.load();

Expand All @@ -1812,6 +1820,8 @@ device_image_plain ProgramManager::build(const device_image_plain &DeviceImage,
std::make_pair(std::make_pair(std::move(SpecConsts), (size_t)ImgPtr),
std::make_pair(PiDeviceAdd, CompileOpts + LinkOpts)),
AcquireF, GetF, CacheOtherDevices);
// getOrBuild is not supposed to return nullptr
assert(BuildResult != nullptr && "Invalid build result");
}

// devive_image_impl shares ownership of PIProgram with, at least, program
Expand Down Expand Up @@ -1865,6 +1875,8 @@ std::pair<RT::PiKernel, std::mutex *> ProgramManager::getOrCreateKernel(

auto BuildResult = getOrBuild<PiKernelT, invalid_object_error>(
Cache, KernelName, AcquireF, GetF, BuildF);
// getOrBuild is not supposed to return nullptr
assert(BuildResult != nullptr && "Invalid build result");
return std::make_pair(BuildResult->Ptr.load(),
&(BuildResult->MBuildResultMutex));
}
Expand Down
3 changes: 2 additions & 1 deletion xptifw/unit_test/xpti_api_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ TEST_F(xptiApiTest, xptiFindEventGoodInput) {

auto Result = xptiMakeEvent("foo", &Payload, 0,
(xpti::trace_activity_type_t)1, &Instance);
EXPECT_NE(Result, nullptr);
ASSERT_NE(Result, nullptr);
EXPECT_EQ(Instance, 1);
auto NewResult = xptiFindEvent(Result->unique_id);
EXPECT_EQ(Result, NewResult);
Expand All @@ -181,6 +181,7 @@ TEST_F(xptiApiTest, xptiQueryPayloadGoodInput) {
EXPECT_NE(Result, nullptr);
EXPECT_EQ(instance, 1);
auto NewResult = xptiQueryPayload(Result);
ASSERT_NE(NewResult, nullptr);
EXPECT_STREQ(Payload.name, NewResult->name);
EXPECT_STREQ(Payload.source_file, NewResult->source_file);
// NewResult->name_sid will have a string ID whereas 'Payload' will not
Expand Down
3 changes: 2 additions & 1 deletion xptifw/unit_test/xpti_correctness_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@ TEST_F(xptiCorrectnessTest, xptiMakeEvent) {
xpti::payload_t p("foo", "foo.cpp", 1, 0, (void *)13);
auto Result =
xptiMakeEvent("foo", &p, 0, (xpti::trace_activity_type_t)1, &Instance);
EXPECT_NE(Result, nullptr);
ASSERT_NE(Result, nullptr);
p = xpti::payload_t("foo", "foo.cpp", 1, 0, (void *)13);
auto NewResult =
xptiMakeEvent("foo", &p, 0, (xpti::trace_activity_type_t)1, &Instance);
ASSERT_NE(NewResult, nullptr);
EXPECT_EQ(Result, NewResult);
EXPECT_EQ(Result->unique_id, NewResult->unique_id);
EXPECT_EQ(Result->reserved.payload, NewResult->reserved.payload);
Expand Down
0