[split #6908 2/3] Fix modern gcc warnings

[goekce]

I split my PR #6908 into three which was already partly reviewed. This is part 2/3.

[gbuildx]

Can one of the admins verify this patch?

[keryell]

So, is it a false positive from g++ 12.1 ?

[stsoe]

So, is it a false positive from g++ 12.1 ?

It's not a false positive that the ptr can be null if the OpenCL checks are disabled. However, default behavior is to validate all OpenCL APIs calls per OpenCL spec and this includes checking for valid objects. If checks are disabled and objects are null, then the program is in error and a segmentation fault is warranted, hiding it behind a conditional and returning CL_SUCESSS is definitely not correct.

[goekce]

Soren is right, the warnings do not happen in Debug build, but in Release. It does not make sense to hide them.

Now I suppress them (give a hint to the compiler that the branch can never happen) only in case of a non-debug build now.

FYI the warning that I was getting is here

In file included from /usr/include/c++/12.2.0/bits/shared_ptr_atomic.h:33,
from /usr/include/c++/12.2.0/memory:78,
from /usr/include/boost/property_tree/ptree_fwd.hpp:18,
from /git-repos/XRT/src/runtime_src/core/common/config_reader.h:24,
from /git-repos/XRT/src/runtime_src/xrt/util/config_reader.h:20,
from /git-repos/XRT/src/runtime_src/xrt/config.h:20,
from /git-repos/XRT/src/runtime_src/xocl/config.h:40,
from /git-repos/XRT/src/runtime_src/xocl/api/clReleaseCommandQueue.cpp:18:
In member function ‘std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::operator--() [with _ITp = unsigned int]’,
inlined from ‘bool xocl::refcount::release()’ at /git-repos/XRT/src/runtime_src/xocl/core/refcount.h:65:13,
inlined from ‘cl_int xocl::clReleaseCommandQueue(cl_command_queue)’ at /git-repos/XRT/src/runtime_src/xocl/api/clReleaseCommandQueue.cpp:40:35,
inlined from ‘cl_int clReleaseCommandQueue(cl_command_queue)’ at /git-repos/XRT/src/runtime_src/xocl/api/clReleaseCommandQueue.cpp:53:39:
/usr/include/c++/12.2.0/bits/atomic_base.h:393:34: error: ‘unsigned int __atomic_sub_fetch_4(volatile void*, unsigned int, int)’ writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
393 |       { return __atomic_sub_fetch(&_M_i, 1, int(memory_order_seq_cst)); }
|                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[goekce]

The effort here is good, but sadly most, if not all instances, of std::shared_ptr<T[]> should be replaced with std::vector<T>. The changes in this PR clearly illustrates that C++ has a flaw in that it should never have had shared_ptr support T[], because developers forget.

According to this comment shared_ptr<T[]> can lead to less overhead compared to vector<T> if T[] does not grow. So there is still a motivation for shared_ptr<T[]>.

[stsoe]

The effort here is good, but sadly most, if not all instances, of std::shared_ptr<T[]> should be replaced with std::vector<T>. The changes in this PR clearly illustrates that C++ has a flaw in that it should never have had shared_ptr support T[], because developers forget.

Problem is developers nearly always forget [] so I wish it wasn't used. But I hear you on the very slight overhead.

[goekce]

Problem is developers nearly always forget [] so I wish it wasn't used

Makes sense. However that should be part of the coding style.

For what it's worth, modern GCC integrates many checks regarding pointers, so the memory management errors could be mitigated easier.

I will leave my patch untouched then.

[dayeh-xilinx]

retest this please. Jenkins server restart.

[gbuildx]

Build Failed! :(

[goekce]

@stsoe just a friendly reminder that you did not respond to my comment above in your review, in case you thought was waiting on me.

[stsoe]

@goekce The warning aspect of this PR was fixed in 7302. If any other issues, please resubmit. Thanks.