RFR: Store cpu features in AOTCodeCache header [v2]

[John R Rose]

On Mon, 14 Jul 2025 22:13:36 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

>> This is the initial version of storing cpu features in the AOTCodeCache to verify runtime env has the same cpu capabilities as the assembly env. It covers both x86 and aarch64.
>> AOTCodeCache header is updated to store the cpu features in arch-dependent form (although its same for currently supported architectures - x86 and aarch64).
>> 
>> It also fixes a bug - the `polling_page_vectors_safepoint_handler_blob` can be null if AVX is not present on a system. This causes crash as this blob's entry point is stored in the address table.
>> I came across this when I did the assembly run with -XX:UseAVX=0 option.
>
> Ashutosh Mehra has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'premain' into aot-cache-feature-flags
>  - Address review comments
>    
>    Signed-off-by: Ashutosh Mehra <asmehra at redhat.com>
>  - Store cpu features in AOTCodeCache header
>    
>    Signed-off-by: Ashutosh Mehra <asmehra at redhat.com>

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 736:

> 734: }
> 735: 
> 736: void VM_Version::insert_features_names(uint64_t features, char* names_buf, size_t buf_size) {

Uh, there is a DRY failure here, which leads to a bug.  (DRY = Don't Repeat Yourself.)

It appears to be a modified copy from another port, and this copy has a buffer overflow bug in the modifications.

And the same bug occurs in another modified copy.

Reviewers and porters should not have to repeatedly debug the same subtle code in multiple platform-specific files.

The bug is that `buf_rem` is a dead variable, while loop-invariant `buf_size` is passed with `buf` to `snprintf`.  A hasty reader might think that `buf` and `buf_len` vary together but they don't; this is a bad naming choice that masks the bug.

Instead of point-fixing the bug (in at least two places), please consider factoring this out somewhere that all platforms can use it, and then all the reviewers can make sure that the single copy is bug-free.  Otherwise, we have a new bug surface on all future ports.  (Yes, this a small thing, but let's do it right.)

I suggest making this be a template function that takes a closure (of type `int->char*`) and iterates it over a range `0..N-1`, accumulating the closure-produced strings (if not null) via `snprintf` into a sized buffer, safely.  Return the number of written characters.  I looked for possibly related APIs and found FormatBuffer but it is poorly documented.  I suggest just making a public static helper function in class outputStream.  That's clean enough IMO.


public: static
template<typename FN>
size_t insert_string_list(char* buf, size_t buf_size, int start, int limit, FN fn) {
  size_t np = 0;
  ...
  for (int i = start; i < limit; i++) {
    const char* str = fn(i);
    if (str == nullptr)  continue;
    const char* comma = (first && !use_first_comma) ? "" : ", ";
    auto res = jio_snprintf(buf + np, buf_size - np, "%s%s", comma, str);
    np += res;
    if (res < 0 || np+1 > buf_size)  return -1;
  }
  return np;
}

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

PR Review Comment: https://git.openjdk.org/leyden/pull/84#discussion_r2206057482