Re: RFR: JDK-8323330: [BACKOUT] JDK-8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
On Wed, 10 Jan 2024 09:18:53 GMT, Matthias Baesken wrote: > There have been concerns raised about > [JDK-8276809](https://bugs.openjdk.org/browse/JDK-8276809) , so bring back > the old behavior. okay - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17341#pullrequestreview-1814102655
Re: RFR: JDK-8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
On Tue, 2 Jan 2024 15:03:12 GMT, Matthias Baesken wrote: > The new test java/awt/font/JNICheck/FreeTypeScalerJNICheck.java introduced > with https://bugs.openjdk.java.net/browse/JDK-8269223 adds -Xcheck:jni , and > shows on Windows server 2019 the following JNI warning , so the test fails on > this Windows version. > > stdout: [WARNING in native method: JNI call made without checking exceptions > when required to from CallStaticVoidMethodV > at > sun.awt.Win32GraphicsEnvironment.initDisplay(java.desktop@23-internal/Native > Method) > at > sun.awt.Win32GraphicsEnvironment.initDisplayWrapper(java.desktop@23-internal/Win32GraphicsEnvironment.java:95) > at > sun.awt.Win32GraphicsEnvironment.(java.desktop@23-internal/Win32GraphicsEnvironment.java:63) > at > sun.awt.PlatformGraphicsInfo.createGE(java.desktop@23-internal/PlatformGraphicsInfo.java:34) > at > java.awt.GraphicsEnvironment$LocalGE.createGE(java.desktop@23-internal/GraphicsEnvironment.java:93) > at > java.awt.GraphicsEnvironment$LocalGE.(java.desktop@23-internal/GraphicsEnvironment.java:84) > at > java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(java.desktop@23-internal/GraphicsEnvironment.java:106) > at FreeTypeScalerJNICheck.runTest(FreeTypeScalerJNICheck.java:53) > at FreeTypeScalerJNICheck.main(FreeTypeScalerJNICheck.java:44) > > > We better add an exception check to get rid of the JNI warning (and also of > the test failure) . Hi Phil, sorry for this, this was not bad intent, just a plain mistake. It is somewhat difficult to remember the exact review rules per project. Help from Skara would certainly be useful here. Cheers, Thomas - PR Comment: https://git.openjdk.org/jdk/pull/17224#issuecomment-1883471813
Re: RFR: JDK-8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
On Tue, 2 Jan 2024 15:03:12 GMT, Matthias Baesken wrote: > The new test java/awt/font/JNICheck/FreeTypeScalerJNICheck.java introduced > with https://bugs.openjdk.java.net/browse/JDK-8269223 adds -Xcheck:jni , and > shows on Windows server 2019 the following JNI warning , so the test fails on > this Windows version. > > stdout: [WARNING in native method: JNI call made without checking exceptions > when required to from CallStaticVoidMethodV > at > sun.awt.Win32GraphicsEnvironment.initDisplay(java.desktop@23-internal/Native > Method) > at > sun.awt.Win32GraphicsEnvironment.initDisplayWrapper(java.desktop@23-internal/Win32GraphicsEnvironment.java:95) > at > sun.awt.Win32GraphicsEnvironment.(java.desktop@23-internal/Win32GraphicsEnvironment.java:63) > at > sun.awt.PlatformGraphicsInfo.createGE(java.desktop@23-internal/PlatformGraphicsInfo.java:34) > at > java.awt.GraphicsEnvironment$LocalGE.createGE(java.desktop@23-internal/GraphicsEnvironment.java:93) > at > java.awt.GraphicsEnvironment$LocalGE.(java.desktop@23-internal/GraphicsEnvironment.java:84) > at > java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(java.desktop@23-internal/GraphicsEnvironment.java:106) > at FreeTypeScalerJNICheck.runTest(FreeTypeScalerJNICheck.java:53) > at FreeTypeScalerJNICheck.main(FreeTypeScalerJNICheck.java:44) > > > We better add an exception check to get rid of the JNI warning (and also of > the test failure) . +1. I agree with Ralf, logging is not that important. Possibly just add a (c-runtime) assert. Up to you. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17224#pullrequestreview-1808867637
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v13]
On Thu, 2 Nov 2023 11:02:27 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes >> the Visual C compiler much less accepting of ill formed code, which will >> improve code quality on Windows in the future. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 42 commits: > > - Merge branch 'master' into patch-10 > - Use permissive- to check for errors now in flags-cflags.m4 > - Formatting part two awt_Frame.cpp > - Formatting awt_Frame.cpp > - Split in awt_Frame.cpp > - Formatting in awt_Component.cpp > - Part two awt_Component.cpp > - Split into declaration and assignment part one awt_Component.cpp > - Formatting in awt_Canvas.cpp > - Split declaration and assignment in awt_Canvas.cpp > - ... and 32 more: https://git.openjdk.org/jdk/compare/64f8253b...0b2bda52 I'm removing myself from this. There are enough eyes on this. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1790521227
Re: RFR: 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler [v2]
On Wed, 9 Aug 2023 06:53:49 GMT, David Holmes wrote: >> I wrote this code ages ago. I'm not sure what's weird or suspicious about >> it, though. The comment at the file's beginning explains this code's >> motivation. >> >> The buffer was never thought to be used for something different than HANDLEs >> or characters, where the assignment of integer literals work. I often use >> char constants for sentinels as debugging aid. As for `'\0'`, that indicates >> to the casual code reader that this is a termination of a string, better >> than had I used a plain 0. > > Because there is nothing to state what T may be, I found assigning character > literals to be odd. If T is char and the buffer is meant to be a C string > then it makes more sense. But for non-char T it just raised questions for me. I see that. We can remove the sentinel stuff, which leaves us with the zero termination. Arguably, this could be done by sub classes that derive from the char instantiation. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1288083649
Re: RFR: 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler [v2]
On Mon, 7 Aug 2023 06:42:41 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). It can be done >> with some effort, given that the significantly stricter gcc can now compile >> an experimental Windows JDK as of 2023, and will serve to significantly cut >> down on monstrosities in ancient Windows code > > Julian Waters 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 22 additional > commits since the last revision: > > - Mismatched declaration in D3DGlyphCache.cpp > - Fields in awt_TextComponent.cpp > - reinterpret_cast needed in AccessBridgeJavaEntryPoints.cpp > - Qualifiers in awt_PrintDialog.h should be removed > - Likewise for awt_DnDDT.cpp > - awt_ole.h include order issue in awt_DnDDS.cpp > - Revert awt_ole.h > - Earlier fix in awt_ole.h was not complete > - Merge branch 'openjdk:master' into patch-10 > - Likewise for awt_Frame.cpp > - ... and 12 more: https://git.openjdk.org/jdk/compare/15f348cb...51230f3d src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1641: > 1639: } > 1640: } > 1641: A possible improvement later (and for a future RFE) would be to use RAII for deletion and then get rid of the labels. awt is one of the few places that uses C++ for native code, so why not. src/java.desktop/windows/native/libawt/windows/awt_TextComponent.cpp line 59: > 57: AwtTextComponent::OleCallback AwtTextComponent::sm_oleCallback; > 58: WNDPROC AwtTextComponent::sm_pDefWindowProc = NULL; > 59: Did the compiler complain here? I'm fine with the change, just wanted to know the reason. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1288013587 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1288015274
Re: RFR: 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler [v2]
On Wed, 9 Aug 2023 04:00:03 GMT, Julian Waters wrote:
>> src/hotspot/os/windows/os_windows.cpp line 2888:
>>
>>> 2886: LONG WINAPI topLevelUnhandledExceptionFilter(struct
>>> _EXCEPTION_POINTERS* exceptionInfo) {
>>> 2887: if (!InterceptOSException) {
>>> 2888: DWORD exception_code =
>>> exceptionInfo->ExceptionRecord->ExceptionCode;
>>
>> I find this less clearer than the original code, that didn't use negation -
>> it was clear that InterceptOSException leads to immediate bailout.
>>
>> What is the problem, the goto? is the compiler complaining? If so, I would
>> vote for duplicating the return statement here (and this would count as an
>> example where discarding out-of-fashion statements like goto actually makes
>> the code worse).
>
> Hi Thomas, the goto jumps over a lot of variable initializations, which
> permissive- doesn't particularly like (the failing compilation can actually
> be seen in the earlier GHA logs)
Ah, okay. Thanks for clarifying.
>> src/java.desktop/windows/native/libawt/windows/awt_DnDDS.cpp line 48:
>>
>>> 46:
>>> 47: #include "jlong.h"
>>> 48: #include "awt.h"
>>
>> Why the reordering of includes?
>
> This is a weird one, but in awt we #define malloc Do_Not_Use_Malloc... And so
> on. Without this reordering awt_ole.h (which includes comdef.h) also uses the
> redefined malloc somewhere (I could not find where in comip.h that it's used)
> which breaks compilation. I do find it strange that without permissive- it
> doesn't break though. Same applies to the other reordering of includes
Oh. That's not good. Having such an undocumented reliance on order of include
just begs to bitrot at some point. Any chance you could unravel that mystery,
maybe in a later RFE? For now, can you please add a comment at those places
where you changed include order for that reason?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1288007914
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1288009545
Re: RFR: 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler [v2]
On Mon, 7 Aug 2023 06:42:41 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). It can be done >> with some effort, given that the significantly stricter gcc can now compile >> an experimental Windows JDK as of 2023, and will serve to significantly cut >> down on monstrosities in ancient Windows code > > Julian Waters 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 22 additional > commits since the last revision: > > - Mismatched declaration in D3DGlyphCache.cpp > - Fields in awt_TextComponent.cpp > - reinterpret_cast needed in AccessBridgeJavaEntryPoints.cpp > - Qualifiers in awt_PrintDialog.h should be removed > - Likewise for awt_DnDDT.cpp > - awt_ole.h include order issue in awt_DnDDS.cpp > - Revert awt_ole.h > - Earlier fix in awt_ole.h was not complete > - Merge branch 'openjdk:master' into patch-10 > - Likewise for awt_Frame.cpp > - ... and 12 more: https://git.openjdk.org/jdk/compare/59027534...51230f3d I think I see now that the added scopes were to prevent variable life scopes from intersecting goto's? - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1670293563
Re: RFR: 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler [v2]
On Mon, 7 Aug 2023 06:42:41 GMT, Julian Waters wrote:
>> We should set the -permissive- flag for the Microsoft Visual C compiler, as
>> was requested by the now backed out
>> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). It can be done
>> with some effort, given that the significantly stricter gcc can now compile
>> an experimental Windows JDK as of 2023, and will serve to significantly cut
>> down on monstrosities in ancient Windows code
>
> Julian Waters 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 22 additional
> commits since the last revision:
>
> - Mismatched declaration in D3DGlyphCache.cpp
> - Fields in awt_TextComponent.cpp
> - reinterpret_cast needed in AccessBridgeJavaEntryPoints.cpp
> - Qualifiers in awt_PrintDialog.h should be removed
> - Likewise for awt_DnDDT.cpp
> - awt_ole.h include order issue in awt_DnDDS.cpp
> - Revert awt_ole.h
> - Earlier fix in awt_ole.h was not complete
> - Merge branch 'openjdk:master' into patch-10
> - Likewise for awt_Frame.cpp
> - ... and 12 more: https://git.openjdk.org/jdk/compare/c9dcbf20...51230f3d
Hi Julian,
Cursory review. I stopped halfway in when I noticed that I had no idea why many
of the changes were done. They did not seem to have an obvious connection to
the title of the JBS, and I did not find an explanation.
I also find the usage of "monstrosities" unnecessary. It is very much a matter
of taste - to me, nothing in the code you changed looks monstrous.
Cheers, Thomas
src/hotspot/os/windows/os_windows.cpp line 2888:
> 2886: LONG WINAPI topLevelUnhandledExceptionFilter(struct
> _EXCEPTION_POINTERS* exceptionInfo) {
> 2887: if (!InterceptOSException) {
> 2888: DWORD exception_code =
> exceptionInfo->ExceptionRecord->ExceptionCode;
I find this less clearer than the original code, that didn't use negation - it
was clear that InterceptOSException leads to immediate bailout.
What is the problem, the goto? Is that just your personal taste, or is the
compiler complaining? If the latter, I would vote for duplicating the return
statement here (and this would count as an example where discarding
out-of-fashion statements like goto actually makes the code worse).
src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp line 216:
> 214: {
> 215: PDATA pData;
> 216: JNI_CHECK_PEER_GOTO(canvas, ret);
Here, and other places: why this scope?
src/java.desktop/windows/native/libawt/windows/awt_DnDDS.cpp line 48:
> 46:
> 47: #include "jlong.h"
> 48: #include "awt.h"
Why the reordering of includes?
src/java.desktop/windows/native/libawt/windows/awt_DnDDT.cpp line 34:
> 32: #include "sun_awt_windows_WDropTargetContextPeer.h"
> 33: #include "awt_Container.h"
> 34: #include "awt_ole.h"
Why?
-
PR Review: https://git.openjdk.org/jdk/pull/15096#pullrequestreview-1568088318
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1287629714
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1287630358
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1287631591
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1287632623
Re: RFR: 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler [v2]
On Mon, 7 Aug 2023 08:14:41 GMT, Julian Waters wrote: >> There are currently only 2 possible types of HMODULE and char/uint8_t for T >> at the moment. Weirdly enough only line 126 errors out without the cast >> while line 114, despite having the same problem, doesn't, but I added the >> cast to both lines for consistency. If someone else knows why I could >> probably deal with this code in a better way besides just casting it to T >> (which I also am reluctant to do) > > I just checked and the value of the sentinel is ultimately the prvalue 88. I > don't know if we'd want to replace all the weird char usages here with > explicit values of 0 (and 88 for the sentinel). Maybe future reviews can help > with that I wrote this code ages ago. I'm not sure what's weird or suspicious about it, though. The comment at the file's beginning explains this code's motivation. The buffer was never thought to be used for something different than HANDLEs or characters, where the assignment of integer literals work. I often use char constants for sentinels as debugging aid. As for `'\0'`, that indicates to the casual code reader that this is a termination of a string, better than had I used a plain 0. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1287626062
Re: RFR: JDK-8313164: src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels adjust releasing of resources
On Wed, 26 Jul 2023 10:43:20 GMT, Matthias Baesken wrote: > In src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels > we release some resources at the end of the function by calling > DeleteObject/DeleteDC. This is recommended by the MS API docs. > However this should be done as well in some early leaving with throw that can > occur in this function. Okay. Did you find this with SonarCloud? Since this is c++, it may be simpler to use RAII for this. Up to you though, the patch is also fine in its current form. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15038#pullrequestreview-1547515501
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
On Mon, 19 Jun 2023 06:55:52 GMT, Axel Boldt-Christmas
wrote:
>> The current implementation for testing generational ZGC with jtreg is
>> implemented with a filter on the mode flag `ZGenerational`. Because of this
>> only environments which set this flag explicitly will run most of the tests.
>> So they get missed in Github Actions and for developers running jtreg
>> locally without supplying the `ZGenerational` flag.
>>
>> The proposed change here is to introduce two new jtreg requirement
>> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
>> effectively behave the same as the existing `vm.gc.` flags but also take
>> the specific ZGC mode in account.
>>
>> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
>> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` will be true.
>>
>> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
>> `vm.gc.ZSinglegen` will also be true.
>>
>> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational`
>> or `vm.gc.ZSinglegen` be true depending on the flags value.
>>
>> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen`
>> will be false.
>>
>> This change also splits the relevant tests into two distinct runs for the
>> two modes. And the respective test ids are set to `ZGenerational` or
>> `ZSinglegen` to make it easier to distinguish the runs.
>>
>> This also solves the issue that some compiler tests will never run with
>> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
>> current filter `vm.opt.final.ZGenerational` requires the flag to be
>> explicit, but these compiler tests uses `vm.flagless`.
>>
>> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` harmonizes
>> the way you specify generational / single gen ZGC test with the way it is
>> done for other gcs with `vm.gc.`
>>
>> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
>> to enable checking if `ZGenerational` is default or not.
>>
>> `vm.opt.final.ZGenerational` is still kept because we still have some
>> reasons to filter based on the supplied flags.
>> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when
>> running with ZGenerational
>> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
>> max heap size for ZGenerational, but it is not the intent to dispatch the
>> test to both G1 and generational ZGC if Generational ZGC is not specified.
>>
>> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also
>> changed to not filter but instead dispatch. However u...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add explicit id to all Skynet.java @test
I'm not the most qualified for ZGC, but I looked this over with a mind on the
planned 21 backport. I wanted to check if any of the existing legacy ZGC tests
had been changed, either accidentally or deliberately. This seems not be the
case.
The remarks are small nits, feel free to ignore them.
How much time do we spend now in GHAs on the additional Zgenerational tests? In
any case, this makes sense.
Cheers, Thomas
test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationTools.java line 298:
> 296: if (selectedGCMode != null) {
> 297: args.add(selectedGCMode);
> 298: }
just to be sure, maybe add a "must not be Z" assert in the else path?
test/hotspot/jtreg/runtime/cds/appcds/TestZGCWithCDS.java line 58:
> 56: public final static String ERR_MSG = "The saved state of
> UseCompressedOops and UseCompressedClassPointers is different from runtime,
> CDS will be disabled.";
> 57: public static void main(String... args) throws Exception {
> 58: String zGenerational = args[0];
assert not null?
-
Marked as reviewed by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14509#pullrequestreview-1485730970
PR Review Comment: https://git.openjdk.org/jdk/pull/14509#discussion_r1233738199
PR Review Comment: https://git.openjdk.org/jdk/pull/14509#discussion_r1233740188
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Sat, 27 May 2023 11:25:41 GMT, Thomas Stuefe wrote: >> This one is not just to get rid of a warning. We get real test errors >> because malloc gets replaced by vec_malloc in log tags. > >> This one is not just to get rid of a warning. We get real test errors >> because malloc gets replaced by vec_malloc in log tags. > > That does not invalidate my argument, nor does it answer my question. Those > test errors could be also fixed by renaming the log tag. Maybe that would be > the better way. > > Also, I'm curious, why does it not complain about "free", which is a logtag > as well? I am basically worried that undefining malloc, even if it seems harmless now, exposes us to difficult-to-investigate problems down the road, since it depends on how the libc devs will reform those macros in the future. I would prefer a simple solution that does not depend on how the libc includes evolve. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207907447
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 20:27:12 GMT, Martin Doerr wrote: > This one is not just to get rid of a warning. We get real test errors because > malloc gets replaced by vec_malloc in log tags. That does not invalidate my argument, nor does it answer my question. Those test errors could be also fixed by renaming the log tag. Maybe that would be the better way. Also, I'm curious, why does it not complain about "free", which is a logtag as well? - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207892378
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Thu, 25 May 2023 18:18:43 GMT, Martin Doerr wrote:
>> src/hotspot/share/utilities/globalDefinitions_xlc.hpp line 47:
>>
>>> 45: #undef malloc
>>> 46: extern void *malloc(size_t) asm("vec_malloc");
>>> 47: #endif
>>
>> Wow! And I don't mean that in a good way. I'm not questioning whether this
>> is correct, just commenting
>> on how crazy it seems that such a thing might be needed.
>
> The crazy thing is that `malloc` is defined! That means all places where we
> use the term malloc are getting replaced without such a workaround. (E.g. for
> log tags.)
So, we do this only for malloc? Not for calloc, posix_memalign, realloc etc?
What about free?
As ugly as defining malloc is (and I remember QADRT), I hesitate about removing
that define.
Removing that define and hard-coding it here assumes 1) our replacement is
equivalent (ok, easy to check) 2) it will always be equivalent in future AIX
versions 3) pointers it returns work with the unchanged free() and realloc()
the system provides, and will always do so.
I don't know... I would not do this just to get rid of a warning.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207078176
Re: RFR: 8304350: Font.getStringBounds calculates wrong width for TextAttribute.TRACKING other than 0.0
On Wed, 5 Apr 2023 13:58:48 GMT, Jonathan Dowland wrote: > This is one proposed solution for https://bugs.openjdk.org/browse/JDK-8304350 > > `java.awt.Font.getStringBounds(char[],int,int,FontRenderContext)` applies a > heuristic to determine whether the question it's answering is "simple" or > not. The bug described in 8304350 only occurs in the simple=true branch. > > Extend the "simple?" heuristic to consider a tracking attribute not-simple > and to use the complex branch in those cases. > > One could argue that the root bug still exists: the simple path goes on to > delegate to `sun.font.FontDesignMetrics.getMetrics(Font,FontRenderContext)`, > although that's a private/internal API. This is fine as a workaround. If we find the root cause of this problem we can remove it. Cheers, Thomas - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13352#pullrequestreview-1382688383
Re: RFR: 8304350: Font.getStringBounds calculates wrong width for TextAttribute.TRACKING other than 0.0
On Wed, 5 Apr 2023 13:58:48 GMT, Jonathan Dowland wrote: > This is one proposed solution for https://bugs.openjdk.org/browse/JDK-8304350 > > `java.awt.Font.getStringBounds(char[],int,int,FontRenderContext)` applies a > heuristic to determine whether the question it's answering is "simple" or > not. The bug described in 8304350 only occurs in the simple=true branch. > > Extend the "simple?" heuristic to consider a tracking attribute not-simple > and to use the complex branch in those cases. > > One could argue that the root bug still exists: the simple path goes on to > delegate to `sun.font.FontDesignMetrics.getMetrics(Font,FontRenderContext)`, > although that's a private/internal API. Seems like a reasonable workaround. Would there be any measurable performance impacts by going the more complex route with Tracking != 0? - PR Comment: https://git.openjdk.org/jdk/pull/13352#issuecomment-1501166929
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 21:15:11 GMT, Roger Riggs wrote: > Refactored the way that build time architecture values are mapped to the > Architecture enum. Thank you for this, Roger. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1161075537
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 10:52:47 GMT, Jaikiran Pai wrote: >> src/java.base/share/classes/jdk/internal/util/Architecture.java line 41: >> >>> 39: AARCH64(), >>> 40: RISCV64(), >>> 41: S390(), >> >> Why getting rid of the X in s390x? There has not been an s390 linux kernel >> in almost ten years. > > Hello Thomas, that change was based on the review comment here > https://github.com/openjdk/jdk/pull/13357#discussion_r1159810942 Okay, Lutz is the expert here. Sorry for the noise. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160637280
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 19:25:19 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unneeded qualified export from java.base to jdk.attach src/java.base/share/classes/jdk/internal/util/Architecture.java line 41: > 39: AARCH64(), > 40: RISCV64(), > 41: S390(), Why getting rid of the X in s390x? There has not been an s390 linux kernel in almost ten years. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160610072
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 07:42:51 GMT, Alan Bateman wrote: > > What I meant was: You define PPCLE. PPCLE specifies ppc, little endian. We > > also have PPC big-endian, it is used by AIX (and you can also run Linux > > with PPC big-endian). Why omit that? > > os.arch for AIX is "ppc64". > > So I think you are saying that there are aix-ppc64 and linux-ppc64le builds > so this enum needs to have values for both ppc64 and ppc64le. Yes, precisely. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1500115177
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 20:39:41 GMT, Roger Riggs wrote:
> > What about PPC (big endian)? Used on AIX?
>
> I'm not aware of any code (in OpenJDK) related to big/little endian that is
> derived from `os.arch`.
>
> > On Arm, it may be useful to know whether we built for thumb mode (We
> > recently had this problem in tests).
>
> For tests, there is the test library `jdk.test.lib.Platform` class with its
> own set of predicates.
>
> For the moment, the idea is to do the minimum to replace OpenJDK internal
> uses of `os.arch`.
What I meant was: You define PPCLE. PPCLE specifies ppc, little endian. We also
have PPC big-endian, it is used by AIX (and you can also run Linux with PPC
big-endian). Why omit that?
os.arch for AIX is "ppc64". It is even used in our codebase, see
https://github.com/openjdk/jdk/blob/c67bbcea92919fea9b6f7bbcde8ba4488289d174/test/hotspot/jtreg/runtime/ElfDecoder/TestElfDirectRead.java#L51
> > Another question, how does this work with Zero?
>
> Zero is orthogonal to architecture; the interpreter is compiled for a
> specific target architecture.
Sorry, I was not clear. Zero is the vehicle to get the OpenJDK to build and run
on hardware we don't directly support. It is not only used in bootstrapping but
finds surprisingly broad use, e.g., for Debian wich itself supports a wide
range of platforms and tries to provide java on all of them.
For these JDKs, IIUC, os.arch would have the value of the target architecture
("PA-RISC", "mips", "alpha" etc). These platforms would be at a disadvantage
now, as @Glavo pointed out: since OPENJDK_TARGET_CPU is fed from uname:
https://github.com/openjdk/jdk/blob/c67bbcea92919fea9b6f7bbcde8ba4488289d174/make/RunTestsPrebuilt.gmk#L182,
they will have a name that expands to an enum that is not compilable. So all
of these platforms will get build errors.
I understand your intention to keep this patch simple, and that you want to
limit the enum value. I also like enums. I wonder whether it would be possible
to have an enum value "other" and map unknown architectures to that.
-
PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1499986742
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 21:17:25 GMT, Glavo wrote: >> The point of Architecture is reflect the architecture as supported by the >> build and the runtime mutually and to reflect dependencies on the target >> hardware. >> >> What did you use as the example that would not compile on the other >> architecture? >> Did you make the other changes to the build that would be needed for a new >> architecture? > >> What did you use as the example that would not compile on the other >> architecture? > > https://github.com/openjdk/jdk/blob/52ca4a70fc3de9e285964f9545ea8cd54e2d9924/src/java.base/share/classes/jdk/internal/util/OperatingSystemProps.java.template#L40 > > I don't understand how the above code can be compiled on architectures such > as LongArch64, MIPS64el, or ARM32 without modifying the source file. > >> Did you make the other changes to the build that would be needed for a new >> architecture? > > I'm not talking about the new architecture, I'm talking about the > architecture that OpenJDK already supports. > > According to my understanding, the above code breaks the support of all > architectures not listed in `Architecture`. But has `Architecture` really > listed all the supported architectures in the OpenJDK mainline? > > For example, I want to compile OpenJDK for 32-bit ARM. I think in this PR, I > cannot compile without modifying the source code because `TARGET_ARCH_arm` > does not exist. But before this PR, I was able to compile. @Glavo Arm32 builds, though, see GHAs (crossbuilding test). As for the others, as long as the ports have not been integrated into mainline, I expect port maintainers will adapt the enum downstream. They do this anyway. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160459041
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 19:25:19 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unneeded qualified export from java.base to jdk.attach Another question, how does this work with Zero? I know e.g. Debian maintains a broad range of Zero JVMs on different target CPUs. Things like ia64 and parisc. What is the os.arch value on those, and how will this work with the limited set of enums we replace it with? - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1499594972
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 19:25:19 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unneeded qualified export from java.base to jdk.attach What about PPC (big endian)? Used on AIX? On Arm, it may be useful to know whether we built for thumb mode (We recently had this problem in tests). - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1499585079
Re: RFR: 8304017: ProblemList com/sun/jdi/InvokeHangTest.java on windows-x64 in vthread mode
On Sat, 11 Mar 2023 17:16:46 GMT, Daniel D. Daugherty wrote: > Trivial fixes to ProblemList 3 different tests: > [JDK-8304017](https://bugs.openjdk.org/browse/JDK-8304017) ProblemList > com/sun/jdi/InvokeHangTest.java on windows-x64 in vthread mode > [JDK-8304018](https://bugs.openjdk.org/browse/JDK-8304018) ProblemList > javax/swing/JColorChooser/Test6827032.java on windows-x64 > [JDK-8304019](https://bugs.openjdk.org/browse/JDK-8304019) ProblemList > java/awt/dnd/MissingDragExitEventTest/MissingDragExitEventTest.java on > windows-x64 Looks good and trivial - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/12990
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]
On Wed, 7 Dec 2022 21:25:11 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this update reviewed? >> >> The sprintf is deprecated in Xcode 14 because of security concerns, and the >> use of it causing building failure. The build could pass if warnings are >> disabled for codes that use sprintf method. For the long run, the sprintf >> could be replaced with snprintf. This patch is trying to check if snprintf >> could be used. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > update on review feedback Good from my side. But someone else should look as well. It's fiddly stuff. - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: JDK-8298170 : Introduce a macro for exception check, free and return
On Wed, 7 Dec 2022 16:27:43 GMT, Roger Riggs wrote:
>
> Good idea, though perhaps the return (and value if any) could be explicit in
> the macro invocation, instead of implicit (plus arg). A single macro would
> suffice, instead of multiples.
>
> Usage Example:
>
> ```
> JNU_CHECK_EXCEPTION_DO(env, {
> free(tab);
> return;
>})
> ```
Neat. I like that.
-
PR: https://git.openjdk.org/jdk/pull/11539
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]
On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan wrote:
>> Hi,
>>
>> May I have this update reviewed?
>>
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the
>> use of it causing building failure. The build could pass if warnings are
>> disabled for codes that use sprintf method. For the long run, the sprintf
>> could be replaced with snprintf. This patch is trying to check if snprintf
>> could be used.
>>
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> comment for snprintf_checked
I looked through all the code and found only some minor issues. I would
appreciate more eyes on this, though.
I still think this would have been less work (for author and reviewers) had we
converted the code to stringStream right away in the hotspot. stringStream
takes care of a lot of this boilerplate stuff.
src/hotspot/os/bsd/attachListener_bsd.cpp line 260:
> 258: // name ("load", "datadump", ...), and is an argument
> 259: int expected_str_count = 2 + AttachOperation::arg_count_max;
> 260: const int max_len = (ver_str_len + 1) +
> (AttachOperation::name_length_max + 1) +
This makes `max_len` a runtime variable, before it was a compile time constant.
Below, we use it to create the buf array. IIRC creating an array with a
variable runtime length is a non-standard compiler extension. I am surprised
this even builds.
src/hotspot/share/adlc/output_c.cpp line 46:
> 44: va_end(args);
> 45: assert(result >= 0, "snprintf error");
> 46: assert(static_cast(result) < len, "snprintf truncated");
Question, what is this assert? The standard CRT assert? Will it fire always, or
just in debug?
src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp line 311:
> 309: for (int i = 0; i < len ; i++) {
> 310: VMStructEntry vmField = JVMCIVMStructs::localHotSpotVMStructs[i];
> 311: const size_t name_buf_size = strlen(vmField.typeName) +
> strlen(vmField.fieldName) + 3 /* "::" */;
nit, could you make this "+ 2 + 1" instead? That makes it a bit clearer that
the last byte is for \0
src/hotspot/share/runtime/os.cpp line 102:
> 100: }
> 101:
> 102: int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {
Can you please assert for buffer len > 0 and < some reasonable max, e.g. 1 gb?
Protects us against neg overflows and other errors.
src/hotspot/share/runtime/os.cpp line 108:
> 106: va_end(args);
> 107: assert(result >= 0, "os::snprintf error");
> 108: assert(static_cast(result) < len, "os::snprintf truncated");
Can you please provide a printout for the truncated string? Proposal:
```assert(static_cast(result) < len, "os::snprintf truncated
("%.200s...")", buf);```
src/hotspot/share/utilities/utf8.cpp line 227:
> 225: } else {
> 226: if (p + 6 >= end) break; // string is truncated
> 227: os::snprintf_checked(p, 7, "\\u%04x", c); // counting terminating
> zero in
I had to think a while until I was sure this is correct :-)
We are sure that c can never be > 0xFF (6 digits), right? But if that is
false, code had been wrong before too.
-
PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v12]
On Tue, 22 Nov 2022 08:02:51 GMT, Kim Barrett wrote:
> Given all the near-duplicated checking of os::snprintf results, I think there
> is a place for a helper function to package this up. Maybe something like
>
> ```
> // in class os
> // Performs snprintf and asserts the result is non-negative (so there was not
> // an encoding error) and that the output was not truncated.
> static int snprintf_checked(char* buf, size_t len, const char* fmt, ...)
> ATTRIBUTE_PRINTF(3, 4);
>
> // in runtime/os.cpp
> int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {
> va_list args;
> va_start(args, fmt);
> int result = os::vsnprintf(buf, len, fmt, args);
> va_end(args);
> assert(result >= 0, "os::snprintf error");
> assert(static_cast(result) < size, "os::snprintf truncated");
> return result;
> }
> ```
>
> (I keep waffling over whether the truncation check should be an assert or a
> guarantee.)
>
> I've not yet gone through all the changes yet to consider which should do
> that checking and which should do something different, such as permitting
> truncation.
>
> I'm not wedded to that name; indeed, I don't like it that much, as it's kind
> of inconveniently long. There's a temptation to have os::snprintf forbid
> truncation and a different function that allows it, but that would require
> careful auditing of all pre-existing uses of os::snprintf too, so no.
How about renaming the existing os::snprintf to something like
os::snprintf_unchecked, make os::snprintf the checked version, then, in
separate RFEs, revert existing uses to the new API. When all uses of
os::snprintf_unchecked are cleared up, remove it.
That would make it possible to revert piecemeal while not racing with new uses
of os::snprintf, since new callers will use the new checking API automatically.
-
PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: JDK-8297523 : Various GetPrimitiveArrayCritical miss result - NULL check
On Fri, 25 Nov 2022 09:15:08 GMT, Matthias Baesken wrote: > There are still a few places where GetPrimitiveArrayCritical calls miss the > result check. This should be adjusted. > A similar case was recently adjusted here : > https://bugs.openjdk.org/browse/JDK-8297480 Hi Matthias, But all this is just theoretical, right, since GPAC does not allocate memory in hotspot? Your patch is fine, though I would prefer OOMEs instead of silently omitting code. Cheers, Thomas - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/11361
Re: RFR: JDK-8297147: UnexpectedSourceImageSize test times out on slow machines when fastdebug is used
On Fri, 18 Nov 2022 13:38:47 GMT, Matthias Baesken wrote: > The image related test UnexpectedSourceImageSize test introduced with > https://bugs.openjdk.org/browse/JDK-8264666 > sometimes times out on slow machines when fastdebug is used. This happens > especially in 11 and 17; in 20 it seems to be a bit faster but we better > change timeouts across releases. Looks good. - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/11240
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v9]
On Fri, 18 Nov 2022 06:42:24 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this update reviewed? >> >> The sprintf is deprecated in Xcode 14 because of security concerns, and the >> use of it causing building failure. The build could pass if warnings are >> disabled for codes that use sprintf method. For the long run, the sprintf >> could be replaced with snprintf. This patch is trying to check if snprintf >> could be used. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > size_t cast Hi @XueleiFan, the last version with the asserts looks fine to me. Thanks for your work! Cheers, Thomas - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14
On Sun, 13 Nov 2022 22:55:52 GMT, Xue-Lei Andrew Fan wrote: >> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use >> `os::snprintf`. >> >> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. >> Regarding `os::snprintf` and `os::vsnprintf`, see >> https://bugs.openjdk.org/browse/JDK-8285506. >> >> I think the only reason we haven't marked `::sprintf` and `::snprintf` >> forbidden >> (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has gotten around >> to dealing with it. `::snprintf` in the list of candidates for >> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been >> marked. But I don't see new bugs for the as-yet unmarked ones. >> >> As a general note, as a reviewer my preference is against non-trivial and >> persnickety code changes that are scattered all over the code base. For >> something like this I'd prefer multiple more bite-sized changes that were >> dealing with specific uses. I doubt everyone agrees with me though. > >> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use >> `os::snprintf`. > > Updated to use os::snprintf, except the files under adlc where the > os::snptintf definition is not included. The use of snprintf could be > cleaned up with existing code in the future. > >> >> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. >> Regarding `os::snprintf` and `os::vsnprintf`, see >> https://bugs.openjdk.org/browse/JDK-8285506. >> >> I think the only reason we haven't marked `::sprintf` and `::snprintf` >> forbidden (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has >> gotten around to dealing with it. `::snprintf` in the list of candidates for >> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been >> marked. But I don't see new bugs for the as-yet unmarked ones. >> >> As a general note, as a reviewer my preference is against non-trivial and >> persnickety code changes that are scattered all over the code base. For >> something like this I'd prefer multiple more bite-sized changes that were >> dealing with specific uses. I doubt everyone agrees with me though. > > It makes sense to me. I'd better focus on the building issue in this PR. > > Thank you for the review! Hi @XueleiFan and @kimbarrett , I agree to the change if we, as Kim suggested, add assertions for truncation where we use the return value of snprintf. I am not fully happy with the solution though, since printing is notoriously runtime-data dependent and runtime data can change in release builds. So we could have truncation at a customer that we never see in our tests with debug builds. But seeing that this patch takes so long now and blocks the MacOS build, I don't want to block it. We can improve the code in follow up RFEs. These places would be a lot simpler with stringStream. Cheers, Thomas - PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v7]
On Wed, 16 Nov 2022 07:03:12 GMT, Xue-Lei Andrew Fan wrote:
>> Hi,
>>
>> May I have this update reviewed?
>>
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the
>> use of it causing building failure. The build could pass if warnings are
>> disabled for codes that use sprintf method. For the long run, the sprintf
>> could be replaced with snprintf. This patch is trying to check if snprintf
>> could be used.
>>
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> address review comments
src/hotspot/os/bsd/attachListener_bsd.cpp line 295:
> 293: char msg[32];
> 294: int msg_len = os::snprintf(msg, sizeof(msg), "%d\n",
> ATTACH_ERROR_BADVERSION);
> 295: write_fully(s, msg, msg_len);
Assuming C99 behavior: safe but only because the buffer is large enough ("%d\n"
needs at most 12 bytes, buffer is 32). Were it to overflow, msg_len would be
larger than sizeof(msg) and we would probably end up reading beyond the message
end in write_fully. So not really better than using sprintf+strlen.
src/hotspot/os/bsd/attachListener_bsd.cpp line 415:
> 413: char msg[32];
> 414: int msg_len = os::snprintf(msg, sizeof(msg), "%d\n", result);
> 415: int rc = BsdAttachListener::write_fully(this->socket(), msg, msg_len);
same
src/hotspot/share/adlc/output_c.cpp line 217:
> 215: const PipeClassOperandForm *tmppipeopnd =
> 216: (const PipeClassOperandForm *)pipeclass->_localUsage[paramname];
> 217: templen += snprintf(_stages[templen], operand_stages_size -
> templen, " stage_%s%c\n",
C99 Behavior: all these are probably safe but only because we never overstepped
the buffer in the first place, the buffer size is pre-calculated. If it is
incorrect and we have a truncation, subsequent writes will write beyond the
buffer.
src/hotspot/share/classfile/javaClasses.cpp line 2527:
> 2525:
> 2526: // Print stack trace line in buffer
> 2527: size_t buf_off = os::snprintf(buf, buf_size, "\tat %s.%s(",
> klass_name, method_name);
Here, and in subsequent uses: assuming C99 behavior of snprintf, if we
truncated in snprintf, buf_off will be > buffer size, (buf + buf_off) point
beyond the buffer, (buf_size - buf_off) will overflow and become very large.
-
PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v7]
On Wed, 16 Nov 2022 07:03:12 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this update reviewed? >> >> The sprintf is deprecated in Xcode 14 because of security concerns, and the >> use of it causing building failure. The build could pass if warnings are >> disabled for codes that use sprintf method. For the long run, the sprintf >> could be replaced with snprintf. This patch is trying to check if snprintf >> could be used. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments I don't think it is safe to use the return value of `os::snprintf` to update buffer positions. `os::snprintf()` calls `os::vsnprintf()` which, at least on Posix, returns the return value of `vnsprintf(3)` verbatim. If native `vsnprintf(3)` conforms to SUSv2 [1], it will return <0 if the buffer size is zero. So for cases where we calculate the buffer size based on what was written, and we are just at the edge of the buffer, we would move the next write position backward. But much worse: if the native `vsnprintf(3)` conforms to C99 (e.g. glibc, BSD libc) they return "number of characters (not including the terminating null character) which would have been written to buffer if bufsz was ignored". [2] So, if the buffer was too small and we truncated, and we use the return value to calculate the next write position, we will write outside the buffer boundaries. Regardless of what behavior we have - C99 or SUSv2 - we cannot just use the return value to update the next write position without first checking the return value. We could - and probably should - decide on C99 or SUSv2 behavior for all platforms, and modify `os::snprintf()` to provide that in an OS-independent way. But unless we decide on some mongrel of both C99 and SUSv2 behaviors, the problem remains. Cheers, Thomas [1] https://pubs.opengroup.org/onlinepubs/7908799/xsh/fprintf.html [2] https://en.cppreference.com/w/c/io/fprintf - PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]
On Wed, 16 Nov 2022 05:45:34 GMT, Xue-Lei Andrew Fan wrote: >> A result of -1 only occurs for an encoding error. An encoding error is only >> possible with multi-byte / wide characters. (See the definition of "encoding >> error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding >> errors, so our uses of snprintf never return -1. > > Updated to use the result from `os::snprtinf` in the new commit. > A result of -1 only occurs for an encoding error. An encoding error is only > possible with multi-byte / wide characters. (See the definition of "encoding > error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding > errors, so our uses of snprintf never return -1. Hi @kimbarrett, I am not sure this was true. E.g. https://stackoverflow.com/questions/65334245/what-is-an-encoding-error-for-sprintf-that-should-return-1 cites some cases where snprintf returns -1 that have nothing to do with multibyte strings. Also, size=0 would return -1 according to SUSv2. Note glibc differs and returns the number of chars it *would* have printed. Which is also dangerous in a different way. If you use that number to update the position, the position is not limited to buffer boundaries. So, I think the result of os::snprintf should not be used to update buffer position, at least not without checking. - PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]
On Tue, 15 Nov 2022 07:13:49 GMT, Kim Barrett wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> delete swp file
>
> src/hotspot/os/bsd/attachListener_bsd.cpp line 294:
>
>> 292: (atoi(buf) != ATTACH_PROTOCOL_VER)) {
>> 293: char msg[32];
>> 294: os::snprintf(msg, sizeof(msg), "%d\n",
>> ATTACH_ERROR_BADVERSION);
>
> Rather than using `strlen(msg)` in the next line, use the result from
> `os::snprintf`.
The problem with using the return value of os::snprintf() is that we need to
handle the -1 case to prevent the position from running backward. Might be
better to use stringStream instead, which should handle the -1 case
transparently.
-
PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]
On Mon, 14 Nov 2022 19:44:17 GMT, Xue-Lei Andrew Fan wrote:
>> Hi,
>>
>> May I have this update reviewed?
>>
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the
>> use of it causing building failure. The build could pass if warnings are
>> disabled for codes that use sprintf method. For the long run, the sprintf
>> could be replaced with snprintf. This patch is trying to check if snprintf
>> could be used.
>>
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> delete swp file
Hi @XueleiFan,
good job, this looks like it was onerous work!
One issue I noticed, in ADLC only: we sometimes use the snprintf return value
to update a position pointer, e.g. in adlc output_c.cpp; should snprintf return
-1, we could run backwards and overstep the beginning of the buffer.
Totally up to you if you fix it, and whether as a follow-up RFE or here. If you
do, the simplest way may be to add a little `stringStream`-like helper like
this to ADLC:
class AdlcStringStream {
char* const _buf;
const size_t _buflen;
size_t _pos;
public:
AdlcStringStream(char* out, size_t outlen) : _buf(out), _buflen(outlen),
_pos(0) {}
void print(const char* fmt, ...) {
if (_pos < _buflen) {
va_list ap;
va_start (ap, fmt);
int written = vsnprintf(_buf + _pos, _buflen - _pos, fmt, ap);
va_end(ap);
if (written > 0) {
_pos += written;
}
}
}
const char* buf() const { return _buf; }
};
and use that instead. Way easier to read the code then. Optionally, the helper
could even handle buffer allocation and destruction.
All other remarks inline. Small issues remain, but nothing drastic.
Cheers, Thomas
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 226:
> 224: char buf[512];
> 225: os::snprintf(buf, sizeof(buf), "0x%02x:0x%x:0x%03x:%d", _cpu,
> _variant, _model, _revision);
> 226: if (_model2) os::snprintf(buf+strlen(buf), sizeof(buf) - strlen(buf),
> "(0x%03x)", _model2);
Here - and in several other places, where we construct a string from multiple
parts - the code would be a simpler with `stringStream`:
char buf[512];
stringStream ss(buf, sizeof(buf));
ss.print("0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
if (_model2) ss.print("(0x%03x)", _model2);
_features_string = os::strdup(buf);
or, using `stringStream`s internal buffer:
stringStream ss;
ss.print("0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
if (_model2) ss.print("(0x%03x)", _model2);
_features_string = ss.base();
No manual offset counting required.
I leave it up to you if you do it that way. The code here is correct as it is.
src/hotspot/share/classfile/javaClasses.cpp line 2562:
> 2560: CompiledMethod* nm = method->code();
> 2561: if (WizardMode && nm != NULL) {
> 2562: os::snprintf(buf + buf_off, buf_size - buf_off, "(nmethod "
> INTPTR_FORMAT ")", (intptr_t)nm);
I think you should update `buf_off` here, because now you overwrite the last
text part. Weird that no test caught that.
All this code here in javaClasses.cpp would benefit from using stringStream.
src/hotspot/share/utilities/utf8.cpp line 521:
> 519: } else {
> 520: if (p + 6 >= end) break; // string is truncated
> 521: os::snprintf(p, 7, "\\u%04x", c);
This should be 6, or? We have 6 characters left before end, assuming end is
exclusive.
Also, maybe use a named constant?
src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_Ports.cpp line 638:
> 636: return;
> 637: }
> 638: snprintf(channelName, 16, "Ch %d", ch);
Can we use a constant here instead of literal 16?
-
Changes requested by stuefe (Reviewer).
PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]
On Mon, 14 Nov 2022 05:32:20 GMT, Xue-Lei Andrew Fan wrote:
>> Hi,
>>
>> May I have this update reviewed?
>>
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the
>> use of it causing building failure. The build could pass if warnings are
>> disabled for codes that use sprintf method. For the long run, the sprintf
>> could be replaced with snprintf. This patch is trying to check if snprintf
>> could be used.
>>
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> include missing os head file
src/hotspot/share/adlc/output_c.cpp line 2570:
> 2568: int idx = inst.operand_position_format(arg_name);
> 2569: if (strcmp(arg_name, "constanttablebase") == 0) {
> 2570: ib += snprintf(ib, (buflen - (ib - idxbuf)), " unsigned idx_%-5s
> = mach_constant_base_node_input(); \t// %s, \t%s\n",
Use sizeof(buffer) instead of buflen?
Also, possibly using a helper macro like this:
#define remaining_buflen(buffer, position) (sizeof(buffer) - (position -
buffer))
would make the code a bit easier on the eye. Or, if not a macro, an inline
helper function, that could assert also array boundaries.
-
PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14
On Sun, 13 Nov 2022 22:55:52 GMT, Xue-Lei Andrew Fan wrote: > Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use > `os::snprintf`. I did not know this was our policy now. Sorry for giving the wrong advice. Maybe we should add this to the hotspot style guide since I'm probably not the only one not knowing this. > > Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. > Regarding `os::snprintf` and `os::vsnprintf`, see > https://bugs.openjdk.org/browse/JDK-8285506. > > I think the only reason we haven't marked `::sprintf` and `::snprintf` > forbidden (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has > gotten around to dealing with it. `::snprintf` in the list of candidates for > https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been > marked. But I don't see new bugs for the as-yet unmarked ones. > > As a general note, as a reviewer my preference is against non-trivial and > persnickety code changes that are scattered all over the code base. For > something like this I'd prefer multiple more bite-sized changes that were > dealing with specific uses. I doubt everyone agrees with me though. I agree with you. Makes backporting a bit easier too. - PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14
On Sun, 13 Nov 2022 08:25:57 GMT, Xue-Lei Andrew Fan wrote: > > could you use `jio_snprintf` instead (see include/jvm_io.h)? That is what > > we usually do for snprintf. jio_snprintf hides platform particularities wrt > > snprintf. > > Good to know that. Thank you! > > While I was doing the replacement from `snprintf` to `jio_snprintf`, I > noticed a lot of existing use of `snprintf` in the files touched in this PR. > What do you think if we have a `snprintf` clean up in a followed PR? > > ``` > hotspot $ find . -type f |xargs grep snprintf |grep -v jio_snprintf |wc > > 2621895 26574 > ``` Hmm, possibly. We may look again at the exact reason why we use jio_snprintf. Maybe it is less important nowadays, with reduced platform number (no solaris) and Windows being more standard conform than it had been in the past. Lets hear what others think. - PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: 8296812: sprintf is deprecated in Xcode 14
On Fri, 11 Nov 2022 22:41:19 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this update reviewed? > > The sprintf is deprecated in Xcode 14 because of security concerns, and the > use of it causing building failure. The build could pass if warnings are > disabled for codes that use sprintf method. For the long run, the sprintf > could be replaced with snprintf. This patch is trying to check if snprintf > could be used. > > Thanks, > Xuelei Hi @XueleiFan, could you use `jio_snprintf` instead (see include/jvm_io.h)? That is what we usually do for snprintf. jio_snprintf hides platform particularities wrt snprintf. Cheers, Thomas - PR: https://git.openjdk.org/jdk/pull/5
Re: RFR: JDK-8292866: Java_sun_awt_shell_Win32ShellFolder2_getLinkLocation check MultiByteToWideChar return value for failures [v2]
On Fri, 26 Aug 2022 07:54:24 GMT, Matthias Baesken wrote: >> https://docs.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-multibytetowidechar >> states about MultiByteToWideChar : "The function returns 0 if it does not >> succeed" and lists a few failure cases. >> However we miss checking the failure case in >> Java_sun_awt_shell_Win32ShellFolder2_getLinkLocation , seems we assume the >> function always works nicely (in most of the JDK coding the return value is >> checked ). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust added return syntax LGTM - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/10015
