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

Remove 'boost::optional'-related false positive -Wmaybe-uninitialized warnings on GCC compiler #15292

Merged
merged 1 commit into from Jan 30, 2019

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Jan 30, 2019

#14711 introduced some warnings when building with gcc compiler.

See:

This gcc issue has been known since version 4.6.0 and last updated in 2017.
From the boost docs:

The default constructor of optional creates an uninitialized optional object.

Also: False positive with -Wmaybe-uninitialized (pointed out by @Empact)

This PR removes these warnings.

cc: @Empact @practicalswift

@hebasto hebasto force-pushed the 20190130-gcc-maybeuninitialized branch from a1d9961 to 595b62f Jan 30, 2019
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jan 30, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jan 30, 2019

Generally we don't work around bugs in a compiler if they are only warning related and fixed in recent versions.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 30, 2019

@MarcoFalke

Generally we don't work around bugs in a compiler if they are only warning related and fixed in recent versions.

Which recent version of gcc do you mean?

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jan 30, 2019

According to the bug report, it was fixed in gcc6. I am using clang7.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jan 30, 2019

I would prefer not to make these changes since I think they make the code less readable, are only applicable to older buggy versions of gcc, and just avoid warnings not errors.

But I will add my utACK 595b62f since the changes do look safe, and other people might want them.

I wonder what would happen if you tried to replace:

Optional<int> height;

with:

Optional<int> height = nullopt;

I think a change like that would be more readable.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 30, 2019

@MarcoFalke

According to the bug report, it was fixed in gcc6. I am using clang7.

I am using gcc 7.3.0 (both Linux Mint 19.1 and Ubuntu Bionic). Warnings are still present.

@ryanofsky

Optional<int> height = nullopt;

I've tried this approach while working on this PR. nullopt did not help. Perhaps this is due to the lazy initialization inside boost::optional.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jan 30, 2019

gcc8 will also spit out a false positive warning, since it can not infer that either none or both of altheight and height are initialized. Not sure if this should be fixed or how.

@Empact
Copy link
Member

@Empact Empact commented Jan 30, 2019

I think "The default constructor of optional creates an uninitialized optional object." may be misleading as default-constructed values are valid, e.g.. boost::optional has an is_initialized member that is false on default initialization, and just means the object is falsey.

Other thoughts:
I would prefer not to use auto here given there are plenty of numeric types.
I would access this functionality via src/optional.h, where the other boost::optional references are.

Incidentally here's the doc where boost recommends this fix.
No opinion on whether this should be fixed.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 30, 2019

@Empact TIL. Thank you.

@hebasto hebasto force-pushed the 20190130-gcc-maybeuninitialized branch from 595b62f to c14d580 Jan 30, 2019
@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 30, 2019

@Empact your comment has been addressed.

@hebasto hebasto changed the title Remove 'boost::optional'-related gcc warnings Remove 'boost::optional'-related false positive -Wmaybe-uninitialized warnings on GCC compiler Jan 30, 2019
@laanwj
Copy link
Member

@laanwj laanwj commented Jan 30, 2019

I'm using gcc 7.3.0 on my main development system so I see these scary warnings all the time, so I'd prefer for this to be merged or the relevant warning disabled (for that gcc).

Edit: tested that this makes the warnings disappear here.

@@ -41,6 +41,8 @@

#include <functional>

#include <boost/optional.hpp>
Copy link
Member

@laanwj laanwj Jan 30, 2019

I'd mildly prefer to redirect this through our optional.h, define our own make_optional like the Option<T> itself instead of boost appearing here direclty.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Jan 30, 2019

Found this PR after seeing the scary warnings... maybe-uninitialized is a pretty potent warning, I wouldn't want to disable it unless it was only on old compilers.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jan 30, 2019

Hmm, one of the warnings disappears when going from gcc7 to gcc8?

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 30, 2019

utACK c14d580 modulo @laanwj's nit regarding optional.h

@hebasto hebasto force-pushed the 20190130-gcc-maybeuninitialized branch from c14d580 to 2d48314 Jan 30, 2019
@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 30, 2019

@laanwj's nit has been addressed.

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 30, 2019

utACK 2d48314

1 similar comment
@laanwj
Copy link
Member

@laanwj laanwj commented Jan 30, 2019

utACK 2d48314

@Empact
Copy link
Member

@Empact Empact commented Jan 30, 2019

utACK 2d48314

@laanwj laanwj merged commit 2d48314 into bitcoin:master Jan 30, 2019
1 check passed
laanwj added a commit that referenced this issue Jan 30, 2019
…-uninitialized warnings on GCC compiler

