RFR: 8308031: Linkers should promote variadic arguments [v6]

Maurizio Cimadamore mcimadamore at openjdk.org
Fri Jun 2 15:55:13 UTC 2023


On Fri, 2 Jun 2023 15:16:28 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and `float` is promoted to `double`, when being passed as variadic argument (see e.g. https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). This patch adds the automatic argument promotion for variadic arguments, to align the implementation with the C specification.
>> 
>> The fallback linker is also updated to use to correct function to link variadic calls (not doing this turned out not to be a problem so far, but it is problematic for instance on Mac/AArch64 when using the fallback linker). Adding the restriction on layouts for all linkers is also partly motivated by the fallback linker rejecting such unsupported variadic layouts already.
>> 
>> I've added a small paragraph to the Linker javadoc as well that explains this.
>> 
>> TestVarArgs needed to be updated in order to account for the difference in the promoted `float` values.
>> 
>> Testing:
>> - local testing on Windows/x64
>> - local testing on Linux/x64 using the fallback linker
>> - tier1-3 + jdk-tier5
>> - manual test run on mac/aarch64 with the fallback linker to verify the correctness of the fallback linker changes.
>
> Jorn Vernee 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 11 additional commits since the last revision:
> 
>  - Merge branch 'master' into VAFixes
>  - undo spurious whitespace changes
>  - automatically apply variadic promotions
>  - move prototype-less name
>  - Rework javadoc
>  - review comments
>  - fix word order
>  - adjust whitespace
>  - simplify test changes
>  - reject invalid variadic layouts
>  - ... and 1 more: https://git.openjdk.org/jdk/compare/5aae3daf...9daadbfc

Looks good. I left minor stylistic comments.

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 261:

> 259:     // and float is promoted to double
> 260:     // See: https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions
> 261:     private static FunctionDescriptor promoteVariadicArgs(FunctionDescriptor function, int firstVariadicArgIndex) {

Maybe it's unneeded optimization, but we're scanning the list of layouts once, and then once again to determine the resulting method type. Should we try to do both in one pass?

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 327:

> 325:     }
> 326: 
> 327:     private static boolean requiresVariadicFloatPromotion(ValueLayout vl) {

Should this be called `requiresVariadicDoublePromotion` ? (In the other method we say `int`, which is the target of the conversion).

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

Marked as reviewed by mcimadamore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14225#pullrequestreview-1457862860
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1214549997
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1214545214


More information about the core-libs-dev mailing list