[crac] RFR: 8376959: [CRaC] Turn simengine into shared library [v3]

[Timofei Pushkin]

On Tue, 3 Feb 2026 13:20:30 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> Right now `simengine` is implemented as an executable, invoked through `libcrexec` that implements the C/R API. Since the programmatic C/R API should be the main interface, any exec API is redundant; let's turn `simengine` into shared (dynamic) library implementing the API on its own.
>> 
>> The `pauseengine` does not need to exist on its own; this can be only an option `pause=true` for `simengine`. The implementation is Linux only (and it is out of scope to fill in a non-Linux implementation - the `pause` option can be just excluded in non-Linux `libsimengine`).
>> 
>> We expect to turn `libcrexec` + `criuengine` into `libcriuengine` in a follow up (not as part of this issue).
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixups

I skipped most of crexec changes since we plan to remove it soon.

Looks like the README should now be moved from crexec to crcommon.

src/java.base/share/native/libcrexec/environment.hpp line 39:

> 37: char **get_environ();
> 38: 
> 39: class Environment {

I would prefer non-templates to be split into hpp/cpp (at least non-trivial methods) but since this is included just once and we plan to remove crexec anyway I don't insist

src/java.base/share/native/libcrexec/user_data.cpp line 31:

> 29: #include <stdint.h>
> 30: #include <stdlib.h>
> 31: #include <string.h>

Suggestion:

#include <cassert>
#include <cerrno>
#include <cstdio>
#include <cstdint>
#include <cstdlib>
#include <cstring>

src/java.base/share/native/libcrexec/user_data.hpp line 45:

> 43:     }
> 44: public:
> 45:     UserData(const char **image_location): _image_location(image_location) {}

Suggestion:

    explicit UserData(const char **image_location): _image_location(image_location) {}

src/java.base/share/native/libcrexec/user_data.hpp line 48:

> 46: 
> 47:     bool set_user_data(const char *name, const void *data, size_t size);
> 48:     crlib_user_data_storage_t *load_user_data();

`crlib/crlib_user_data.h` should be included as that is where `crlib_user_data_storage_t` is defined.

src/java.base/share/native/libsimengine/simengine.cpp line 36:

> 34: #endif // LINUX
> 35: 
> 36: #include <new>

Suggestion:

#include <cerrno>
#include <cstring>
#include <cstdio>
#include <cstdlib>
#include <new>
#ifdef LINUX
#include <signal.h>
#include <unistd.h>
#endif // LINUX

src/java.base/share/native/libsimengine/simengine.cpp line 43:

> 41: #include "jni.h"
> 42: 
> 43: #define SIMENGINE "simengine: "

Unused

src/java.base/share/native/libsimengine/simengine.cpp line 68:

> 66: struct SimEngine: public crlib_conf_t {
> 67:   char* _image_location = nullptr;
> 68:   bool _pause;

Suggestion:

  bool _pause = false;

src/java.base/share/native/libsimengine/simengine.cpp line 71:

> 69: 
> 70:   bool _has_restore_data = false;
> 71:   int _restore_data = 0;

I believe we discussed this already at some point, but in my understanding public fields should not have underscores as if this is a plain struct, otherwise they should be private and have a public accessor method when needed.

src/java.base/share/native/libsimengine/simengine.cpp line 99:

> 97:   if (!image_constraints_persist(conf, engine->_image_location) ||
> 98:     !image_score_persist(conf, engine->_image_location)) {
> 99:     return -1;

Suggestion:

  if (!image_constraints_persist(conf, engine->_image_location) ||
      !image_score_persist(conf, engine->_image_location)) {
    return -1;

src/java.base/share/native/libsimengine/simengine.cpp line 141:

> 139:   if (!engine->_pause) {
> 140:     LOG("restore is not supported as a separate action by this engine, "
> 141:         "it always restores a process immediately after checkpointing it");

On Linux this is not true anymore, it should be e.g. "restore is not fully supported by this engine, if you used pause=true option on checkpoint use it again on restore to wake that process up". On Linux the old message should stay until pausing is implemented.

src/java.base/share/native/libsimengine/simengine.cpp line 175:

> 173:   abort(); // engine->_pause should be false
> 174:   return -1;
> 175: #endif LINUX;

Suggestion:

#endif // LINUX;

src/java.base/share/native/libsimengine/simengine.cpp line 179:

> 177: 
> 178: static bool can_configure(crlib_conf_t* conf, const char* key) {
> 179:     return !strcmp(key, opt_image_location) LINUX_ONLY(|| !strcmp(key, opt_pause));

Starting from here and going below the indentation changed from 2 to 4

src/java.base/share/native/libsimengine/simengine.cpp line 255:

> 253:   return
> 254:     "simengine - CRaC-engine used for development & testing; does not implement "
> 255:     "actual process snapshot & rehydration but only simulates these.";

Suggestion:

    "actual process snapshot & restoration but only simulates these.";

-------------

PR Review: https://git.openjdk.org/crac/pull/289#pullrequestreview-3744787224
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759047695
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759087831
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759080104
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759078508
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759110955
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759189648
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759196998
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759151173
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759210268
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759229132
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759199940
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759242958
PR Review Comment: https://git.openjdk.org/crac/pull/289#discussion_r2759175708