JDK-8135181 : Re-enable '-Wconversion' for GCC 4.3 and later
  • Type: Enhancement
  • Component: infrastructure
  • Sub-Component: other
  • Affected Version: 9
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2015-09-08
  • Updated: 2024-11-01
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
tbdUnresolved
Related Reports
Relates :  
JDK-8233144 - undefined behavior: signed integer overflow
Relates :  
JDK-8160310 - HotSpot cannot be built with GCC 6
Relates :  
JDK-8135179 - Fix conversion warning after 8067341
Relates :  
JDK-8281455 - Change JVM options with small ranges from 64 to 32 bits, for gc_globals.hpp
Relates :  
JDK-8281213 - Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
Relates :  
JDK-8281214 - Unsafe use of long in VMThread::setup_periodic_safepoint_if_needed
Relates :  
JDK-8314258 - checked_cast doesn't properly check some cases
Relates :  
JDK-8313892 - Fix -Wconversion warnings in java.desktop code
Sub Tasks
JDK-8177480 :  
Prepare GC code for -Wconversion - Open
JDK-8177481 :  
Prepare Runtime code for -Wconversion - Closed
JDK-8177482 :  
Prepare Compiler code for -Wconversion - Open
JDK-8177483 :  
Prepare MnM code for -Wconversion - Open
Description
Currently, we disable '-Wconversion' for GCC 4.3 and later because it produces too many warnings which are by default treated as errors. All the warnings are about narrowing conversions which may result in a potential data loss.

It may be a good idea to fix all these implicit narrowing conversions and re-enable -Wconversion warning by default.

Unfortunately there's quite some code which has to be changed in order to meet the requirements of '-Wconversion'. A quick research showed that there are currently 137 different implicit narrowing conversions (see below) spread over 1785 different code locations:

