Memory Leakage in test_conformance/atomics/test_atomics.cpp

[incognito1729]

Hi all,

I was going through test_atomics.cpp and found lots of memory leakage due to conditional statements(line no. 26,36,47,58). So I would like to ask, instead of handling manually free function why are we not using unique_ptr here? I understood some function are using aligned memory allocation/deallocation like free_mtdata(). So here I would like to propose to use wrapper class or custom deleter with smart pointer.

char *valids is used for incrementing at particular index(see line 6,10,28). I would to suggest to use heap allocated memory of type int8_t instead of char for better coding practice and understanding.

Attaching one function for quick overview.

@bashbaug @nikhiljnv tagging for quick response.

Disclaimer: I am new to this codebase, Apologies if I misunderstood the use of constructs and why they are implemented in the way they are.

1. bool test_atomic_xchg_verify_int(size_t size, cl_int *refValues,
2.                                  cl_int finalValue)
3. {
4.     /* For xchg, each value from 0 to size - 1 should have an entry in the ref
5.      * array, and ONLY one entry */
6.     char *valids;
7.     size_t i;
8.     char originalValidCount = 0;
9. 
10.     valids = (char *)malloc(sizeof(char) * size);
11.     memset(valids, 0, sizeof(char) * size);
12. 
13.     for (i = 0; i < size; i++)
14.     {
15.         if (refValues[i] == INT_TEST_VALUE)
16.         {
17.             // Special initial value
18.             originalValidCount++;
19.             continue;
20.         }
21.         if (refValues[i] < 0 || (size_t)refValues[i] >= size)
22.         {
23.             log_error(
24.                 "ERROR: Reference value %zu outside of valid range! (%d)\n", i,
25.                 refValues[i]);
26.             return false;
27.         }
28.         valids[refValues[i]]++;
29.     }
30. 
31.     /* Note: ONE entry will have zero count. It'll be the last one that
32.      executed, because that value should be the final value outputted */
33.     if (valids[finalValue] > 0)
34.     {
35.         log_error("ERROR: Final value %d was also in ref list!\n", finalValue);
36.         return false;
37.     }
38.     else
39.         valids[finalValue] = 1; // So the following loop will be okay
40. 
41.     /* Now check that every entry has one and only one count */
42.     if (originalValidCount != 1)
43.     {
44.         log_error("ERROR: Starting reference value %d did not occur "
45.                   "once-and-only-once (occurred %d)\n",
46.                   65191, originalValidCount);
47.         return false;
48.     }
49.     for (i = 0; i < size; i++)
50.     {
51.         if (valids[i] != 1)
52.         {
53.             log_error("ERROR: Reference value %zu did not occur "
54.                       "once-and-only-once (occurred %d)\n",
55.                       i, valids[i]);
56.             for (size_t j = 0; j < size; j++)
57.                 log_info("%d: %d\n", (int)j, (int)valids[j]);
58.             return false;
59.         }
60.     }
61. 
62.     free(valids);
63.     return true;
64. }

[svenvh]

instead of handling manually free function why are we not using unique_ptr here?

Large parts of the code base predate std::unique_ptr and haven't been modernized yet.

I understood some function are using aligned memory allocation/deallocation like free_mtdata(). So here I would like to propose to use wrapper class or custom deleter with smart pointer.

A wrapper class already exists: MTdataHolder. Some places already make use of it, but using it everywhere requires some further investment (both for making the changes and code review time).