8314114: Fix -Wconversion warnings in os code, primarily linux #15229
Conversation
|
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
Webrevs
|
| @@ -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) + | |||
There was a problem hiding this comment.
The reason I changed it was to assign into size_t below without sign conversion.
There was a problem hiding this comment.
Sure, but can't it be size_t itself in the first place?
There was a problem hiding this comment.
Ok, doesn't reintroduce warnings to change back to size_t.
|
|
||
| do { | ||
| int 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.
This gets a sign comparison warning without the cast, which thankfully we do enable.
There was a problem hiding this comment.
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.
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!");
| ~~^~~~~~~
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
…_cast<> future could fix the dll_address_to_x functions.
…_cast<> future could fix the dll_address_to_x functions.
There was a problem hiding this comment.
Hi Coleen,
I had a read through these changes. Remarks inline.
This is an immense work, respect.
When and where I had been grumbling, it was because whatever we change must be maintainable in the future. So, if we now do X in various places, X must not be arbitrary but be a consistent rule we can follow in the future, lest all this work bitrots quickly.
One proposal, possibly for a future RFE: could we maybe make checked_cast a smaller name? One gripe with C++ cast expressions I have is that they look ugly and give me RSI. Since this is our own name, and it is really omnipresent now, could we not shorten it (similar to macro names like "p2i")? For example cc. As in
size_t len
foo_takes_int(cc<int>(len))
or even better as a macro
foo_takes_int(cc(int, len))
Cheers, Thomas
src/hotspot/os/linux/os_linux.cpp
Outdated
| @@ -423,7 +423,7 @@ static const char *unstable_chroot_error = "/proc file system not found.\n" | |||
| "environment on Linux when /proc filesystem is not mounted."; | |||
|
|
|||
| void os::Linux::initialize_system_info() { | |||
| set_processor_count(sysconf(_SC_NPROCESSORS_CONF)); | |||
| set_processor_count(checked_cast<int>(sysconf(_SC_NPROCESSORS_CONF))); | |||
There was a problem hiding this comment.
@coleenp: The only case where sysconf could be not a small positive integer is when it returns an error, -1, and checked_cast will not catch that since it still fits an int.
Up to you, but I'd either leave this unchecked, or convert the parameter for set_processor_count to unsigned.
change func() to a template function
I hope we don't do that, that sounds terrible. I still have to find a modern IDE that can work well with all the template code we have. The more templates we use, the worse features like call graph display work in IDEs like CDT or CLion.
|
|
||
| do { | ||
| int 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.
I'm curious, who is warning here? Is this a static code analysis? Since I don't think Oracle builds AIX, right?
|
Thanks for reviewing, Thomas. I like your idea of renaming checked_cast to cc or something shorter. I'd love for the error message to be improved since I seem to be now hitting when testing a lot. |
There's a C++ style that goes with idiomatic C++ programs. I chose |
…ax_len size_t in the three places.
|
@coleenp This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 33 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>
Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>
|
Thanks Dean, David and Thomas. I think your comments improved the change significantly. |
|
Going to push as commit 9ded868.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
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.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15229/head:pull/15229$ git checkout pull/15229Update a local copy of the PR:
$ git checkout pull/15229$ git pull https://git.openjdk.org/jdk.git pull/15229/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15229View PR using the GUI difftool:
$ git pr show -t 15229Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15229.diff
Webrev
Link to Webrev Comment