warning: conversion to ‘CardIdx_t {aka int}’ from ‘uintptr_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘char’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘char’ from ‘short unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘CodeBuffer::csize_t {aka int}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘CodeBuffer::csize_t {aka int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘CodeBuffer::csize_t {aka int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘CodeBuffer::csize_t {aka int}’ from ‘ptrdiff_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘CodeBuffer::csize_t {aka int}’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘CodeSection::csize_t {aka int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘jlong {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘julong {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘double’ from ‘uintx {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘Elf_Word {aka unsigned int}’ from ‘Elf64_Addr {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘Elf_Word {aka unsigned int}’ from ‘Elf64_Xword {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘double’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘float’ from ‘unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘InCSetState::in_cset_state_t {aka signed char}’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int16_t {aka short int}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘int32_t {aka int}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int32_t {aka int}’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int32_t {aka int}’ from ‘jlong {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int32_t {aka int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘int32_t {aka int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘int32_t {aka int}’ from ‘uintx {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int8_t {aka signed char}’ from ‘int32_t {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘int8_t {aka signed char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘int8_t {aka signed char}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘intArray::etype {aka int}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘intArray::etype {aka int}’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘Elf64_Xword {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘jlong {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘julong {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘__off_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘u8 {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘uintptr_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘int’ from ‘uintx {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘jboolean {aka unsigned char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘jboolean {aka unsigned char}’ from ‘jint {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘jbyte {aka signed char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘jbyte {aka signed char}’ from ‘jint {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘jchar {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘jchar {aka short unsigned int}’ from ‘jint {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘jfloat {aka float}’ from ‘double’ may alter its value [-Wconversion]
warning: conversion to ‘jfloat {aka float}’ from ‘jdouble {aka double}’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘int64_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘jlong {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘jint {aka int}’ from ‘u8 {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘jshort {aka short int}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘jshort {aka short int}’ from ‘jint {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘jubyte {aka unsigned char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘juint {aka unsigned int}’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘juint {aka unsigned int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘juint {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘jushort {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘long unsigned int:40’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘narrowKlass {aka unsigned int}’ from ‘uint64_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘narrowOop {aka unsigned int}’ from ‘uint64_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘OptoReg::Name {aka int}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘OptoReg::Name {aka int}’ from ‘uintptr_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘OSThread::thread_id_t {aka int}’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘s4 {aka int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘s_char {aka signed char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘short int’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘short int’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘short int’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘short int’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘short int’ from ‘OptoReg::Name {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int:11’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int’ from ‘jint {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘short unsigned int’ from ‘unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘size_t {aka long unsigned int}’ from ‘double’ may alter its value [-Wconversion]
warning: conversion to ‘u1 {aka unsigned char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘u1 {aka unsigned char}’ from ‘u2 {aka short unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘u1 {aka unsigned char}’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘u2 {aka short unsigned int}’ from ‘int32_t {aka int}’ may alter its value [-Wconversion]
warning: conversion to ‘u2 {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘u2 {aka short unsigned int}’ from ‘u4 {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘u2 {aka short unsigned int}’ from ‘uintptr_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘u2 {aka short unsigned int}’ from ‘unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘u4 {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘uint16_t {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘uint32_t {aka unsigned int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘uint8_t {aka unsigned char}’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘uint8_t {aka unsigned char}’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘address_word {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘int64_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘jlong {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘uintptr_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘uint {aka unsigned int}’ from ‘uintx {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char:1’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char:2’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char’ from ‘int’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char’ from ‘MEMFLAGS {aka MemoryType}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char’ from ‘u2 {aka short unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char’ from ‘u4 {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char’ from ‘uint {aka unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned char’ from ‘unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘intptr_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘intx {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘julong {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘long int’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘__off_t {aka long int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘uintptr_t {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘unsigned int’ from ‘uintx {aka long unsigned int}’ may alter its value [-Wconversion]
warning: conversion to ‘u_short {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
warning: the result of the conversion is unspecified because ‘-0x00000000000000001’ is outside the range of type ‘Dependencies::DepType’ [-Wconversion]
Comments
-Wconversion probably does more than we want. For gcc, I think we want "-Wconversion -Wno-float-conversion" for now, to warn about potentially lossy integral->integral conversions. The floating point cases tend to be different. There it is common for float->integral to be intentionally discarding the fractional part, and for integral->float to be indifferent to the potential loss of precision. We might want some specific conversion functions for those use-cases so we can later enable -Wfloat-conversion, assuming we think it's worthwhile doing that. For clang, I think we want -Wimplicit-int-conversion to warn about potentially lossy integral->integral conversions. clang seems to have a much richer categorization of warnings in this area than currently in gcc.
16-10-2023
With the 17 changes to RFEs linked to JDK-8177481, the number of Wconversion errors in Hotspot are now: Number of files: 226 Total: 5399 Average: 23 Top files producing warnings (due to inclusion mostly): 1350 /hotspot/share/opto/optoreg.hpp 882 /hotspot/share/opto/node.hpp 783 /hotspot/share/opto/loopnode.hpp 244 /hotspot/share/compiler/compilerThread.hpp 140 /hotspot/share/gc/g1/g1CardSetContainers.inline.hpp 126 /hotspot/share/opto/type.hpp 117 /hotspot/cpu/x86/x86.ad 111 /hotspot/share/opto/block.hpp 110 /hotspot/cpu/x86/matcher_x86.hpp If we fixed these header files, we could put a PRAGMA_ALLOW_LOSSY_CONVERSIONS in the rest until they're incrementally fixed (assuming that we can restrict the -Wconversion option to Hotspot files in the build system). ie: +#ifdef _WIN32 + +// Turn off -Wconversion warnings +__pragma(warning(disable : 4244)) + +#else // !_WIN32 + +// Turn off -Wconversion warnings +# pragma GCC diagnostic ignored "-Wconversion" + +#endif // !_WIN32 +
18-08-2023
I made some progress on fixing the Runtime warnings with some patches broken down by area. The approach was to a. fix wrongly typed integers while managing fan-out from type changes, b. make *limited* local changes to fix code to not get Wconversion, then c. add checked-casts, pointer_delta_as_int() and other wrapper calls. In earlier changes I used direct casts when obvious that the int fit, but reviewers didn't like that. This approach was time consuming but imo the right one. The 64 bit int <---> double conversion warnings were fixed with C style reinterpret casts, which may or may not be problematic in the future. The runtime changes are linked to the Runtime subtask of this issue, and all start with Fix -Wconversion warning in something code. When turning on Wconversion in the build like above, there are tens of thousands of warnings in java.desktop, jdk, and jtreg test code. The googletest code has been fixed so we're getting a new version. The java.desktop warnings come from a third party application so might not be fixable see: https://bugs.openjdk.org/browse/JDK-8313892 I think there should be changes to the build system to only build certain components with -Wconversion, starting with Hotspot src when more Hotspot code is fixed. We could turn off conversions (ALLOW_LOSSY_CONVERSIONS) for selected files that haven't been fixed, which should eventually be a small set once the compiler and gc changes are made.
15-08-2023
[~jvernee] There is now a way to disable a warning per individual file in the build system, so technically this can be achieved now. However, there's a *lot* of files to have this warning disabled for. I counted to 981 files in hotspot that triggered this warning. That is about 2/3 of all hotspot files.
07-11-2022
I think a possible way to make progress on this, is to enable this warning globally (and the equivalent on Windows), and then dissable the warning on a per-file basis using a macro/pragma. That way at least new warnings don't creep into files that are clean today. Then, warnings can be addressed and the pragma removed on a per-file basis or in groups of related files.
14-07-2022
Perhaps one sub-task could be to change all hotspot options that are unnecessarily declared as 64 bits to be 32 bits. For example, changing ObjectAlignmentInBytes (range 8 - 256) from intx to int prevents warnings for assignments such as this in filemap.cpp: int _obj_alignment = ObjectAlignmentInBytes; This would require other changes such as not using INTX_FORMAT when printing ObjectAlignmentInBytes, but these changes would be small and contained compared to changing all assignees of ObjectAignmentInBytes to intx, and changing their assignees, etc.
02-02-2022
The main infra bug (this one) cannot be moved forward until all subtasks has been fixed. For the record, here's a patch that applies (as of today anyway) to enable -Wconversion to try and fix the sub tasks: diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4 --- a/make/autoconf/flags-cflags.m4 +++ b/make/autoconf/flags-cflags.m4 @@ -143,7 +143,7 @@ # Additional warnings that are not activated by -Wall and -Wextra WARNINGS_ENABLE_ADDITIONAL="-Wpointer-arith -Wsign-compare \ -Wunused-function -Wundef -Wunused-value -Wreturn-type \ - -Wtrampolines" + -Wtrampolines -Wconversion" WARNINGS_ENABLE_ADDITIONAL_CXX="-Woverloaded-virtual -Wreorder" WARNINGS_ENABLE_ALL_CFLAGS="-Wall -Wextra -Wformat=2 $WARNINGS_ENABLE_ADDITIONAL" WARNINGS_ENABLE_ALL_CXXFLAGS="$WARNINGS_ENABLE_ALL_CFLAGS $WARNINGS_ENABLE_ADDITIONAL_CXX"
12-06-2020
Isn't parts of this already enabled on Windows? I remember getting conversion related warnings on Windows that other platforms didn't complain about. If so there shouldn't be a terrible amount of warnings, at least not in shared code. I think -Wsign-conversion is a good flag to enable as well. It should probably have its own RFE though.
12-04-2017
For C++, -Wconversion does not warn about unsigned <-> signed conversions. For that one needs to use -Wsign-conversion. I bet that generates lots of warnings :)
12-04-2017
The main issue is an infrastructure issue. Preparation of the code to be able to enable the warning should obviously be done by the Hotspot teams, but these are separate tasks.
23-03-2017
Kim, I completely agree. Thank you for pointing this out. Please feel free to move this to the appropriate component/subcomponent.
04-10-2016
The bug was previously hotspot->build so was moved to infrastructure->build. Seems it should not really have been hotspot->build in the first place. Is it worth creating subtasks to handle the warnings in code in different areas?
04-10-2016
Why is this an infastructure->build issue? Yes, there's a *very* small piece that involves changing the build system to add the option under appropriate circumstances. The vast majority of the work is the elimination of the warnings that would result from that, which will involve C++ code changes in many places.
03-10-2016
Moved to infrastructure->build as we now have the new hotspot-build system in place.
02-08-2016
The attached integer_cast.patch is a prototype integer/float => integer converter that range checks narrowing conversions. TBDs include: contains some metaprogramming facilities that ought to be repackaged; requires fix for JDK-8086005 in order to use <limits>; problem accessing <limits> at all on Solaris; questions about whether checks should be optional or unconditional.
09-12-2015
Some of the problematic conversions might be eliminated by rewriting the code. For a specific example: src/share/vm/gc/g1/g1IHOPControl.cpp size_t G1AdaptiveIHOPControl::actual_target_threshold() const double safe_total_heap_percentage = MIN2((double)(_heap_reserve_percent + _heap_waste_percent), 100.0); return MIN2( (G1CollectedHeap::heap()->max_capacity() * (100.0 - safe_total_heap_percentage) / 100.0), (_target_occupancy * (100.0 - _heap_waste_percent) / 100.0) ); The result of the MIN2 is then implicitly narrowed to size_t as the result of the function. This calculation could instead be written as size_t safe_total_heap_percentage = MIN2(_heap_reserve_percent + _heap_wast_percent, 100); return MIN2( (G1CollectedHeap::heap()->max_capacity() / 100) * (100 - safe_total_heap_percentage), (_target_occupancy / 100) * (100 - _heap_waste_percent) ); The absolute rounding error is small (<= 100), making the relative rounding error likely insignificant. This does obfuscate the code a bit though.
09-12-2015
Some of the problematic conversions are around arithmetic involving percentages. These percentages are usually [0,100] integer values, which are converted to floating point for use in some calculation, with the final result converted back to an integer value. The final conversion is the problem. It might be worthwhile creating a Percentage class to encapsulate these kinds of calculations. It might be that expression templates would be useful in such an endeavor.
09-12-2015
Many of the problematic conversions are due to simple type mismatches, especially signed vs unsigned differences. Cleaning those up could go a long way. Doing so can be quite difficult though, because the fanout for any individual type change can be large.
09-12-2015
I suggested using boost::numeric_cast<> as *inspiration*, not to slavishly copy it. My current thinking is to add 3 operations to cover conversions to integer types: checked_integer_cast, unchecked_integer_cast, and integer_cast. An explicit unchecked_integer_cast should be accompanied by analysis that the conversion is safe or otherwise inappropriate. integer_cast uses the checked or unchecked form, based on #ifdef ASSERT. Do something similar for the much smaller set of float conversions.
20-11-2015
boost::numeric_cast<> adds a runtime check for every narrowing conversion plus the overhead for error handling. In the case of boost::numeric_cast<> error handling is using C++ exceptions which I don't think we want to introduce into HotSpot (at least not for this feature). In our case that would probably end in a assert/guarantee. I think even if we would only use this in debug configurations that would still be an unacceptably high overhead because every narrowing conversion like: int i; (char)i; would turn into something like: if (i >= 0 && i <= MAX_CHAR) { (char)i; } else { assert("Data loss due to narrowing conversion"); } I would be nevertheless be interesting to see what the actual overhead would be for hotspot, so if you have a prototype I'd be interested to see some numbers :) Another problem with a boost::numeric_cast<> kind of solution is that the hotspot code heavily uses the fact that according to the C/C++ standard unsigned/signed integral type conversions must obey the laws of arithmetic modulo 2n. It is not clear to me how we can automatically detect if a conversion like (unsinged int)-1 is intended (because we want to get MAX_UNSIGNED_INT) or is an error. By the way, I still think that a manual cast is still better than an implicit one because it means that somebody has reasoned about that specific code. Of course his reasoning may be wrong or the preconditions may change over time, but that's another problem.
20-11-2015
This should be addressed by introducing and making use of some (optionally) checked conversion functions. boost::numeric_cast<>() might be used as inspiration. Adding unchecked (C-style or C++-style) cast operators is really no better than leaving the warning turned off.
19-11-2015