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

build: add --enable-werror and warn on vla's #9789

Merged
merged 2 commits into from Feb 23, 2017

Conversation

@theuni
Copy link
Member

commented Feb 17, 2017

There are few compelling reasons to allow these.

Note that travis should fail with this as-is, because clang produces a few VLA's in hash.h. This can be merged once those are cleaned up.

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

after two hash.h VLAs fixed:
ACK e1cea61

@gmaxwell
Copy link
Contributor

left a comment

Please warn instead of error, the hash.h behavior showed that the ability to consistently give the same results here depends on how effective the optimizer is at turning things constant.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

For Travis this should be a hard error, to avoid VLAs (as detected by our compiler) from getting into the code base. For user builds I'm fine with making it just a warning.

@theuni

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2017

@laanwj I like the idea of having Travis yell about new warnings. Not just this one.

Many projects have an --enable-werror setting. How about we keep this as a warning, add a --enable-werror, and turn it on for travis (after cleaning up the straggler warnings we have)? We can always disable them case-by-case as they come up, and that keeps Werror from affecting regular user builds.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Sounds good to me.

@theuni theuni force-pushed the theuni:no-vla branch Feb 22, 2017

@theuni theuni changed the title build: disallow variable length arrays build: add --enable-werror and warn on vla's Feb 22, 2017

@theuni

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2017

Updated. Split into 2 commits in case we want to backport the vla warning.

We'll definitely have to do some housecleaning if we enable Werror wholesale for Travis, and I suspect that will cause some grumbles about changing code for the sake of shutting up the compiler.

So maybe instead we should begin to slowly add warnings we definitely want Travis to error on?

@shyii

This comment has been minimized.

Copy link

commented Feb 22, 2017

OK

@shyii

This comment has been minimized.

Copy link

commented on b602fe0 Feb 22, 2017

Ue

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

Cory and I talked a bit about this in IRC.

My 2¢:

I think that the usefulness of travis is somewhat finicky. If builds are failing for some large set of style reasons, then this is going to really slow down feature branch development, where this can be fixed up later but getting functional results are more important. Or people will place lower value on red-x's from Travis during development if errors are frequent. Furthermore, just building locally isn't really an option as I think I'm not in the minority that use Travis to test against whatever build platforms travis tests. Ideally, Travis would have multi-axis failures, where you can say code passes functional tests but not style tests. Code that passes functional tests could be ready for review from others, and in some cases, nudging the compiler in the right direction is non-obvious (as is the case with some of the VLA things)! This could be done by only running style checks on bitcoin/bitcoin.

One problem with just enabling style checks on bitcoin/bitcoin (and not on forks) is that it will be really frustrating if you pass travis on your fork, and then you PR master and then it fails due to style checks you didn't run yourself.

I think one way to do this is to add another (few?) travis build targets where strong-er flags are tested. That way, you can see easily if your build fail was from styler errors or from functional errors. It's not ideal in that a style failed build will still get an overall red-x, but at least one can see that a build fail was caused by style & not functionality without needing to muck around in the travis logs.

Another nice way to do this would be to add a style-check branch that has the style checks enabled, that one can PR-against on their own branch before PR'ing master, and only enable the style checks on any PR in bitcoin/bitcoin OR on PR to any branch named style-check.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

@JeremyRubin I agree in general but please, let's not get into that discussion here. This is not merely a style check! VLAs cause stack-protection to not work for that function, so effectively decrease the hardening of the code. I'm fine with making it the only warning that stops travis, but it should do that.

build: add --enable-werror option
This turns some compiler warnings into errors. Useful for c-i.

@theuni theuni force-pushed the theuni:no-vla branch to 205830a Feb 23, 2017

@theuni

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

Ok, I narrowed werror to only vla's for now. We can add to the list as we come up with more obvious errors.

As @JeremyRubin had a few concerns about how to actually hook this up to travis, I left that out here. We can do that as a next step.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

utACK 205830a

@laanwj laanwj merged commit 205830a into bitcoin:master Feb 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Feb 23, 2017
Merge #9789: build: add --enable-werror and warn on vla's
205830a build: add --enable-werror option (Cory Fields)
b602fe0 build: warn about variable length arrays (Cory Fields)
laanwj added a commit that referenced this pull request Feb 23, 2017
build: warn about variable length arrays
Github-Pull: #9789
Rebased-From: b602fe0
laanwj added a commit that referenced this pull request Feb 23, 2017
build: add --enable-werror option
This turns some compiler warnings into errors. Useful for c-i.

Github-Pull: #9789
Rebased-From: 205830a
@str4d str4d referenced this pull request Dec 1, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Dec 1, 2017
Auto merge of #2786 - str4d:2074-build, r=<try>
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 1, 2017
Auto merge of #2786 - str4d:2074-build, r=<try>
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 15, 2017
Auto merge of #2786 - str4d:2074-build, r=str4d
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
kotodev added a commit to koto-dev/koto.old that referenced this pull request Jan 25, 2018
Auto merge of #2786 - str4d:2074-build, r=str4d
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.
codablock added a commit to codablock/dash that referenced this pull request Jan 26, 2018
Merge bitcoin#9789: build: add --enable-werror and warn on vla's
205830a build: add --enable-werror option (Cory Fields)
b602fe0 build: warn about variable length arrays (Cory Fields)
renium9 added a commit to koto-dev/koto.old that referenced this pull request Feb 6, 2018
Auto merge of #2786 - str4d:2074-build, r=str4d
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

# Conflicts:
#	configure.ac
#	src/Makefile.am
#	src/Makefile.gtest.include
#	src/Makefile.test.include
#	zcutil/build.sh
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#9789: build: add --enable-werror and warn on vla's
205830a build: add --enable-werror option (Cory Fields)
b602fe0 build: warn about variable length arrays (Cory Fields)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
Merge bitcoin#9789: build: add --enable-werror and warn on vla's
205830a build: add --enable-werror option (Cory Fields)
b602fe0 build: warn about variable length arrays (Cory Fields)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.