Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Marked as reviewed by djelinski (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2180081756
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution FWIW, when I added `-lstdc++` before both `-static-libstdc++` and replaced LDCXX with LD, the code compiled and linked just fine. Both GCC and G++ call the same linker, and the parameter differences are well documented. It's only a matter of deciding if we want to keep the complexity of selecting the executable to use or not. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966368056
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE can we get rid of LDCXX? On my system LDCXX is mapped to `g++` and LD is `gcc`; I searched for the differences, and the only thing I could find is that `g++` implicitly adds `-lstdc++ -shared-libgcc`; I suppose we could explicitly add these options, and use gcc everywhere. See: https://stackoverflow.com/a/172592 https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1965991536
Integrated: 8311301: MethodExitTest may fail with stack buffer overrun
On Tue, 4 Jul 2023 17:33:02 GMT, Daniel Jeliński wrote: > Please review this test-only fix that fixes the size of variables that are > used in methods that expect a pointer. > > On Windows, type `long` is 32 bits, pointers are 64 bits large. The method > `GetThreadLocalStorage` writes a pointer (64 bits) to the address given by > its parameter, which overflows a `long`. The code generated by VS compiler > ignores this, but the code generated by clang crashes the test. > > No new tests. MethodExitTest continues to pass on supported platforms, and > passes on clang+win with this fix. This pull request has now been integrated. Changeset: 3d813ae3 Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/3d813ae39f4422dd47473608eb8911e2483c6c32 Stats: 20 lines in 1 file changed: 2 ins; 0 del; 18 mod 8311301: MethodExitTest may fail with stack buffer overrun Reviewed-by: kevinw, dholmes, cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/14770
Re: RFR: 8311301: MethodExitTest may fail with stack buffer overrun [v2]
On Thu, 6 Jul 2023 07:08:00 GMT, David Holmes wrote: >> Daniel Jeliński has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use void* instead > > test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp > line 127: > >> 125: static void* tls_data = 0; >> 126: static const void* const tls_data1 = (const void*)0x111; >> 127: static const void* const tls_data2 = (const void*)0x222; > > Was this necessitated by the change to `void*`? If so I had not expected that. I guess I could have changed the logging code to use `"%d", (int)(intptr_t)tls_data` and call it a day, but it would be ugly. - PR Review Comment: https://git.openjdk.org/jdk/pull/14770#discussion_r1254138298
Re: RFR: 8311301: MethodExitTest may fail with stack buffer overrun [v2]
On Wed, 5 Jul 2023 06:40:11 GMT, Daniel Jeliński wrote: >> Please review this test-only fix that fixes the size of variables that are >> used in methods that expect a pointer. >> >> On Windows, type `long` is 32 bits, pointers are 64 bits large. The method >> `GetThreadLocalStorage` writes a pointer (64 bits) to the address given by >> its parameter, which overflows a `long`. The code generated by VS compiler >> ignores this, but the code generated by clang crashes the test. >> >> No new tests. MethodExitTest continues to pass on supported platforms, and >> passes on clang+win with this fix. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Use void* instead Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/14770#issuecomment-1623247781
Re: RFR: 8311301: MethodExitTest may fail with stack buffer overrun
On Tue, 4 Jul 2023 17:33:02 GMT, Daniel Jeliński wrote: > Please review this test-only fix that fixes the size of variables that are > used in methods that expect a pointer. > > On Windows, type `long` is 32 bits, pointers are 64 bits large. The method > `GetThreadLocalStorage` writes a pointer (64 bits) to the address given by > its parameter, which overflows a `long`. The code generated by VS compiler > ignores this, but the code generated by clang crashes the test. > > No new tests. MethodExitTest continues to pass on supported platforms, and > passes on clang+win with this fix. The diff to intptr_t was an order of magnitude smaller. But I agree, the code looks better without the casts. - PR Comment: https://git.openjdk.org/jdk/pull/14770#issuecomment-1621120295
Re: RFR: 8311301: MethodExitTest may fail with stack buffer overrun [v2]
> Please review this test-only fix that fixes the size of variables that are > used in methods that expect a pointer. > > On Windows, type `long` is 32 bits, pointers are 64 bits large. The method > `GetThreadLocalStorage` writes a pointer (64 bits) to the address given by > its parameter, which overflows a `long`. The code generated by VS compiler > ignores this, but the code generated by clang crashes the test. > > No new tests. MethodExitTest continues to pass on supported platforms, and > passes on clang+win with this fix. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Use void* instead - Changes: - all: https://git.openjdk.org/jdk/pull/14770/files - new: https://git.openjdk.org/jdk/pull/14770/files/0b537cb1..e3a71308 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14770&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14770&range=00-01 Stats: 19 lines in 1 file changed: 2 ins; 0 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14770.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14770/head:pull/14770 PR: https://git.openjdk.org/jdk/pull/14770
RFR: 8311301: MethodExitTest may fail with stack buffer overrun
Please review this test-only fix that fixes the size of variables that are used in methods that expect a pointer. On Windows, type `long` is 32 bits, pointers are 64 bits large. The method `GetThreadLocalStorage` writes a pointer (64 bits) to the address given by its parameter, which overflows a `long`. The code generated by VS compiler ignores this, but the code generated by clang crashes the test. No new tests. MethodExitTest continues to pass on supported platforms, and passes on clang+win with this fix. - Commit messages: - Fix buffer overrun Changes: https://git.openjdk.org/jdk/pull/14770/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14770&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311301 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14770.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14770/head:pull/14770 PR: https://git.openjdk.org/jdk/pull/14770
Integrated: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. This pull request has now been integrated. Changeset: 055b4b42 Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/055b4b426cbc56d97e82219f3dd3aba1ebf977e4 Stats: 223 lines in 74 files changed: 0 ins; 0 del; 223 mod 8310948: Fix ignored-qualifiers warning in Hotspot Reviewed-by: kbarrett, dholmes - PR: https://git.openjdk.org/jdk/pull/14674
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1617559670
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Mon, 3 Jul 2023 06:47:04 GMT, Daniel Jeliński wrote:
>> src/hotspot/cpu/aarch64/aarch64.ad line 2288:
>>
>>> 2286:
>>> //=
>>> 2287:
>>> 2288: const bool Matcher::match_rule_supported(int opcode) {
>>
>> Have to wonder if these were all meant to be `bool Match:xxx() const {`?
>
> Yes, I think that may have been the original intent. I'll add const on these
> functions.
...actually these methods are static, and static functions can't be
const-qualified.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14674#discussion_r1250385599
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Mon, 3 Jul 2023 00:19:56 GMT, David Holmes wrote:
>> Please review this attempt to fix ignored-qualifiers warning.
>>
>> Example warnings:
>>
>> src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier
>> on return type has no effect [-Wignored-qualifiers]
>>CompiledMethod* volatile code() const;
>>^
>>
>>
>> src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type
>> qualifiers ignored on cast result type [-Wignored-qualifiers]
>> 65 | event.set_source((const ModuleEntry* const)from_module);
>>|^
>>
>>
>> The proposed fix removes the ignored qualifiers.
>> In a few AD files I replaced `const` with `constexpr` where I noticed that
>> the method is returning a compile-time constant, and other platforms use
>> `constexpr` on the same method.
>>
>> Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux
>> complete without errors. Cross-compile GHA builds also pass.
>
> src/hotspot/cpu/aarch64/aarch64.ad line 2288:
>
>> 2286:
>> //=
>> 2287:
>> 2288: const bool Matcher::match_rule_supported(int opcode) {
>
> Have to wonder if these were all meant to be `bool Match:xxx() const {`?
Yes, I think that may have been the original intent. I'll add const on these
functions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14674#discussion_r1250340625
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Can I get another +1 on this? or should I proceed with splitting? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1614738114
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. I don't think that was the intention. There's a comment on `Method::_code` stating that the pointer can change at any point (so the pointer was supposed to be volatile), and `class CompiledMethod` does not have any `volatile` - qualified methods. There are similar cases where `const` pointer to a class was returned involving `JavaThread`, `ReferenceProcessor`, `PSCardTable` and `ShenandoahHeapRegion`; I suppose we could review if `const` could be added to the returned class, but that's outside of the scope of this PR. - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1612435102
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. David: I think this part of the spec is relevant here: > A non-class non-array prvalue cannot be > [cv-qualified](https://en.cppreference.com/w/cpp/language/cv), [...]. (Note: > a function call or cast expression may result in a prvalue of non-class > cv-qualified type, but the cv-qualifier is generally immediately stripped > out.) [source](https://en.cppreference.com/w/cpp/language/value_category) given that the cv qualifiers are immediately stripped by the compiler, there's no point in providing them. In the particular volatile pointer case: the function performs a volatile read to get the pointer value (address). That address can then be used in a non-volatile manner. Kim: I realize that it's a big change, so thank you very much for reviewing it anyway! I was prepared to split it up, just wanted to know if this warning is something we want to fix. - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1611905364
RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
Please review this attempt to fix ignored-qualifiers warning. Example warnings: src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier on return type has no effect [-Wignored-qualifiers] CompiledMethod* volatile code() const; ^ src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type qualifiers ignored on cast result type [-Wignored-qualifiers] 65 | event.set_source((const ModuleEntry* const)from_module); |^ The proposed fix removes the ignored qualifiers. In a few AD files I replaced `const` with `constexpr` where I noticed that the method is returning a compile-time constant, and other platforms use `constexpr` on the same method. Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete without errors. Cross-compile GHA builds also pass. - Commit messages: - Fix other platforms - Fix shenandoah - Fix ignored-qualifiers warning Changes: https://git.openjdk.org/jdk/pull/14674/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14674&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310948 Stats: 223 lines in 74 files changed: 0 ins; 0 del; 223 mod Patch: https://git.openjdk.org/jdk/pull/14674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14674/head:pull/14674 PR: https://git.openjdk.org/jdk/pull/14674
Re: RFR: 8308286 Fix clang warnings in linux code [v8]
On Fri, 23 Jun 2023 08:03:45 GMT, Artem Semenov wrote: >> When using the clang compiler to build OpenJDk on Linux, we encounter >> various "warnings as errors". >> They can be fixed with small changes. > > Artem Semenov has updated the pull request incrementally with one additional > commit since the last revision: > > update LGTM - Marked as reviewed by djelinski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14033#pullrequestreview-1494691850
Re: RFR: 8308286 Fix clang warnings in linux code [v6]
On Thu, 22 Jun 2023 09:58:19 GMT, Daniel Jeliński wrote: >> Artem Semenov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 241: > >> 239: DISABLED_WARNINGS_gcc_OGLPaints.c := format-nonliteral, >> \ >> 240: DISABLED_WARNINGS_gcc_sun_awt_X11_GtkFileDialogPeer.c >> := parentheses, \ >> 241: DISABLED_WARNINGS_gcc_X11SurfaceData.c := >> implicit-fallthrough pointer-to-int-cast, \ > > Suggestion: > > DISABLED_WARNINGS_gcc_GLXSurfaceData.c := unused-function, \ > DISABLED_WARNINGS_gcc_gtk2_interface.c := parentheses type-limits, \ > DISABLED_WARNINGS_gcc_gtk3_interface.c := parentheses type-limits > unused-function, \ > DISABLED_WARNINGS_gcc_OGLBufImgOps.c := format-nonliteral, \ > DISABLED_WARNINGS_gcc_OGLPaints.c := format-nonliteral, \ > DISABLED_WARNINGS_gcc_sun_awt_X11_GtkFileDialogPeer.c := parentheses, > \ > DISABLED_WARNINGS_gcc_X11SurfaceData.c := implicit-fallthrough > pointer-to-int-cast, \ Please remove the extra spaces. - PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1239364317
Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v5]
On Fri, 23 Jun 2023 02:25:47 GMT, Julian Waters wrote: >> Julian Waters has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Whitespace >> - Revert >> - _MSC_VER > > @djelinski Sorry for the tag, it's just that this PR has been collecting dust > for months now Hi @TheShermanTanker these alignment instructions were introduced in JDK-8220348 and JDK-8239856 to deal with Windows 32 failures. Windows 32 build is going away soon. As far as I could tell, the declspec was only introduced to deal with an overzealous assert. @TheRealMDoerr @RealCLanger @tstuefe @sspitsyn since you were responsible for the introduction of the alignment specifiers, could you help with reviewing this? - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1603717283
Re: RFR: 8308286 Fix clang warnings in linux code [v6]
On Thu, 22 Jun 2023 09:13:21 GMT, Artem Semenov wrote: >> When using the clang compiler to build OpenJDk on Linux, we encounter >> various "warnings as errors". >> They can be fixed with small changes. > > Artem Semenov has updated the pull request incrementally with one additional > commit since the last revision: > > update make/modules/java.desktop/lib/Awt2dLibraries.gmk line 241: > 239: DISABLED_WARNINGS_gcc_OGLPaints.c := format-nonliteral, \ > 240: DISABLED_WARNINGS_gcc_sun_awt_X11_GtkFileDialogPeer.c := > parentheses, \ > 241: DISABLED_WARNINGS_gcc_X11SurfaceData.c := > implicit-fallthrough pointer-to-int-cast, \ Suggestion: DISABLED_WARNINGS_gcc_GLXSurfaceData.c := unused-function, \ DISABLED_WARNINGS_gcc_gtk2_interface.c := parentheses type-limits, \ DISABLED_WARNINGS_gcc_gtk3_interface.c := parentheses type-limits unused-function, \ DISABLED_WARNINGS_gcc_OGLBufImgOps.c := format-nonliteral, \ DISABLED_WARNINGS_gcc_OGLPaints.c := format-nonliteral, \ DISABLED_WARNINGS_gcc_sun_awt_X11_GtkFileDialogPeer.c := parentheses, \ DISABLED_WARNINGS_gcc_X11SurfaceData.c := implicit-fallthrough pointer-to-int-cast, \ make/modules/java.desktop/lib/Awt2dLibraries.gmk line 260: > 258: DISABLED_WARNINGS_clang_aix_sun_awt_X11_GtkFileDialogPeer.c := > parentheses, \ > 259: DISABLED_WARNINGS_clang_aix_awt_InputMethod.c := sign-compare, \ > 260: DISABLED_WARNINGS_clang_screencast_pipewire.c := > format-nonliteral, \ move this one closer to the other clang (non-aix) suppressions - PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1238296391 PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1238297332
Re: RFR: 8308286 Fix clang warnings in linux code [v5]
On Sun, 11 Jun 2023 16:38:31 GMT, Artem Semenov wrote: >> When using the clang compiler to build OpenJDk on Linux, we encounter >> various "warnings as errors". >> They can be fixed with small changes. > > Artem Semenov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - update > - update > - update > - update > - 8308286 Fix clang warnings in linux code Please update copyright years where they aren't at 2023 yet. I verified these changes on Ubuntu 20.04 + clang 10; they are both necessary and sufficient to make the build pass with warnings as errors. I saw one more warning: clang: warning: argument unused during compilation: '-static-libstdc++' [-Wunused-command-line-argument] in `BUILD_SYSLOOKUPLIB_link.log`, but that one does not break the build. I'm going to approve this once you fix the mentioned issues. Thanks for working on this! make/modules/java.desktop/lib/Awt2dLibraries.gmk line 235: > 233: DISABLED_WARNINGS_gcc := int-to-pointer-cast, \ > 234: DISABLED_WARNINGS_gcc_awt_Taskbar.c := parentheses, \ > 235: DISABLED_WARNINGS_clang_awt_Taskbar.c := parentheses, \ please group the disabled warnings by compiler (gcc, then clang, then clang_aix) - PR Review: https://git.openjdk.org/jdk/pull/14033#pullrequestreview-1489972241 PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1236510850
Re: RFR: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote: > When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. According to our docs, [clang is a supported compiler for Linux](https://github.com/openjdk/jdk/blob/master/doc/building.md#native-compiler-toolchain-requirements). Here's an example how warning exclusion is implemented today: https://github.com/openjdk/jdk/blob/master/make/modules/java.desktop/lib/Awt2dLibraries.gmk#L827 - PR Comment: https://git.openjdk.org/jdk/pull/14033#issuecomment-1563956337
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
On Mon, 13 Mar 2023 15:55:27 GMT, Daniel Jeliński wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use NULL where appropriate
we never use any of the JNU_XXX functions to report errno on Windows as far as
I could tell. And even if we did, we'd need to call SetLastError(0) before
JNU_Throw to make it work, which we never did. I think we're ok here.
-
PR: https://git.openjdk.org/jdk/pull/12922
Integrated: 8303814: getLastErrorString should avoid charset conversions
On Wed, 8 Mar 2023 11:30:27 GMT, Daniel Jeliński wrote:
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
This pull request has now been integrated.
Changeset: baf11e73
Author:Daniel Jeliński
URL:
https://git.openjdk.org/jdk/commit/baf11e734f7b5308490edc74f3168744c0857b24
Stats: 151 lines in 8 files changed: 21 ins; 44 del; 86 mod
8303814: getLastErrorString should avoid charset conversions
Reviewed-by: naoto, cjplummer, rriggs
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v2]
On Mon, 13 Mar 2023 15:05:04 GMT, Roger Riggs wrote:
>> Daniel Jeliński has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Address review comments
>> - Mention that the returned text is static and thread safe
>> - Define buffer size
>
> src/java.base/share/native/libzip/zip_util.c line 812:
>
>> 810:
>> 811: if (strlen(name) >= PATH_MAX) {
>> 812: if (pmsg) {
>
> Some pre-existing references to pmsg still use implicit comparison, though
> not material to this PR, can you update them to compare with NULL.
done.
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
Daniel Jeliński has updated the pull request incrementally with one additional
commit since the last revision:
Use NULL where appropriate
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12922/files
- new: https://git.openjdk.org/jdk/pull/12922/files/ea91b651..efd72a1d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=01-02
Stats: 12 lines in 1 file changed: 0 ins; 0 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/12922.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12922/head:pull/12922
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v2]
On Thu, 9 Mar 2023 18:08:32 GMT, Naoto Sato wrote: >> Daniel Jeliński has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Address review comments >> - Mention that the returned text is static and thread safe >> - Define buffer size > > src/java.base/share/native/libzip/zip_util.c line 767: > >> 765: * or NULL if an error occurred. If a zip error occurred then *pmsg will >> 766: * be set to the error message text if pmsg != 0. Otherwise, *pmsg will >> be >> 767: * set to NULL. Caller doesn't need to free the error message. > > I'd put some more context here why the caller does not need to free. (as it > is a static text) Added; also mentioned that we want the buffer to be thread-safe. Let me know if that's what you had in mind. > src/java.base/windows/native/libjava/jni_util_md.c line 80: > >> 78: 0, >> 79: buf, >> 80: sizeof(buf) / sizeof(WCHAR), > > Maybe better to #define the size 256 so that this division is not needed. done - PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v2]
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
Daniel Jeliński has updated the pull request incrementally with three
additional commits since the last revision:
- Address review comments
- Mention that the returned text is static and thread safe
- Define buffer size
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12922/files
- new: https://git.openjdk.org/jdk/pull/12922/files/8ab8d729..ea91b651
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=00-01
Stats: 8 lines in 4 files changed: 2 ins; 0 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/12922.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12922/head:pull/12922
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v2]
On Fri, 10 Mar 2023 21:47:45 GMT, Roger Riggs wrote:
>> Daniel Jeliński has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Address review comments
>> - Mention that the returned text is static and thread safe
>> - Define buffer size
>
> src/java.base/share/native/libjava/jni_util.c line 133:
>
>> 131: if (s != NULL) {
>> 132: jobject x = NULL;
>> 133: if (messagelen) {
>
> Avoid implicit compare with 0; use `messagelen > 0` or similar.
preexisting, but fixed.
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions
On Thu, 9 Mar 2023 00:17:42 GMT, Naoto Sato wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> src/java.base/windows/native/libnio/ch/FileDispatcherImpl.c line 208:
>
>> 206:
>> 207: if (result == 0) {
>> 208: JNU_ThrowIOExceptionWithLastError(env, "Write failed");
>
> Could be replaced with `JNU_ThrowIOException`?
If we got here, `WriteFile` just failed and `GetLastError` contains interesting
information.
`JNU_ThrowIOExceptionWithLastError` will generate specific error message in
user's language, `JNU_ThrowIOException` would just throw `Write failed`. I
don't think we want to change this.
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions
On Wed, 8 Mar 2023 23:23:33 GMT, Chris Plummer wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> We don't have a test for the SA changes you made. The best way to test it is
> with clhsdb. Run the following against a JVM pid:
>
> `$ jhsdb clhsdb --pid `
>
> Use "jstack -v" to get a native pc from a frame, and then try `disassemble`
> on the address. It most likely will produce an exception since it can't find
> hsdis, which is actually what we want to be testing in this case:
>
>
> hsdb> disassemble 0x7f38b371fca0
> Error: sun.jvm.hotspot.debugger.DebuggerException: hsdis-amd64.so: cannot
> open shared object file: No such file or directory
>
>
> You'll need to test separately on Windows and any unix platform.
Thanks @plummercj for the instructions. Here's the results:
Linux, with this change applied:
hsdb> disassemble 0x7f3484558da0
Error: sun.jvm.hotspot.debugger.DebuggerException: hsdis-amd64.so: cannot open
shared object file: No such file or directory
Windows, EN, with the change:
hsdb> disassemble 0x0107d4dae0c6
Error: sun.jvm.hotspot.debugger.DebuggerException: The specified module could
not be found
Windows, misconfigured CN, without the change:
hsdb> disassemble 0x0200d60de0b4
Error: sun.jvm.hotspot.debugger.DebuggerException: ?
Windows, misconfigured CN, with the change:
hsdb> disassemble 0x01fab996e0b4
Error: sun.jvm.hotspot.debugger.DebuggerException: 找不到指定的模块。
(note: I had to run `chcp 65001` in the console, otherwise the exception would
still display incorrectly)
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
On Wed, 8 Mar 2023 11:30:27 GMT, Daniel Jeliński wrote:
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
I think it's fine to remove `getLastErrorString` from the list of libjava
exports. After my changes it's no longer used outside libjava, and it was never
a part of the JDK's public interface.
Regarding consistency with `getErrorString`, I'm not too concerned about that;
the functions have similar names but serve different purposes.
-
PR: https://git.openjdk.org/jdk/pull/12922
RFR: 8303814: getLastErrorString should avoid charset conversions
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
This patch modifies the `getLastErrorString` method to return a `jstring`.
Thanks to that we can avoid unnecessary back and forth conversions between
Unicode and other charsets on Windows.
Other changes include:
- the Windows implementation of `getLastErrorString` no longer checks `errno`.
I verified all uses of the method and confirmed that `errno` is not used
anywhere.
- While at it, I found and fixed a few calls to
`JNU_ThrowIOExceptionWithLastError` that were done in context where `LastError`
was not set.
- jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
`JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to have
identical behavior.
- zip_util was modified to return static messages instead of generated ones.
The generated messages were not observed anywhere, because they were replaced
by a static message in ZIP_Open, which is the only method used by other native
code.
- `getLastErrorString` is no longer exported by libjava.
Tier1-3 tests continue to pass.
No new automated regression test; testing this requires installing a language
pack that cannot be displayed in the current code page.
Tested this manually by installing Chinese language pack on English Windows 11,
selecting Chinese language, then checking if the message on exception thrown by
`InetAddress.getByName("nonexistent.local");` starts with `"不知道这样的主机。"` (or
`"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
change, the exception message started with a row of question marks.
-
Commit messages:
- Update copyrights
- Remove LastError
- Change getLastErrorString to return a jstring
Changes: https://git.openjdk.org/jdk/pull/12922/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8303814
Stats: 138 lines in 8 files changed: 19 ins; 44 del; 75 mod
Patch: https://git.openjdk.org/jdk/pull/12922.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12922/head:pull/12922
PR: https://git.openjdk.org/jdk/pull/12922
Integrated: 8300024: Replace use of JNI_COMMIT mode with mode 0
On Thu, 12 Jan 2023 09:23:49 GMT, Daniel Jeliński wrote: > Please review this patch that fixes a few memory leaks in JNI code. > > [The latest > documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) > of JNI functions makes an explicit note about the use of JNI_COMMIT: > >> If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of >> the elements in `array`, then a final call to >> *ReleaseArrayElements* passing a `mode` argument of "0" or >> `JNI_ABORT`, should be made to free the `elems` buffer > > No new regression test. I manually verified the Linux fix using ClhdsbPstack > test in root mode. Also, tier1-2 tests on mach5 continue to pass. This pull request has now been integrated. Changeset: 50e7df91 Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/50e7df91c77d436937fff9134174ddb8a8dd4dd7 Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod 8300024: Replace use of JNI_COMMIT mode with mode 0 Reviewed-by: amenkov, sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/11963
Integrated: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 12:08:51 GMT, Daniel Jeliński wrote: > Please review this fix for DwarfParser cleaner. > > The original code registered the cleaner using a lambda that captured a > reference to the parser object; as a result, the object was never GCed, and > the cleaner never ran. > > In this version I moved the lambda creation to a static method, so that it > can't capture a reference to the parser. > > Additionally, the new code uses a static cleaner; the cleaner object creation > and cleanup are heavy operations (every cleaner comes with its own thread), > and it's preferable to use a single shared cleaner where possible. > > I verified manually that the native `destroyDwarfContext` method is called > after this fix. No new automated test. Existing tier1-2 tests continue to > pass. This pull request has now been integrated. Changeset: fe7fca01 Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/fe7fca0128ca3a7b514c49d1508ca64499a8bb8e Stats: 8 lines in 1 file changed: 5 ins; 1 del; 2 mod 8300032: DwarfParser resource leak Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 21:26:09 GMT, Chris Plummer wrote: > This seems like a pretty serious flaw with using Lambdas and Cleaners that > probably should be brought up with the libs and language teams. They are aware of this issue; it's even mentioned in [this article](https://inside.java/2022/05/25/clean-cleaner/): > For example, if `chars` is a field, the lambda could refer to `this.chars` > inadvertently capturing `this`. [...] One way to ensure that this is not > captured is to create the lambda in a static method. Its scope does not have > this so it cannot accidentally be captured. As far as I could tell, the cleaners you pointed out use local variables only. As long as the lambda does not reference any object fields or instance methods, `this` is not captured. - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
On Thu, 12 Jan 2023 21:02:23 GMT, Chris Plummer wrote: > There are occurrences of JNI_COMMIT on macos in libawt and libsaproc. Is > there a reason you did not fix these also? My search for JNI_COMMIT filtered out all *.m files. Thanks for pointing that out. libsaproc is corrected now. I will fix AWT in a separate PR. - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0 [v2]
> Please review this patch that fixes a few memory leaks in JNI code. > > [The latest > documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) > of JNI functions makes an explicit note about the use of JNI_COMMIT: > >> If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of >> the elements in `array`, then a final call to >> *ReleaseArrayElements* passing a `mode` argument of "0" or >> `JNI_ABORT`, should be made to free the `elems` buffer > > No new regression test. I manually verified the Linux fix using ClhdsbPstack > test in root mode. Also, tier1-2 tests on mach5 continue to pass. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: MacOS fix - Changes: - all: https://git.openjdk.org/jdk/pull/11963/files - new: https://git.openjdk.org/jdk/pull/11963/files/cecacd13..bea9f536 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11963&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11963&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11963.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11963/head:pull/11963 PR: https://git.openjdk.org/jdk/pull/11963
RFR: 8300032: DwarfParser resource leak
Please review this fix for DwarfParser cleaner. The original code registered the cleaner using a lambda that captured a reference to the parser object; as a result, the object was never GCed, and the cleaner never ran. In this version I moved the lambda creation to a static method, so that it can't capture a reference to the parser. Additionally, the new code uses a static cleaner; the cleaner object creation and cleanup are heavy operations (every cleaner comes with its own thread), and it's preferable to use a single shared cleaner where possible. I verified manually that the native `destroyDwarfContext` method is called after this fix. No new automated test. Existing tier1-2 tests continue to pass. - Commit messages: - Fix dwarf cleaner Changes: https://git.openjdk.org/jdk/pull/11965/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11965&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300032 Stats: 8 lines in 1 file changed: 5 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11965.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11965/head:pull/11965 PR: https://git.openjdk.org/jdk/pull/11965
RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
Please review this patch that fixes a few memory leaks in JNI code. [The latest documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) of JNI functions makes an explicit note about the use of JNI_COMMIT: > If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of > the elements in `array`, then a final call to > *ReleaseArrayElements* passing a `mode` argument of "0" or > `JNI_ABORT`, should be made to free the `elems` buffer No new regression test. I manually verified the Linux fix using ClhdsbPstack test in root mode. Also, tier1-2 tests on mach5 continue to pass. - Commit messages: - Fix release mode Changes: https://git.openjdk.org/jdk/pull/11963/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11963&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300024 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11963.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11963/head:pull/11963 PR: https://git.openjdk.org/jdk/pull/11963
Integrated: 8299593: getprotobyname should not be used
On Wed, 4 Jan 2023 12:56:18 GMT, Daniel Jeliński wrote: > Please review this patch that removes the remaining uses of non-reentrant > `getprotobyname` function. > > While the protocol number for TCP could theoretically be modified to > something other than the default `IPPROTO_TCP`, that scenario would likely > not work, and is not something that we are willing to support. [Existing > code](https://github.com/openjdk/jdk/blob/2f3f3b618500b5f112fabca30d4c6780b2a8e723/src/java.base/unix/native/libnet/net_util_md.c#L355) > in networking area is using `IPPROTO_TCP` for `TCP_NODELAY`, and no issues > were reported. > > Tier 1-3 tests still pass. This pull request has now been integrated. Changeset: d03a5d95 Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/d03a5d9580ef3b9be4d766ff9a11d6fd5fa133f9 Stats: 8 lines in 2 files changed: 0 ins; 4 del; 4 mod 8299593: getprotobyname should not be used Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/11842
RFR: 8299593: getprotobyname should not be used
Please review this patch that removes the remaining uses of non-reentrant `getprotobyname` function. While the protocol number for TCP could theoretically be modified to something other than the default `IPPROTO_TCP`, that scenario would likely not work, and is not something that we are willing to support. [Existing code](https://github.com/openjdk/jdk/blob/2f3f3b618500b5f112fabca30d4c6780b2a8e723/src/java.base/unix/native/libnet/net_util_md.c#L355) in networking area is using `IPPROTO_TCP` for `TCP_NODELAY`, and no issues were reported. Tier 1-3 tests still pass. - Commit messages: - Copyright - Remove getprotobyname Changes: https://git.openjdk.org/jdk/pull/11842/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11842&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299593 Stats: 8 lines in 2 files changed: 0 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/11842.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11842/head:pull/11842 PR: https://git.openjdk.org/jdk/pull/11842
Integrated: 8292233: Increase symtab hash table size
On Thu, 11 Aug 2022 07:12:17 GMT, Daniel Jeliński wrote: > Resize the hash table to minimize collisions. The correct size was already > calculated, but was not used. This pull request has now been integrated. Changeset: 083e014d Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/083e014d0caf673f5da04229ba263f45724cb418 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8292233: Increase symtab hash table size Reviewed-by: kevinw, cjplummer - PR: https://git.openjdk.org/jdk/pull/9834
RFR: 8292233: Increase symtab hash table size
Resize the hash table to minimize collisions. The correct size was already calculated, but was not used. - Commit messages: - Increase hash table size Changes: https://git.openjdk.org/jdk/pull/9834/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9834&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8292233 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9834.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9834/head:pull/9834 PR: https://git.openjdk.org/jdk/pull/9834
Integrated: 8289768: Clean up unused code
On Tue, 5 Jul 2022 20:19:10 GMT, Daniel Jeliński wrote: > This patch removes many unused variables and one unused label reported by the > compilers when relevant warnings are enabled. > > The unused code was found by compiling after removing `unused` from the list > of disabled warnings for > [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) > and > [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), > and enabling > [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) > MSVC warning. > > I only removed variables that were uninitialized or initialized without side > effects. I verified that the removed variables were not used in any > `#ifdef`'d code. I checked that the changed code still compiles on Windows, > Linux and Mac, both in release and debug versions. This pull request has now been integrated. Changeset: 04c47da1 Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/04c47da118b2870d1c7525348a2ffdf9cd1cc0a4 Stats: 98 lines in 46 files changed: 0 ins; 69 del; 29 mod 8289768: Clean up unused code Reviewed-by: dfuchs, lancea, weijun, naoto, cjplummer, alanb, michaelm, chegar - PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: 8289768: Clean up unused code [v3]
On Fri, 8 Jul 2022 07:08:46 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - Update copyright > - Remove more unused variables Copyrights updated. - PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: 8289768: Clean up unused code [v3]
> This patch removes many unused variables and one unused label reported by the > compilers when relevant warnings are enabled. > > The unused code was found by compiling after removing `unused` from the list > of disabled warnings for > [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) > and > [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), > and enabling > [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) > MSVC warning. > > I only removed variables that were uninitialized or initialized without side > effects. I verified that the removed variables were not used in any > `#ifdef`'d code. I checked that the changed code still compiles on Windows, > Linux and Mac, both in release and debug versions. Daniel Jeliński has updated the pull request incrementally with two additional commits since the last revision: - Update copyright - Remove more unused variables - Changes: - all: https://git.openjdk.org/jdk/pull/9383/files - new: https://git.openjdk.org/jdk/pull/9383/files/b4cd5374..da30ef90 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9383&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9383&range=01-02 Stats: 30 lines in 26 files changed: 0 ins; 4 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/9383.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9383/head:pull/9383 PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: 8289768: Clean up unused code [v2]
On Thu, 7 Jul 2022 19:06:52 GMT, Chris Plummer wrote: >> Daniel Jeliński 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 four additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin' into unused-variables >> - Remove unused variables (windows) >> - Remove unused variables (macos) >> - Remove unused variables (linux) > > src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c line 308: > >> 306: ELF_SHDR* shbuf = NULL; >> 307: ELF_SHDR* cursct = NULL; >> 308: ELF_PHDR* phbuf = NULL; > > phbuf is also essentially unused. The only reference is to free it if > non-null, but it's always NULL, so that code can be removed too. Good catch! Also removed similar code from macOS version. - PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: 8289768: Clean up unused code [v2]
On Wed, 6 Jul 2022 05:32:29 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński 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 four additional > commits since the last revision: > > - Merge remote-tracking branch 'origin' into unused-variables > - Remove unused variables (windows) > - Remove unused variables (macos) > - Remove unused variables (linux) test failure is unrelated, caused by [JDK-8289619](https://bugs.openjdk.org/browse/JDK-8289619) - PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: 8289768: Clean up unused code [v2]
> This patch removes many unused variables and one unused label reported by the > compilers when relevant warnings are enabled. > > The unused code was found by compiling after removing `unused` from the list > of disabled warnings for > [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) > and > [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), > and enabling > [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) > MSVC warning. > > I only removed variables that were uninitialized or initialized without side > effects. I verified that the removed variables were not used in any > `#ifdef`'d code. I checked that the changed code still compiles on Windows, > Linux and Mac, both in release and debug versions. Daniel Jeliński 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 four additional commits since the last revision: - Merge remote-tracking branch 'origin' into unused-variables - Remove unused variables (windows) - Remove unused variables (macos) - Remove unused variables (linux) - Changes: - all: https://git.openjdk.org/jdk/pull/9383/files - new: https://git.openjdk.org/jdk/pull/9383/files/915680c0..b4cd5374 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9383&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9383&range=00-01 Stats: 491 lines in 22 files changed: 458 ins; 5 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/9383.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9383/head:pull/9383 PR: https://git.openjdk.org/jdk/pull/9383
RFR: 8289768: Clean up unused code
This patch removes many unused variables and one unused label reported by the compilers when relevant warnings are enabled. The unused code was found by compiling after removing `unused` from the list of disabled warnings for [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) and [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), and enabling [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) MSVC warning. I only removed variables that were uninitialized or initialized without side effects. I verified that the removed variables were not used in any `#ifdef`'d code. I checked that the changed code still compiles on Windows, Linux and Mac, both in release and debug versions. - Commit messages: - Remove unused variables (windows) - Remove unused variables (macos) - Remove unused variables (linux) Changes: https://git.openjdk.org/jdk/pull/9383/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9383&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8289768 Stats: 69 lines in 45 files changed: 0 ins; 65 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/9383.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9383/head:pull/9383 PR: https://git.openjdk.org/jdk/pull/9383