2d48314 Remove 'boost::optional'-related gcc warnings (Hennadii Stepanov)

Pull request description:

  #14711 introduced some warnings when building with gcc compiler.

  See:
  - #14711 (comment) by @laanwj
  - #14711 (review) by @ryanofsky

  This gcc [issue](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47679) has been known since version 4.6.0 and last updated in 2017.
  From the boost [docs](https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/quick_start/optional_automatic_variables.html):
  > The default constructor of `optional` creates an _uninitialized_ `optional` object.

  Also: [False positive with -Wmaybe-uninitialized](https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html) ([pointed out](#15292 (comment)) by @Empact)

  This PR removes these warnings.

  cc: @Empact @practicalswift

Tree-SHA512: 752ae3c3ca6282bbf98726236fbc3069ab9d1aee57ae2ec2668b32e4541e7bc1acb15b7d6fa9e2b6daf1ec29c0987a1053ee1ca0f523b71367ff911221c58c94
@hebasto hebasto deleted the 20190130-gcc-maybeuninitialized branch Jan 30, 2019
laanwj added a commit that referenced this issue Feb 5, 2020
e9434ee Remove false positive GCC warning (Hennadii Stepanov)

Pull request description:

  On master (f05c1ac) GCC compiler fires a false positive `-Wmaybe-uninitialized`:

  ```
  wallet/wallet.cpp: In static member function ‘static std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)’:
  wallet/wallet.cpp:3913:27: warning: ‘*((void*)& time_first_key +8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
           Optional<int64_t> time_first_key;
                             ^~~~~~~~~~~~~~
  ```

  The same as #15292.

  This PR leverages a workaround and removes the warning.

ACKs for top commit:
  laanwj:
    ACK e9434ee, removes the warning for me (gcc 7.4.0)
  kristapsk:
    ACK e9434ee

Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
sidhujag added a commit to syscoin/syscoin that referenced this issue Feb 9, 2020
e9434ee Remove false positive GCC warning (Hennadii Stepanov)

Pull request description:

  On master (f05c1ac) GCC compiler fires a false positive `-Wmaybe-uninitialized`:

  ```
  wallet/wallet.cpp: In static member function ‘static std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)’:
  wallet/wallet.cpp:3913:27: warning: ‘*((void*)& time_first_key +8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
           Optional<int64_t> time_first_key;
                             ^~~~~~~~~~~~~~
  ```

  The same as bitcoin#15292.

  This PR leverages a workaround and removes the warning.

ACKs for top commit:
  laanwj:
    ACK e9434ee, removes the warning for me (gcc 7.4.0)
  kristapsk:
    ACK e9434ee

Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Apr 11, 2020
Summary: Backport of core [[bitcoin/bitcoin#15292 | PR15292]].

Test Plan:
With GCC:
  ninja check
Make sure the warnings are gone (tested with GCC 7.5).

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D5696
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this issue Oct 16, 2020
Summary
---

While compiling on Gitian using GCC 7 (and locally using GCC 8) we have
observed that there are various warnings related to uninitialized values
being used -- these warnings are false positives.  After auditing the
code it is clear to me that they are not real uninitialized usages.
GCC's static checker gets confused by the rather complex code that ends
up unitializing these values.

This commit fixes that issue.

Backport status
---

This is a backport of core #15292:
- bitcoin/bitcoin#15292

Original commit message:
    Remove 'boost::optional'-related gcc warnings

Test Plan
---

- `ninja all check-all`

No behavioral or other changes were introduced.  This is a purely
cosmetic warning-suppression commit.  Even the "uninitialized" values
are now "initialized" to the identical default thing with this commit.
sidhujag added a commit to syscoin-core/syscoin that referenced this issue Nov 10, 2020
e9434ee Remove false positive GCC warning (Hennadii Stepanov)

Pull request description:

  On master (f05c1ac) GCC compiler fires a false positive `-Wmaybe-uninitialized`:

  ```
  wallet/wallet.cpp: In static member function ‘static std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)’:
  wallet/wallet.cpp:3913:27: warning: ‘*((void*)& time_first_key +8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
           Optional<int64_t> time_first_key;
                             ^~~~~~~~~~~~~~
  ```

  The same as bitcoin#15292.

  This PR leverages a workaround and removes the warning.

ACKs for top commit:
  laanwj:
    ACK e9434ee, removes the warning for me (gcc 7.4.0)
  kristapsk:
    ACK e9434ee

Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants