Skip to content
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.

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

Memory Leakage in test_conformance/atomics/test_atomics.cpp #1793

Open
incognito1729 opened this issue Aug 1, 2023 · 1 comment · May be fixed by #2169
Open

Memory Leakage in test_conformance/atomics/test_atomics.cpp #1793

incognito1729 opened this issue Aug 1, 2023 · 1 comment · May be fixed by #2169
Labels
mobica-backlog Issue approved by WG for Mobica to work on

Comments

@incognito1729
Copy link
Contributor

incognito1729 commented Aug 1, 2023

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
Copy link
Member

svenvh commented Aug 2, 2023

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).

@bashbaug bashbaug added the mobica-backlog Issue approved by WG for Mobica to work on label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobica-backlog Issue approved by WG for Mobica to work on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants