build: ensure we aren't using GNU extensions #18088

fanquake · 2020-02-07T04:39:05Z

Since we started using the ax_cxx_compile_stdcxx.m4 macro we've been passing [noext] to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn off extension support so they would fail to compile".

However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former.

anonymous structs

./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct]
        struct {

This is fixed in b849212.

variadic macros

./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
            ::Unserialize(s, VARINT(nVersionDummy));

This is taken care of in #18087.

The LOG_TIME_* macros introduced in #16805 make use of a GNU extension.

In file included from validation.cpp:22:
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
                                                                                                  ^
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
                                                                                           ^
6 warnings generated.

This is fixed in 081a0ab64eb442bc85c4d4a4d3bc2c8e97ac2a6d and 612e8e138b97fc5ad2f38847300132a8fc423c3f.

prevention

To ensure that usage doesn't creep back in we can add -Wgnu to our compile time flags, which will make Clang warn whenever it encounters GNU extensions.

This would close #14130.
Also related to #17230, where it's suggested we use a GNU extension, the gnu::pure attribute.

DrahtBot · 2020-02-07T09:42:54Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj · 2020-02-07T12:21:10Z

Concept ACK. If we can do without compiler-specific extensions we should.

Empact · 2020-02-07T15:29:31Z

Concept ACK 👍

Empact · 2020-02-07T16:38:47Z

Here's a set of changes to remove the Wgnu-zero-variadic-macro-arguments cases:
fanquake/bitcoin@no_gnu_extensions...Empact:2020-02-no_gnu_extensions

fanquake · 2020-02-10T12:11:48Z

Here's a set of changes

Thanks, I've taken some of those changes.

theuni · 2020-02-10T19:52:34Z

Concept ACK, this was for sure the intent of [noext].

practicalswift · 2020-02-11T06:43:29Z

Concept ACK -- inside the standard is better than outside the standard

fanquake · 2020-02-11T08:07:39Z

Rebased on master now that #18087 is in, fixed up the sanitizer issue in prevector (bad rebase) and re-ordered some commits.

practicalswift · 2020-02-11T12:50:24Z

ACK 6af7c41771c2b3adeffe81ce06a70e0dbbef5360 -- diff looks correct

src/logging.h

elichai · 2020-03-01T13:21:54Z

Isn't __COUNTER__ a GNU extension too?
https://renenyffenegger.ch/notes/development/languages/C-C-plus-plus/preprocessor/macros/predefined/__COUNTER__
https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

saw it here again: 3cb2664#diff-998357bc40c89d644815a6fd52c99c9fR213

We can probably replace all usages of __COUNTER__ with a combination of __FILE__ and __LINE__

These are a gnu extension warned against by: gnu-zero-variadic-macro-arguments

When compiling with Clang, this will warn when GNU extensions are used. Info: https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu

fanquake · 2020-04-30T10:04:10Z

Rebased after #18591.

vasild · 2020-05-04T06:00:11Z

utACK 0ae8f18

If we currently do not see any warnings due to the newly introduced -Wgnu then I would suggest to also add -Werror=gnu (if compiling with --enable-werror). So that a newly introduced warning in the future does not sneak - printed, but unnoticed in CI logs.

dongcarl · 2020-05-04T17:20:02Z

ACK 0ae8f18

Read relevant gcc docs, built, and ran tests.

practicalswift · 2020-05-04T18:24:06Z

ACK 0ae8f18 -- diff looks correct

Agree with @vasild about -Werror=gnu as part of --enable-werror. May also be done in a follow-up.

maflcko · 2020-05-04T22:53:47Z

ACK 0ae8f18

0ae8f18 build: add -Wgnu to compile flags (fanquake) 3a0fd77 Remove use of non-standard zero variadic macros (Ben Woosley) 49f6178 Drop unused LOG_TIME_MICROS helper (Ben Woosley) 5d49999 prevector: Avoid unnamed struct, which is a GNU extension (DesWurstes) Pull request description: Since we [started using](bitcoin#7165) the `ax_cxx_compile_stdcxx.m4` macro we've been passing `[noext]` to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn _off_ extension support so they would fail to compile". However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former. #### anonymous structs ```bash ./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct] struct { ``` This is fixed in bitcoin@b849212. #### variadic macros ```bash ./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments] ::Unserialize(s, VARINT(nVersionDummy)); ``` This is taken care of in bitcoin#18087. The `LOG_TIME_*` macros introduced in bitcoin#16805 make use of a [GNU extension](https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html). ```bash In file included from validation.cpp:22: ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__) ^ ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] ./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] ./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__) ^ 6 warnings generated. ``` This is fixed in 081a0ab64eb442bc85c4d4a4d3bc2c8e97ac2a6d and 612e8e138b97fc5ad2f38847300132a8fc423c3f. #### prevention To ensure that usage doesn't creep back in we can add [`-Wgnu`](https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu) to our compile time flags, which will make Clang warn whenever it encounters GNU extensions. This would close bitcoin#14130. Also related to bitcoin#17230, where it's suggested we use a GNU extension, the `gnu::pure` attribute. ACKs for top commit: practicalswift: ACK 0ae8f18 -- diff looks correct MarcoFalke: ACK 0ae8f18 vasild: utACK 0ae8f18 dongcarl: ACK 0ae8f18 Tree-SHA512: c517404681ef8edf04c785731d26105bac9f3c9c958605aa24cbe399c649e7c5ee0c4aa8e714fd2b2d335e2fbea4d571e09b0dec36678ef871f0a6683ba6bb7f

practicalswift · 2020-05-05T09:58:13Z

@vasild Would you mind taking on the -Werror=gnu thing? :)

Stop the build if a warning is emitted due to `-Wgnu` and `--enable-werror` has been used. As usual - this would help notice such a warning that is about to be introduced in new code. This is a followup to bitcoin#18088 build: ensure we aren't using GNU extensions

vasild · 2020-05-05T12:52:45Z

@vasild Would you mind taking on the -Werror=gnu thing? :)

Here it is: #18887 build: enable -Werror=gnu

a30b0a2 build: enable -Werror=gnu (Vasil Dimov) Pull request description: Stop the build if a warning is emitted due to `-Wgnu` and `--enable-werror` has been used. As usual - this would help notice such a warning that is about to be introduced in new code. This is a followup to #18088 build: ensure we aren't using GNU extensions ACKs for top commit: practicalswift: ACK a30b0a2 Empact: ACK a30b0a2 Tree-SHA512: f81b71cf3ee4db88b6f664c571075e0d30800a604f067f44273f256695a1dea533779db2ac859dd0a4cd8b66289c3e45f4aff1cfadfa160a1c354237167b05e2

a30b0a2 build: enable -Werror=gnu (Vasil Dimov) Pull request description: Stop the build if a warning is emitted due to `-Wgnu` and `--enable-werror` has been used. As usual - this would help notice such a warning that is about to be introduced in new code. This is a followup to bitcoin#18088 build: ensure we aren't using GNU extensions ACKs for top commit: practicalswift: ACK a30b0a2 Empact: ACK bitcoin@a30b0a2 Tree-SHA512: f81b71cf3ee4db88b6f664c571075e0d30800a604f067f44273f256695a1dea533779db2ac859dd0a4cd8b66289c3e45f4aff1cfadfa160a1c354237167b05e2

Stop the build if a warning is emitted due to `-Wgnu` and `--enable-werror` has been used. As usual - this would help notice such a warning that is about to be introduced in new code. This is a followup to bitcoin/bitcoin#18088 build: ensure we aren't using GNU extensions

Summary: Remove the use of gnu extensions: - anonymous structs - variadic macros with no argument And add a Clang warning for detecting their future use. Backport of core [[bitcoin/bitcoin#18088 | PR18088]]. Test Plan: With clang: ninja all check Check there is no warning. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9109

a30b0a2 build: enable -Werror=gnu (Vasil Dimov) Pull request description: Stop the build if a warning is emitted due to `-Wgnu` and `--enable-werror` has been used. As usual - this would help notice such a warning that is about to be introduced in new code. This is a followup to bitcoin#18088 build: ensure we aren't using GNU extensions ACKs for top commit: practicalswift: ACK a30b0a2 Empact: ACK bitcoin@a30b0a2 Tree-SHA512: f81b71cf3ee4db88b6f664c571075e0d30800a604f067f44273f256695a1dea533779db2ac859dd0a4cd8b66289c3e45f4aff1cfadfa160a1c354237167b05e2

fanquake added Brainstorming Build system Needs gitian build labels Feb 7, 2020

fanquake requested review from jamesob, sipa and theuni February 7, 2020 04:39

fanquake mentioned this pull request Feb 7, 2020

Get rid of VARINT default argument #18087

Merged

DrahtBot removed the Needs gitian build label Feb 8, 2020

fanquake force-pushed the no_gnu_extensions branch from 4fa56b2 to 612e8e1 Compare February 10, 2020 12:10

bitcoin deleted a comment from DrahtBot Feb 10, 2020

fanquake force-pushed the no_gnu_extensions branch from 612e8e1 to 6af7c41 Compare February 11, 2020 08:06

fanquake marked this pull request as ready for review February 11, 2020 08:06

maflcko reviewed Feb 11, 2020

View reviewed changes

src/logging.h Outdated Show resolved Hide resolved

This was referenced Feb 11, 2020

build: Enable Clang's -Wconditional-uninitialized to warn on potential uninitialized reads #17895

Closed

prevector: avoid misaligned member accesses #17708

Merged

DrahtBot added the Needs rebase label Feb 12, 2020

fanquake force-pushed the no_gnu_extensions branch from 6af7c41 to c9ab71a Compare February 12, 2020 23:40

DrahtBot removed the Needs rebase label Feb 13, 2020

elichai mentioned this pull request Mar 1, 2020

refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler #18234

Merged

fanquake force-pushed the no_gnu_extensions branch from c9ab71a to 1f6f29e Compare March 20, 2020 04:07

Empact and others added 2 commits April 30, 2020 18:02

Remove use of non-standard zero variadic macros

3a0fd77

These are a gnu extension warned against by: gnu-zero-variadic-macro-arguments

build: add -Wgnu to compile flags

0ae8f18

When compiling with Clang, this will warn when GNU extensions are used. Info: https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu

fanquake force-pushed the no_gnu_extensions branch from 05fb3da to 0ae8f18 Compare April 30, 2020 10:03

This was referenced May 1, 2020

build: warn on potentially uninitialized reads #18843

Merged

build: avoid repetitions when enabling warnings in configure.ac #18857

Closed

fanquake merged commit e727c2b into bitcoin:master May 4, 2020

fanquake deleted the no_gnu_extensions branch May 4, 2020 23:45

vasild mentioned this pull request May 5, 2020

build: enable -Werror=gnu #18887

Merged

hebasto mentioned this pull request May 31, 2020

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

build: ensure we aren't using GNU extensions #18088

build: ensure we aren't using GNU extensions #18088

Uh oh!

Conversation

fanquake commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

anonymous structs

variadic macros

prevention

Uh oh!

DrahtBot commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Feb 7, 2020

Uh oh!

Empact commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Feb 10, 2020

Uh oh!

theuni commented Feb 10, 2020

Uh oh!

practicalswift commented Feb 11, 2020

Uh oh!

fanquake commented Feb 11, 2020

Uh oh!

practicalswift commented Feb 11, 2020

Uh oh!

Uh oh!

elichai commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 30, 2020

Uh oh!

vasild commented May 4, 2020

Uh oh!

dongcarl commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 4, 2020

Uh oh!

maflcko commented May 4, 2020

Uh oh!

practicalswift commented May 5, 2020

Uh oh!

vasild commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

fanquake commented Feb 7, 2020 •

edited

Loading

DrahtBot commented Feb 7, 2020 •

edited

Loading

laanwj commented Feb 7, 2020 •

edited

Loading

Empact commented Feb 7, 2020 •

edited

Loading

elichai commented Mar 1, 2020 •

edited

Loading

dongcarl commented May 4, 2020 •

edited

Loading