[crac] RFR: 8368929: [CRaC] Move CPUFeatures check to C/R engine [v4]

Timofei Pushkin tpushkin at openjdk.org
Fri Oct 3 12:02:30 UTC 2025 

On Thu, 2 Oct 2025 19:22:10 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> Right now the logic checking if CPU features used before checkpoint match current CPU features is in VM code. VM stores and retrieves CPU features through C/R API's user_data extension. This is convenient when we have a single image that can be either accepted or rejected, but does not offer the flexibility for C/R engine to select the best image for current execution environment.
>> 
>> The goal of this issue is to move to a declarative API that will express the requirements using more abstract means, labels (for CPU architecture) and bitmaps (for CPU features).
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix NoCPUFeaturesTest

Regarding `crexec` changes, I wonder if it would be easier for us to use the C++ stdlib fully (`std::vector`, `std::unordered_map`) catching all exceptions...

src/hotspot/cpu/x86/vm_version_x86.hpp line 617:

> 615:     static constexpr size_t print_buffer_length() {
> 616:       return MAX_CPU_FEATURES;
> 617:     }

I believe by the time the corresponding printing function is used the resource area is already initialized. It thus should be possible to make it allocate a resource array internally instead of making the callers use this to allocate on stack. Or there can be two functions: one that uses a provided buffer and its wrapper that allocates a resource array.

src/java.base/share/native/libcrexec/crexec.cpp line 142:

> 140:   &restore_data_extension.header,
> 141:   &user_data_extension.header,
> 142:   &image_constraints_extension.header,

Since we replace the direct use of user data with this new extension it should be a bit more efficient to put the new extension above the old one

src/java.base/share/native/libcrexec/crexec.cpp line 837:

> 835:   if (error) {
> 836:     return error;
> 837:   }

Nitpick: let's move this before initializing `restore_data_str` as this is a part of argument validation while `restore_data_str` and below is preparation for exec-ing

src/java.base/share/native/libcrexec/hashtable.hpp line 143:

> 141:   *value_ptr = std::forward<T>(value);
> 142:   return true;
> 143: }

I believe this usage of `std::forward` is incorrect as `T` is deduced from the hashtable class and not from the `value` parameter, so with this change it is now not possible to pass lvalues as `value`. I think the best would be to leave `T value` as it was and replace `std::forward` with `std::move` — lvalues will be copied then moved, rvalues will be just moved.

Same for `LinkedList::add`.

src/java.base/share/native/libcrexec/image_constraints.cpp line 39:

> 37: 
> 38: static FILE *open_tags(const char *image_location, const char *mode) {
> 39:   char fname[PATH_MAX];

In `crexec.cpp` we for some reason have our own conditional definition of `PATH_MAX`, I guess it caused problems at some point... Maybe create some `common.hpp` or `crexec.hpp` where `PATH_MAX` and `CREXEC` would be defined?

src/java.base/share/native/libcrexec/image_constraints.cpp line 46:

> 44:   FILE *f = fopen(fname, mode);
> 45:   if (f == nullptr) {
> 46:     fprintf(stderr, CREXEC "cannot open(%s) %s: %s\n", fname, mode, strerror(errno));

Suggestion:

    fprintf(stderr, CREXEC "cannot open %s in %s mode: %s\n", fname, mode, strerror(errno));

src/java.base/share/native/libcrexec/image_constraints.cpp line 142:

> 140:     return errno == ENOENT ? RESTORE_ERROR_NOT_FOUND : RESTORE_ERROR_NO_ACCESS;
> 141:   }
> 142:   char line[strlen(BITMAP_PREFIX) + _max_name_length + 1 + _max_value_length + 2];

AFAIR MSVC does not support arrays of non-constexpr size

src/java.base/share/native/libcrexec/image_constraints.cpp line 146:

> 144:   while (fgets(line, sizeof(line), f)) {
> 145:     char *eq = strchr(line, '=');
> 146:     char *nl = strchr(eq + 1, '\n');

I believe parsing will be broken if `=` is encountered in the tag name itself. Same for `\n` although that is less likely.

src/java.base/share/native/libcrexec/image_constraints.cpp line 160:

> 158:         .data = strdup(eq + 1),
> 159:         .data_length = (size_t) (nl - eq),
> 160:       });

Missing error checking here and below (`add`, `strdup`, `malloc`)

src/java.base/share/native/libcrexec/image_constraints.hpp line 36:

> 34: class ImageConstraints {
> 35: private:
> 36:   enum TagType {

It can have a smaller base type, and also making it a class is more idiomatic I believe
Suggestion:

  enum class TagType : std::uint8_t {

src/java.base/share/native/libcrexec/image_constraints.hpp line 93:

> 91: 
> 92:   const size_t _max_name_length = 256;
> 93:   const size_t _max_value_length = 256;

Suggestion:

  static constexpr size_t _MAX_NAME_LENGTH = 256;
  static constexpr size_t _MAX_VALUE_LENGTH = 256;

src/java.base/share/native/libcrexec/image_constraints.hpp line 95:

> 93:   const size_t _max_value_length = 256;
> 94: 
> 95:   bool check_tag(const char *name) {

"check" has too broad meaning IMO
Suggestion:

  bool has_tag(const char *name) const {

src/java.base/share/native/libcrexec/image_constraints.hpp line 119:

> 117:       .data = strdup(value),
> 118:       .data_length = value_length,
> 119:     });

`add` and `strdup`s can fail — this should be checked

src/java.base/share/native/libcrexec/linkedlist.hpp line 42:

> 40:   size_t _size;
> 41: public:
> 42:   LinkedList(): _head(nullptr), _tail(nullptr), _size(0) {}

Suggestion:

  Node *_head = nullptr;
  Node *_tail = nullptr;
  size_t _size = 0;
public:

src/java.base/share/native/libcrexec/linkedlist.hpp line 52:

> 50:   }
> 51: 
> 52:   T& add(T&& item) {

With the current style this should be:
Suggestion:

  T &add(T &&item) {

src/java.base/share/native/libcrexec/linkedlist.hpp line 53:

> 51: 
> 52:   T& add(T&& item) {
> 53:     Node *node = new(std::nothrow) Node();

Should do a null check after this

src/java.base/share/native/libcrexec/linkedlist.hpp line 74:

> 72:       node = node->_next;
> 73:     }
> 74:   }

I don't insist but implementing an `std::iterator` does not seem too cumbersome and would allow to use the normal for loop

src/java.base/share/native/libcrexec/linkedlist.hpp line 76:

> 74:   }
> 75: 
> 76:   size_t size() {

Suggestion:

  size_t size() const {

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

PR Review: https://git.openjdk.org/crac/pull/266#pullrequestreview-3298008166
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401210494
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401621379
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401643142
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401437652
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401543883
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401555404
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401594497
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401576791
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401598125
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401495490
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401506384
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401501558
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401523886
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401237419
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401444477
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401231987
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401266298
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2401258129

More information about the crac-dev mailing list