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.

Sign up for GitHub

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

Jump to bottom

Fix GCC warning maybe-uninitialized for PromiseValue<T> constructor #60

Merged
merged 1 commit into from
Aug 24, 2024
Merged

Fix GCC warning maybe-uninitialized for PromiseValue<T> constructor #60

merged 1 commit into from
Aug 24, 2024

Conversation

Poldraunic
Copy link
Contributor

@Poldraunic Poldraunic commented Jun 26, 2024 •
edited
Loading

Use std::shared_ptr for PromiseValue<T>::m_data instead of QSharedPoitner<T>

In optimized builds (such as -O2) GCC emits "maybe-uninitialized" warning for QSharedPointer<T>::d. This happens only for optimized builds because that is when GCC tracks lifetimes[1]. In a project with lots of QtPromise uses this produces a heap of bogus warnings. This warning has been previously reported to a Qt team, but they're confident it is a compiler bug[2][3]. However, this is a second time and issue raised with QSharedPointer and warnings to it. The first time it was addressed here: #34.

GCC output 

[95/111] Building CXX object tests/auto/qtpromise/warnings/CMakeFiles/qtpromise.tests.auto.warnings.dir/tst_warnings.cpp.o
FAILED: tests/auto/qtpromise/warnings/CMakeFiles/qtpromise.tests.auto.warnings.dir/tst_warnings.cpp.o 
/usr/bin/c++ -DQT_CONCURRENT_LIB -DQT_CORE_LIB -DQT_DEPRECATED_WARNINGS -DQT_NO_DEBUG -DQT_NO_KEYWORDS -DQT_TESTCASE_BUILDDIR=\"/home/joe/programming/qtpromise/build\" -DQT_TESTLIB_LIB -I/home/joe/programming/qtpromise/build/tests/auto/qtpromise/warnings/qtpromise.tests.auto.warnings_autogen/include -I/home/joe/programming/qtpromise/include -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtConcurrent -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -isystem /usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ -isystem /usr/include/x86_64-linux-gnu/qt5/QtTest -std=gnu++11 -Werror -Wpedantic -Wall -Wextra -Wconversion -Wdouble-promotion -Wformat=2 -Wlogical-op -Wmissing-noreturn -Wold-style-cast -Wsign-conversion -Wswitch-default -Wunused-local-typedefs -pedantic-errors -Wduplicated-cond -Wduplicated-branches -fprofile-arcs -ftest-coverage -O0 -g -O2 -Werror=maybe-uninitialized -fPIC -MD -MT tests/auto/qtpromise/warnings/CMakeFiles/qtpromise.tests.auto.warnings.dir/tst_warnings.cpp.o -MF tests/auto/qtpromise/warnings/CMakeFiles/qtpromise.tests.auto.warnings.dir/tst_warnings.cpp.o.d -o tests/auto/qtpromise/warnings/CMakeFiles/qtpromise.tests.auto.warnings.dir/tst_warnings.cpp.o -c /home/joe/programming/qtpromise/tests/auto/qtpromise/warnings/tst_warnings.cpp
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer.h:48,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qdebug.h:54,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qcborcommon.h:45,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qcborvalue.h:45,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qcborarray.h:43,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QtCore:38,
                 from /usr/include/x86_64-linux-gnu/qt5/QtConcurrent/QtConcurrentDepends:3,
                 from /usr/include/x86_64-linux-gnu/qt5/QtConcurrent/QtConcurrent:3,
                 from /home/joe/programming/qtpromise/tests/auto/qtpromise/warnings/tst_warnings.cpp:8:
