Skip to content

[SYCL] Fixed bug reported by a static verifier #10452

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 19 commits into from
Aug 24, 2023
Merged

[SYCL] Fixed bug reported by a static verifier #10452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6df2c6a
Coverity - Copy without assign issues
eunakim0103 Jul 18, 2023
e5c94c7
Coverity - logically dead code removal
eunakim0103 Jul 18, 2023
f337375
Coverity - Missing assignment operator issues fixed
eunakim0103 Jul 18, 2023
bed2656
added a default case in switch to check BinaryType
eunakim0103 Jul 18, 2023
e86d89a
added assert and return in the default case
eunakim0103 Jul 19, 2023
4bf1d35
added print out to check the BinaryType value
eunakim0103 Jul 19, 2023
a6172cb
fixed print statement to flush the message
eunakim0103 Jul 19, 2023
f0f732d
initialzed the BinaryType
eunakim0103 Jul 20, 2023
d81d3fc
Merge branch 'sycl' into coverity_bug_fix_new
eunakim0103 Aug 21, 2023
c537282
removed assert() to check the default case of the BinaryType value
eunakim0103 Aug 21, 2023
dd51099
clang format error fixed
eunakim0103 Aug 21, 2023
6f06a55
user defined assignment operator added
eunakim0103 Aug 21, 2023
cf98c3f
added a user defined assignment operator
eunakim0103 Aug 21, 2023
95cae55
user defined assignment operator fixed to delete since they are not used
eunakim0103 Aug 21, 2023
81d0cee
Added an user-defined assignment operator
eunakim0103 Aug 23, 2023
c2738e7
Fixed clang-format error
eunakim0103 Aug 23, 2023
e3ccb19
Update program_impl.cpp
eunakim0103 Aug 23, 2023
3db657a
Change the comment phrase
eunakim0103 Aug 23, 2023
9a7da9a
The comment has been improved
eunakim0103 Aug 23, 2023
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
1 change: 1 addition & 0 deletions sycl/source/detail/global_handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class GlobalHandler {

GlobalHandler(const GlobalHandler &) = delete;
GlobalHandler(GlobalHandler &&) = delete;
GlobalHandler &operator=(const GlobalHandler &) = delete;

void registerSchedulerUsage(bool ModifyCounter = true);
Scheduler &getScheduler();
Expand Down
11 changes: 6 additions & 5 deletions sycl/source/detail/program_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ program_impl::program_impl(ContextImplPtr Context,
sycl::detail::pi::PiDevice Device =
getSyclObjImpl(MDevices[0])->getHandleRef();
// TODO check build for each device instead
cl_program_binary_type BinaryType;
cl_program_binary_type BinaryType = PI_PROGRAM_BINARY_TYPE_NONE;
Plugin->call<PiApiKind::piProgramGetBuildInfo>(
MProgram, Device, PI_PROGRAM_BUILD_INFO_BINARY_TYPE,
sizeof(cl_program_binary_type), &BinaryType, nullptr);
Expand All @@ -184,20 +184,21 @@ program_impl::program_impl(ContextImplPtr Context,
OptionsVector.data(), nullptr);
std::string Options(OptionsVector.begin(), OptionsVector.end());
switch (BinaryType) {
case PI_PROGRAM_BINARY_TYPE_NONE:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good diagnostic to find errors during some code refactoring, now it is removed, so suggest adding the default case and add assert(false) in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion but basically this "Logically dead code" is because there is code to check if BinaryType is PI_PROGRAM_BINARY_TYPE_NONE and throw an exception in line 173. Coverity reported case PI_PROGRAM_BINARY_TYPE_NONE is redundancy and logically dead code.

173 if (BinaryType == PI_PROGRAM_BINARY_TYPE_NONE) {
174 throw invalid_object_error(

Copy link
Contributor

Choose a reason for hiding this comment

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

Still it is helpful for us during the refactoring if the exception is removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a default case as advised

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe do

switch () {
case 1:
 ...
 return;
case 2:
case 3:
  ...
  return;
}
assert(false) or __builtin_unreachable();

instead?

Copy link
Contributor Author

@eunakim0103 eunakim0103 Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree logically. I need to check the BinaryType values if there is any cases it shouldn't return in the any other part of code since the original code doesn't have default case and return. It may cause behavior changes when BinaryType is neither PI_PROGRAM_BINARY_TYPE_NONE nor when other values are not covered in the case selections if we add assert or return. I will investigate and update shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no other values of BinaryType which are not covered in switch case. I believe we can add assert(false) and return in the default case. Thanks!

Copy link
Contributor Author

@eunakim0103 eunakim0103 Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems the BinaryType value is invalid and switch-default addition caught it. The BinaryType was 3 in Intel Gen9 system which did not happen with my testing machine.

  • This may be an uninitialized variable issue. I fixed it to be initialized with 0 (PI_PROGRAM_BINARY_TYPE_NONE) --> this is wrong. With the initialization it still appears 3.
  • Here's where that came from: UR_PROGRAM_BUILD_INFO_BINARY_TYPE = 3

cl_program_binary_type BinaryType;

typedef enum {
PI_PROGRAM_BINARY_TYPE_NONE = 0x0,
PI_PROGRAM_BINARY_TYPE_COMPILED_OBJECT = 0x1,
PI_PROGRAM_BINARY_TYPE_LIBRARY = 0x2,
PI_PROGRAM_BINARY_TYPE_EXECUTABLE = 0x4
} _pi_program_binary_type;

command stderr:
online_compiler_L0.cpp.tmp.out: /__w/llvm/llvm/src/sycl/source/detail/program_impl.cpp:200: sycl::_V1::detail::program_impl::program_impl(sycl::_V1::detail::ContextImplPtr, pi_native_handle, sycl::_V1::detail::pi::PiProgram): Assertion `false && "BinaryType is invalid."' failed.

assert(false);
break;
case PI_PROGRAM_BINARY_TYPE_COMPILED_OBJECT:
MState = program_state::compiled;
MCompileOptions = Options;
MBuildOptions = Options;
break;
return;
case PI_PROGRAM_BINARY_TYPE_LIBRARY:
case PI_PROGRAM_BINARY_TYPE_EXECUTABLE:
MState = program_state::linked;
MLinkOptions = "";
MBuildOptions = Options;
return;
}
// TODO: uncomment the assert when UR_PROGRAM_BINARY_TYPE_EXECUTABLE
// value matches to PI_PROGRAM_BINARY_TYPE_EXECUTABLE value
// assert(false && "BinaryType is invalid.");
}

program_impl::program_impl(ContextImplPtr Context,
Expand Down
3 changes: 3 additions & 0 deletions sycl/source/detail/scheduler/commands.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ bool CurrentCodeLocationValid() {
struct DemangleHandle {
char *p;
DemangleHandle(char *ptr) : p(ptr) {}

DemangleHandle &operator=(const DemangleHandle &) = delete;

~DemangleHandle() { std::free(p); }
};
static std::string demangleKernelName(std::string Name) {
Expand Down
2 changes: 2 additions & 0 deletions sycl/source/detail/scheduler/leaves_collection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ class LeavesCollection {
(!MGenericIsActive && MHACIt != Rhs.MHACIt));
}

IteratorT &operator=(const IteratorT<IsConst> &) = delete;

// pre-increment
IteratorT<IsConst> &operator++() {
increment();
Expand Down
2 changes: 2 additions & 0 deletions sycl/source/detail/xpti_registry.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ class XPTIScope {
}
}

XPTIScope &operator=(const XPTIScope &) = delete;

xpti::trace_event_data_t *traceEvent() { return MTraceEvent; }

uint8_t streamID() { return MStreamID; }
Expand Down
2 changes: 2 additions & 0 deletions xpti/src/xpti_proxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ class ProxyLoader {
tryToEnable();
}

ProxyLoader &operator=(const ProxyLoader &) = delete;

~ProxyLoader() {
// If the loading of the framework library was
// successful, we should close the handle in the
Expand Down