RFR: 8314114: Fix -Wconversion warnings in os code, primarily linux [v2]

Fri Aug 11 03:44:58 UTC 2023

On Thu, 10 Aug 2023 22:52:30 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change fixes various -Wconversion warnings from files in the os directories and related, widening types or adding checked_cast<> where narrowing.
>> Tested with tier1 on linux/macosx/windows and tier1-4 on linux-x64-debug, windows-x64-debug.
>
> Coleen Phillimore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
> 
>  - Dean's suggested changes.
>  - Merge branch 'master' into os-conversion
>  - Fix cgroupSubsystem_linux.cpp
>  - Unset Wconversion for testing.
>  - Fix signals to take int buflen, fixed return types of syscalls
>  - Fix attachListener.cpp warnings.
>  - change send/recv parameter type to match OS.
>  - Fix -Wconversion warnings in os_linux.

Sorry Coleen lots of comments/queries - many of which apply to multiple sites (I didn't flag them all).

Thanks.

src/hotspot/os/aix/attachListener_aix.cpp line 279:

> 277:   // name ("load", "datadump", ...), and <arg> is an argument
> 278:   int expected_str_count = 2 + AttachOperation::arg_count_max;
> 279:   const unsigned max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +

size_t instead?

src/hotspot/os/aix/attachListener_aix.cpp line 296:

> 294:     // hang in the clean-up when shutting down.
> 295:     n = read(s, buf+off, left);
> 296:     assert(n <= checked_cast<ssize_t>(left), "buffer was too small, impossible!");

why ssize_t here?

src/hotspot/os/bsd/attachListener_bsd.cpp line 273:

> 271: 
> 272:   do {
> 273:     ssize_t n;

Why ssize_t here but int in the AIX version?

src/hotspot/os/linux/os_linux.cpp line 404:

> 402: // thread id is used to access /proc.
> 403: pid_t os::Linux::gettid() {
> 404:   int64_t rslt = syscall(SYS_gettid);

syscall returns an int

src/hotspot/os/linux/os_linux.cpp line 426:

> 424: 
> 425: void os::Linux::initialize_system_info() {
> 426:   set_processor_count(checked_cast<int>(sysconf(_SC_NPROCESSORS_CONF)));

checked_cast seems excessive here and on other sysconf calls.

src/hotspot/os/linux/os_linux.cpp line 745:

> 743:   Monitor* sync = osthread->startThread_lock();
> 744: 
> 745:   osthread->set_thread_id(checked_cast<int>(os::current_thread_id()));

This doesn't look right: `current_thread_id()` returns `intx` and `set_thread_id` takes `thread_id_t`

src/hotspot/os/linux/os_linux.cpp line 1272:

> 1270:     fp = os::fopen("/proc/self/stat", "r");
> 1271:     if (fp) {
> 1272:       statlen = checked_cast<int>(fread(stat, 1, 2047, fp));

checked_cast seems unnecessary given we're reading max 2047 elements.

src/hotspot/os/linux/os_linux.cpp line 1604:

> 1602: 
> 1603:   Elf32_Ehdr elf_head;
> 1604:   int diag_msg_max_length=ebuflen-checked_cast<int>(strlen(ebuf));

can we add spaces around operators while changing this please.

src/hotspot/os/linux/os_linux.cpp line 2669:

> 2667:         // determine if this is a legacy image or modules image
> 2668:         // modules image doesn't have "jre" subdirectory
> 2669:         len = (int)strlen(buf);

Why raw cast here when other uses of strlen have had checked_cast applied?

src/hotspot/os/linux/os_linux.cpp line 2991:

> 2989: #endif
> 2990: 
> 2991:   return (retval == -1) ? checked_cast<int>(retval) : cpu;

If retval is -1 you can just return -1. And if cpu is uint and the function returns int then surely we need a cast on cpu here?

src/hotspot/os/linux/waitBarrier_linux.cpp line 40:

> 38: #endif
> 39: 
> 40: static intptr_t futex(volatile int *addr, int futex_op, int op_arg) {

Why do this? syscall returns an int.

src/hotspot/os/linux/waitBarrier_linux.cpp line 54:

> 52:   assert(_futex_barrier != 0, "Should be armed/non-zero.");
> 53:   _futex_barrier = 0;
> 54:   intptr_t s = futex(&_futex_barrier,

futex returns an int

src/hotspot/os/posix/os_posix.cpp line 92:

> 90: static jlong initial_time_count = 0;
> 91: 
> 92: static int64_t clock_tics_per_sec = 100;

why do we need to do this? It would get promoted in any expression using 64-bit variables

src/hotspot/os/posix/os_posix.cpp line 448:

> 446: void os::Posix::print_uptime_info(outputStream* st) {
> 447:   int bootsec = -1;
> 448:   int64_t currsec = time(nullptr);

We should probably cast here even if the compiler doesn't complain, as we don't know what a time_t actually is.

src/hotspot/os/posix/os_posix.cpp line 803:

> 801: 
> 802: ssize_t os::recv(int fd, char* buf, size_t nBytes, uint flags) {
> 803:   RESTARTABLE_RETURN_INT(::recv(fd, buf, nBytes, flags));

Note the name of the macro: this is supposed to return int. We need a new macro for RESTARTABLE_RETURN_SSIZE_T

src/hotspot/os/posix/os_posix.cpp line 1335:

> 1333: static void to_abstime(timespec* abstime, jlong timeout,
> 1334:                        bool isAbsolute, bool isRealtime) {
> 1335:   DEBUG_ONLY(int64_t max_secs = MAX_SECS;)

Actually I think this should be a time_t.

src/hotspot/os/posix/os_posix.hpp line 48:

> 46: 
> 47: #define RESTARTABLE_RETURN_INT(_cmd) do { \
> 48:   ssize_t _result; \

Ah no - please don't do this. Please define a new macro.

src/hotspot/os/posix/signals_posix.cpp line 1732:

> 1730:     if (sig > MAX2(SIGSEGV, SIGBUS) &&  // See 4355769.
> 1731:         sig < NSIG) {                   // Must be legal signal and fit into sigflags[].
> 1732:       PosixSignals::SR_signum = checked_cast<int>(sig);

checked cast seems unnecessary given the range check

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15229#pullrequestreview-1572936556
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290841203
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290841532
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290843224
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290846602
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290847411
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290848989
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290849910
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290850648
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290851238
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290852797
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290854029
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290854138
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290854821
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290855178
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290855777
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290856533
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290856892
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1290857533