[crac] RFR: 8364662: [CRaC] Process restore options in the common arguments parser
Timofei Pushkin
tpushkin at openjdk.org
Thu Aug 7 09:02:37 UTC 2025
On Thu, 7 Aug 2025 08:01:45 GMT, Radim Vansa <rvansa at openjdk.org> wrote:
>> Options for restore are now processed during the common VM arguments parsing stage.
>>
>> Notable improvements compared to the old code:
>> - Options from `-XX:Flags=<file>` and `-XX:VMOptionsFile=<file>` are now also considered
>> - Option aliasing is now handled correctly
>>
>> **Behavioral change**. If `CRaCIgnoreRestoreIfUnavailable` is true it is now not an error to specify non-restore-settable options on restore — these will just be omitted from forwarding to the restored JVM. This makes it possible to specify non-restore-settable to be used after a failed restore.
>
> src/hotspot/share/runtime/arguments.cpp line 1046:
>
>> 1044: }
>> 1045:
>> 1046: void Arguments::build_jvm_restore_flags(const char* arg) {
>
> I would use `append_jvm_restore_flags` - `build` implies the final operation on a builder.
I follow the style of the class: there are `Arguments::build_jvm_args` and `Arguments::build_jvm_flags` which do exactly the same thing, just for two different arrays.
> src/hotspot/share/runtime/crac.cpp line 638:
>
>> 636: // should be concatenated. But with the current code the last occurence
>> 637: // will just overwrite the previous ones.
>> 638: assert(!JVMFlag::find_flag(cursor)->ccstr_accumulates(),
>
> Ouch! I think that using multiple `-XX:CRaCEngineOptions` would be pretty common (at least some of my scripts do that). Could we fix this?
>
> And this error should be guarantee/another check that is applied in release build, too because it verifies user input, not some invariants in internal components.
This is not an issue for `CRaCEngineOptions` because it is not applied in the restored process (you can see it is handled in another branch above).
Currently there are no `ccstrlist` restore-settables actually being applied in the restored process, that is why I just added this assert for now. Its purpose is to fail when we add such option.
-------------
PR Review Comment: https://git.openjdk.org/crac/pull/258#discussion_r2259626820
PR Review Comment: https://git.openjdk.org/crac/pull/258#discussion_r2259620693
More information about the crac-dev
mailing list