[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