Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8314114: Fix -Wconversion warnings in os code, primarily linux #15229

Closed
wants to merge 18 commits into from
Closed
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/hotspot/os/aix/attachListener_aix.cpp
Original file line number Diff line number Diff line change
@@ -108,7 +108,7 @@ class AixAttachListener: AllStatic {
static bool is_shutdown() { return _shutdown; }

// write the given buffer to a socket
static int write_fully(int s, char* buf, int len);
static int write_fully(int s, char* buf, size_t len);

static AixAttachOperation* dequeue();
};
@@ -276,7 +276,7 @@ AixAttachOperation* AixAttachListener::read_request(int s) {
// where <ver> is the protocol version (1), <cmd> is the command
// name ("load", "datadump", ...), and <arg> is an argument
int expected_str_count = 2 + AttachOperation::arg_count_max;
const int max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +
const unsigned max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_t instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I changed it was to assign into size_t below without sign conversion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but can't it be size_t itself in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, doesn't reintroduce warnings to change back to size_t.

AttachOperation::arg_count_max*(AttachOperation::arg_length_max + 1);

char buf[max_len];
@@ -285,15 +285,15 @@ AixAttachOperation* AixAttachListener::read_request(int s) {
// Read until all (expected) strings have been read, the buffer is
// full, or EOF.

int off = 0;
int left = max_len;
size_t off = 0;
size_t left = max_len;

do {
int n;
ssize_t n;
// Don't block on interrupts because this will
// hang in the clean-up when shutting down.
n = read(s, buf+off, left);
assert(n <= left, "buffer was too small, impossible!");
assert(n <= checked_cast<ssize_t>(left), "buffer was too small, impossible!");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ssize_t here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets a sign comparison warning without the cast, which thankfully we do enable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, who is warning here? Is this a static code analysis? Since I don't think Oracle builds AIX, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AIX code is a copy of the linux code. I got a sign conversion warning on the linux code.

src/hotspot/os/linux/attachListener_linux.cpp:275:14: warning: comparison of integer expressions of different signedness: 'ssize_t' {aka 'long int'} and 'size_t' {aka 'long unsigned int'} [-Wsign-compare]
275 | assert(n <= left, "buffer was too small, impossible!");
| ~~^~~~~~~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of how the basic integer type system is broken. You define size variables as size_t but then get ssize_t back from functions like read, and then you get warnings if you combine them in particular ways. In this particular case I don't even understand the conversion warning as semantically a maximum ssize_t must be < the maximum size_t so where is the loss? It also seems to me that the strictly correct way to address this would be to ensure the ssize_t variable is not -1 and then cast it to size_t, rather than downcasting the size_t variable to ssize_t.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the compiler complaining because the comparison will be done as unsigned, and a negative value turns into a huge unsigned value? To do a loss-less compare, both sides would need to be widened to a common signed type that can represent either side, like __int128_t. There's probably an equivalent way to do it with 64-bit values, but I think it would involve more than one compare.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is the problem. My point is that the type system leads you into that problem even though you are trying to use the "correct" types.

buf[max_len - 1] = '\0';
if (n == -1) {
return nullptr; // reset by peer or other error
@@ -414,9 +414,9 @@ AixAttachOperation* AixAttachListener::dequeue() {
}

// write the given buffer to the socket
int AixAttachListener::write_fully(int s, char* buf, int len) {
int AixAttachListener::write_fully(int s, char* buf, size_t len) {
do {
int n = ::write(s, buf, len);
ssize_t n = ::write(s, buf, len);
if (n == -1) {
if (errno != EINTR) return -1;
} else {
4 changes: 2 additions & 2 deletions src/hotspot/os/aix/os_aix.cpp
Original file line number Diff line number Diff line change
@@ -2985,7 +2985,7 @@ int os::get_core_path(char* buffer, size_t bufferSize) {
jio_snprintf(buffer, bufferSize, "%s/core or core.%d",
p, current_process_id());

return strlen(buffer);
return checked_cast<int>(strlen(buffer));
}

bool os::start_debugging(char *buf, int buflen) {
@@ -3023,7 +3023,7 @@ static inline time_t get_mtime(const char* filename) {
int os::compare_file_modified_times(const char* file1, const char* file2) {
time_t t1 = get_mtime(file1);
time_t t2 = get_mtime(file2);
return t1 - t2;
return primitive_compare(t1, t2);
}

bool os::supports_map_sync() {
2 changes: 1 addition & 1 deletion src/hotspot/os/aix/os_perf_aix.cpp
Original file line number Diff line number Diff line change
@@ -77,7 +77,7 @@ static bool read_psinfo(const u_longlong_t& pid, psinfo_t& psinfo) {

FILE* fp;
char buf[BUF_LENGTH];
int len;
size_t len;

jio_snprintf(buf, BUF_LENGTH, "/proc/%llu/psinfo", pid);
fp = fopen(buf, "r");
18 changes: 9 additions & 9 deletions src/hotspot/os/bsd/attachListener_bsd.cpp
Original file line number Diff line number Diff line change
@@ -102,7 +102,7 @@ class BsdAttachListener: AllStatic {
static int listener() { return _listener; }

// write the given buffer to a socket
static int write_fully(int s, char* buf, int len);
static int write_fully(int s, char* buf, size_t len);

static BsdAttachOperation* dequeue();
};
@@ -257,7 +257,7 @@ BsdAttachOperation* BsdAttachListener::read_request(int s) {
// where <ver> is the protocol version (1), <cmd> is the command
// name ("load", "datadump", ...), and <arg> is an argument
int expected_str_count = 2 + AttachOperation::arg_count_max;
const int max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +
const unsigned max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +
AttachOperation::arg_count_max*(AttachOperation::arg_length_max + 1);

char buf[max_len];
@@ -266,21 +266,21 @@ BsdAttachOperation* BsdAttachListener::read_request(int s) {
// Read until all (expected) strings have been read, the buffer is
// full, or EOF.

int off = 0;
int left = max_len;
size_t off = 0;
size_t left = max_len;

do {
int n;
ssize_t n;
RESTARTABLE(read(s, buf+off, left), n);
assert(n <= left, "buffer was too small, impossible!");
assert(n <= checked_cast<ssize_t>(left), "buffer was too small, impossible!");
buf[max_len - 1] = '\0';
if (n == -1) {
return nullptr; // reset by peer or other error
}
if (n == 0) {
break;
}
for (int i=0; i<n; i++) {
for (ssize_t i=0; i<n; i++) {
if (buf[off+i] == 0) {
// EOS found
str_count++;
@@ -383,9 +383,9 @@ BsdAttachOperation* BsdAttachListener::dequeue() {
}

// write the given buffer to the socket
int BsdAttachListener::write_fully(int s, char* buf, int len) {
int BsdAttachListener::write_fully(int s, char* buf, size_t len) {
do {
int n = ::write(s, buf, len);
ssize_t n = ::write(s, buf, len);
if (n == -1) {
if (errno != EINTR) return -1;
} else {
4 changes: 2 additions & 2 deletions src/hotspot/os/bsd/os_bsd.cpp
Original file line number Diff line number Diff line change
@@ -2208,9 +2208,9 @@ static inline struct timespec get_mtime(const char* filename) {
int os::compare_file_modified_times(const char* file1, const char* file2) {
struct timespec filetime1 = get_mtime(file1);
struct timespec filetime2 = get_mtime(file2);
int diff = filetime1.tv_sec - filetime2.tv_sec;
int diff = primitive_compare(filetime1.tv_sec, filetime2.tv_sec);
if (diff == 0) {
return filetime1.tv_nsec - filetime2.tv_nsec;
diff = primitive_compare(filetime1.tv_nsec, filetime2.tv_nsec);
}
return diff;
}
16 changes: 8 additions & 8 deletions src/hotspot/os/linux/attachListener_linux.cpp
Original file line number Diff line number Diff line change
@@ -103,7 +103,7 @@ class LinuxAttachListener: AllStatic {
static int listener() { return _listener; }

// write the given buffer to a socket
static int write_fully(int s, char* buf, int len);
static int write_fully(int s, char* buf, size_t len);

static LinuxAttachOperation* dequeue();
};
@@ -257,7 +257,7 @@ LinuxAttachOperation* LinuxAttachListener::read_request(int s) {
// where <ver> is the protocol version (1), <cmd> is the command
// name ("load", "datadump", ...), and <arg> is an argument
int expected_str_count = 2 + AttachOperation::arg_count_max;
const int max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +
const unsigned max_len = (sizeof(ver_str) + 1) + (AttachOperation::name_length_max + 1) +
AttachOperation::arg_count_max*(AttachOperation::arg_length_max + 1);

char buf[max_len];
@@ -266,21 +266,21 @@ LinuxAttachOperation* LinuxAttachListener::read_request(int s) {
// Read until all (expected) strings have been read, the buffer is
// full, or EOF.

int off = 0;
int left = max_len;
size_t off = 0;
size_t left = max_len;

do {
int n;
ssize_t n;
RESTARTABLE(read(s, buf+off, left), n);
assert(n <= left, "buffer was too small, impossible!");
assert(n <= checked_cast<ssize_t>(left), "buffer was too small, impossible!");
buf[max_len - 1] = '\0';
if (n == -1) {
return nullptr; // reset by peer or other error
}
if (n == 0) {
break;
}
for (int i=0; i<n; i++) {
for (ssize_t i=0; i<n; i++) {
if (buf[off+i] == 0) {
// EOS found
str_count++;
@@ -383,7 +383,7 @@ LinuxAttachOperation* LinuxAttachListener::dequeue() {
}

// write the given buffer to the socket
int LinuxAttachListener::write_fully(int s, char* buf, int len) {
int LinuxAttachListener::write_fully(int s, char* buf, size_t len) {
do {
ssize_t n = ::write(s, buf, len);
if (n == -1) {
Loading