RFR (S): 8143215: gcc 4.1.2: fix three issues breaking the build.

[Volker Simonis]

Hi Goetz, Kim,

for me this fix is a good because it is a simple and pragmatic
solution to an annoying problem.
You can consider it as reviewed from my side.

@Kim: I can partially understand your concerns - after all that's why
I opened JDK-8135181.

But 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 call to
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.

Regards,
Volker


On Fri, Nov 20, 2015 at 2:04 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> On Nov 19, 2015, at 4:44 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>>
>> OK, so I went back and checked it all ...
>> 1.) -Wconversion is set since ever:
>>     http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/3e82d72933d0
>> 2.) The warning is not triggered by -Wconversion, it is enabled per default in gcc 4.1.2:
>
> Does the warning still occur if -Wconversion is explicitly turn it
> off, e.g. add -Wno-conversion?  Unfortunately, after reading these
>
> http://lists.apple.com/archives/xcode-users/2005/Oct/msg00147.html
> https://gcc.gnu.org/wiki/NewWconversion
>
> I suspect it does, which would be unfortunate. But confirmation would
> be helpful, and would strongly affect my opinion about how to proceed.
>
> [Note that the NewWconversion page suggests we really ought not be
> using the pre-gcc4.3 -Wconversion either.  But if -Wno-conversion
> doesn't do anything useful for you then that's a separate issue.]
>
> An important (to me) reason to not hide these warnings via unchecked
> casts is that doing so makes it harder to properly address
> JDK-8135181, because some of the places that ought to be modified as
> part of that will be harder to find.
>
>> And I would really appreciate if the following two issues could be kept separate:
>>
>> - Decision what compiler is supported with which settings.
>> - Fixes for a supported compiler/flag combination.
>
> I don't think those are separable. If a particular configuration
> rejects otherwise working code, then the configuration ought to be
> looked at. Even if it's decided that the code must be changed rather
> than the configuration, if the modified code is clearly a workaround
> rather than a "natural" alternative, I think the workaround should be
> made obvious and traceable to the problematic configuration.
> Otherwise, future readers cannot tell whether the code is odd for a
> good reason, and the workaround is hard to clean up in the future.
>
> The fact that this particular problem has been cropping up repeatedly
> suggests to me that some concerted effort should be applied to the
> problem.  I spent a little bit of time today prototyping an
> integer_cast function, which would cover most of the cases mentioned
> in JDK-8135181 and all in the changeset under discussion.  It doesn't
> look too difficult, but I haven't finished or really tested it yet.
>