fix: supress GCC13 warnings by Kumataro · Pull Request #24434 · opencv/opencv

Kumataro · 2023-10-21T10:27:22Z

For ubuntu 23.10, supress GCC13 warnings.

OpenCV version: 4.x
Operating System / Platform: Ubuntu 23.10
Compiler & compiler version: GCC 13.2.0

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

opencv-alalek · 2023-10-21T14:29:29Z

modules/dnn/src/layers/layers_common.cpp

    bool is_global = params.get<bool>("global_pooling", false);
-    globalPooling.resize(3);
+    globalPooling.reserve(3);
    globalPooling[0] = params.get<bool>("global_pooling_d", is_global);


should be .resize()
.reserve() is wrong (during reallocation container will lost stored data)

Thank you for your review!
I believe that stored data in globalPooling should not be cared.

After globalPooling.resize(3), data in globalPooling[0..2] are kept. If globalPooling has more data, they will be lost.

Data in globalPooling[0..2] are overwrited following statements. So they will be lost.

opencv/modules/dnn/src/layers/layers_common.cpp

Lines 147 to 156 in c2f909f

void getPoolingKernelParams(const LayerParams &params, std::vector<size_t>& kernel, std::vector<bool>& globalPooling,

std::vector<size_t>& pads_begin, std::vector<size_t>& pads_end,

std::vector<size_t>& strides, cv::String &padMode)

{

bool is_global = params.get<bool>("global_pooling", false);

globalPooling.resize(3);

globalPooling[0] = params.get<bool>("global_pooling_d", is_global);

globalPooling[1] = params.get<bool>("global_pooling_h", is_global);

globalPooling[2] = params.get<bool>("global_pooling_w", is_global);

(if it is correct) I'm sorry that pull request code has a bug.

globalPooling.clear() is needed to reset verctor elements before globalPooling.reserve(3).
globalPooling.size() will be update when set globalPooling[0...2] = ....
I think it will be the same condition as it was before this fix.

I think this may be a gcc misjudgment. But vector<bool> is special complex case to handle by each bit. It is possible that an incorrect memory copy range bug may occur really when vector::resize() is called.

So I believe that avoiding to call resize() is better than suppessing warning.

kmtr@kmtr-None:~/work/build4-main$ vi ../opencv/modules/dnn/src/layers/layers_common.cpp kmtr@kmtr-None:~/work/build4-main$ ninja [1/19] Building CXX object modules/dnn/CMakeFiles/opencv_dnn.dir/src/layers/layers_common.cpp.o In file included from /usr/include/c++/13/array:43, from /home/kmtr/work/opencv/modules/core/include/opencv2/core/cvdef.h:794, from /home/kmtr/work/opencv/modules/core/include/opencv2/core.hpp:52, from /home/kmtr/work/opencv/modules/dnn/src/layers/../precomp.hpp:49, from /home/kmtr/work/opencv/modules/dnn/src/layers/layers_common.cpp:43: In static member function ‘static _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = long unsigned int; _Up = long unsigned int; bool _IsMove = false]’, inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:506:30, inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:533:42, inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:540:31, inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:633:7, inlined from ‘std::vector<bool, _Alloc>::iterator std::vector<bool, _Alloc>::_M_copy_aligned(const_iterator, const_iterator, iterator) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/stl_bvector.h:1306:28, inlined from ‘void std::vector<bool, _Alloc>::_M_fill_insert(iterator, size_type, bool) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/vector.tcc:879:34, inlined from ‘std::vector<bool, _Alloc>::iterator std::vector<bool, _Alloc>::insert(const_iterator, size_type, const bool&) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/stl_bvector.h:1206:16, inlined from ‘void std::vector<bool, _Alloc>::resize(size_type, bool) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/stl_bvector.h:1252:10, inlined from ‘void cv::dnn::getPoolingKernelParams(const dnn4_v20230620::LayerParams&, std::vector<long unsigned int>&, std::vector<bool>&, std::vector<long unsigned int>&, std::vector<long unsigned int>&, std::vector<long unsigned int>&, cv::String&)’ at /home/kmtr/work/opencv/modules/dnn/src/layers/layers_common.cpp:152:25: /usr/include/c++/13/bits/stl_algobase.h:437:30: warning: ‘void* __builtin_memmove(void*, const void*, long unsigned int)’ forming offset 8 is out of the bounds [0, 8] [-Warray-bounds=] 437 | __builtin_memmove(__result, __first, sizeof(_Tp) * _Num); | ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [19/19] Linking CXX executable bin/opencv_test_gapi

