[crac] RFR: 8355974: [CRaC] Move CPUFeatures verification to the parent process of JVM [v11]

[Timofei Pushkin]

On Tue, 13 May 2025 18:01:35 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> Jan Kratochvil has updated the pull request incrementally with six additional commits since the last revision:
>> 
>>  - Fix LoggingVMlogOpenTestNegative.java failing in Github CI
>>  - Remove S3 support
>>  - Revert "Reimplement S3"
>>    
>>    This reverts commit 719e7568262f87d7367c6656827ece796e35cadb.
>>  - Revert "More S3 fixes but it is untested and for 99.9% not yet working"
>>    
>>    This reverts commit 5ce33434f66913f83b53941b4f9aff8d012f6372.
>>  - More S3 fixes but it is untested and for 99.9% not yet working
>>  - Reimplement S3
>
> src/hotspot/share/runtime/crac.cpp line 133:
> 
>> 131:         "with the selected CRaC engine");
>> 132:       break;
>> 133:   }
> 
> A UX improvement: if the user asks to ignore CPU features or the platform doesn't support them the user should not see warnings/errors about the engine not being able to store them.
> 
> Suggestion:
> 
>   if (!VM_Version::ignore_cpu_features()) {
>     VM_Version::CPUFeaturesBinary data;
>     if (VM_Version::cpu_features_binary(&data)) {
>       switch (_engine->prepare_user_data_api()) {
>         case CracEngine::ApiStatus::OK:
>           if (!_engine->cpufeatures_store(&data)) {
>             return JVM_CHECKPOINT_ERROR;
>           }
>           break;
>         case CracEngine::ApiStatus::ERR:
>           return JVM_CHECKPOINT_ERROR;
>         case CracEngine::ApiStatus::UNSUPPORTED:
>           log_warning(crac)("Cannot store CPUFeatures for checkpoint "
>             "with the selected CRaC engine");
>           break;
>       }
>     }
>   }

I realized the above won't work with the current restore code: if we won't record the features because the user asked us to ignore them but the platform and engine supports them then the restore code will error because the features are not present.

But without that the UX is not great and I don't really understand how `CPUFeatures=ignore` is supposed to work (it is not restore-settable but it is not used on checkpoint).

So maybe it's better to change the restore code to warn instead of erroring when the features are not present (and the warning should ideally be silenceable by `CPUFeatures=ignore` or `IgnoreCPUFeatures`).

> src/java.base/share/native/libcrexec/crexec.cpp line 559:
> 
>> 557:   }
>> 558:   FILE *f = fopen(fname, "w");
>> 559:   if (f == NULL) {
> 
> A nitpick, would also be great to replace in the other places in the file
> Suggestion:
> 
>   if (f == nullptr) {

Would also be great to use `static_cast` and friends instead of C-style casts but since it can be cumbersome to replace sometimes I don't insist

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

PR Review Comment: https://git.openjdk.org/crac/pull/227#discussion_r2087398239
PR Review Comment: https://git.openjdk.org/crac/pull/227#discussion_r2087470455