-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8308288: Fix xlc17 clang warnings in shared code #14146
Conversation
👋 Welcome back JoKern65! A progress list of the required criteria for merging this PR into |
@JoKern65 The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
In the macro define checks, you both check macros _AIX and AIX, sometimes this, sometimes that. Might be possible to just use one of the two for consistency, but I am fine with using both as well, leave this up to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I can't review the warning suppressions. Without more information
about exactly what warnings are being suppressed and where, I have no way to
evaluate whether suppression is an appropriate response. It could be that some
of the warnings are correctly pointing out (possibly serious) flaws that
really need to be fixed. Continuing to hide them doesn't seem like a winning
strategy to me.
I also suggest avoiding omnibus changes like this when possible (which it is
here). Just because it's all about removing warnings from a new toolchain
doesn't make it a cohesive change. That makes it harder to review and to
manage the review, because it is larger and affects a wide range of areas, so
may need many reviewers. And the whole thing might get stalled by discussion
of one piece.
@@ -692,6 +701,15 @@ ifeq ($(ENABLE_HEADLESS_ONLY), false) | |||
LIBSPLASHSCREEN_CFLAGS += -DPNG_POWERPC_VSX_OPT=0 | |||
endif | |||
endif | |||
# temporarily disable PNG_POWERPC_VSX_OPT which would be set, because | |||
# if defined(__PPC64__) && defined(__ALTIVEC__) && defined(__VSX__) | |||
# is true, the then needed libpng function .png_init_filter_functions_vsx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/then//
&& defined(__open_xl_version__) && __open_xl_version__ >= 17 | ||
#undef malloc | ||
extern void *malloc(size_t) asm("vec_malloc"); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -125,7 +125,7 @@ Java_GetXSpace_getSpace0 | |||
} | |||
#else | |||
struct statfs buf; | |||
int result = statfs((const char*)chars, &buf); | |||
int result = statfs((char*)chars, &buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this working around a bug in IBM's declaration?
Also, pre-existing, the cast seems really suspicious. The type of chars
is jchar*
, which is a
sequence of 16bit characters. Does this actually work? If so, how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is IBMs declaration of statfs
extern int statfs(char *, struct statfs *);
So the compiler will not accept a const char*
Indeed I do not know if this ever worked, but my change makes it not worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the documentation of statfs on AIX https://www.ibm.com/docs/en/aix/7.2?topic=s-statfs-fstatfs-statfs64-fstatfs64-ustat-subroutine (showing the IBM declaration Joachim told us) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, pre-existing, the cast seems really suspicious. The type of
chars
isjchar*
, which is a sequence of 16bit characters. Does this actually work? If so, how?
Probably a jchar to char conversion would be needed for the array elements, maybe like this here (or is there a better utility function in the codebase) ? See jCharArrayToCKCharArray
void jCharArrayToCKCharArray(JNIEnv *env, const jcharArray jArray, CK_CHAR_PTR *ckpArray, CK_ULONG_PTR ckpLength) |
I am not aware of an AIX statfs for wchars but maybe I miss something. But it is a separate issue of Java_GetXSpace_getSpace0 anyway and should be handled in another bug.
Here are the reasons for the disabled warnings in make/modules/java.base/Lib.gmk src/java.base/unix/native/libnet/DefaultProxySelector.c:378:41:22: error: passing arguments to a function without a prototype is deprecated in all versions of C and is not supported in C2x [-Werror,-Wdeprecated-non-prototype] src/java.base/unix/native/libnet/NetworkInterface.c:1612:24: error: arithmetic on a pointer to void is a GNU extension [-Werror,-Wgnu-pointer-arith] src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c:1251:7: error: '_ALLBSD_SOURCE' is not defined, evaluates to 0 [-Werror,-Wundef]
|
Here are the reasons for the disabled warnings in make/modules/java.base/lib/CoreLibraries.gmk src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:638 comparison of integers of different signs: 'int' and 'unsigned long' 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] 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] |
Should probably better be |
We disable this warning too for clang on all platforms in BUILD_LIBJLI ( DISABLED_WARNINGS_clang := format-nonliteral deprecated-non-prototype, \ ), so I think we should do it the same way for BUILD_LIBJLI_STATIC on AIX. |
We should probably change to size_t ret (from type int), then the warning would not occur, correct ? |
Here are the reasons for the disabled warnings in make/modules/java.desktop/lib/Awt2dLibraries.gmk 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] 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] src/java.desktop/share/native/common/java2d/opengl/OGLPaints.c:581:48: error: format string is not a string literal [-Werror,-Wformat-nonliteral] src/java.desktop/share/native/common/java2d/opengl/OGLBufImgOps.c:153:48: error: format string is not a string literal [-Werror,-Wformat-nonliteral] src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c:1095:41: error: '&&' within '||' [-Werror,-Wlogical-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] 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] 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] |
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: |
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] |
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 |
#ifdef _AIX | ||
#include <alloca.h> | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these conditionals be included in globalDefinitions_xlc.hpp instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle the #include <alloca.h>
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least the whole jdk actually builds with #include <alloca.h>
in globalDefinitions_xlc.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@JoKern65 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 89 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kimbarrett, @prrace) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Hi, As this PR is big and spans several components I split I guess we need to find a way to fix the issue with the malloc Thanks for your help so far! |
Hi, As this PR is big and spans several components I split I guess we need to find a way to fix the issue with the malloc Thanks for your help so far! |
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.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14146/head:pull/14146
$ git checkout pull/14146
Update a local copy of the PR:
$ git checkout pull/14146
$ git pull https://git.openjdk.org/jdk.git pull/14146/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14146
View PR using the GUI difftool:
$ git pr show -t 14146
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14146.diff
Webrev
Link to Webrev Comment