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

[5.0] Disable EOS VM OC's subjective compilation limits in unit tests #1843

Merged
merged 6 commits into from
Nov 7, 2023

Conversation

linh2931
Copy link
Member

@linh2931 linh2931 commented Oct 31, 2023

When EOS VM OC is used in unit tests, contract actions are not dispatched until EOS VM OC has finished compilation. If a unit test violates EOS VM OC's subjective compilation limits, the unit test will fail. This is most notably problematic for EOS EVM because EOS EVM's exhaustive tests run afoul of OC's subjective limits. Another problem with OC's subjective limits are they prevent ASAN from being used, because ASAN's 16TB of virtual memory usage is flagged by OC's subjective limits.

This PR disables OC subjective limits: cpu limits, vm limits, stack size limit, and generated code size limit in unit tests. It also adds unit tests to make sure limits to work properly.

Resolves #1573

libraries/chain/webassembly/runtimes/eos-vm-oc/LLVMJIT.cpp Outdated Show resolved Hide resolved
unittests/eosvmoc_limits_tests.cpp Outdated Show resolved Hide resolved
unittests/eosvmoc_limits_tests.cpp Outdated Show resolved Hide resolved
unittests/eosvmoc_limits_tests.cpp Outdated Show resolved Hide resolved
unittests/eosvmoc_limits_tests.cpp Outdated Show resolved Hide resolved
unittests/eosvmoc_limits_tests.cpp Outdated Show resolved Hide resolved
unittests/eosvmoc_limits_tests.cpp Outdated Show resolved Hide resolved
@arhag arhag linked an issue Nov 1, 2023 that may be closed by this pull request
Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only feedback I'd consider important is moving the libtester configuration in to default_config() if we can, which ought to remove that large if( .. && I believe.

Using an optional and getting rid of soft/hard as part of the config isn't a blocker.

// libtester disables the limits in all tests, except enforces the limits
// in the tests in unittests/eosvmoc_limits_tests.cpp.
rlimit cpu_limits {20u, 20u};
rlimit vm_limits {512u*1024u*1024u, 512u*1024u*1024u};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think there is a need for both soft/hard limits here. I can't think of a reason we'd ever set them differently. It also wouldn't be appropriate if we ever ran on Windows.

cfg.eosvmoc_config.vm_limits.rlim_cur = RLIM_INFINITY;
cfg.eosvmoc_config.stack_size_limit = std::numeric_limits<uint64_t>::max();
cfg.eosvmoc_config.generated_code_size_limit = std::numeric_limits<size_t>::max();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but feel std::optional would be a better choice for these items. default_config() could be modified to simply .reset() each of them,

static std::pair<controller::config, genesis_state> default_config(const fc::temp_directory& tempdir, std::optional<uint32_t> genesis_max_inline_action_size = std::optional<uint32_t>{}) {
controller::config cfg;
cfg.blocks_dir = tempdir.path() / config::default_blocks_dir_name;
cfg.state_dir = tempdir.path() / config::default_state_dir_name;
cfg.state_size = 1024*1024*16;
cfg.state_guard_size = 0;
cfg.contracts_console = true;
cfg.eosvmoc_config.cache_size = 1024*1024*8;
// don't use auto tier up for tests, since the point is to test diff vms
cfg.eosvmoc_tierup = chain::wasm_interface::vm_oc_enable::oc_none;

and since that occurs before validating_tester's Lambda conf_edit, eosvmoc_limits_tests can still override them back to however it wants them.

Regardless of optional usage, doing this part in default_config() feels like right spot since that's where other libtester-specific configuration items are set.

An optional should be able to be plumbed through easily, and something like

if(conf.cpu_limits.rlim_cur != RLIM_INFINITY) {
setrlimit(RLIMIT_CPU, &conf.cpu_limits);
}

becomes

if(conf.cpu_limits) {
   struct rlimit cpu_limit = {*conf.cpu_limit, *conf.cpu_limit};
   setrlimit(RLIMIT_CPU, &cpu_limit);
}

@linh2931
Copy link
Member Author

linh2931 commented Nov 7, 2023

The only feedback I'd consider important is moving the libtester configuration in to default_config() if we can, which ought to remove that large if( .. && I believe.

Using an optional and getting rid of soft/hard as part of the config isn't a blocker.

Thank you Matt @spoonincode for you suggestions! They make code much simpler. I have incorporated them.

};

}}}

FC_REFLECT(rlimit, (rlim_cur)(rlim_max))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this FC_REFLECT now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I missed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests: disable EOS VM OC's subjective limits during unit tests
3 participants