8368929: [CRaC] Move CPUFeatures check to C/R engine #266
Conversation
|
@jankratochvil Please give this a review, since you've authored most of the modified code. |
|
👋 Welcome back rvansa! A progress list of the required criteria for merging this PR into |
|
@rvansa This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1 new commit pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
ready
Webrevs
|
| #define RESTORE_ERROR_UNKNOWN -1 // generic error | ||
| #define RESTORE_ERROR_NOT_FOUND -2 // image location does not contain an image | ||
| #define RESTORE_ERROR_NO_ACCESS -3 // image cannot be accessed/retrieved (permissions or I/O issue) | ||
| #define RESTORE_ERROR_INVALID -4 // image is not suitable (e.g. corruption or wrong architecture) | ||
| #define RESTORE_ERROR_MEMORY -5 // memory allocation failure during restore | ||
| #define RESTORE_ERROR_PROCINFO -6 // the process cannot fetch information about itself |
There was a problem hiding this comment.
Cannot it be an enum?
There was a problem hiding this comment.
The API already defines return value as int, we don't want to change that part.
| static constexpr size_t print_buffer_length() { | ||
| return MAX_CPU_FEATURES; | ||
| } |
There was a problem hiding this comment.
I believe by the time the corresponding printing function is used the resource area is already initialized. It thus should be possible to make it allocate a resource array internally instead of making the callers use this to allocate on stack. Or there can be two functions: one that uses a provided buffer and its wrapper that allocates a resource array.
| } | ||
|
|
||
| T& add(T&& item) { | ||
| Node *node = new(std::nothrow) Node(); |
There was a problem hiding this comment.
Should do a null check after this
| Node *_head; | ||
| Node *_tail; | ||
| size_t _size; | ||
| public: | ||
| LinkedList(): _head(nullptr), _tail(nullptr), _size(0) {} |
There was a problem hiding this comment.
| Node *_head; | |
| Node *_tail; | |
| size_t _size; | |
| public: | |
| LinkedList(): _head(nullptr), _tail(nullptr), _size(0) {} | |
| Node *_head = nullptr; | |
| Node *_tail = nullptr; | |
| size_t _size = 0; | |
| public: |
| } | ||
| } | ||
|
|
||
| size_t size() { |
There was a problem hiding this comment.
| size_t size() { | |
| size_t size() const { |
| template<typename Func> void foreach(Func f) const { | ||
| Node *node = _head; | ||
| while (node) { | ||
| f(node->_item); | ||
| node = node->_next; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't insist but implementing an std::iterator does not seem too cumbersome and would allow to use the normal for loop
| while (fgets(line, sizeof(line), f)) { | ||
| char *eq = strchr(line, '='); | ||
| char *nl = strchr(eq + 1, '\n'); |
There was a problem hiding this comment.
I believe parsing will be broken if = is encountered in the tag name itself. Same for \n although that is less likely.
| if (f == nullptr) { | ||
| return errno == ENOENT ? RESTORE_ERROR_NOT_FOUND : RESTORE_ERROR_NO_ACCESS; | ||
| } | ||
| char line[strlen(BITMAP_PREFIX) + _max_name_length + 1 + _max_value_length + 2]; |
There was a problem hiding this comment.
AFAIR MSVC does not support arrays of non-constexpr size
| tags.add({ | ||
| .type = LABEL, | ||
| .name = strdup(line + strlen(LABEL_PREFIX)), | ||
| .data = strdup(eq + 1), | ||
| .data_length = (size_t) (nl - eq), | ||
| }); |
There was a problem hiding this comment.
Missing error checking here and below (add, strdup, malloc)
| &user_data_extension.header, | ||
| &image_constraints_extension.header, |
There was a problem hiding this comment.
Since we replace the direct use of user data with this new extension it should be a bit more efficient to put the new extension above the old one
| int error = conf->image_constraints().validate(conf->argv()[ARGV_IMAGE_LOCATION]); | ||
| if (error) { | ||
| return error; | ||
| } |
There was a problem hiding this comment.
Nitpick: let's move this before initializing restore_data_str as this is a part of argument validation while restore_data_str and below is preparation for exec-ing
Right now the logic checking if CPU features used before checkpoint match current CPU features is in VM code. VM stores and retrieves CPU features through C/R API's user_data extension. This is convenient when we have a single image that can be either accepted or rejected, but does not offer the flexibility for C/R engine to select the best image for current execution environment.
The goal of this issue is to move to a declarative API that will express the requirements using more abstract means, labels (for CPU architecture) and bitmaps (for CPU features).
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/266/head:pull/266$ git checkout pull/266Update a local copy of the PR:
$ git checkout pull/266$ git pull https://git.openjdk.org/crac.git pull/266/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 266View PR using the GUI difftool:
$ git pr show -t 266Using diff file
Download this PR as a diff file:
https://git.openjdk.org/crac/pull/266.diff
Using Webrev
Link to Webrev Comment