Maybe this will work?

globalPooling.assign({ params.get<bool>("global_pooling_d", is_global), params.get<bool>("global_pooling_h", is_global), params.get<bool>("global_pooling_w", is_global), });

Thank you very much for your comments, I agree with you !

mshabunin · 2023-10-25T10:32:08Z

modules/stitching/src/camera.cpp

    k(0,0) = focal; k(0,2) = ppx;
    k(1,1) = focal * aspect; k(1,2) = ppy;
-    return std::move(k);
+    return k;


Maybe so?

Suggested change

return k;

return Mat(k);

Or the whole function should be rewritten, so that local variable being returned would match returned type.

Yes, it works well, thank you ! GCC13 suggested to remove std::move() because it is redundant code. However clang on macOS/xcode suggests to keep std:move() because type conversion. I think using Mat() is simple and the best instead of supressing warning.

mshabunin · 2023-10-25T10:33:01Z

modules/video/src/optical_flow_io.cpp

    }
    file.close();
-    return std::move(flow);
+    return flow;


Same as previous comment.

Suggested change

return flow;

return Mat(flow);

Similar to the above, I also modified the code to use Mat(). thank you very much !

* fix: supress GCC13 warnings * fix for review and compile-warning on MacOS

fix: supress GCC13 warnings

f122bac

opencv-alalek added the category: build/install label Oct 21, 2023

opencv-alalek reviewed Oct 21, 2023

View reviewed changes

mshabunin reviewed Oct 25, 2023

View reviewed changes

fix for review and compile-warning on MacOS

d63c480

mshabunin approved these changes Oct 25, 2023

View reviewed changes

asmorkalov assigned mshabunin Oct 26, 2023

asmorkalov added this to the 4.9.0 milestone Oct 26, 2023

asmorkalov merged commit 1911c63 into opencv:4.x Oct 26, 2023

asmorkalov mentioned this pull request Nov 3, 2023

(5.x) Merge 4.x #24486

Merged

IskXCr pushed a commit to Haosonn/opencv that referenced this pull request Dec 20, 2023

fix: supress GCC13 warnings (opencv#24434)

d807a69

* fix: supress GCC13 warnings * fix for review and compile-warning on MacOS

thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024

fix: supress GCC13 warnings (opencv#24434)

15b5394

* fix: supress GCC13 warnings * fix for review and compile-warning on MacOS

thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024

fix: supress GCC13 warnings (opencv#24434)

56929e8

* fix: supress GCC13 warnings * fix for review and compile-warning on MacOS

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: supress GCC13 warnings#24434

fix: supress GCC13 warnings#24434
asmorkalov merged 2 commits intoopencv:4.xfrom
Kumataro:fixWarningForGCC13

Kumataro commented Oct 21, 2023

Uh oh!

opencv-alalek Oct 21, 2023

Uh oh!

Kumataro Oct 21, 2023

Uh oh!

Kumataro Oct 21, 2023

Uh oh!

mshabunin Oct 25, 2023

Uh oh!

Kumataro Oct 25, 2023

Uh oh!

mshabunin Oct 25, 2023

Uh oh!

Kumataro Oct 25, 2023

Uh oh!

mshabunin Oct 25, 2023

Uh oh!

Kumataro Oct 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	void getPoolingKernelParams(const LayerParams &params, std::vector<size_t>& kernel, std::vector<bool>& globalPooling,
	std::vector<size_t>& pads_begin, std::vector<size_t>& pads_end,
	std::vector<size_t>& strides, cv::String &padMode)
	{
	bool is_global = params.get<bool>("global_pooling", false);
	globalPooling.resize(3);
	globalPooling[0] = params.get<bool>("global_pooling_d", is_global);
	globalPooling[1] = params.get<bool>("global_pooling_h", is_global);
	globalPooling[2] = params.get<bool>("global_pooling_w", is_global);

Uh oh!

Conversation

Kumataro commented Oct 21, 2023

Pull Request Readiness Checklist

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants