RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc

Fri Mar 29 08:14:35 UTC 2024

On Thu, 28 Mar 2024 16:50:20 GMT, Joachim Kern <jkern at 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