Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v4]
On Thu, 1 Jun 2023 15:28:33 GMT, Jorn Vernee 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. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > move prototype-less name After some offline discussion with Maurizio, we concluded that the linker should automatically apply the needed argument promotions, rather than reject the 'incorrect' layouts. This is more convenient to the user because they don't have to figure out how to do the promotions, but also because they don't have to figure out the platform dependent promotion process. - PR Comment: https://git.openjdk.org/jdk/pull/14225#issuecomment-1573892839
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v4]
On Thu, 1 Jun 2023 15:28:33 GMT, Jorn Vernee 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. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > move prototype-less name Thanks for the review! I've updated the CSR with the finalized version of the javadoc. Could you take a look at that as well? https://bugs.openjdk.org/browse/JDK-8309205 - PR Comment: https://git.openjdk.org/jdk/pull/14225#issuecomment-1572275551
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v4]
> 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. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: move prototype-less name - Changes: - all: https://git.openjdk.org/jdk/pull/14225/files - new: https://git.openjdk.org/jdk/pull/14225/files/e806fae5..1f1e22eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14225&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14225&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14225.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14225/head:pull/14225 PR: https://git.openjdk.org/jdk/pull/14225
