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

eunakim0103 · 2023-07-18T11:53:52Z

The static verifier tool reported following three issues and fixed with adding a copy constructor & assignment operator, and removing dead code.

Logically dead code
Missing assignment operator
Copy without assign

Signed-off-by: Kim, Euna <[email protected]>

dm-vodopyanov · 2023-07-18T15:21:33Z

xpti/src/xpti_proxy.cpp

+  ProxyLoader(bool loaded, xpti_plugin_handle_t fw_plugin_handle)
+      : m_loaded(loaded), m_fw_plugin_handle(fw_plugin_handle) {}
+
+  ProxyLoader(const ProxyLoader &rhs)
+      : ProxyLoader(rhs.m_loaded, rhs.m_fw_plugin_handle) {}
+


Why we need these two new constructors? Looks like they are not used in the code

You are right -- It should be as below as other missing assignment operator Coverity violation. Will fix.
ProxyLoader&operator=(const ProxyLoader&) = delete;

dm-vodopyanov · 2023-07-18T15:29:43Z

sycl/source/detail/scheduler/commands.cpp

@@ -70,6 +70,20 @@ bool CurrentCodeLocationValid() {
 struct DemangleHandle {
  char *p;
  DemangleHandle(char *ptr) : p(ptr) {}
+
+  DemangleHandle &operator=(const DemangleHandle &rhs) {


Same question here, we don't have any assignment in the code, shouldn't it be deleted (= delete)?

Same reason as above:
2023850 Missing assignment operator
This class, which frees resources in its destructor, does not have a user-written copy assignment operator, but is copy-assigned. Unless the resource is managed separately from the copy operation, this will cause use-after-free errors.

Class that owns resources lacks a user-written assignment operator

That doesn't answer the question why we can't use = delete; instead.

dm-vodopyanov · 2023-07-18T15:31:58Z

sycl/source/detail/scheduler/leaves_collection.hpp

@@ -221,6 +221,18 @@ class LeavesCollection {
              (!MGenericIsActive && MHACIt != Rhs.MHACIt));
    }

+    IteratorT &operator=(const IteratorT<IsConst> &Rhs) {


Looks like this copy assignment operator is not used too in the code

dm-vodopyanov · 2023-07-18T15:46:57Z

sycl/source/detail/program_impl.cpp

@@ -184,9 +184,6 @@ program_impl::program_impl(ContextImplPtr Context,
      OptionsVector.data(), nullptr);
  std::string Options(OptionsVector.begin(), OptionsVector.end());
  switch (BinaryType) {
-  case PI_PROGRAM_BINARY_TYPE_NONE:


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.

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(

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

Added a default case as advised

Maybe do

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

instead?

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.

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!

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.

Signed-off-by: Kim, Euna <[email protected]>

dm-vodopyanov · 2023-07-18T22:39:04Z

@aelovikov-intel is the failure in SYCL Pre Commit on Linux / Linux / e2e-tests (lin_intel, SYCL E2E on Intel CPU/GEN9 GPU, Linux, gen known?

stderr:
Warning: ONEAPI_DEVICE_SELECTOR environment variable is set to opencl:cpu.
To see the correct device id, please unset ONEAPI_DEVICE_SELECTOR.


lit.py: /__w/llvm/llvm/llvm/sycl/test-e2e/lit.cfg.py:413: note: SG sizes for opencl:cpu: 
6 errors, exiting.
FAILED: CMakeFiles/check-sycl-e2e /__w/llvm/llvm/build-e2e/CMakeFiles/check-sycl-e2e 
cd /__w/llvm/llvm/build-e2e && /usr/bin/python3.10 /__w/llvm/llvm/llvm/llvm/utils/lit/lit.py -sv .
ninja: build stopped: subcommand failed.

aelovikov-intel · 2023-07-18T23:13:33Z

For HIP we had a faulty runner amdgpu-2 that has been rebooted. GEN9 failure is new to me. Regardless, we've switched to GEN12 earlier today, so I'd suggest to merge in origin/sycl.

Signed-off-by: Kim, Euna <[email protected]>

dm-vodopyanov · 2023-08-23T13:01:00Z

sycl/source/detail/program_impl.cpp

@@ -197,6 +194,13 @@ program_impl::program_impl(ContextImplPtr Context,
    MState = program_state::linked;
    MLinkOptions = "";
    MBuildOptions = Options;
+    break;
+  default:
+    // TODO: assert() will be reverted when UR_PROGRAM_BINARY_TYPE_EXECUTABLE


@eunakim0103, @aelovikov-intel proposed another place for the assert (link), can you implement his option?

Fixed the break to return and changed the position of adding assert()

sycl/source/detail/program_impl.cpp

Co-authored-by: Dmitry Vodopyanov <[email protected]>

aelovikov-intel · 2023-08-23T17:05:18Z

eunakim0103 requested a review from aelovikov-intel 2 hours ago

I'm not closely familiar with this code and you already have an approval needed to merge. Please explain/fix the test failure and ping @intel/llvm-gatekeepers afterwards.

eunakim0103 · 2023-08-23T23:46:55Z

All test passed. This PR is ready for merge @intel/llvm-gatekeepers

dm-vodopyanov · 2023-08-24T11:16:55Z

@eunakim0103 please don't hide flaky issues, if tests passed after the re-run, please still report them. It's also highly encouraged to submit issues on them in intel/llvm.
In this case, fail on Intel Gen12, issue #10955:

TIMEOUT: SYCL :: Basic/sycl_2020_images/host_task_sampled_image_read_linear.cpp (512 of 1621)
******************** TEST 'SYCL :: Basic/sycl_2020_images/host_task_sampled_image_read_linear.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';    /__w/llvm/llvm/toolchain/bin//clang++   -fsycl -fsycl-targets=spir64 /__w/llvm/llvm/llvm/sycl/test-e2e/Basic/sycl_2020_images/host_task_sampled_image_read_linear.cpp -o /__w/llvm/llvm/build-e2e/Basic/sycl_2020_images/Output/host_task_sampled_image_read_linear.cpp.tmp.out
: 'RUN: at line 3';    /__w/llvm/llvm/build-e2e/Basic/sycl_2020_images/Output/host_task_sampled_image_read_linear.cpp.tmp.out
--
Exit Code: -9
Timeout: Reached timeout of 600 seconds

eunakim0103 added 3 commits July 18, 2023 04:47

Coverity - Copy without assign issues

6df2c6a

Coverity - logically dead code removal

e5c94c7

Signed-off-by: Kim, Euna <[email protected]>

Coverity - Missing assignment operator issues fixed

f337375

Signed-off-by: Kim, Euna <[email protected]>

eunakim0103 requested a review from a team as a code owner July 18, 2023 11:53

eunakim0103 requested a review from dm-vodopyanov July 18, 2023 11:53

eunakim0103 mentioned this pull request Jul 18, 2023

[SYCL] Coverity bug fix #10446

Closed

eunakim0103 temporarily deployed to aws July 18, 2023 12:24 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 18, 2023 13:15 — with GitHub Actions Inactive

dm-vodopyanov changed the title ~~[SYCL] Coverity bug fix - corrected~~ [SYCL] Coverity bug fixes Jul 18, 2023

eunakim0103 temporarily deployed to aws July 18, 2023 14:09 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 18, 2023 15:40 — with GitHub Actions Inactive

dm-vodopyanov reviewed Jul 18, 2023

View reviewed changes

added a default case in switch to check BinaryType

bed2656

Signed-off-by: Kim, Euna <[email protected]>

eunakim0103 temporarily deployed to aws July 18, 2023 17:00 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 18, 2023 17:57 — with GitHub Actions Inactive

added assert and return in the default case

e86d89a

Signed-off-by: Kim, Euna <[email protected]>

eunakim0103 temporarily deployed to aws July 19, 2023 19:18 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 19, 2023 19:57 — with GitHub Actions Inactive

eunakim0103 marked this pull request as draft July 19, 2023 20:02

added print out to check the BinaryType value

4bf1d35

Signed-off-by: Kim, Euna <[email protected]>

eunakim0103 temporarily deployed to aws July 19, 2023 20:54 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 19, 2023 21:35 — with GitHub Actions Inactive

fixed print statement to flush the message

a6172cb

Signed-off-by: Kim, Euna <[email protected]>

eunakim0103 temporarily deployed to aws July 19, 2023 23:10 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 20, 2023 00:05 — with GitHub Actions Inactive

initialzed the BinaryType

f0f732d

Signed-off-by: Kim, Euna <[email protected]>

eunakim0103 temporarily deployed to aws July 20, 2023 01:13 — with GitHub Actions Inactive

eunakim0103 temporarily deployed to aws July 20, 2023 01:54 — with GitHub Actions Inactive

eunakim0103 marked this pull request as ready for review August 21, 2023 14:10

eunakim0103 and others added 8 commits August 21, 2023 07:15

Merge branch 'sycl' into coverity_bug_fix_new

d81d3fc

removed assert() to check the default case of the BinaryType value

c537282

Signed-off-by: Kim, Euna <[email protected]>

clang format error fixed

dd51099

Signed-off-by: Kim, Euna <[email protected]>

user defined assignment operator added

6f06a55

Signed-off-by: Kim, Euna <[email protected]>

added a user defined assignment operator

cf98c3f

Signed-off-by: Kim, Euna <[email protected]>

user defined assignment operator fixed to delete since they are not used

95cae55

Signed-off-by: Kim, Euna <[email protected]>

Added an user-defined assignment operator

81d0cee

Fixed clang-format error

c2738e7

dm-vodopyanov reviewed Aug 23, 2023

View reviewed changes

Update program_impl.cpp

e3ccb19

Fixed the break to return and changed the position of adding assert()

dm-vodopyanov reviewed Aug 23, 2023

View reviewed changes

sycl/source/detail/program_impl.cpp Outdated Show resolved Hide resolved

sycl/source/detail/program_impl.cpp Outdated Show resolved Hide resolved

eunakim0103 and others added 2 commits August 23, 2023 09:38

Change the comment phrase

3db657a

Co-authored-by: Dmitry Vodopyanov <[email protected]>

The comment has been improved

9a7da9a

Co-authored-by: Dmitry Vodopyanov <[email protected]>

dm-vodopyanov approved these changes Aug 23, 2023

View reviewed changes

eunakim0103 requested a review from aelovikov-intel August 23, 2023 14:53

eunakim0103 changed the title ~~[SYCL] Coverity bug fixes~~ [SYCL] Fixed bug reported by a static verifier Aug 24, 2023

dm-vodopyanov mentioned this pull request Aug 24, 2023

Timeout on Basic/sycl_2020_images/host_task_sampled_image_read_linear.cpp with unrelated changes #10955

Closed

dm-vodopyanov merged commit 3921244 into intel:sycl Aug 24, 2023

againull mentioned this pull request Oct 24, 2023

[SYCL] Add an user-defined copy constructor #11637

Merged

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

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

Uh oh!

Conversation

eunakim0103 commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eunakim0103 Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eunakim0103 Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eunakim0103 Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eunakim0103 Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov commented Jul 18, 2023

Uh oh!

aelovikov-intel commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dm-vodopyanov Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aelovikov-intel commented Aug 23, 2023

Uh oh!

eunakim0103 commented Aug 23, 2023

Uh oh!

dm-vodopyanov commented Aug 24, 2023

Uh oh!

Uh oh!

eunakim0103 commented Jul 18, 2023 •

edited

Loading

eunakim0103 Jul 18, 2023 •

edited

Loading

eunakim0103 Jul 18, 2023 •

edited

Loading

eunakim0103 Jul 19, 2023 •

edited

Loading

eunakim0103 Jul 19, 2023 •

edited

Loading

aelovikov-intel commented Jul 18, 2023 •

edited

Loading

dm-vodopyanov Aug 23, 2023 •

edited

Loading