RFR: 8314114: Fix -Wconversion warnings in os code, primarily linux [v2]
Coleen Phillimore
coleenp at openjdk.org
Fri Aug 11 13:29:59 UTC 2023
On Fri, 11 Aug 2023 02:34:35 GMT, David Holmes <dholmes at openjdk.org> wrote:
>> 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.
>
> 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?
The reason I changed it was to assign into size_t below without sign conversion.
> 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?
This gets a sign comparison warning without the cast, which thankfully we do enable.
> 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?
I was trying to be consistent with linux and failed. This code is duplicated or mostly duplicated for aix, bsd and linux. Somebody should fix that.
> 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
It doesn't. I got a Wconversion warning converting to int here. I wouldn't have changed it otherwise.
src/hotspot/os/linux/os_linux.cpp:404:21: warning: conversion from 'long int' to 'int' may change value [-Wconversion]
404 | int rslt = syscall(SYS_gettid);
| ~~~~~~~^~~~~~~~~~~~
> 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.
One of you wants checked_cast<> everywhere and another of you doesn't. Given that we don't know from local code inspection that the result fits in an int, I chose checked_cast<> for these 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:745:48: warning: conversion from 'intx' {aka 'long int'} to 'OSThread::thread_id_t' {aka 'int'} may change value [-Wconversion]
745 | osthread->set_thread_id(os::current_thread_id());
| ~~~~~~~~~~~~~~~~~~~~~^~
> 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.
Made statlen size_t to match fread return.
> 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.
ok
> 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?
Most of the vm code casts strlen result to int because it's always an int. strlen returning size_t for a small buffer is really annoying. Maybe a checked_cast<> would catch overflow though and maybe it's a good idea. It's weird how fgets takes int and not size_t.
> 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?
I don't have -Wsign-conversion on. I'm not that much of a masochist. Fixed return value though.
> 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.
It does not.
> 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/linux/waitBarrier_linux.cpp:41:17: warning: conversion from 'long int' to 'int' may change value [-Wconversion]
41 | return syscall(SYS_futex, addr, futex_op, op_arg, nullptr, nullptr, 0);
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 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:1211:31: warning: conversion from 'long int' to 'int' may change value [-Wconversion]
1211 | clock_tics_per_sec = sysconf(_SC_CLK_TCK);
| ~~~~~~~^~~~~~~~~~~~~
> 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.
seeing if time_t will compile, that would be better.
> 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
I renamed the macro, since the callers all return size_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.
agree.
> 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.
Given that the three functions that use this return ssize_t, I've renamed the 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
I agree but I added it to make other reviewers happy.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291254942
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291255326
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291283317
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291258484
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291259017
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291259541
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291289074
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291269177
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291266749
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291270521
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291271264
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291282135
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291275060
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291277737
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291272464
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291278408
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291281519
PR Review Comment: https://git.openjdk.org/jdk/pull/15229#discussion_r1291290714
More information about the hotspot-dev
mailing list