RFR: 8341471: Reversed field layout caused by unstable sorting [v2]

[Johan Sjölen]

On Tue, 8 Oct 2024 10:26:42 GMT, Fei Gao <fgao at openjdk.org> wrote:

>> For class Test:
>> 
>>     public class Test {
>>         char a000;
>>         char a001;
>>         char a002;
>>         char a003;
>>         char a004;
>>         char a005;
>>         char a006;
>>         char a007;
>>         char a008;
>>         char a009;
>>         char a00a;
>>         char a00b;
>>     }
>> 
>> 
>> We found its field layout on macOS was:
>> 
>> 
>> Layout of class Test
>>   Instance fields:
>>    @0 12/- RESERVED
>>    @12 "a00b" C 2/2 REGULAR
>>    @14 "a001" C 2/2 REGULAR
>>    @16 "a002" C 2/2 REGULAR
>>    @18 "a003" C 2/2 REGULAR
>>    @20 "a004" C 2/2 REGULAR
>>    @22 "a005" C 2/2 REGULAR
>>    @24 "a006" C 2/2 REGULAR
>>    @26 "a007" C 2/2 REGULAR
>>    @28 "a008" C 2/2 REGULAR
>>    @30 "a009" C 2/2 REGULAR
>>    @32 "a00a" C 2/2 REGULAR
>>    @34 "a000" C 2/2 REGULAR
>> 
>> 
>> `a000` was put in the end while `a00b` was put in the beginning.
>> 
>> Fields get sorted according to size in [1]. `qsort()` on macOS reverses the order of fields with the same size. We should extend the comparison function to preserve the order on macOS, as we did on Windows.
>> 
>> Tier 1-3 passed on macOS.
>> 
>> [1] https://github.com/openjdk/jdk/blob/3ee94e040a7395d11a294a6b660d707c97f188f8/src/hotspot/share/classfile/fieldLayoutBuilder.cpp#L102
>
> Fei Gao 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:
> 
>  - Extend the comparison function to all platforms
>  - Merge branch 'master' into field_layout_fix
>  - 8341471: [macOS_aarch64] Reversed field layout caused by unstable sorting
>    
>    For class Test:
>    ```
>        public class Test {
>            char a000;
>            char a001;
>            char a002;
>            char a003;
>            char a004;
>            char a005;
>            char a006;
>            char a007;
>            char a008;
>            char a009;
>            char a00a;
>            char a00b;
>        }
>    ```
>    
>    We found its field layout on macOS was:
>    
>    Layout of class Test
>      Instance fields:
>       @0 12/- RESERVED
>       @12 "a00b" C 2/2 REGULAR
>       @14 "a001" C 2/2 REGULAR
>       @16 "a002" C 2/2 REGULAR
>       @18 "a003" C 2/2 REGULAR
>       @20 "a004" C 2/2 REGULAR
>       @22 "a005" C 2/2 REGULAR
>       @24 "a006" C 2/2 REGULAR
>       @26 "a007" C 2/2 REGULAR
>       @28 "a008" C 2/2 REGULAR
>       @30 "a009" C 2/2 REGULAR
>       @32 "a00a" C 2/2 REGULAR
>       @34 "a000" C 2/2 REGULAR
>    
>    `a000` was put in the end while `a00b` was put in the beginning.
>    
>    Fields get sorted according to size in [1]. `qsort()` on macOS
>    reverses the order of fields with the same size. We should extend
>    the comparison function to preserve the order on macOS, as we
>    did on Windows.
>    
>    Tier 1-3 passed on macOS.
>    
>    [1] https://github.com/openjdk/jdk/blob/3ee94e040a7395d11a294a6b660d707c97f188f8/src/hotspot/share/classfile/fieldLayoutBuilder.cpp#L102

Please add a comment that mentions the reason for the branch, for example

```c++
// ensure stable sort
if (...) {

}

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

PR Review: https://git.openjdk.org/jdk/pull/21382#pullrequestreview-2354323590