Use a stack size slightly smaller than 1MB #935
Conversation
This is motivated by discussions in #924 The stack size on Android and iOS is 1MB, so we want to detect any possible stack overflow issues there while we are testing our software on the desktop. The limit is 1 million bytes instead of exactly 1MB, so we can be on the safe side.
config.nims
Outdated
| @@ -7,7 +7,7 @@ if defined(windows): | |||
| # disable timestamps in Windows PE headers - https://wiki.debian.org/ReproducibleBuilds/TimestampsInPEBinaries | |||
| switch("passL", "-Wl,--no-insert-timestamp") | |||
| # increase stack size | |||
| switch("passL", "-Wl,--stack,8388608") | |||
| switch("passL", "-Wl,--stack,1000000") | |||
There was a problem hiding this comment.
This is only on Windows. For POSIX systems see https://linux.die.net/man/2/setrlimit (with RLIMIT_STACK).
You can set the stack size as a define in "config.nims", so we don't duplicate that value.
There was a problem hiding this comment.
And it's only on Windows that CI's now failing -- coincidence, or?
- make the stack size limit a Nim define and use it in "config.nims" for the Windows C compiler flag and in "beacon_node.nim" for the POSIX runtime setting. - warn when the open file limit is bellow 1024 (macOS, by default) - increase the macOS open file limit in Travis - add "-Wstack-usage=..." C compiler flag. No GCC warnings visible at the stack limit we set, but any would be visible by defining "cwarnings" in Nim.
Sure, with |
|
I'm not sure I understand. Why wouldn't a single function trigger this warning if a single Ideally, we want this warning to be enabled in all builds, it shouldn't be opt-in. If it can be an error, that's even better. |
|
some stack sizes for commonly used stuff (mainnet, 0.11.1): |
You're right, it would.
I can do both, but I have to enable GCC warnings for that and I can't make them any less verbose than this: https://gist.githubusercontent.com/stefantalpalaru/527d234bf64e454c2fc4a91aa3dbe464/raw/708fc6f5de4b34696c2aea74d08d5dda4960b440/gcc_warnings.txt (not without changing the standard library to add |
|
Maybe some filtering of the output with grep would solve the problem, but I'm worried from the fact that the Nim compiler used to hang when the C compiler output becomes too large (due to the many warnings). Seems to have been fixed here: |
|
Grepping seems hard actually, but why is it not possible to disable all GCC warnings except the ones of interest? |
We don't have that option: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html I can either disable all warnings (including warnings as errors) which Nim does by default with |
- also make the stack-usage warning an error - only works with `-d:cwarnings`
|
The Windows failure is caused, indeed, by stack overflow, @tersec: I don't know how reliable GDB's backtrace is, when frames 8 and 9 have clearly wrong file/line info. |
|
New Linux segfault: |
|
Good hint from beacon_chain/spec/datatypes.nim:407 HistoricalBatch* = object
block_roots* : array[SLOTS_PER_HISTORICAL_ROOT, Eth2Digest]
state_roots* : array[SLOTS_PER_HISTORICAL_ROOT, Eth2Digest] |
|
Mostly implemented in other PRs, except the runtime stack limiting. We might want to pick that one too, in the future. |
arnetheduck
force-pushed
the
unstable
branch
2 times, most recently
from
657f9d5 to
a4667d1
Compare
|
closing as obsolete - compile-time filtering is there and introducing runtime testing would require a fresh PR / approach anyway as most things in this PR have changed since |
This is motivated by discussions in #924
The stack size on Android and iOS is 1MB, so we want to detect
any possible stack overflow issues there while we are testing
our software on the desktop. The limit is 1 million bytes instead
of exactly 1MB, so we can be on the safe side.