Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc
On Fri, 29 Mar 2024 07:56:10 GMT, Thomas Stuefe wrote:
>> src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 50:
>>
>>> 48: #undef malloc
>>> 49: extern void *malloc(size_t) asm("vec_malloc");
>>> 50: #endif
>>
>> This `#if` is not needed if we are building on AIX 7.2 TL5 SP7 or higher.
>> This is the case in our build environment, but I think IBM is still building
>> with SP5, which would run into build errors if I remove this `#if` now.
>
> While looking at this, I noticed that my question in
> https://github.com/openjdk/jdk/pull/14146#discussion_r1207078176 and
> followups had never been answered. Do you know the answers now?
>
> Quoting myself:
>
>> So, we do this only for malloc? Not for calloc, posix_memalign, realloc etc?
>> What about free?
>> Removing that define and hard-coding it here assumes ... pointers it returns
>> work with the unchanged free() and realloc() the system provides, and will
>> always do so.
>> 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.
Other than that, and kind of depending on your answer: How important is it that
we catch every use of the original malloc? Can be safely mix the original
malloc with vec_malloc if logging is not involved?
I am asking, because from that it depends whether this hunk needs to appear
right behind `#include ` or whether we can move it into the middle of
the file together with the other AIX stuff.
Because, if we move it into the middle of the file, we may miss any uses of
malloc that may happen in system headers (would be unusual for that to happen
but with IBM one never knows).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544201412
Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc
On Thu, 28 Mar 2024 16:57:00 GMT, Joachim Kern 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
>
> src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 50:
>
>> 48: #undef malloc
>> 49: extern void *malloc(size_t) asm("vec_malloc");
>> 50: #endif
>
> This `#if` is not needed if we are building on AIX 7.2 TL5 SP7 or higher.
> This is the case in our build environment, but I think IBM is still building
> with SP5, which would run into build errors if I remove this `#if` now.
While looking at this, I noticed that my question in
https://github.com/openjdk/jdk/pull/14146#discussion_r1207078176 and followups
had never been answered. Do you know the answers now?
Quoting myself:
> So, we do this only for malloc? Not for calloc, posix_memalign, realloc etc?
> What about free?
> Removing that define and hard-coding it here assumes ... pointers it returns
> work with the unchanged free() and realloc() the system provides, and will
> always do so.
> 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544199335
Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc
On Fri, 29 Mar 2024 07:18:47 GMT, Thomas Stuefe 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
>
> 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()`.
Check copyrights in this file and all others. Adapt SAP and Oracle copyrights.
> 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
Here, and in other places too where you cast a pointer to fit into PTR_FORMAT
or INTPTR_FORMAT
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544172412
PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1544181011
Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc
On Thu, 28 Mar 2024 16:50:20 GMT, Joachim Kern 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
> 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