In member function ‘void QSharedPointer<T>::deref() [with T = int]’,
    inlined from ‘QSharedPointer<T>::~QSharedPointer() [with T = int]’ at /usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:316:30,
    inlined from ‘static QSharedPointer<T> QSharedPointer<T>::create(Args&& ...) [with Args = {int}; T = int]’ at /usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:453:5,
    inlined from ‘QtPromisePrivate::PromiseValue<T>::PromiseValue(T&&) [with T = int]’ at /home/joe/programming/qtpromise/include/../src/qtpromise/qpromise_p.h:96:30,
    inlined from ‘void QtPromisePrivate::PromiseData<T>::resolve(V&&) [with V = int; T = int]’ at /home/joe/programming/qtpromise/include/../src/qtpromise/qpromise_p.h:511:17,
    inlined from ‘void QtPromisePrivate::PromiseResolver<T>::resolve(V&&) [with V = int; T = int]’ at /home/joe/programming/qtpromise/include/../src/qtpromise/qpromiseresolver.h:62:34:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:459:12: error: ‘<unnamed>.QtPromisePrivate::PromiseValue<int>::m_data.QSharedPointer<int>::d’ may be used uninitialized [-Werror=maybe-uninitialized]
  459 |     { deref(d); }
      |       ~~~~~^~~
In file included from /home/joe/programming/qtpromise/include/../src/qtpromise/qpromise.h:12,
                 from /home/joe/programming/qtpromise/include/QtPromise:11,
                 from /home/joe/programming/qtpromise/tests/auto/qtpromise/warnings/tst_warnings.cpp:9:
/home/joe/programming/qtpromise/include/../src/qtpromise/qpromise_p.h: In member function ‘void QtPromisePrivate::PromiseResolver<T>::resolve(V&&) [with V = int; T = int]’:
/home/joe/programming/qtpromise/include/../src/qtpromise/qpromise_p.h:511:17: note: ‘<anonymous>’ declared here
  511 |         m_value = PromiseValue<T>{std::forward<V>(value)};
      |         ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[103/111] Building CXX object tests/auto/qtpromise/qpromise/CMakeFiles/qtpromise.tests.auto.qpromise.reduce.dir/tst_reduce.cpp.o
ninja: build stopped: subcommand failed.

[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmaybe-uninitialized
[2] https://bugreports.qt.io/browse/QTBUG-14637
[3] https://bugreports.qt.io/browse/QTBUG-77641

Poldraunic
Poldraunic commented Jun 26, 2024
View reviewed changes
tests/auto/qtpromise/warnings/CMakeLists.txt Outdated
)

