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

[Timofei Pushkin]

On Tue, 7 Oct 2025 14:01:31 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 include problems

src/hotspot/share/runtime/crac.cpp line 107:

> 105: 
> 106:   // If this is a second checkpoint we should use a clear configuration
> 107:   if (_generation != 0 && !_engine->reset_conf()) {

Generation numbering starts from 1, so it should be `_generation > 1`

src/hotspot/share/runtime/crac.cpp line 107:

> 105: 
> 106:   // If this is a second checkpoint we should use a clear configuration
> 107:   if (_generation != 0 && !_engine->reset_conf()) {

Why do we need to reset the configuration now? Is it because user data is auto-reset while the tags are not? Ideally they should act the same to make the behavior more predictable. IMO it would be better if both were kept until explicitly cleared by the user, if that is possible. But that would require updating the user data API probably...

Anyway, shouldn't the CPU features stay the same between checkpoints? Then it would be more efficient to set them only on the first checkpoint instead of resetting the whole configuration each time.

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

> 134: 
> 135: template<class T> template<typename TT>
> 136: bool Hashtable<T>::put(const char* key, TT&& value) {

A total nitpick, but now we have `const char *key` in the declaration and `const char* key` in the definition. When I was rewriting crexec into C++ I used the `type *x`, `type * const x` style everywhere. But since there is no linter for this in CI it seems that there is no sense in trying to keep it in a single style, it will diverge with time anyway...

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

> 65:   }
> 66:   if (strlen(name) >= _MAX_NAME_SIZE) {
> 67:     fprintf(stderr, CREXEC "%s %s is too long\n", type, name);

Would be a better UX if we specify how many symbols is allowed
Suggestion:

    fprintf(stderr, CREXEC "%s %s name is too long, at most %zu chars allowed\n", type, name, _MAX_NAME_SIZE - 1;

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

> 68:     return false;
> 69:   }
> 70:   if (value_size >= _MAX_VALUE_SIZE) {

I think it should be:
Suggestion:

  if (value_size > _MAX_VALUE_SIZE) {

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

> 69:   }
> 70:   if (value_size >= _MAX_VALUE_SIZE) {
> 71:       fprintf(stderr, CREXEC "%s %s value is too long (%zu bytes)\n", type, name, value_size);

Suggestion:

      fprintf(stderr, CREXEC "%s %s value is too long: %zu bytes > %zu allowed\n", type, name, value_size, _MAX_VALUE_SIZE);

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

> 138: }
> 139: 
> 140: static inline unsigned char from_hex(char c, bool& err) {

I'd say it's a better style to take a pointer when you plan to write into the argument since that becomes more obvious at the call site, but this is opinionated

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

> 127:   }
> 128: 
> 129:   bool is_failed(const char* name) {

Suggestion:

  bool is_failed(const char* name) const {

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

> 53:     } else {
> 54:       _tail = node;
> 55:     }

Suggestion:

    if (_tail) {
      _tail->_next = node;
    }
    _tail = node;

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

PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413008490
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413057655
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413154149
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413235569
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413241858
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413240096
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413265426
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413186999
PR Review Comment: https://git.openjdk.org/crac/pull/266#discussion_r2413123902