On Tue, 30 May 2023 17:25:35 GMT, Jorn Vernee <jver...@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 restricts the layouts that can be used as variadic layouts to > what is allowed by 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 the > restriction. Comments on that are welcome, but please explain. > > The tests are updated to no longer try to link variadic functions with the > illegal layouts, and I've added some more negative tests to TestIllegalLink. > > Testing: > - local testing on Windows/x64 > - tier1-3 + jdk-tier5 (ongoing) > - manual test run on mac/aarch64 with the fallback linker to verify the > correctness of the fallback linker changes. Looks good src/java.base/share/classes/java/lang/foreign/Linker.java line 415: > 413: * It should be noted that values passed as variadic arguments undergo > default argument promotion in C. Each value of > 414: * type {@code float} is converted to {@code double}, and each integral > type smaller than {@code int} is converted to > 415: * {@code int}. As such, the native linker will reject attempts to link > a function with variadic parameters which have maybe "will reject attempts to link a variadic function if one or more parameter layouts in the provided function descriptor which correspond to a variadic argument is a value layout such that ..." test/jdk/java/foreign/StdLibTest.java line 386: > 384: return arena.allocateUtf8String("str"); > 385: }, "str"), > 386: CHAR(int.class, C_INT, "%c", arena -> (int) 'h', (int) 'h'), // > promote to int, per C spec is it important to retain this, given that there's already an INT case? ------------- Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14225#pullrequestreview-1453564448 PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211924259 PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211927081
