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

EVM: Different segmentation faults when running the test suite with EVMC #575

Closed
jlokier opened this issue Mar 31, 2021 · 7 comments · Fixed by #598, #586 or #588
Closed

EVM: Different segmentation faults when running the test suite with EVMC #575

jlokier opened this issue Mar 31, 2021 · 7 comments · Fixed by #598, #586 or #588

Comments

@jlokier
Copy link
Contributor

jlokier commented Mar 31, 2021

@mjfh reported that make test ENABLE_EVMC=1 fails. From #573 (comment) @mjfh:

  • compiling as make test ENABLE_EVMC=1 crashes at test no. 7 with

     [..]
     [OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/returndatacopy_afterFailing_create.json
     Segmentation fault
     subtest no: 7 failed: testgeneralstatejson
    
     [Suite] tracer json tests
       [OK] tests/fixtures/TracerTests/block48915.json
       [OK] tests/fixtures/TracerTests/block46147.json
     [..]
    

    and can be reproduced as ./build/all_test 7, and terminates at test no. 24 as

     [...]
     [OK] 30_keys_0_storage.json
    
     [Suite] Misc test suite
       [OK] EthAddress to int
     stack trace: (most recent call last)
     /status/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/nimscript.nim(416, 18)
     /status/nimbus-eth1/nimbus.nimble(37, 8) testTask
     /status/nimbus-eth1/nimbus.nimble(34, 8) test
     /status/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/nimscript.nim(260, 7) exec
     /status/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/nimscript.nim(260, 7) Error: unhandled exception: FAILED: build/all_tests [OSError]
     make: *** [Makefile:112: test] Error 1
    

    and this one will not be reproduced running ./build/all_test 24

Turns out it fails on different tests for different people, but not the targets in CI. From #573 (comment) @jlokier:

  • No. 7 is fine for me on the master branch.
    However, no. 9 fails on master on Ubuntu Server 19.10 64-bit x86 with a SIGSEGV.
    On MacOS, all the tests pass fine.
    Reverting to before the last big submodule bump didn't change the result, so it's been around a while.

    The final error message is a rather cryptic Error: unhandled exception: FAILED: build/all_tests [OSError], for which the cause is a SIGSEGV a few thousand lines earlier. I found it using strace -f.

    I wondered why it's not getting caught in CI, and saw we don't have a Linux 64-bit EVMC CI setup. Just Mac 64-bit (which works) and Linux 32-bit (which also works apparently).

@jlokier
Copy link
Contributor Author

jlokier commented Mar 31, 2021

The "no. 7" segfault occurs in "new generalstate json tests":

./build/all_tests 7
[Suite] new generalstate json tests
  [OK] tests/fixtures/eth_tests/GeneralStateTests/stSubroutine/simpleSubroutine.json
  [OK] tests/fixtures/eth_tests/GeneralStateTests/stSubroutine/subroutineAtEndOfCode.json
...
  [OK] tests/fixtures/eth_tests/GeneralStateTests/stReturnDataTest/returndatacopy_after_failing_delegatecall.json
  [OK] tests/fixtures/eth_tests/GeneralStateTests/stReturnDataTest/returndatacopy_afterFailing_create.json
  [OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/returndatacopy_afterFailing_create.json
Segmentation fault

@mjfh, can you say what build environment this is occurring in, so we can reproduce it?

@jlokier
Copy link
Contributor Author

jlokier commented Mar 31, 2021

The "no. 9" segfault occurs in "persist block json tests":

./build/all_tests 9
[Suite] persist block json tests
  [OK] tests/fixtures/PersistBlockTests/block2463413.json
  [OK] tests/fixtures/PersistBlockTests/block1927662.json
  [OK] tests/fixtures/PersistBlockTests/block248032.json
  [OK] tests/fixtures/PersistBlockTests/block47205.json
  [OK] tests/fixtures/PersistBlockTests/block46400.json
  [OK] tests/fixtures/PersistBlockTests/block46402.json
Segmentation fault

This occurs consistently on a 64-bit x86 Ubuntu Server 19.10 system, using make test EVMC_HOST=1. On this target the "no. 7" segfault does not occur.

Confirmed with versions from b6ad47f (when EVMC_HOST was added) to cb957b7 (current master).

@jlokier jlokier added the tests label Mar 31, 2021
@jangko
Copy link
Contributor

jangko commented Apr 1, 2021

saw we don't have a Linux 64-bit EVMC CI setup.

It has been commented out together with windows 64 bit EVMC CI test. Both linux 64bit and windows 64 bit test crash for different reason.

I have two primary suspect:

  1. EVMC proc signature, we(or me) abuse the Nim to C FFI to avoid datatypes conversion. Usually 'Nim int' vs 'C int' mismatch can easily crash any program.
  2. each EVM recursion will return data to it's parent/caller. and at this point the EVMC to nimbus datatypes conversion happen. Probably I have do something stupid with the conversion because it is using pointer trick iirc.

But I cannot reproduce it on my Windows 10, 64 bit. I can reproduce it in virtualbox using linux 64 bit.

@mjfh
Copy link
Contributor

mjfh commented Apr 1, 2021

the command

 ulimit -s 20000

does the job on my machine, tests run ok now. my original setting on deb/x64 was 8192, so this might explain the difference to the ubuntu machine. In the gdb i saw it crashing at a stack nesting level of more than 6k.

@jangko
Copy link
Contributor

jangko commented Apr 1, 2021

that is a bad sign if we consume more stack than other programs do. too many layer calls. and it is not good at all if we have to tell our user, please set your system stack limit higher.
other eth client run the same tests, and they are not running out of stack.

I remember evmone use some kind of continuation call to reduce this layers.

@jlokier
Copy link
Contributor Author

jlokier commented Apr 6, 2021

If it's just excessive stack use, that's ok, as we are intending to refactor out the deep recursion for many good reasons. There's no need to worry about this error if that's all it is, let's just fix the recursion. Maybe adjust the test environment temporarily to have a larger stack so other CI can continue.

@jlokier jlokier removed the tests label Apr 26, 2021
jlokier added a commit that referenced this issue Apr 26, 2021
This patch reduces stack space used with EVM in ENABLE_EVMC=1 mode, from 13 MB
worst case to 550 kB, a 24x reduction which fixes the "stack problem".

This completes fixing the "stack problem" and closes #575 (`EVM: Different
segmentation faults when running the test suite with EVMC`).

After this patch, it is possible to re-enable the CI targets which had to be
disabled due to #575.

This change is a required precursor for switching over to "nearly EVMC" as the
clean and focused Nimbus-internal API between EVM and sync/database processes,
and is also key to the use of Chronos `async` in those processes when calling
the EVM.

(The motivation is the internal interface has to be substantially changed
_anyway_ for the parallel sync and database processes, and EVMC turns out to be
well-designed and well-suited for this.  It provides good separation between
modules, and suits our needs better than our other current interface.  Might as
well use a good one designed by someone else.  EVMC is 98% done in Nimbus
thanks to great work done before by @jangko, and we can use Nimbus-specific
extensions where we need flexibility, including for performance.  Being aligned
with the ecosystem is a useful bonus feature.)

All tests below were run on Ubuntu 20.04 LTS server, x86-64.  This matches one
of the targets that has been disabled for a while in CI in EVMC mode due to
stack overflow crashing the tests, so it's a good choice.

Measurements before
===================

Testing commit `e76e0144 2021-04-22 11:29:42 +0700 add submodules: graphql and
toml-serialization`.

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 16384 # Requires larger stack than default to avoid crash.
    $ ./build/all_tests 9 | tee tlog
    [Suite] persist block json tests
    ...
    Stack range 38416 depthHigh 3
    ...
    Stack range 13074720 depthHigh 1024
    [OK] tests/fixtures/PersistBlockTests/block1431916.json

These tests use 13.07 MB of stack to run, and so crash with the default stack
limit on Ubuntu Server 20.04 (8MB).  Exactly 12768 bytes per EVM call stack
frame.

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 16384 # Requires larger stack than default.
    $ ./build/all_tests 7 | tee tlog
    [Suite] new generalstate json tests
        ...
    Stack range 14384 depthHigh 2
        ...
    Stack range 3495456 depthHigh 457
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
    ...
    Stack range 3709600 depthHigh 485
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
        ...
    Stack range 7831600 depthHigh 1024
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json

These tests use 7.83MB of stack to run.  About 7648 bytes per EVM call stack
frame.  It _only just_ avoids crashing with the default Ubuntu Server stack
limit of 8 MB.  However, it still crashes on Windows x86-64, which is why the
Windows CI EVMC target is currently disabled.

On Linux where this passes, this is so borderline that it affects work and
testing of the complex storage code, because that's called from the EVM.

Also, this greatly exceeds the default thread stack size.

Measurements after
==================

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 600 # Because we can!  600k stack.
    $ ./build/all_tests 9 | tee tlog
    [Suite] persist block json tests
    ...
    Stack range 1936 depthHigh 3
    ...
        Stack range 556272 depthHigh 1022
        Stack range 556512 depthHigh 1023
        Stack range 556816 depthHigh 1023
        Stack range 557056 depthHigh 1024
        Stack range 557360 depthHigh 1024
        [OK] tests/fixtures/PersistBlockTests/block1431916.json

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 600 # Because we can!  600k stack.
    $ ./build/all_tests 7 | tee tlog
    [Suite] new generalstate json tests
        ...
    Stack range 1392 depthHigh 2
        ...
    Stack range 248912 depthHigh 457
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
    ...
    Stack range 264144 depthHigh 485
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
        ...
        Stack range 557360 depthHigh 1024
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stStaticCall/static_CallRecursiveBombPreCall.json

For both tests, a satisfying *544 bytes* per EVM call stack frame, and EVM
takes less than 600 kB total.  With other overheads, both tests run in 600 kB
stack total at maximum EVM depth.

We must add some headroom on this for database activity called from the EVM,
and different compile targets.  But it means the EVM itself is no longer a
stack burden.

This is much smaller than the default thread stack size on Linux (2MB), with
plenty of margin.  (Just fyi, it isn't smaller than a _small_ thread stack on
Linux from a long time ago (128kB), and some small embedded C targets.)

This size is well suited to running EVMs in threads.

Further reduction
=================

This patch solves the stack problem.  Windows and Linux 64-bit EVMC CI targets
can be re-enabled, and there is no longer a problem with stack usage.

We can reduce further, to ~340 bytes per frame and 350 kB total, while still
complying with the EVMC spec.  But as this involves changing how errors are
handled to comply fully with EVMC, and removing `dispose` calls.  It doesn't
seem worth doing that now there are other EVMC changes in progress, though, as
the real stack problem is now solved.

A Nimbus-specific extension will allow us to avoid recursion with EVMC anyway,
bringing bytes per frame to zero.  We want this anyway, to support Chronos
`async` which parallel transaction processing is built around.

Interop with non-Nimbus over EVMC won't let us avoid recursion, but then we
can't control the stack frame size either.  To avoid stack overflow in interop
I anticipate using the method in (Aleth)
[https://github.com/ethereum/aleth/blob/6e96ce34e3f131e2d42f3cb00741b54e05ab029d/libethereum/ExtVM.cpp#L61].

Smoke check other versions of GCC and Clang/LLVM
================================================

As all builds including Windows use GCC or Apple's Clang/LLVM, this is just to
verify we're in the right ballpark on all targets.  I've only checked `x86_64`
though, not 32-bit, and not ARM.

It's interesting to see GCC 10 uses less stack.  This is because it optimises
`struct` returns better, sometimes skipping an intermediate copy.  Here it
benefits the EVMC API, but I found GCC 10 also improves the larger stack usage
of the rest of `nimbus-eth1` as well.

Apple clang 12.0.0 (clang-1200.0.26.2) on MacOS 10.15:

- 544 bytes per EVM call stack frame

GCC 10.3.0 (Ubuntu 10.3.0-1ubuntu1) on Ubuntu 21.04:

- 464 bytes per EVM call stack frame

GCC 10.2.0 (Ubuntu 10.2.0-5ubuntu1~20.04) on Ubuntu 20.04 LTS:

- 464 bytes per EVM call stack frame

GCC 11.0.1 20210417 (experimental; Ubuntu 11-20210417-1ubuntu1) on Ubuntu 21.04:

- 8 bytes per EVM call stack frame

GCC 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 8.4.0 (Ubuntu 8.4.0-3ubuntu2) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 7.5.0 (Ubuntu 7.5.0-6ubuntu2) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) on Ubuntu 19.10:

- 528 bytes per EVM call stack frame

Signed-off-by: Jamie Lokier <[email protected]>
@jlokier jlokier reopened this Apr 26, 2021
@jlokier
Copy link
Contributor Author

jlokier commented Apr 26, 2021

This issue is now resolved due to the three PRs shown above: #586, #588, #598.

Previous worst case stage usage for some EVM programs was about 13 MB, exceeding the default stack size on various targets.

Now it's down to about 550 kB worst case (EVMC mode with recursion), 50 kB (non-EVMC mode with recursion), and not much at all (non-EVMC with no recursion). This is small enough to use in many threads.

It's been tested locally, but matrix CI results are taking a long time to even start, still queued after 3 hours.

Because this was an intermittent issue, only affecting some targets, and even then it wobbled about a bit, it's worth validating the fix now and into the future. This has been done by limiting the stack to 1 MB on all targets during make test.

jlokier added a commit that referenced this issue Apr 27, 2021
This patch reduces stack space used with EVM in ENABLE_EVMC=1 mode, from 13 MB
worst case to 550 kB, a 24x reduction, and fixes stack overflows.

This finishes the "stack problem" and closes #575 (EVM: Different segmentation
faults when running the test suite with EVMC).

It also closes #256 (recursive EVM call trigger unrecoverable stack overflow).

After this patch, it is possible to re-enable the CI targets which had to be
disabled due to #575.

This change is a required precursor for switching over to "nearly EVMC" as the
clean and focused Nimbus-internal API between EVM and sync/database processes,
and is also key to the use of Chronos `async` in those processes when calling
the EVM.

(The motivation is the internal interface has to be substantially changed
_anyway_ for the parallel sync and database processes, and EVMC turns out to be
well-designed and well-suited for this.  It provides good separation between
modules, and suits our needs better than our other current interface.  Might as
well use a good one designed by someone else.  EVMC is 98% done in Nimbus
thanks to great work done before by @jangko, and we can use Nimbus-specific
extensions where we need flexibility, including for performance.  Being aligned
with the ecosystem is a useful bonus feature.)

All tests below were run on Ubuntu 20.04 LTS server, x86-64.  This matches one
of the targets that has been disabled for a while in CI in EVMC mode due to
stack overflow crashing the tests, so it's a good choice.

Measurements before
===================

Testing commit `e76e0144 2021-04-22 11:29:42 +0700 add submodules: graphql and
toml-serialization`.

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 16384 # Requires larger stack than default to avoid crash.
    $ ./build/all_tests 9 | tee tlog
    [Suite] persist block json tests
    ...
    Stack range 38416 depthHigh 3
    ...
    Stack range 13074720 depthHigh 1024
    [OK] tests/fixtures/PersistBlockTests/block1431916.json

These tests use 13.07 MB of stack to run, and so crash with the default stack
limit on Ubuntu Server 20.04 (8MB).  Exactly 12768 bytes per EVM call stack
frame.

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 16384 # Requires larger stack than default.
    $ ./build/all_tests 7 | tee tlog
    [Suite] new generalstate json tests
        ...
    Stack range 14384 depthHigh 2
        ...
    Stack range 3495456 depthHigh 457
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
    ...
    Stack range 3709600 depthHigh 485
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
        ...
    Stack range 7831600 depthHigh 1024
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json

These tests use 7.83MB of stack to run.  About 7648 bytes per EVM call stack
frame.  It _only just_ avoids crashing with the default Ubuntu Server stack
limit of 8 MB.  However, it still crashes on Windows x86-64, which is why the
Windows CI EVMC target is currently disabled.

On Linux where this passes, this is so borderline that it affects work and
testing of the complex storage code, because that's called from the EVM.

Also, this greatly exceeds the default thread stack size.

Measurements after
==================

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 600 # Because we can!  600k stack.
    $ ./build/all_tests 9 | tee tlog
    [Suite] persist block json tests
    ...
    Stack range 1936 depthHigh 3
    ...
        Stack range 556272 depthHigh 1022
        Stack range 556512 depthHigh 1023
        Stack range 556816 depthHigh 1023
        Stack range 557056 depthHigh 1024
        Stack range 557360 depthHigh 1024
        [OK] tests/fixtures/PersistBlockTests/block1431916.json

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 600 # Because we can!  600k stack.
    $ ./build/all_tests 7 | tee tlog
    [Suite] new generalstate json tests
        ...
    Stack range 1392 depthHigh 2
        ...
    Stack range 248912 depthHigh 457
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
    ...
    Stack range 264144 depthHigh 485
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
        ...
        Stack range 557360 depthHigh 1024
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stStaticCall/static_CallRecursiveBombPreCall.json

For both tests, a satisfying *544 bytes* per EVM call stack frame, and EVM
takes less than 600 kB total.  With other overheads, both tests run in 600 kB
stack total at maximum EVM depth.

We must add some headroom on this for database activity called from the EVM,
and different compile targets.  But it means the EVM itself is no longer a
stack burden.

This is much smaller than the default thread stack size on Linux (2MB), with
plenty of margin.  (Just fyi, it isn't smaller than a _small_ thread stack on
Linux from a long time ago (128kB), and some small embedded C targets.)

This size is well suited to running EVMs in threads.

Further reduction
=================

This patch solves the stack problem.  Windows and Linux 64-bit EVMC CI targets
can be re-enabled, and there is no longer a problem with stack usage.

We can reduce further to ~340 bytes per frame and 350 kB total, while still
complying with EVMC.  But as this involves changing how errors are handled to
comply fully with EVMC, and removing `dispose` calls, it's not worth doing now
while there are other EVMC changes in progress that will have the same effect.

A Nimbus-specific extension will allow us to avoid recursion with EVMC anyway,
bringing bytes per frame to zero.  We need the extension anyway, to support
Chronos `async` which parallel transaction processing is built around.

Interop with non-Nimbus over EVMC won't let us avoid recursion, but then we
can't control the stack frame size either.  To prevent stack overflow in
interop I anticipate using (this method in Aleth)
[https://github.com/ethereum/aleth/blob/6e96ce34e3f131e2d42f3cb00741b54e05ab029d/libethereum/ExtVM.cpp#L61].

Smoke test other versions of GCC and Clang/LLVM
===============================================

As all builds including Windows use GCC or Apple's Clang/LLVM, this is just to
verify we're in the right ballpark on all targets.  I've only checked `x86_64`
though, not 32-bit, and not ARM.

It's interesting to see GCC 10 uses less stack.  This is because it optimises
`struct` returns better, sometimes skipping an intermediate copy.  Here it
benefits the EVMC API, but I found GCC 10 also improves the larger stack usage
of the rest of `nimbus-eth1` as well.

Apple clang 12.0.0 (clang-1200.0.26.2) on MacOS 10.15:

- 544 bytes per EVM call stack frame

GCC 10.3.0 (Ubuntu 10.3.0-1ubuntu1) on Ubuntu 21.04:

- 464 bytes per EVM call stack frame

GCC 10.2.0 (Ubuntu 10.2.0-5ubuntu1~20.04) on Ubuntu 20.04 LTS:

- 464 bytes per EVM call stack frame

GCC 11.0.1 20210417 (experimental; Ubuntu 11-20210417-1ubuntu1) on Ubuntu 21.04:

- 8 bytes per EVM call stack frame

GCC 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 8.4.0 (Ubuntu 8.4.0-3ubuntu2) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 7.5.0 (Ubuntu 7.5.0-6ubuntu2) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) on Ubuntu 19.10:

- 528 bytes per EVM call stack frame

Signed-off-by: Jamie Lokier <[email protected]>
jlokier added a commit that referenced this issue Jun 8, 2021
Proper nested call functionality is being skipped in this iteration of new EVMC
host code to keep it simpler, to allow testing and architecture to be built
around the less complicated non-nested cases first.

Instead, nested calls use the old `Computation` path, and bypass any
third-party EVM that may be loaded.  The results are the same, and mixing
different EVMs in this way is actually permitted in the EVMC specification.

This approach also means third-party EVMs we test don't need to support
precompiles and we don't need to specially handle those cases.
(E.g. "evmone" doesn't support precompiles, just EVM1 opcodes).

(These before/after scope actions are approximately copy-pasted from
`nimbus/vm/evmc_host.nim`, making their detailed behaviour "obviously correct".
Of course they are subject to tests as well.  The small stack property of
a3c8a5c "EVMC: Small stacks when using EVMC, closes #575 (segfaults)" is
carefully retained.)

Signed-off-by: Jamie Lokier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants