Fix Coverity issues for acl_mem_test.cpp

[IlanTruanovsky]

This mainly fixes various PRINTF_ARGS errors.

[IlanTruanovsky]

There is an additional Coverity error for this file that is causing me some confusion and that I will need to look into some more before implementing. This issue's fix should be added to the PR before being merged with main. This error output is quite large, so I'll only leave in the important bits so we can discuss it:

test/acl_mem_test.cpp:2655:5:
  Type: Out-of-bounds access (ARRAY_VS_SINGLETON)

...

test/acl_mem_test.cpp:2655:5:
  11. address_of: Taking address with "&fill_event" yields a singleton pointer.
...

src/acl_event.cpp:988:3:
  13.1.18.33.4.2. path: Condition "num_events > 0", taking true branch.
src/acl_event.cpp:988:3:
  12.1.18.33.4.3. path: Condition "events == NULL", taking false branch.
src/acl_event.cpp:993:3:
  12.1.18.33.4.4. path: Condition "num_events == 0", taking true branch.
src/acl_event.cpp:993:3:
  12.1.18.33.4.5. path: Condition "events != NULL", taking false branch.
src/acl_event.cpp:999:3:
  12.1.18.33.4.6. path: Condition "i < num_events", taking true branch.
src/acl_event.cpp:1000:5:
  12.1.18.33.4.7. ptr_arith: Performing pointer arithmetic on "events" in expression "events + i".

To explain what's happening, we pass in a pointer to a single event fill_event to a function that at first glance seems to increment it by i, making it point to undesired memory if i > 1. However, this never happens in practice since we pass in the number of events and i < # of events.

What's even stranger (and possibly the cause of Coverity misrecognizing this issue) is that Coverity is contradicting itself in its own analysis...
According to the output, It's assuming num_events > 0 and that num_events == 0, and a similar contradiction happens with events. Not sure what's up with that.

[IlanTruanovsky]

The easiest (and maybe only?) solution to this would be to mark it as a false positive using a comment. I don't think it's possible to remove this Coverity issue by rearranging/reordering nearby code. Maybe there is a better way that I'm not thinking of.

Any thoughts @pcolberg? I'll continue investigating this issue.

[pcolberg]

The easiest (and maybe only?) solution to this would be to mark it as a false positive using a comment. I don't think it's possible to remove this Coverity issue by rearranging/reordering nearby code. Maybe there is a better way that I'm not thinking of.

Any thoughts @pcolberg? I'll continue investigating this issue.

I have yet to understand Coverity's analysis, but I am confident this issue can be resolved too.

[pcolberg]

The Coverity issue is resolved by passing an array:

--- a/test/acl_mem_test.cpp
+++ b/test/acl_mem_test.cpp
@@ -2652,9 +2652,10 @@ MT_TEST(acl_mem, fill_image) {
     CHECK_EQUAL(CL_INVALID_EVENT_WAIT_LIST,
                 clEnqueueFillImage(m_cq, image, fill_color_int, origin, region,
                                    1, NULL, &fill_event)); // invalid wait list
+    cl_event event_wait_list[1];
     CHECK_EQUAL(CL_INVALID_EVENT_WAIT_LIST,
                 clEnqueueFillImage(m_cq, image, fill_color_int, origin, region,
-                                   0, &fill_event,
+                                   0, event_wait_list,
                                    &fill_event)); // invalid wait list
 
     CHECK_EQUAL(1, acl_ref_count(image));

I have yet to understand what the underlying issue is.

[pcolberg]

There are actually three cases of ARRAY_VS_SINGLETON that are caused by the same two loops with pointer arithmetic over events. The following (obviously wrong) change resolves those three issues.

--- a/src/acl_event.cpp
+++ b/src/acl_event.cpp
@@ -843,12 +843,14 @@ cl_int acl_create_event(cl_command_queue command_queue, cl_uint num_events,
 
       // Fill active_dependency[] by removing duplicates from events[].
       for (cl_uint i = 0; i < num_events; i++) {
+#if 0
         if (active_dependency.find(events[i]) == active_dependency.end()) {
           // If an event is already finished, no need to add a dependency on it
           if (!acl_event_is_done(events[i])) {
             active_dependency.insert(events[i]);
           }
         }
+#endif
       }
       // Now add dependency arcs.
       for (cl_event dependency : active_dependency) {
@@ -997,6 +999,7 @@ cl_int acl_check_events_in_context(cl_context context, cl_uint num_events,
   }
 
   for (i = 0; i < num_events; i++) {
+#if 0
     cl_event event = events[i];
 
     // check the event and context based on the event status
@@ -1010,6 +1013,7 @@ cl_int acl_check_events_in_context(cl_context context, cl_uint num_events,
       acl_context_callback(context, "Event from incorrect context");
       return CL_INVALID_CONTEXT;
     }
+#endif
   }
   return CL_SUCCESS;
 }

[pcolberg]

This can be merged since these fixes are fine and not related to the ARRAY_VS_SINGLETON issue. You may or may not want to do more investigation into why Coverity is getting confused, but from a web search for ARRAY_VS_SINGLETON I could not find any hints other than this being a false positive. You can go ahead and open a new PR with workarounds similar to #263 (comment) that resolve all three cases of ARRAY_VS_SINGLETON and document your analysis in the commit message.

[IlanTruanovsky]

The Coverity issue is resolved by passing an array:

--- a/test/acl_mem_test.cpp
+++ b/test/acl_mem_test.cpp
@@ -2652,9 +2652,10 @@ MT_TEST(acl_mem, fill_image) {
     CHECK_EQUAL(CL_INVALID_EVENT_WAIT_LIST,
                 clEnqueueFillImage(m_cq, image, fill_color_int, origin, region,
                                    1, NULL, &fill_event)); // invalid wait list
+    cl_event event_wait_list[1];
     CHECK_EQUAL(CL_INVALID_EVENT_WAIT_LIST,
                 clEnqueueFillImage(m_cq, image, fill_color_int, origin, region,
-                                   0, &fill_event,
+                                   0, event_wait_list,
                                    &fill_event)); // invalid wait list
 
     CHECK_EQUAL(1, acl_ref_count(image));

I have yet to understand what the underlying issue is.

Yesterday I was attempting this solution but I didn't finish running the Coverity scan.

Glad to know it works!