8385359: [CRaC] aarch64: c7g.medium <-> c8g.medium FAIL: vector length by jankratochvil · Pull Request #331 · openjdk/crac

jankratochvil · 2026-07-23T02:35:59Z

It is a draft src/hotspot/share/runtime/crac_engine.cpp needs to be reworked. I believe crlib_image_constraints::get_failed_bitmap should become get_bitmap as this code needs to query image's CPUFeatures to find out its vector length to configure the CPU before restore. Otherwise one could also create some new user data. @rvansa?

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8385359: [CRaC] aarch64: c7g.medium <-> c8g.medium FAIL: vector length (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/331/head:pull/331
$ git checkout pull/331

Update a local copy of the PR:
$ git checkout pull/331
$ git pull https://git.openjdk.org/crac.git pull/331/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 331

View PR using the GUI difftool:
$ git pr show -t 331

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/331.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-07-23T02:38:01Z

👋 Welcome back jkratochvil! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-07-23T02:38:44Z

@jankratochvil This change is no longer ready for integration - check the PR body for details.

rvansa · 2026-07-23T06:40:01Z

The image constraints was meant as structured data that is validated by the engine. The check is performed in the engine as to move JVM out of the loop when there are more images and score-based criteria. On the other hand, user_data is meant to be data opaque to the engine - so this fits better into the picture.

The only downside of adding get_bitmap and get_label methods to the API is that in practice the values will have to be stored in the configuration until it is destroyed - this is not a strong con, the data is minimal. Duplicating entry storage in user data ain't ideal either, anyway. Let's not change get_failed_bitmap, though - the API should evolve append-only.

@TimPushkin what's your opinion on the suggested API change?

TimPushkin · 2026-07-23T14:56:43Z

Adding get_bitmap and get_label to the constraints API LGTM: if we have setters it feels natural to have getters.

TimPushkin · 2026-07-24T07:10:07Z

Nitpick: consider using pre_restore instead of restore_pre — the former convention seems more common in HotSpot

mlbridge · 2026-07-26T16:23:58Z

Webrevs

TimPushkin · 2026-07-28T14:35:11Z

+static constexpr char cpuarch_name[] = "cpu.arch";
+static constexpr char cpufeatures_name[] = "cpu.features";
+
+bool CracEngine::pre_restore_bitmap_hook(const unsigned char *value, size_t value_size) const {


_image_location is the only thing used from CracEngine, and it's only used in logging which I believe could be cut — we don't include it in any other logs. Then this could be static and CracEngine::pre_restore_bitmap_hook_trampoline wouldn't be needed.

This comment suggests removing user_data to keep the API minimal for the current use case. My intention, however, was to make the API stable enough for future extensions.
If changes to the existing API are acceptable, the proposed register_label_hook is a superset of require_label and could therefore replace it. The same applies to bitmap.

As I wrote in the previous comment, I don't suggest removing user_data from the API, just not to use it in the way we do now (which means that for now it'll be unused by us, but that's OK).

If changes to the existing API are acceptable, the proposed register_label_hook is a superset of require_label and could therefore replace it. The same applies to bitmap.

That would require adding more restrictions to how the new API can be used. The engine may use constraints API to select one of many images => the hooks can be called multiple times, including for images that won't end up being the one selected for restore => the hooks should not treat incompatible values as errors (e.g. in the current impl error logs should be replaced with debug logs). This is worth documenting in the API so that people don't call stuff like exit there.

I do not agree removing standard error reporting to save 8 normal lines of code but OK, implemented.

I'm not sure with which part of my previous comment you don't agree. None of them were about saving code really.

If with not using user_data, i.e. removing image path from the error messages: I suggest this because we do not do this in any other messages so it's weird to include it in only these particular ones. If you think we need to include it then I'd propose to do that in some more high-level message, e.g. the one reporting the overall restore failure (I think we should have one, not sure).

If with replacing error messages with debug messages: I suggest this because with multi-selection they are not really errors. I agree that this is not great / is incorrect for the non-multi-selection case, but I do not see a way to fix this, the JVM does not know what case this is.

I expected _image_location to be used in another error message, but I see it isn't. In that case, it's fine that the new error messages don't print _image_location either. My mistake.

If crlib_image_constraints_t is called for multiple images, including those that are not ultimately selected, then I placed the CPU reconfiguration in the wrong place.

For example:

We are running on a vec256 CPU.

The selector checks a vec128 image.

set_maximum_sve_vector_length(16) is called.

The selector then checks and selects a vec256 image.

The algorithm does nothing because both the image and the native CPU support vec256.

However, the CPU has already been reconfigured to vec128.

I'll fix this, but it means the CPU may be reconfigured multiple times while evaluating multiple candidate images.

Is there an existing JDK test case or documentation covering multiple images? If so, I should add a corresponding test to test/jdk/jdk/crac/CPUFeatures/CPUFeaturesAWS.sh.

I believe we have multi-selection (called auto-selection) only downstream, and even there I'm not sure we have JDK-level tests for it (haven't checked) since it is an engine-level feature

Maybe it makes sense downstreaming this before integrating the PR in case some other changes will surface

OK, though I still have to fix JDK-8385359 first.

_ignore_glibc_not_using->_cpu_features_ignore

Co-authored-by: Timofei Pushkin <pushkin.td@gmail.com>

CracEngine::pre_restore -> CracEngine::register_constraints_hooks VM_Version::pre_restore -> VM_Version::process_image_cpu_features similar for VM_Version::pre_restore_needed

TimPushkin · 2026-07-29T07:48:15Z

+static constexpr char cpuarch_name[] = "cpu.arch";
+static constexpr char cpufeatures_name[] = "cpu.features";
+
+bool CracEngine::pre_restore_bitmap_hook(const unsigned char *value, size_t value_size) const {


As I wrote in the previous comment, I don't suggest removing user_data from the API, just not to use it in the way we do now (which means that for now it'll be unused by us, but that's OK).

If changes to the existing API are acceptable, the proposed register_label_hook is a superset of require_label and could therefore replace it. The same applies to bitmap.

That would require adding more restrictions to how the new API can be used. The engine may use constraints API to select one of many images => the hooks can be called multiple times, including for images that won't end up being the one selected for restore => the hooks should not treat incompatible values as errors (e.g. in the current impl error logs should be replaced with debug logs). This is worth documenting in the API so that people don't call stuff like exit there.

Rename pre_restore_bitmap_hook -> bitmap_constraint_hook

should_skip_cpu_features_check() -> !should_check_cpu_features()

TimPushkin · 2026-07-30T06:26:57Z

+  if (ret == 0) {
+    ShouldNotReachHere();
+  } else {


Engine is a separate user-provided piece of software. We should not hard-crash if it misbehaves (where possible).

Actually, according to the engine API spec any return code of restore is treated as an error code, even 0, so the previous behavior was also incorrect. We should not check the return value at all, merely log it.

TimPushkin · 2026-07-30T06:34:05Z

  return _options;
 }

+bool CracEngine::bitmap_constraint_hook(const unsigned char *value, size_t value_size, void *user_data/*unused*/) {


Since it does not use anything from CracEngine, why not make it .cpp-level static?

TimPushkin · 2026-07-30T06:41:45Z

 }
+
+bool VM_Version::process_image_cpu_features(const VM_Features *image_featuresp) {
+  int want;


want_sve_vector_length

TimPushkin · 2026-07-30T06:53:08Z

+  if (supports_sve() && get_current_sve_vector_length() == 32) {
+    set_feature(CPU_SVE256);
+  }


The rest of the code seems to assume that !supports_feature(CPU_SVE256) => supports SVE128 (or less?), but I read there are CPUs with SVE vector lengths higher than 256 bits. Not sure JDK can run on them though. Is it guaranteed somewhere that higher lengths are not possible in this code?

AI also complains about smaller sizes, resending verbatim:

"get_os_cpu_info sets the bit only when VL is exactly 32, so a VL=64 machine (A64FX etc.) is indistinguishable from VL=16. On restore, want = image_supports_sve256 ? 32 : 16 then hard-sets VL to 16 — including when checkpointing and restoring on the same VL=64 host, where the restored JVM's code was compiled for MaxVectorSize=64. That's a silent miscompile/SIGILL, not a clean failure."

"process_image_cpu_features(nullptr) recomputes 16/32 from _features rather than restoring what get_current_sve_vector_length() was before the hook ran. With -XX:MaxVectorSize=16 on an SVE256 CPU plus a failed restore and -XX:+CRaCIgnoreRestoreIfUnavailable (where the VM keeps running), the process ends up with VL=32 while its compiled code assumed 16."

TimPushkin · 2026-07-30T07:29:55Z

+  if (!hooks_result) {
+    return false;
+  }


Maybe it would be a good idea to run all checks instead of failing fast so that the user sees the hole picture?

It will duplicate the error message for the SVE feature, but at least this way CheckCPUFeaturesMessage would be employed which can contain some useful info. BTW, if we make hooks' messages debug (as would be more correct for the multiple-selection case) the duplication could be avoided, otherwise CheckCPUFeaturesMessage should be incorporated into the hooks' messages somehow...

TimPushkin · 2026-07-30T07:31:56Z

+  _hooks.foreach([&](const Hook& hook) {
+    bool found = false;
+    tags.foreach([&](const Tag& t) {
+      if (!hooks_result || strcmp(hook.name, t.name) != 0) {


With this, if there are multiple hooks and one fails, every next hook will print the Hook did not find its tag line below

TimPushkin · 2026-07-30T07:42:21Z

+      VM_Features sve256;
+      sve256.set_feature(VM_Feature_Flag::CPU_SVE256);
+      ss.print("Specified -XX:CPUFeatures=%s have unset CPU_SVE256=%s, this CPU has CPUFeatures=%s but it cannot be disabled as PR_SVE_SET_VL reports %d: %m",
+               _features.print_numbers(), sve256.print_numbers(), _cpu_features.print_numbers(), got);


print_numbers does resource allocation, so ResourceMark could be used.

There are other usages of it without the mark, but they are pre-existing.

TimPushkin · 2026-07-30T07:43:07Z

+      if (!Abstract_VM_Version::should_check_cpu_features()) {
+        return true;
+      }
+      ResourceMark rm;


#include "memory/resourceArea.hpp" should be added

TimPushkin · 2026-07-30T07:43:44Z

+};
+
+static bool register_bitmap_hook(crlib_conf_t* conf, const char *name, ImageConstraints::BitmapHook hook, void *user_data) {
+  return COMMON(conf)->image_constraints.register_bitmap_hook(name, hook, user_data);
+};


Suggested change

};

static bool register_bitmap_hook(crlib_conf_t* conf, const char *name, ImageConstraints::BitmapHook hook, void *user_data) {

return COMMON(conf)->image_constraints.register_bitmap_hook(name, hook, user_data);

};

}

static bool register_bitmap_hook(crlib_conf_t* conf, const char *name, ImageConstraints::BitmapHook hook, void *user_data) {

return COMMON(conf)->image_constraints.register_bitmap_hook(name, hook, user_data);

}

TimPushkin · 2026-07-30T07:45:20Z

  struct VM_Features: public Zero_Features {};
  static bool cpu_features_binary(VM_Features *data) { return false; }
-  static bool check_cpu_features_skip() { return true; }
+  static bool can_use_cpu_features() { return false; }


The two new methods and restore_failed_check should be added to ppc/riscv/s390

TimPushkin · 2026-07-30T07:56:24Z

+    bash -c 'echo $$'
+    (set +x;while [ $(bash -c 'echo $$') -le $tid ];do :;done)
+    bash -c 'echo $$'


Nitpick: since the script uses dash in its shebang, maybe use dash here as well?

TimPushkin · 2026-07-30T07:58:12Z

 }
 internal_restore() {
  javasetup
+  tid=$(echo cr/core-*.img|tr -cd ' 0-9'|sed 's/^.* //') # highest


Breaks when crossing a power of ten because of the lexicographic sort

8385359: [CRaC] aarch64: c7g.medium <-> c8g.medium FAIL: vector length

b14c031

jankratochvil marked this pull request as draft July 23, 2026 02:36

rvansa reviewed Jul 23, 2026

View reviewed changes

Comment thread src/hotspot/cpu/aarch64/vm_version_aarch64.cpp Outdated

Comment thread src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp Outdated

jankratochvil added 8 commits July 23, 2026 10:44

rename: boolvalue -> image_supports_sve256

8c52894

Fix Gitlab CI

d985dad

Fix whitespace

e08bde7

Fix one forgotten maximum_sve_vector_length set

fa57bb5

Fix aarch64 build

a46f297

Remove dead code

b46ded6

Fix CheckCPUFeaturesTest.java

d5f84fa

Fix linux-cross-compile

15449dd

TimPushkin reviewed Jul 23, 2026

View reviewed changes

Comment thread src/hotspot/cpu/aarch64/vm_version_aarch64.cpp Outdated

jankratochvil added 2 commits July 23, 2026 20:48

Fix SIGSEGV

0f62afa

Follow -XX::CheckCPUFeatures=skip

b86c5ff

jankratochvil added 11 commits July 24, 2026 14:35

restore_pre -> pre_restore

768aae4

Provide should_skip_cpu_features_check()

588ee21

+get_label, +get_bitmap

52d526c

Fix get_label,get_bitmap

cb71018

Compilation fix

8db9bc7

Compilation fix #2

cbb0746

Fix a race in CPUFeaturesAWS.sh

be48727

An unrelated fix for get_failed_bitmap

09f983f

%m fix

5e060d9

Fix secondary arches compilation

5ea5d93

Fix ups

9e5927d

jankratochvil added 4 commits July 25, 2026 08:56

Fix compilation

205080e

get_* -> register_*_hook

c3d3644

fixups

7c8c827

Compilation fix

000138b

jankratochvil marked this pull request as ready for review July 26, 2026 16:18

openjdk Bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 26, 2026

jankratochvil added 2 commits July 26, 2026 22:44

Fix the 1st x86_64 testcase regresion

7eb8b88

Fix aarch64 compilation

fc737cc

TimPushkin reviewed Jul 28, 2026

View reviewed changes

jankratochvil and others added 4 commits July 28, 2026 23:30

check_cpu_features_skip()->get_cpu_features_ignore()

4c4a201

_ignore_glibc_not_using->_cpu_features_ignore

Message fix Invalid -> Incompatible

0340e76

Co-authored-by: Timofei Pushkin <pushkin.td@gmail.com>

Use prepare_image_constraints_api()

32cf9c8

rename:

416e168

CracEngine::pre_restore -> CracEngine::register_constraints_hooks VM_Version::pre_restore -> VM_Version::process_image_cpu_features similar for VM_Version::pre_restore_needed

TimPushkin reviewed Jul 29, 2026

View reviewed changes

jankratochvil added 8 commits July 29, 2026 19:51

Remove pre_restore_bitmap_hook_trampoline

c1253a6

Rename pre_restore_bitmap_hook -> bitmap_constraint_hook

Rename get_cpu_features_ignore() -> cpu_features_ignore()

297ce42

Inline should_skip_cpu_features_check()

d417007

Fix aarch64 compilation

8b6f0af

cpu_features_ignore() -> !can_use_cpu_features()

6473fb2

should_skip_cpu_features_check() -> !should_check_cpu_features()

Fix vec128/256 for image selection

667defa

Fix aarch64 multiple images selection

a78f4a3

+restore_cpuinfo()

28104d1

TimPushkin reviewed Jul 30, 2026

View reviewed changes

jankratochvil marked this pull request as draft July 30, 2026 07:54

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 30, 2026

TimPushkin reviewed Jul 30, 2026

View reviewed changes

Cosmetic message unification

77a7638

Uh oh!

Conversation

jankratochvil commented Jul 23, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Jul 23, 2026

Uh oh!

openjdk Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvansa commented Jul 23, 2026

Uh oh!

Uh oh!

Uh oh!

TimPushkin commented Jul 23, 2026

Uh oh!

Uh oh!

TimPushkin commented Jul 24, 2026

Uh oh!

mlbridge Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimPushkin Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimPushkin Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jankratochvil commented Jul 23, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Jul 23, 2026 •

edited

Loading

mlbridge Bot commented Jul 26, 2026 •

edited

Loading

TimPushkin Jul 29, 2026 •

edited

Loading

TimPushkin Jul 30, 2026 •

edited

Loading