On Thu, 28 Mar 2024 16:50:20 GMT, Joachim Kern <jk...@openjdk.org> wrote:
> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain was removed by > [JDK-8327701](https://bugs.openjdk.org/browse/JDK-8327701). > Now we also switch the HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc, removing the > last xlc rudiment. > This means merging the AIX specific content of > utilities/globalDefinitions_xlc.hpp and utilities/compilerWarnings_xlc.hpp > into the corresponding gcc files on the on side and removing the > defined(TARGET_COMPILER_xlc) blocks in the code, because the > defined(TARGET_COMPILER_gcc) blocks work out of the box for the new AIX > compiler. > The rest of the changes are needed because of using > utilities/compilerWarnings_xlc.hpp the compiler is much more nagging about > ill formatted printf Hi @JoKern65 , this is a nice simplification! There are many casts that should be unnecessary (casting size_t to fit into SIZE_FORMAT) or that should use `p2i` (all those casts to fit pointers into PTR_FORMAT or INTPTR_FORMAT). I did not mark all of them. We also have a new macro for printing memory ranges, RANGEFMT and RANGEFMTARGS. Maybe some call sites could that instead and make the code shorter and better to read? But I leave that up to you. Cheers, and happy Easter! src/hotspot/os/aix/loadlib_aix.cpp line 120: > 118: (lm->is_in_vm ? '*' : ' '), > 119: (uintptr_t)lm->text, (uintptr_t)lm->text + lm->text_len, > 120: (uintptr_t)lm->data, (uintptr_t)lm->data + lm->data_len, Please don't cast, use `p2i()`. src/hotspot/os/aix/os_aix.cpp line 314: > 312: ErrnoPreserver ep; > 313: log_trace(os, map)("disclaim failed: " RANGEFMT " errno=(%s)", > 314: RANGEFMTARGS(p, (long)maxDisclaimSize), Wait, why are these casts needed? maxDisclaimSize is size_t, RANGEFMT uses SIZE_FORMAT. That should work without cast. src/hotspot/os/aix/os_aix.cpp line 651: > 649: lt.print("Thread is alive (tid: " UINTX_FORMAT ", kernel thread id: > " UINTX_FORMAT > 650: ", stack [" PTR_FORMAT " - " PTR_FORMAT " (" SIZE_FORMAT "k > using %luk pages)).", > 651: os::current_thread_id(), (uintx) kernel_thread_id, > (uintptr_t)low_address, (uintptr_t)high_address, Use p2i, not cast src/hotspot/os/aix/os_aix.cpp line 1212: > 1210: st->print_cr("physical free : " SIZE_FORMAT, (unsigned > long)mi.real_free); > 1211: st->print_cr("swap total : " SIZE_FORMAT, (unsigned > long)mi.pgsp_total); > 1212: st->print_cr("swap free : " SIZE_FORMAT, (unsigned > long)mi.pgsp_free); A better way to do this would be to change AIX::meminfo to use size_t. We should have done this when introducing that API. src/hotspot/os/aix/os_aix.cpp line 1399: > 1397: os->print("[" PTR_FORMAT " - " PTR_FORMAT "] (" UINTX_FORMAT > 1398: " bytes, %ld %s pages), %s", > 1399: (uintptr_t)addr, (uintptr_t)addr + size - 1, size, size / > pagesize, describe_pagesize(pagesize), p2i src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 62: > 60: #include <errno.h> > 61: > 62: #if defined(LINUX) || defined(_ALLBSD_SOURCE) || defined(_AIX) What else is left? Could we just remove this line altogether now? src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 83: > 81: #error "xlc version not supported, macro __open_xl_version__ not found" > 82: #endif > 83: #endif // AIX Can probably be shortened like this: Suggestion: #ifdef _AIX #if !defined(__open_xl_version__) || (__open_xl_version__ < 17) #error "this xlc version is not supported" #endif #endif // AIX src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 103: > 101: #endif > 102: > 103: #if !defined(LINUX) && !defined(_ALLBSD_SOURCE) && !defined(_AIX) I believe this whole section can be removed now. At least I have no idea who this is for. What gcc versions does OpenJDK still support, then, beside these platforms. Also, any gcc platform not on linux or bsd would have hit the #error below at line 132. src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 128: > 126: #if defined(__APPLE__) > 127: inline int g_isnan(double f) { return isnan(f); } > 128: #elif defined(LINUX) || defined(_ALLBSD_SOURCE) || defined(_AIX) I think this can be just #else src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 131: > 129: inline int g_isnan(float f) { return isnan(f); } > 130: inline int g_isnan(double f) { return isnan(f); } > 131: #else and this can be removed ------------- Changes requested by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18536#pullrequestreview-1967997963 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544171741 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544174303 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544176004 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544177911 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544181163 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544187429 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544207978 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544190654 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544191835 PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544193002
