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

[theuni]

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]

after two hash.h VLAs fixed:
ACK e1cea61

[gmaxwell]

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]

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]

@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]

Sounds good to me.

[theuni]

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]

OK

[JeremyRubin]

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]

@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.

[theuni]

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]

utACK 205830a