[crac] RFR: Fix up CPUFeatures on top of Merge jdk:jdk-25+26 [v2]

Timofei Pushkin tpushkin at openjdk.org
Fri Jul 25 13:46:29 UTC 2025


On Fri, 25 Jul 2025 09:49:14 GMT, Jan Kratochvil <jkratochvil at openjdk.org> wrote:

>> This pull request contains: https://github.com/openjdk/crac/pull/248
>> I think the CRaC project does not provide the pr/NUMBER branches for dependent patches.
>> 
>> This pull request enables the CPUFeatures again.
>
> Jan Kratochvil has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - update test/lib-test/jdk/test/whitebox/CPUInfoTest.java
>  - remove args
>  - List CPUFeatures in Java
>  - Fix static_assert

Would be great if others took a look too, I won't say I've read into every detail of the changes

src/hotspot/cpu/x86/vm_version_x86.cpp line 919:

> 917:       return retval;
> 918:     }
> 919:   }

`str` does not seem to be modified in the loop, won't we be parsing only the first part of it on each iteration?

src/hotspot/cpu/x86/vm_version_x86.cpp line 920:

> 918:     }
> 919:   }
> 920:   vm_exit_during_initialization(err_msg("VM option 'CPUFeatures=%s' must be of the form: 0xnum,0xnum", str));

This implies `count == 2`, maybe we should add a static assert?

src/hotspot/cpu/x86/vm_version_x86.cpp line 1117:

> 1115:               assert(!shouldnotuse.supports_feature(CPU_MOVBE), "CPU_MOVBE in both _features and shouldnotuse cannot happen");
> 1116:               // FMA is 2012+, AVX2+BMI1+BMI2+LZCNT are 2013+, MOVBE is 2015+
> 1117:               shouldnotuse.set_feature( CPU_MOVBE);

Suggestion:

              shouldnotuse.set_feature(CPU_MOVBE);

src/hotspot/cpu/x86/vm_version_x86.cpp line 2542:

> 2540: 
> 2541: // Print the feature names as " = feat1, ..., featN\n";
> 2542: void VM_Version::missing_features(VM_Version::VM_Features features_missing) {

`features_missing` could be a const reference.

Maybe it is even better to make this a const method of `VM_Version::VM_Features` itself? I also think `print_missing_features` would be a better name. And I don't understand why is it marked `/*[[noreturn]]*/` in the declaration.

src/hotspot/cpu/x86/vm_version_x86.cpp line 2566:

> 2564:   if (ShowCPUFeatures) {
> 2565:     char buf[MAX_CPU_FEATURES * 16];
> 2566:     (*data).print_numbers_and_names(buf, sizeof(buf));

Suggestion:

    data->print_numbers_and_names(buf, sizeof(buf));

src/hotspot/cpu/x86/vm_version_x86.cpp line 2579:

> 2577:     (*data & _features).print_numbers(buf_use, sizeof(buf_use));
> 2578:     char buf_have[MAX_CPU_FEATURES];
> 2579:     (*data).print_numbers(buf_have, sizeof(buf_have));

Suggestion:

    data->print_numbers(buf_have, sizeof(buf_have));

src/hotspot/cpu/x86/vm_version_x86.cpp line 2670:

> 2668:     char buf__features[MAX_CPU_FEATURES];
> 2669:     _features.print_numbers(buf__features, sizeof(buf__features));
> 2670:     tty->print("Specified -XX:CPUFeatures=%s; this machine's CPU features are %s", buf_CPUFeatures_parsed, buf__features);

Suggestion:

    char buf_features[MAX_CPU_FEATURES];
    _features.print_numbers(buf_features, sizeof(buf_features));
    tty->print("Specified -XX:CPUFeatures=%s; this machine's CPU features are %s", buf_CPUFeatures_parsed, buf_features);

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

> 31: #include "utilities/macros.hpp"
> 32: #include "utilities/sizes.hpp"
> 33: #include <utility>

It worries me that this one is not among [the explicitly permitted std lib headers](https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#c-standard-library) so we may start getting compilation errors at some point...

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

PR Review: https://git.openjdk.org/crac/pull/250#pullrequestreview-3055191329
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2230982726
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2230975019
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2230991321
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2231014067
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2231026254
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2231029414
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2231034527
PR Review Comment: https://git.openjdk.org/crac/pull/250#discussion_r2230946338


More information about the crac-dev mailing list