if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
target_compile_options(qtpromise.tests.auto.warnings
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to use add_compile_options but it didn't seem to work. For this test to work, we need to compile it with optimizations, however qtpromise_add_test function sets optimization level to -O0. What I don't like here is how I have to refer to a test by it's internal name.

@Poldraunic Poldraunic force-pushed the suppress-maybe-uninitialized-gcc-warning branch 5 times, most recently from 935dd56 to cd084f8 Compare June 27, 2024 15:37
@Poldraunic Poldraunic changed the title Suppress GCC warning maybe-uninitialized for PromiseValue Fix GCC warning maybe-uninitialized for PromiseValue<T> constructor Jun 27, 2024
@Poldraunic Poldraunic force-pushed the suppress-maybe-uninitialized-gcc-warning branch 2 times, most recently from 22ebe5e to 69b9543 Compare June 27, 2024 17:01
@Poldraunic Poldraunic force-pushed the suppress-maybe-uninitialized-gcc-warning branch from 69b9543 to 5402fe5 Compare July 17, 2024 09:16
@Poldraunic
Copy link
Contributor Author

@simonbrunel, hi! Any chance you could take a look please?

@simonbrunel
Copy link
Owner

Thanks @Poldraunic for this great PR and sorry for the wait!

Moving initialization to a constructor body magically fixes the issue

I'm always reluctant when things happen magically since it seems the underlying issue is not really solved, neither understood (both linked Qt bugs are now closed, quite old and without any resolution). I'm actually missing the information of which versions of Qt is impacted? (v5, v6 or both?)

I remember this PR from @pwuertz which I think fixes a similar warning by using std::shared_ptr instead of QSharedPointer. I'm not sure why at that time it has not been changed in QPromiseValue but maybe we should adopt the same approach? Any idea of the side effects of such change?

Thoughts?

@Poldraunic
Copy link
Contributor Author

@simonbrunel, hi! Thanks for the reply.

I am wary of magical fixes as well. For this one, however, issue seems to stem from compiler itself which is basically arcane knowledge. Qt bug reports were made against versions Qt 4 and Qt 5. Furthermore, I couldn't repro this (or similar) warning with Clang/MSVC compilers. That is why I decided explanation from Qt staff was good enough for me.

I am seeing this warning on Qt 5.15. I believe I haven't actually tried testing with Qt 6, but I did check the sources for QSharedPointer and there was no difference to class layout.

All in all, I am in agreement. I will compile this snippet with Qt 6 later this week and check if it has any difference. As for using std::shared_ptr: there should be no meaningful
behavioural differences. Qt classes, such as this one, are designed to be a drop-in replacement for STL. If that's okay with you and compilers do not emit warnings for std::shared_ptr, I will use it instead.

Thank you.

@simonbrunel
Copy link
Owner

If that's okay with you and compilers do not emit warnings for std::shared_ptr, I will use it instead

Yes, I think that's the way to go if that suppresses the warning and all tests still pass (I may need to have a look why Travis CI does not want to run tests anymore).

@Poldraunic Poldraunic force-pushed the suppress-maybe-uninitialized-gcc-warning branch 3 times, most recently from b68ba17 to f2bfb19 Compare August 6, 2024 20:17
@Poldraunic
Copy link
Contributor Author

@simonbrunel done! I have switched QSharedPointer to std::shared_ptr and encountered no issues.

@simonbrunel
Copy link
Owner

Thanks! I'm not sure I would keep the unit tests introduced in this PR. The reason is that the bug was in the Qt lib (QSharedPointer), which is not anymore used, thus having a regression test "against QSharedPointer" does not really make sense.

@Poldraunic
Copy link
Contributor Author

@simonbrune, I agree. Will remove later today.

@Poldraunic Poldraunic force-pushed the suppress-maybe-uninitialized-gcc-warning branch from f2bfb19 to bc1fada Compare August 9, 2024 08:32
@Poldraunic
Copy link
Contributor Author

@simonbrunel, done!

@simonbrunel
Copy link
Owner

Thanks @Poldraunic! I will merge it but first I would like to try to replace Travis CI by GitHub actions to run unit tests (Travis is no free anymore for open source projects).

@Poldraunic @simonbrunel
Use std::shared_ptr for PromiseValue<T>::m_data instead of QSharedPoi…
0bc9d4d 
…tner<T>

In optimized builds (such as -O2) GCC emits "maybe-uninitialized" warning for QSharedPointer<T>::d.  This happens only for optimized builds because that is when GCC tracks lifetimes[1]. In a project with lots of QtPromise uses this produces a heap of bogus warnings. This warning has been previously reported to a Qt team, but they're confident it is a compiler bug[2][3]. However, this is a second time and issue raised with QSharedPointer and warnings to it. The first time it was addressed here: simonbrunel#34.

[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmaybe-uninitialized
[2] https://bugreports.qt.io/browse/QTBUG-14637
[3] https://bugreports.qt.io/browse/QTBUG-77641
@simonbrunel simonbrunel force-pushed the suppress-maybe-uninitialized-gcc-warning branch from bc1fada to 0bc9d4d Compare August 24, 2024 06:43
@simonbrunel simonbrunel merged commit 1a905cb into simonbrunel:master Aug 24, 2024
6 checks passed
@simonbrunel
Copy link
Owner

@Poldraunic It's now merged, thanks for this really good PR! (and sorry for the long wait)

@simonbrunel simonbrunel added this to the Version 0.8 milestone Aug 24, 2024
@Poldraunic
Copy link
Contributor Author

Awesome! Thank you very much!

@Poldraunic Poldraunic deleted the suppress-maybe-uninitialized-gcc-warning branch August 24, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
Version 0.8
Development

Successfully merging this pull request may close these issues.
2 participants
@Poldraunic @simonbrunel