Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ Hi, As this PR is big and spans several components I split off the java.base, java.desktop and the sercivability/security issues into extra JBS issues. https://bugs.openjdk.org/browse/JDK-8309219 Fix xlc17 clang 1.5 warnings in java.base https://bugs.openjdk.org/browse/JDK-8309224 Fix xlc17 clang 1.5 warnings in java.desktop https://bugs.openjdk.org/browse/JDK-8309225 Fix xlc17 clang 1.5 warnings in security and servicability I’ll move the changes from this pull request into new pull requests. I will incorporate the requested changes right in the new PRs. I will reuse this issue 8308388 for the hotspot changes but come up with a new, smaller PR. @colleenp, I will move alloca.h to the globalDefinitions_xlc.hpp. @prrace, I will come up with an identical PR for the client files (java.desktop), but improve the comment as @kimbarrett proposed @mbaesken, I will use AIX and take up some of the other fixes you proposed. I guess we need to find a way to fix the issue with the malloc in globalDefinitions_xlc.hpp in the upcoming PR for hotspot. Thanks for your help so far! Hi, As this PR is big and spans several components I split off the java.base, java.desktop and the sercivability/security issues into extra JBS issues. https://bugs.openjdk.org/browse/JDK-8309219 Fix xlc17 clang 1.5 warnings in java.base https://bugs.openjdk.org/browse/JDK-8309224 Fix xlc17 clang 1.5 warnings in java.desktop https://bugs.openjdk.org/browse/JDK-8309225 Fix xlc17 clang 1.5 warnings in security and servicability I’ll move the changes from this pull request into new pull requests. I will incorporate the requested changes right in the new PRs. I will reuse this issue 8308388 for the hotspot changes but come up with a new, smaller PR. @colleenp, I will move alloca.h to the globalDefinitions_xlc.hpp. @prrace, I will come up with an identical PR for the client files (java.desktop), but improve the comment as @kimbarrett proposed @MBaesken, I will use AIX and take up some of the other fixes you proposed. I guess we need to find a way to fix the issue with the malloc in globalDefinitions_xlc.hpp in the upcoming PR for hotspot. Thanks for your help so far! - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1572023812 PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1572024628
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ I am Ok with the client-libs changes here. I did not look at anything else. So you'll need more approvals before its ready to push. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14146#pullrequestreview-1454101589
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Tue, 30 May 2023 21:44:17 GMT, JoKern65 wrote: >> src/hotspot/share/runtime/javaThread.cpp line 115: >> >>> 113: #include >>> 114: #endif >>> 115: >> >> Could these conditionals be included in globalDefinitions_xlc.hpp instead? > > In principle the `#include ` could be included in > globalDefinitions_xlc.hpp. But alloca.h implements it with a > `#undef alloca` > `#define alloca(size) __builtin_alloca (size)` > If this new global define does not introduce new trouble (even in future > coding) when it is seen in every source, we can go this way. > Are there any obstacles from anyone else? At least the whole jdk actually builds with `#include ` in globalDefinitions_xlc.hpp - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1211272616
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Tue, 30 May 2023 18:42:54 GMT, Coleen Phillimore wrote: >> JoKern65 has updated the pull request incrementally with one additional >> commit since the last revision: >> >> forgotton _ > > src/hotspot/share/runtime/javaThread.cpp line 115: > >> 113: #include >> 114: #endif >> 115: > > Could these conditionals be included in globalDefinitions_xlc.hpp instead? In principle the `#include ` could be included in globalDefinitions_xlc.hpp. But alloca.h implements it with a `#undef alloca` `#define alloca(size) __builtin_alloca (size)` If this new global define does not introduce new trouble (even in future coding) when it is seen in every source, we can go this way. Are there any obstacles from anyone else? - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1210853233
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ src/hotspot/share/runtime/javaThread.cpp line 115: > 113: #include > 114: #endif > 115: Could these conditionals be included in globalDefinitions_xlc.hpp instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1210684136
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Sat, 27 May 2023 15:33:37 GMT, Kim Barrett wrote: >> 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. > > Is it possible to see the stdlib.h source code that is being a problem? > Maybe more eyes can come up > with a better solution, or at least come to a better understanding of why we > have to go this way. Technically it's our fault. The standard library is permitted to provide a macro replacements for (nearly?) any function. (C99 7.1.3/1 3rd bullet) But it's really annoying. Presumably AIX is only doing so for a subset of the allocation functions, e.g. not "free" for example. Having "malloc" defined as a macro seems like it should raise all kinds of havoc, not just around log tags. Consider our "os::malloc" function? The preprocessor knows nothing about C++ namespace qualifiers. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1210388482
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Sat, 27 May 2023 11:50:11 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. >> >> 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. Is it possible to see the stdlib.h source code that is being a problem? Maybe more eyes can come up with a better solution, or at least come to a better understanding of why we have to go this way. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1208039639
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 Fri, 26 May 2023 16:58:41 GMT, Thomas Stuefe wrote: >> 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. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207308708
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: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ Just a side note: The warning elimination is new for AIX compilers. We had given it up at earlier times: https://bugs.openjdk.org/browse/JDK-8202325 I still appreciate if we can get warning free and it makes sense to review all of them. But, I wouldn't require it for JDK21. - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564406986
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote:
>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the
>> OpenJDK C/C++ code.
>
> JoKern65 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> forgotton _
Here are the reasons for the disabled warnings in
make/modules/java.desktop/lib/Awt2dLibraries.gmk
/data/d042520/pr/jdk/src/java.desktop/unix/native/common/awt/awt_GraphicsEnv.h:53:12:
error: a function declaration without a prototype is deprecated in all
versions of C and is treated as a zero-parameter prototype in C2x, conflicting
with a previous declaration [-Werror,-Wdeprecated-non-prototype]
extern int XShmQueryExtension();
^
/usr/include/X11/extensions/XShm.h:91:6: note: conflicting prototype is here
Bool XShmQueryExtension(
^
solved by adding line (generic, because several source files are involved)
DISABLED_WARNINGS_clang_aix := deprecated-non-prototype, \
src/java.desktop/unix/native/libawt_xawt/xawt/awt_Taskbar.c:158:11: error:
using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (m = fp_unity_launcher_entry_get_quicklist(entry)) {
~~^~
solved by adding line
DISABLED_WARNINGS_clang_aix_awt_Taskbar.c := parentheses, \
src/java.desktop/share/native/common/java2d/opengl/OGLPaints.c:581:48: error:
format string is not a string literal [-Werror,-Wformat-nonliteral]
snprintf(cycleCode, sizeof(cycleCode), noCycleCode, texCoordCalcCode);
^~~
solved by adding line
DISABLED_WARNINGS_clang_aix_OGLPaints.c := format-nonliteral, \
src/java.desktop/share/native/common/java2d/opengl/OGLBufImgOps.c:153:48:
error: format string is not a string literal [-Werror,-Wformat-nonliteral]
snprintf(finalSource, sizeof(finalSource), convolveShaderSource,
^~~~
solved by adding line
DISABLED_WARNINGS_clang_aix_OGLBufImgOps.c := format-nonliteral, \
src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c:1095:41: error:
'&&' within '||' [-Werror,-Wlogical-op-parentheses]
if ((synth_state & MOUSE_OVER) != 0 && (synth_state & PRESSED) == 0 ||
^~~ ~~
src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c:1180:29: error:
using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (init_result = (NULL == gtk2_widgets[_GTK_CHECK_MENU_ITEM_TYPE]))
^~~
solved by adding line
DISABLED_WARNINGS_clang_aix_gtk2_interface.c := parentheses
logical-op-parentheses, \
src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:903:29: error:
using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (init_result = (NULL == gtk3_widgets[_GTK_BUTTON_TYPE]))
^~
solved by adding line
DISABLED_WARNINGS_clang_aix_gtk3_interface.c := parentheses, \
src/java.desktop/unix/native/libawt_xawt/awt/sun_awt_X11_GtkFileDialogPeer.c:87:26:
error: using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (pendingException = (*env)->ExceptionOccurred(env)) {
~^~~~
solved by adding line
DISABLED_WARNINGS_clang_aix_sun_awt_X11_GtkFileDialogPeer.c := parentheses, \
src/java.desktop/aix/native/libawt_xawt/awt/awt_InputMethod.c:1969:32: error:
comparison of integers of different signs: 'Window' (aka 'unsigned long') and
'jlong' (aka 'long') [-Werror,-Wsign-compare]
if (currentFocusWindow != w) {
~~ ^ ~
solved by adding line
DISABLED_WARNINGS_clang_aix_awt_InputMethod.c := sign-compare, \
Here is the reason for the disabled warning in
make/modules/java.security.jgss/Lib.gmk
src/java.security.jgss/share/native/libj2gss/NativeUtil.h:30:
/data/d042520/pr/jdk/src/java.security.jgss/share/native/libj2gss/gssapi.h:48:5:
error: 'TARGET_OS_MAC' is not defined, evaluates to 0 [-Werror,-Wundef]
#if TARGET_OS_MAC && (defined(__ppc__) || defined(__ppc64__) ||
defined(__i386__) || defined(__x86_64__))
^
make/modules/java.security.jgss/Lib.gmk add line
DISABLED_WARNINGS_clang_aix := undef, \
Here is the reason for the disabled warning in
make/modules/jdk.jdwp.agent/Lib.gmk
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c:718:33: error:
suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
struct in6_addr
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 10:18:37 GMT, JoKern65 wrote:
> Here are the reasons for the disabled warnings in
> make/modules/java.base/lib/CoreLibraries.gmk
> DISABLED_WARNINGS_clang_aix_ProcessHandleImpl_unix.c := sign-compare,
> DISABLED_WARNINGS_clang_aix := gnu-pointer-arith, DISABLED_WARNINGS_clang :=
> gnu-pointer-arith format-nonliteral deprecated-non-prototype, \
>
> src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:638 comparison of
> integers of different signs: 'int' and 'unsigned long' if (ret <
> sizeof(psinfo_t)) { ^
>
`fread` returns a `size_t ` on Linux and AIX, could you please check Mac/BSD
too ?
https://github.com/openjdk/jdk/blob/d3b9b364da8c11c9b4dd14a6451a7b24f41202e7/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c#L636
We should probably change to size_t ret (from type int), then the warning
would not occur, correct ?
-
PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564207011
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 10:18:37 GMT, JoKern65 wrote: > src/java.base/share/native/libjli/java.c:2311:22: error: format string is not > a string literal [-Werror,-Wformat-nonliteral] vfprintf(stderr, fmt, vl); ^~~ We disable this warning too for clang on all platforms in BUILD_LIBJLI ( DISABLED_WARNINGS_clang := **format-nonlitera**l deprecated-non-prototype, \ ), so I think we should do it the same way for BUILD_LIBJLI_STATIC on AIX. - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564193886
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote:
>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the
>> OpenJDK C/C++ code.
>
> JoKern65 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> forgotton _
Here are the reasons for the disabled warnings in
make/modules/java.base/lib/CoreLibraries.gmk
DISABLED_WARNINGS_clang_aix_ProcessHandleImpl_unix.c := sign-compare, \
DISABLED_WARNINGS_clang_aix := gnu-pointer-arith, \
DISABLED_WARNINGS_clang := gnu-pointer-arith format-nonliteral
deprecated-non-prototype, \
src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:638 comparison of
integers of different signs: 'int' and 'unsigned long'
if (ret < sizeof(psinfo_t)) {
^
src/java.base/aix/native/libjli/java_md_aix.c:42:38: error: arithmetic on a
pointer to void is a GNU extension [-Werror,-Wgnu-pointer-arith]
addr < p->ldinfo_textorg + p->ldinfo_textsize) {
~ ^
src/java.base/share/native/libzip/zlib/inffast.c:74:20: error: a function
definition without a prototype is deprecated in all versions of C and is not
supported in C2x [-Werror,-Wdeprecated-non-prototype]
void ZLIB_INTERNAL inflate_fast(strm, start)
^
src/java.base/share/native/libjli/java.c:2311:22: error: format string is not a
string literal [-Werror,-Wformat-nonliteral]
vfprintf(stderr, fmt, vl);
^~~
-
PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564165195
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk > on AIX , we run into various "warnings as errors". > Some of those are in shared codebase and could be addressed by small > adjustments. > A lot of those changes are in hotspot, some might be somewhere else in the > OpenJDK C/C++ code. JoKern65 has updated the pull request incrementally with one additional commit since the last revision: forgotton _ - Changes: - all: https://git.openjdk.org/jdk/pull/14146/files - new: https://git.openjdk.org/jdk/pull/14146/files/2699fdce..da38fb2d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14146=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14146=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14146.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14146/head:pull/14146 PR: https://git.openjdk.org/jdk/pull/14146
