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

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Aug 10, 2023

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

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8314114: Fix -Wconversion warnings in os code, primarily linux (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15229/head:pull/15229
$ git checkout pull/15229

Update a local copy of the PR:
$ git checkout pull/15229
$ git pull https://git.openjdk.org/jdk.git pull/15229/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15229

View PR using the GUI difftool:
$ git pr show -t 15229

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15229.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 10, 2023

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 10, 2023
@openjdk
Copy link

openjdk bot commented Aug 10, 2023

@coleenp The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 10, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 10, 2023

Copy link
Contributor Author

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

Thanks for the quick feedback.

src/hotspot/os/aix/attachListener_aix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/aix/os_aix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

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

Thanks.

@@ -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.


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!");
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.

src/hotspot/os/bsd/attachListener_bsd.cpp Show resolved Hide resolved
src/hotspot/os/linux/cgroupSubsystem_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/os_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/os_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/os_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/os_posix.hpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/signals_posix.cpp Outdated Show resolved Hide resolved
…_cast<> future could fix the dll_address_to_x functions.
…_cast<> future could fix the dll_address_to_x functions.
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@@ -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)));
Copy link
Member

@tstuefe tstuefe Aug 12, 2023

Choose a reason for hiding this comment

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

@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.

@dean-long:

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.

src/hotspot/os/linux/os_linux.cpp Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved

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!");
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?

@coleenp
Copy link
Contributor Author

coleenp commented Aug 12, 2023

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.

@theRealAph
Copy link
Contributor

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

There's a C++ style that goes with idiomatic C++ programs. I chose checked_cast<T> because it is almost obvious to the reader familiar with C++ idioms what it's likely to do. In general, where there is precedent, I think we should go with existing C++ style.

@openjdk
Copy link

openjdk bot commented Aug 14, 2023

@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:

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

Reviewed-by: dholmes, dlong

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 master branch:

  • 37c6b23: 8308340: C2: Idealize Fma nodes
  • 583cb75: 8313406: nep_invoker_blob can be simplified more
  • 0074b48: 8312597: Convert TraceTypeProfile to UL
  • 1f1c5c6: 8314241: Add test/jdk/sun/security/pkcs/pkcs7/SignerOrder.java to ProblemList
  • f142470: 8311981: Test gc/stringdedup/TestStringDeduplicationAgeThreshold.java#ZGenerational timed out
  • 595fdd3: 8314059: Remove PKCS7.verify()
  • 49b2984: 8313854: Some tests in serviceability area fail on localized Windows platform
  • c132176: 8114830: (fs) Files.copy fails due to interference from something else changing the file system
  • e56d3bc: 8313657: com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors
  • 4b2703a: 8313678: SymbolTable can leak Symbols during cleanup
  • ... and 23 more: https://git.openjdk.org/jdk/compare/88b4e3b8539c2beb29ad92bd74b300002c2ef84b...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 14, 2023
coleenp and others added 4 commits August 14, 2023 16:46
Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>
Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

For the sake of moving forward, nothing further from me.

Thanks.

@coleenp
Copy link
Contributor Author

coleenp commented Aug 15, 2023

Thanks Dean, David and Thomas. I think your comments improved the change significantly.
/integrate

@openjdk
Copy link

openjdk bot commented Aug 15, 2023

Going to push as commit 9ded868.
Since your change was applied there have been 38 commits pushed to the master branch:

  • a02d65e: 8310308: IR Framework: check for type and size of vector nodes
  • dff99f7: 8313782: Add user-facing warning if THPs are enabled but cannot be used
  • f4e72c5: 8313949: Missing word in GPLv2 license text in StackMapTableAttribute.java
  • 6338927: 8314197: AttachListener::pd_find_operation always returning nullptr
  • b7dee21: 8314244: Incorrect file headers in new tests from JDK-8312597
  • 37c6b23: 8308340: C2: Idealize Fma nodes
  • 583cb75: 8313406: nep_invoker_blob can be simplified more
  • 0074b48: 8312597: Convert TraceTypeProfile to UL
  • 1f1c5c6: 8314241: Add test/jdk/sun/security/pkcs/pkcs7/SignerOrder.java to ProblemList
  • f142470: 8311981: Test gc/stringdedup/TestStringDeduplicationAgeThreshold.java#ZGenerational timed out
  • ... and 28 more: https://git.openjdk.org/jdk/compare/88b4e3b8539c2beb29ad92bd74b300002c2ef84b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 15, 2023
@openjdk openjdk bot closed this Aug 15, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 15, 2023
@openjdk
Copy link

openjdk bot commented Aug 15, 2023

@coleenp Pushed as commit 9ded868.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@coleenp coleenp deleted the os-conversion branch August 15, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
6 participants