RFR: Store cpu features in AOTCodeCache header [v2]

[Ashutosh Mehra]

On Tue, 15 Jul 2025 00:45:05 GMT, John R Rose <jrose at openjdk.org> wrote:

>> 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;
> }

@rose00 Thanks for catching that bug. I have refactored the code a bit more than you suggested by adding the static method in the FormatBuffer. I think FormatBuffer is a good fit for this use case as the client code doesn't need to worry about the temporary buffer and size anymore. Please check https://github.com/openjdk/leyden/pull/84/commits/9018729b6cc618a38a9cc8e0ec595664e2f35e5e to see the changes in isolation from rest of the patch. Does it look good?

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

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