-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Changes from 11 commits
3a30b43
fc8a1b7
feb26a9
b0e6d5d
b577d33
d289f15
5958da0
fd5464d
6777a2a
ab81b2e
e29c997
df403d2
c4c4584
6d3f476
7f0d3dc
1b80429
84905d7
b31e3a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) + | ||
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!"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why ssize_t here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t instead?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.